Re: CURSORICON_SimulateLoadingFromResourceW param check. VFP6 EXEs now work.

[Uwe Bonnes]

> > > > > "C" == C Daniel Mojoli B  writes:

C> Add minimal parameter validation to function
    C> CURSORICON_SimulateLoadingFromResourceW(...). We check that the icon
Providing a test case for such special behaviour is a good idea...

CURSORICON_SimulateLoadingFromResourceW param check. VFP6 EXEs now work.

[C. Daniel Mojoli B.]

Uwe Bonnes wrote:
> > > > > > "C" == C Daniel Mojoli B  writes:
>
> C> Add minimal parameter validation to function
>    C> CURSORICON_SimulateLoadingFromResourceW(...). We check that the icon
> Providing a test case for such special behaviour is a good idea...

I created this patch as a result of consulting I performed at a 
customer's site. I no longer have access to the winedbg info I had 
there, so I can't provide a test. The situation is very simple to 
describe. Essentially this is what happens:

1) VFP6 runtime encounters a command that requires loading a resource, 
e.g. MODIFY SCREEN to put some background image.

2) The VFP6 runtime extracts the resource from the APP or EXE and places 
it in a temp file, e.g. 12345678.TMP.

3) Since the VFP6 runtime cannot use file extension information as it 
would if you were DOing SOME.PRG, it decides to try different ways of 
loading it. At some point before hitting the right loading function it 
tests whether the resource is a cursor icon by calling 
LoadImageW(type=IMAGE_CURSOR). This is IMHO, a terrible way to program, 
depending on failure semantics.

4) Now we are in Wine turf for the first time and LoadImageW -> 
CURSORICON_Load -> CURSORICON_SimulateLoadingFromResourceW.

5) CURSORICON_SimulateLoadingFromResourceW maps the file to memory 
(cursoricon.c:483), then tests if the file is a RIFF file 
(cursoricon.c:488).

6) Execution continues until L507, where it is assumed the mapped file 
is of icon structure. Here Wine tests if the number of icon entries != 
0, but our illegal file (JPEG, GIF, etc) hardly has a zero byte value at 
all, so we pass the test.

7) We hit L513 and cause an exception when executing the following 
expression: bits->idEntries[i]. The problem is that we are looping with 
our index bound to the garbage number of icon entries! That garbage is 
almost assured to be too large and we loop past the assigned memory.

8) Exception!

I created the patch using exactly the same technique L488 uses to test 
for the RIFF case. I simply further test if that magic corresponds to 
icon file magic, right between steps 5 and 6, before any other file data 
is used.

Even if I hadn't encountered a problem with VFP6, I consider the patch 
should be applied on the grounds it simply checks the magic of  file to 
conform to what is should be.

CURSORICON_SimulateLoadingFromResourceW param check. VFP6 EXEs now work.

[Alexandre Julliard]

"C. Daniel Mojoli B."  writes:
> - We hit L513 and cause an exception when executing the following
>
> expression: bits->idEntries[i]. The problem is that we are looping
> with our index bound to the garbage number of icon entries! That
> garbage is almost assured to be too large and we loop past the
> assigned memory.

The proper fix is to make sure the code doesn't crash no matter what
garbage it gets as input. Checking the magic number doesn't guarantee
that the rest of the file is correct.

CURSORICON_SimulateLoadingFromResourceW param check. VFP6 EXEs now work.

[C. Daniel Mojoli B.]

Alexandre Julliard wrote:
> "C. Daniel Mojoli B."  writes:
> > - We hit L513 and cause an exception when executing the following
> >
> > expression: bits->idEntries[i]. The problem is that we are looping
> > with our index bound to the garbage number of icon entries! That
> > garbage is almost assured to be too large and we loop past the
> > assigned memory.
>
> The proper fix is to make sure the code doesn't crash no matter what
> garbage it gets as input. Checking the magic number doesn't guarantee
> that the rest of the file is correct.

I agree with the principle, but still advocate the inclusion of the 
magic number patch because it allows to fail-fast. Even when someone 
eventually patches for complete validity and bounds checking, the 
fail-fast magic number test remains useful.

(And yes, Uwe, your test reflected the condition I encountered. Sorry 
for not answering earlier.)

CURSORICON_SimulateLoadingFromResourceW param check. VFP6 EXEs now work.

[Alexandre Julliard]

"C. Daniel Mojoli B."  writes:
> I agree with the principle, but still advocate the inclusion of the
> magic number patch because it allows to fail-fast. Even when someone
> eventually patches for complete validity and bounds checking, the
> fail-fast magic number test remains useful.

I don't think failing fast is important at all; on the contrary, the
normal case is that the icon file is valid, and making that case
slower to make the rare case faster is not a good trade-off.