Re: Bug? wcsxfrm causing memory corruption

[Corinna Vinschen <corinna-cygwin at cygwin dot com>]

Hi Erik,

On May 22 14:50, Erik Bray wrote:
> On Sun, May 21, 2017 at 6:23 AM, Duncan Roe wrote:
> > On Wed, May 10, 2017 at 11:30:46AM +0200, Erik Bray wrote:
> >> [...]
> >> The attached test demonstrates the bug.  Given an output buffer of N
> >> wide characters, wcsxfrm will cause bytes beyond the destination size
> >> to be reversed. I believe it might actually be a bug in the underlying
> >> LCMapStringW workhorse (this is on Windows 10; have not tested other
> >> versions).
> >>
> >> According to its docs [1], the cchDest argument (size of the
> >> destination buffer) is treated as a *byte* count when using
> >> LCMAP_SORTKEY.  However, for the purposes of applying the
> >> LCMAP_BYTEREV transformation it seems to be treating the output size
> >> (in bytes) as character count.  So in the example I give, where the
> >> output sort key is 7 bytes (including the null terminator), it swaps
> >> *14* bytes--the bytes including the sort key as well as the next 7
> >> adjacent bytes.  This is obviously a problem if the destination buffer
> >> is allocated out of some larger memory pool.
> >>
> >> This definitely has to be a bug, right?  Or at least very poorly
> >> documented on MS's part.  A workaround would either be to not use
> >> LCMAP_BYTEREV and just swap the bytes manually, or in a second call to
> >> LCMapStringW with LCMAP_BYTEREV and the correct character count...
> >> [...]
> > Hi Erik,
> >
> > I get
> > [...]
> Yes, that's the same.  Thanks for giving it a try--I should have
> included example output in my original message.
> 
> You can see in the last case that without LCMAP_BYTEREV it writes the sequence
> 
> 0x0e 0x09 0x01 0x01 0x01 0x01 0x00
> 
> with a terminating 0x00.  Bytes after that remain unchanged.  In the
> other two examples *with* LCMAP_BYTEREV, the terminating 0x00 gets
> swapped with the 0x07 after it, but this documented and expected
> behavior of LCMapStringW, and is already accounted for in Cygwin's
> wcsxfrm.  What is undocumented, and unexpected, is that it also byte
> swaps 3 more byte pairs after the actual sort key, which can corrupt
> memory unexpectedly.

Incredible piece of detective work.  Yeah, this looks very much like a
bug in LCMapStringW, embarrassingly one I didn't notice at the time of
writing the code.  I just tested this on W7 and the problem was already
present.

I don't know if this could be fixed by documentation.  There's just no
safe way to combine LCMAP_SORTKEY with LCMAP_BYTEREV it seems, unless
you always allocate a buffer at least twice as big as actually required
for the sort key.  

So, yeah, I think manual swaping after calling LCMapStringW without
LCMAP_BYTEREV is the way to go.  I'll prepare a fix.


Thanks for tracking down and the testcase,
Corinna

-- 
Corinna Vinschen                  Please, send mails regarding Cygwin to
Cygwin Maintainer                 cygwin AT cygwin DOT com
Red Hat

Attachment: signature.asc
Description: PGP signature