[Bug 19839] New: Access violation when closing ImgBurn 2.5.0.0 if its 'Disc Layout Editor' window has ever been opened.

http://bugs.winehq.org/show_bug.cgi?id=19839

Summary: Access violation when closing ImgBurn 2.5.0.0 if its
                    'Disc Layout Editor' window has ever been opened.
           Product: Wine
           Version: 1.1.28
          Platform: PC
               URL: http://www.imgburn.com
        OS/Version: Linux
            Status: UNCONFIRMED
          Severity: normal
          Priority: P2
         Component: -unknown
        AssignedTo: [e-mail]
        ReportedBy: [e-mail]

v2.5.0.0 of ImgBurn has a new Advanced build mode interface known as the 'Disc
Layout Editor'.

If you open the window at all (as in, 'full stop' and not even doing anything
with it) and then close the program, it'll crash with an access violation.

Open the program and set the 'Mode' to 'Build', set the 'Input' to 'Advanced'
 and then click the big 'Show Disc Layout Editor' button. Once you've done that
and you attempt to close the program, it'll crash.

I'm unable to reproduce this issue on any real installation of Windows
(from Windows 95 -> Windows 7) so I can only assume it's something in Wine
(sorry!).

[Bug 19839] Access violation when closing ImgBurn 2.5.0.0 if its 'Disc Layout Editor' window has ever been opened.

http://bugs.winehq.org/show_bug.cgi?id=19839

Juan Lang  changed:

What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|UNCONFIRMED                 |NEW
     Ever Confirmed|0                           |1

--- Comment #1 from Juan Lang   2009-08-25 12:55:40 ---
Confirming.  Thanks for the detailed bug report.

[Bug 19839] Access violation when closing ImgBurn 2.5.0.0 if its 'Disc Layout Editor' window has ever been opened.

http://bugs.winehq.org/show_bug.cgi?id=19839

Juan Lang  changed:

What    |Removed                     |Added
----------------------------------------------------------------------------
          Component|-unknown                    |comctl32

--- Comment #2 from Juan Lang   2009-08-25 13:03:59 ---
I installed a native common controls library using 'winetricks cc580', and the
access violation goes away.  Moving component to comctl32.

[Bug 19839] Access violation when closing ImgBurn 2.5.0.0 if its 'Disc Layout Editor' window has ever been opened.

http://bugs.winehq.org/show_bug.cgi?id=19839

--- Comment #3 from Juan Lang   2009-08-25 16:21:27 ---
> From a +relay,+seh log:

0009:Ret  window proc 0x1de06cb
(hwnd=0x201a8,msg=WM_GETTEXT,wp=00000001,lp=0032ef54) retval=ffffffff
0009:Ret  user32.SendMessageA() retval=7fffffff ret=0080e46c
trace:seh:raise_exception code=c0000005 flags=0 addr=0x7da1dd ip=007da1dd
tid=0009

The return value from WM_GETTEXT looks a little funny: it's returning -1.  MSDN
says the return value is the number of characters copied, excluding the NULL
terminator.  A little closer look reveals it's the statusbar control that's
returning -1.  I'll attach a patch in a sec, though it'll need a test to
confirm it.

[Bug 19839] Access violation when closing ImgBurn 2.5.0.0 if its 'Disc Layout Editor' window has ever been opened.

http://bugs.winehq.org/show_bug.cgi?id=19839

