mmio: Ensure FOURCC string conversions are null-terminated.

[Tim Cadogan-Cowper]

Thanks Eric for the prompt feedback.

I agree, the %4.4s form should work.  But currently it doesn't, at least
not always.  I don't know exactly *why* yet but I can tell you *when*.
When Oblivion attempts to play a .wav file entitled
"data\sound\fx\npc\horse\foot\armor\npc_horse_foot_armor_01.wav" the
code goes into mmioDescend and catastrophically dies when it hits:

TRACE("ckid=%4.4s fcc=%4.4s cksize=%08X !\n",
     (LPCSTR)&lpck->ckid,
      srchType ? (LPCSTR)&lpck->fccType:"<na>",
       lpck->cksize); 

with a "wine_dbg_vprintf: debugstr buffer overflow".  More specifically
when I separate out the individual elements of this TRACE call we find
it dies on:

TRACE("ckid=%4.4s\n", (LPCSTR)&lpck->ckid);

The value of lpck->ckid at the time this call is made is 9FFEA1FE.

Moving on, if I adopt a null-terminated string approach as per the
patch, inserting a new mmioFOURCCToString function, and
replacing the above trace call with something like:

mmioFOURCCToString((DWORD)lpck->ckid, szCkId);
mmioFOURCCToString((DWORD)lpck->fccType, szFCCType);
TRACE("ckid=%08X('%s') fcc=%08X('%s') cksize=%u\n",
      (DWORD)lpck->ckid, szCkId, (DWORD)lpck->fccType, szFCCType,
      lpck->cksize);

there is no crash and everything proceeds swimmingly.

Evidence?  I've attached a modified form of mmio.c and the trace log it
produces which shows the two approaches working side by side.
You can see in line 6953 that the null-terminated string approach
proposed by the patch clears the hurdle, but the current %4.4s
approach dies (line 6957) with a buffer overflow
when it tries to convert and print ckId, which in turn triggers the
crash described in bug 10280.

Why? Not sure yet, and I won't be able to look at this again for a
while, but perhaps you can work it out by looking at the attached
log. Maybe it's obvious, but I admit my programming skills are a bit
rusty these days and if it's obvious I'm missing it.  I also respect
the fact that you guys have been looking at this much longer than I
have. However I can confidently say that the null-terminated string
approach stops this crash, and this approach respects byte order,
which presumably is the way Wine eventually want to go in the interests
of portability.

Some other options that will stop the crash and fix bug 10280:

(i) Don't attempt to convert the FOURCC values to strings.  Just
    print out the DWORD value, and let any debuggers work out
    what it says with their ASCII table handy.

(ii) Delete the TRACE calls that refer to ckId.

I still think this patch should be applied, as it should
not cause any regressions, it respects byte order and it stops
some nasty crashes (even if we don't know exactly why yet).

Tim


From: Eric Pouech <eric.pouech at orange.fr>
Subject: Re: mmio: Ensure FOURCC string conversions are null-terminated.
Message-Id: <AANLkTikG-t3Ww-y_nA6sHedZd6sL0yae9ZtDYEamjF5m at mail.gmail.com>
Date: Sun, 18 Jul 2010 17:53:20 +0200

the explanations for the bug fix look really strange
all the printf use %4.4s form, which will limit output to only 4 chars,
whatever the string is terminated or not
so the crash doesn't come from here
looking quickly at the patch, several remark arise
- in the second & third modification to the TRACE call, are you sure the
test to srcType is relevant (I don't have the code handy, but it may be a
wrong cut & paste)
- more importantly, as the %4.4s form implies a 4 char strings, the strings
used in case of NULL value must be 4 characters long, instead we may get
into trouble
A+

2010/7/18 Tim Cadogan-Cowper <tccowper at yahoo.com.au>

> Three TRACE debug calls in the mmioDescend function of mmio.c attempt
> to print FOURCC data, by directly casting it to LPCSTR.  This is
> dangerous as: (i) FOURCC data types are DWORDs, i.e. four bytes (one for
> each char), with no null terminating byte (see
> http://msdn.microsoft.com/en-us/library/dd375802%28VS.85%29.aspx); and
> (ii) it assumes the byte order on the host architecture is little  endian.
>
> Because the LPCSTR casts are not necessarily null-terminated there will
> occasionally be buffer overflows in mmio's TRACE debug strings, with a
> consequent crash to desktop and debug output beginning with
>  'wine_dbg_vprintf:
> debugstr buffer overflow...'  For a consistent and predictable case  where
> this
> occurs, see Bug 10280 "Oblivion: Horse Armour Crash"
> (http://bugs.winehq.org/show_bug.cgi?id=10280).
>
> This patch fixes the problem (and any crashes caused by it) by using a
> new internal function in mmio - mmioFOURCCToString - which converts  FOURCC
> data to strings that are correctly null-terminated and respect byte  order.
> I have tested with Oblivion and can confirm it fixes Bug 10280 on my
> machine (ubuntu 10.04).  Furthermore the patch will ensure that chunk  IDs
> and
> fourccTypes are correctly printed in the mmio debug channel (at the  moment
> they are not).
>
> Tim
>
>
>
>
>
>


-- 
-- 
Eric Pouech


      
-------------- next part --------------
An embedded and charset-unspecified text was scrubbed...
Name: TraceWithMethodComparison.txt
URL: <http://www.winehq.org/pipermail/wine-patches/attachments/20100719/24c549e8/attachment-0001.txt>
-------------- next part --------------
An embedded and charset-unspecified text was scrubbed...
Name: mmio.c
URL: <http://www.winehq.org/pipermail/wine-patches/attachments/20100719/24c549e8/attachment-0001.asc>