[Bug 19759] New: SLTG_ReadString does not null terminate

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

Summary: SLTG_ReadString does not null terminate
           Product: Wine
           Version: 1.1.27
          Platform: PC
        OS/Version: Linux
            Status: UNCONFIRMED
          Severity: normal
          Priority: P2
         Component: oleaut32
        AssignedTo: [e-mail]
        ReportedBy: [e-mail]

Created an attachment (id=23133)
 --> (http://bugs.winehq.org/attachment.cgi?id=23133)
Patch to properly null terminate

According to MSDN http://msdn.microsoft.com/en-us/library/ms221069.aspx the
BSTR string should be null terminated. SLTG_ReadString is not doing so as the
strings being read are not themselves null terminated and are read based on the
length. It causes the string length somehow to be misunderstood by
applications.

The attached screenshots show OLE Viewer from Visual Studio 6 showing a
TypeInfo header: one in Wine and the other on Windows.

The attached patch (against latest git) fixed the problem for me. It also fixes
another chunk of similar code.

[Bug 19759] SLTG_ReadString does not null terminate

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

--- Comment #1 from Sunil Mohan Adapa   2009-08-17 02:46:51 ---
Created an attachment (id=23134)
 --> (http://bugs.winehq.org/attachment.cgi?id=23134)
Screenshot on Wine

Screenshot of Visual Studio 6 OLE Viewer showing an SLTG TypeLib with problem
on Wine

[Bug 19759] SLTG_ReadString does not null terminate

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

--- Comment #2 from Sunil Mohan Adapa   2009-08-17 02:47:31 ---
Created an attachment (id=23135)
 --> (http://bugs.winehq.org/attachment.cgi?id=23135)
Screenshot on Windows

Screenshot of Visual Studio 6 OLE Viewer showing an SLTG TypeLib on Windows

[Bug 19759] SLTG_ReadString does not null terminate

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

Sunil Mohan Adapa  changed:

What    |Removed                     |Added
----------------------------------------------------------------------------
           Keywords|                            |patch

[Bug 19759] SLTG_ReadString does not null terminate

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

--- Comment #3 from Vitaliy Margolen   2009-08-17 08:43:09 ---
Something doesn't look right in your patch. SysAllocStringLen already
zero-terminates it. This means something overwrites that last \0, which is the
problem. The only thing that can do that in this code is MultiByteToWideChar
because it gets the wrong size...

[Bug 19759] SLTG_ReadString does not null terminate

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

--- Comment #4 from Sunil Mohan Adapa   2009-08-17 10:11:37 ---
(In reply to comment #3)
> Something doesn't look right in your patch. SysAllocStringLen already
> zero-terminates it. This means something overwrites that last \0, which is the
> problem.

MSDN documentation for SysAllocStringLen says that when the input string is
NULL, the newly allocated buffer will be uninitialized. I assumed Wine was
doing the same while it seems to zero-fill. It may not be good to depend on
this Wine-only behaviour.

In light of this, our bug is purely because of the incorrect length (-1) passed
to SysAllocStringLen. MultiByteToWideChar's length will only include the NULL
if the input string has it.
> The only thing that can do that in this code is MultiByteToWideChar
> because it gets the wrong size...

These are purely ASCII strings with correctly interpreted length values (I
checked the binary file)

[Bug 19759] SLTG_ReadString does not null terminate

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

--- Comment #5 from Juan Lang   2009-08-17 10:43:56 ---
Patches should be sent to wine-patches, they're not picked up from here.  The
correction to SLTG_ReadString looks fine, though I agree with Vitaliy that the
change to SLTG_DoVars looks more suspect.

[Bug 19759] SLTG_ReadString does not null terminate

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

--- Comment #6 from Sunil Mohan Adapa   2009-08-17 11:21:25 ---
(In reply to comment #5)
> Patches should be sent to wine-patches, they're not picked up from here.

Thank you for mentioning, I shall send it to the list.
> The
> correction to SLTG_ReadString looks fine, though I agree with Vitaliy that the
> change to SLTG_DoVars looks more suspect.

Change to SLTG_DoVars, strictly speaking, is not required given that Wine's
implementation of SysAllocStringLen is zero-filling the buffer. However, I
think the change is good to have, as this behaviour is not guaranteed (by other
implementations?).

So, then, shall I remove both the NULL terminations from the patch?

[Bug 19759] SLTG_ReadString does not null terminate

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

--- Comment #7 from Juan Lang   2009-08-17 19:33:11 ---
(In reply to comment #6)
> Change to SLTG_DoVars, strictly speaking, is not required given that Wine's
> implementation of SysAllocStringLen is zero-filling the buffer. However, I
> think the change is good to have, as this behaviour is not guaranteed (by other
> implementations?).

On one hand, I think you're right that this is consistent.  I was wondering
whether MultiByteToWideChar might already be NULL-terminating, but that was
only based on a cursory read.  Since you're passing an explicit source string
length to it, it doesn't guarantee that the destination is NULL terminated.  So
explicitly NULL terminating it does make the expectation clear.

On the other hand, since you're changing one part of oleaut32, it's impossible
to be using a different implementation of SysAllocStringLen at the same time.

As usual, a test case would resolve these difficulties once and for all, but
writing one may be impossible:  the buffer returned by SysAllocStringLen could
depend on the compiler and flags used by Microsoft when building oleaut32. 
While perhaps the buffer isn't guaranteed to be set entirely to be zero, there
is one test at least that checks that an empty string (length 0) has a NULL
terminator in it [1].  Since the test passes consistently across a wide variety
of Windows platforms [2], I'd say it's at least suggestive that while Microsoft
doesn't _claim_ to provide a zero buffer, in fact it does, or at the very least
a NULL-terminated one.

In summary, I'd suggest you keep the first change but omit the second:  the
second really does seem superfluous, and while in principle it can't hurt, it
might raise objections that would keep your patch out longer than necessary.

[1] http://source.winehq.org/source/dlls/oleaut32/tests/vartype.c#L5229
[2] http://test.winehq.org/data/tests/oleaut32:vartype.html

[Bug 19759] SLTG_ReadString does not null terminate

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

--- Comment #8 from Juan Lang   2009-08-17 19:35:09 ---
(In reply to comment #6)
> So, then, shall I remove both the NULL terminations from the patch?

And the more direct answer:  yes, do.  See my rather wordy last comment for my
justification ;-)

[Bug 19759] SLTG_ReadString does not null terminate

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

--- Comment #9 from Sunil Mohan Adapa   2009-08-18 00:29:29 ---
(In reply to comment #7)
[...]
> While perhaps the buffer isn't guaranteed to be set entirely to be zero, there
> is one test at least that checks that an empty string (length 0) has a NULL
> terminator in it [1].

This test case should shield our assumption.

[...]
> In summary, I'd suggest you keep the first change but omit the second:  the
> second really does seem superfluous, and while in principle it can't hurt, it
> might raise objections that would keep your patch out longer than necessary.

[...]

(In reply to comment #8)
> And the more direct answer:  yes, do.  See my rather wordy last comment for my
> justification ;-)

Thank you Juan, I have now posted a patch that only includes the length
correction and does not set NULL terminators.

[Bug 19759] SLTG_ReadString does not null terminate

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

Juan Lang  changed:

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

--- Comment #10 from Juan Lang   2009-08-18 10:13:04 ---
You patch was committed in f7f50d1252342db0fc035f9066b6ab10ae5f20a6.  Thanks
for contributing to Wine!

[Bug 19759] SLTG_ReadString does not null terminate

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

Saulius K.  changed:

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

[Bug 19759] SLTG_ReadString does not null terminate

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

Alexandre Julliard  changed:

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

--- Comment #11 from Alexandre Julliard   2009-08-21 13:01:03 ---
Closing bugs fixed in 1.1.28.