--- Comment #4 from Juan Lang   2009-08-25 16:25:19 ---
Created an attachment (id=23250)
 --> (http://bugs.winehq.org/attachment.cgi?id=23250)
Patch: Return the number of characters copied in WM_GETTEXT even if the buffer
is too small

This fixes the crash for me, although it deserves a test case to confirm it.

[Bug 19839] Access violation when closing ImgBurn 2.5.0.0 if its 'Disc Layout Editor' window has ever been opened.

http://bugs.winehq.org/show_bug.cgi?id=19839

Juan Lang  changed:

What    |Removed                     |Added
----------------------------------------------------------------------------
           Keywords|                            |download, patch

[Bug 19839] Access violation when closing ImgBurn 2.5.0.0 if its 'Disc Layout Editor' window has ever been opened.

http://bugs.winehq.org/show_bug.cgi?id=19839

Anastasius Focht  changed:

What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |[e-mail]

--- Comment #5 from Anastasius Focht   2009-08-25 17:03:54 ---
Hello,

looks like dupe of bug 13411

Regards

[Bug 19839] Access violation when closing ImgBurn 2.5.0.0 if its 'Disc Layout Editor' window has ever been opened.

http://bugs.winehq.org/show_bug.cgi?id=19839

Juan Lang  changed:

What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |[e-mail]

[Bug 19839] Access violation when closing ImgBurn 2.5.0.0 if its 'Disc Layout Editor' window has ever been opened.

http://bugs.winehq.org/show_bug.cgi?id=19839

--- Comment #6 from LIGHTNING UK!   2009-08-25 19:26:48 ---
I haven't looked at anything except your patch (i.e no actual source code) but
wouldn't you simply have to 'return 0;' rather than 'return -1' ? You can't
copy '-1' characters into the buffer so '-1' isn't a valid return value
(according to MSDN anyway - and as you pointed out earlier).

The bit in your patch where you memcpy looks like it could cause its own access
violation if 'size' is larger than the length of 'infoPtr->parts[0].text' - no
?
> > memcpy (buf, infoPtr->parts[0].text, (size - 1) * sizeof(WCHAR));

and along the same line, why is this being used (again, I've NOT looked at the
source code, just what I can see in your patch!)...
> >      strcpyW (buf, infoPtr->parts[0].text);

...rather that one that takes into account the actual max length of 'buf' (as
specified by the 'size' parameter) - strLcpyW or whatever it would be called.

[Bug 19839] Access violation when closing ImgBurn 2.5.0.0 if its 'Disc Layout Editor' window has ever been opened.

http://bugs.winehq.org/show_bug.cgi?id=19839

--- Comment #7 from Juan Lang   2009-08-25 19:35:48 ---
(In reply to comment #6)
> I haven't looked at anything except your patch (i.e no actual source code) but
> wouldn't you simply have to 'return 0;' rather than 'return -1' ?

No.  I suppose I could have tested that as well, that is, what should it return
when the buffer is 2 characters in length rather than 1, and the status bar
text is longer than 1 character?  I followed MSDN here.
> The bit in your patch where you memcpy looks like it could cause its own access
> violation if 'size' is larger than the length of 'infoPtr->parts[0].text' - no
> ?

This condition is impossible due to the if clause:
    len = strlenW (infoPtr->parts[0].text);

if (size > len) {
(snip)
    else {

The else branch, which you're seeing, only gets run if size is NOT greater than
the length of infoPtr->parts[0].text.

[Bug 19839] Access violation when closing ImgBurn 2.5.0.0 if its 'Disc Layout Editor' window has ever been opened.

http://bugs.winehq.org/show_bug.cgi?id=19839

--- Comment #8 from LIGHTNING UK!   2009-08-25 19:48:11 ---
(In reply to comment #7)
> No.  I suppose I could have tested that as well, that is, what should it return
> when the buffer is 2 characters in length rather than 1, and the status bar
> text is longer than 1 character?  I followed MSDN here.

If size (wParam in WM_GETTEXT message) is 2 then it only has room for 1 actual
character and the terminating null terminator so the return value should be 1.

This is regardless of how long 'infoPtr->parts[0].text' is because only 1
character can be copied into buf - the terminating null isn't counted in the
return value.

I'm sure we're reading MSDN the same way :)
> This condition is impossible due to the if clause:
>     len = strlenW (infoPtr->parts[0].text);
>     if (size > len) {
> (snip)
>     else {
> The else branch, which you're seeing, only gets run if size is NOT greater than
> the length of infoPtr->parts[0].text.

Ah ok, that makes sense with a little more context. :)

[Bug 19839] Access violation when closing ImgBurn 2.5.0.0 if its 'Disc Layout Editor' window has ever been opened.

http://bugs.winehq.org/show_bug.cgi?id=19839

--- Comment #9 from Juan Lang   2009-08-25 21:18:26 ---
(In reply to comment #8)
> If size (wParam in WM_GETTEXT message) is 2 then it only has room for 1 actual
> character and the terminating null terminator so the return value should be 1.

Yes, precisely.  So, since size is _not_ greater than len (the length of
infoPtr->parts[0].text), the else branch is taken, and size - 1 characters are
copied to the output buffer, and size - 1 is returned.  Make sense?

[Bug 19839] Access violation when closing ImgBurn 2.5.0.0 if its 'Disc Layout Editor' window has ever been opened.

http://bugs.winehq.org/show_bug.cgi?id=19839

--- Comment #10 from LIGHTNING UK!   2009-08-26 04:10:40 ---
(In reply to comment #9)
> Yes, precisely.  So, since size is _not_ greater than len (the length of
> infoPtr->parts[0].text), the else branch is taken, and size - 1 characters are
> copied to the output buffer, and size - 1 is returned.  Make sense?

Yup, so long as the potential case where size == 0 is handled somewhere else
higher up. If size < 2, the return value should in theory always be 0.

[Bug 19839] Access violation when closing ImgBurn 2.5.0.0 if its 'Disc Layout Editor' window has ever been opened.

http://bugs.winehq.org/show_bug.cgi?id=19839

--- Comment #11 from LIGHTNING UK!   2009-08-26 04:51:10 ---
Oops, seems that if size is 0, windows actually returns the number of
characters that *would* be copied into the buffer *if* there was sufficient
allocation.

So this should work?

len = strlenW (infoPtr->parts[0].text);

if(size == 0)
    {
      return len;
    }
    else if(size > len)
    {
      strcpyW (buf, infoPtr->parts[0].text);
      return len;
    }
    else
    {
      memcpy (buf, infoPtr->parts[0].text, (size - 1) * sizeof(WCHAR));
      buf[size - 1] = 0; // This has to be 'size - 1' rather than just 'size'
or you'll get an access overrun?
      return size - 1;
    }

[Bug 19839] Access violation when closing ImgBurn 2.5.0.0 if its 'Disc Layout Editor' window has ever been opened.

http://bugs.winehq.org/show_bug.cgi?id=19839

--- Comment #12 from Juan Lang   2009-08-26 10:51:04 ---
(In reply to comment #11)
> Oops, seems that if size is 0, windows actually returns the number of
> characters that *would* be copied into the buffer *if* there was sufficient
> allocation.

Thanks, I hadn't tested that.  Right you are.  I sent an updated patch that
addresses this case.
>   memcpy (buf, infoPtr->parts[0].text, (size - 1) * sizeof(WCHAR));
>   buf[size - 1] = 0; // This has to be 'size - 1' rather than just 'size'
>
> or you'll get an access overrun?

Heh.  Right.

[Bug 19839] Access violation when closing ImgBurn 2.5.0.0 if its 'Disc Layout Editor' window has ever been opened.

http://bugs.winehq.org/show_bug.cgi?id=19839

Juan Lang  changed:

What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|NEW                         |RESOLVED
         Resolution|                            |FIXED

--- Comment #13 from Juan Lang   2009-08-27 10:48:29 ---
Fixed by commit 390a248e0649cf5cc4c4e8a476f0747bc204c3ba.  Thanks for your help
:)

[Bug 19839] Access violation when closing ImgBurn 2.5.0.0 if its 'Disc Layout Editor' window has ever been opened.

http://bugs.winehq.org/show_bug.cgi?id=19839

Alexandre Julliard  changed:

What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|RESOLVED                    |CLOSED

--- Comment #14 from Alexandre Julliard   2009-09-02 14:31:34 ---
Closing bugs fixed in 1.1.29.

[Bug 19839] Access violation when closing ImgBurn 2.5.0.0 if its 'Disc Layout Editor' window has ever been opened.

http://bugs.winehq.org/show_bug.cgi?id=19839

Nikolay Sivov  changed:

What    |Removed                     |Added
----------------------------------------------------------------------------
      Fixed by SHA1|                            |390a248e0649cf5cc4c4e8a476f
                   |                            |0747bc204c3ba