Strange code in misc/registry.c

[Lionel Ulmer]

Hi all,

Having lately some strange crashes when I start Wine with any --debugmsg
command, I started to look at where the problem occured.
> From my investigations, it came in the file misc/registry.c while loading
> 'system.1st' (yeah, I know, I should run in a no-Windows install, but well,
> my Wine installation is 6 years old and still working, so I won't change now
> :-) ).

Anyway, the faulty function is '_strdupnA'.

I added some traces and I have this :

0009:trace:reg:_strdupnA System\CurrentControlSet\Control\MediaProperties\PrivateProperties\Joystick (0x3c003f40) 79
0009:trace:reg:_strdupnA 0x3c003f90

This means that the string given as a parameter has the pointer
'0x3c003f40', the length parameter is '79' and it returns '0x3c003f90'.

It crashes later on at :

0009:trace:reg:_strdupnA System\CurrentControlSet\Control\MediaProperties\PrivateProperties\Joystick\OEM (0x3c003f90) 114
0009:trace:reg:_strdupnA 0x3c003418

So we give the '0x3c003f90' pointer (which is of length 79 as seen in the
previous debug output) but (if you look at the code), will read 114 bytes
from it (as the function uses 'memcpy' and not 'strncpy').

The attached patch fixes all crashes on my box, but well, as I am not very
familiar with this code, sent it to wine-devel and not wine-patches for
review by Registry gurus :-)

            Lionel

-- 
		 Lionel Ulmer - http://www.bbrox.org/

[Dmitry Timoshkov]

"Lionel Ulmer" <lionel.ulmer(a)free.fr> wrote:
> The attached patch fixes all crashes on my box, but well, as I am not very
> familiar with this code, sent it to wine-devel and not wine-patches for
> review by Registry gurus :-)

I'm not a Registry guru by all means :-), but IMO the real bugs are at lines
701:
        new_key_name = _strdupnA(key_name,strlen(key_name)+dkh->keynamelen+1);
and 1011:
        new_key_name = _strdupnA(key_name,strlen(key_name)+nk->name_len+1);

which incorrectly do request to copy too much bytes.

-- 
Dmitry.

[Lionel Ulmer]

> I'm not a Registry guru by all means :-), but IMO the real bugs are at lines
> 701:
>         new_key_name = _strdupnA(key_name,strlen(key_name)+dkh->keynamelen+1);
> and 1011:
>         new_key_name = _strdupnA(key_name,strlen(key_name)+nk->name_len+1);

Not really as the code does this :

        /* create new subkey name */
        new_key_name = _strdupnA(key_name,strlen(key_name)+dkh->keynamelen+1);
        if (strcmp(new_key_name,"") != 0) strcat(new_key_name,"\\");
        strncat(new_key_name,dkh->name,dkh->keynamelen);
		
So basically it does 'duplicate my string but add XXX bytes to it as I want
to strcat to it a new string of len XXX'.

                    Lionel

-- 
		 Lionel Ulmer - http://www.bbrox.org/

[Dmitry Timoshkov]

"Lionel Ulmer" <lionel.ulmer(a)free.fr> wrote:
> Not really as the code does this :
>
> /* create new subkey name */
>         new_key_name = _strdupnA(key_name,strlen(key_name)+dkh->keynamelen+1);
>         if (strcmp(new_key_name,"") != 0) strcat(new_key_name,"\\");
>         strncat(new_key_name,dkh->name,dkh->keynamelen);
>
> So basically it does 'duplicate my string but add XXX bytes to it as I want
> to strcat to it a new string of len XXX'.

It's clearly a bug, since the code asks for trouble by requesting to
read more data than it actually should. In that case the code has to do:

new_key_name = malloc(strlen(key_name)+dkh->keynamelen+1);
strcpy(new_key_name, key_name);
if (strcmp(new_key_name,"") != 0) strcat(new_key_name,"\\");
strncat(new_key_name,dkh->name,dkh->keynamelen);

-- 
Dmitry.

[Lionel Ulmer]

> It's clearly a bug, since the code asks for trouble by requesting to
> read more data than it actually should. In that case the code has to do:
>
> new_key_name = malloc(strlen(key_name)+dkh->keynamelen+1);
> strcpy(new_key_name, key_name);

Well, these two lines are EXACTLY what the strndup code does :-)

          Lionel

-- 
		 Lionel Ulmer - http://www.bbrox.org/

[Dmitry Timoshkov]

"Lionel Ulmer" <lionel.ulmer(a)free.fr> wrote:
> > It's clearly a bug, since the code asks for trouble by requesting to
> > read more data than it actually should. In that case the code has to do:
> >
> > new_key_name = malloc(strlen(key_name)+dkh->keynamelen+1);
> > strcpy(new_key_name, key_name);
>
> Well, these two lines are EXACTLY what the strndup code does :-)

I don't think so. memcpy != strcpy.

-- 
Dmitry.

[Lionel Ulmer]

> > > new_key_name = malloc(strlen(key_name)+dkh->keynamelen+1);
> > > strcpy(new_key_name, key_name);
> >
> > Well, these two lines are EXACTLY what the strndup code does :-)
>
> I don't think so. memcpy != strcpy.

I meant to add 'with my fix attached to my first mail'.

But well, will let the masters fix it (I am currently happpy with the fix I
have in my tree).

       Lionel

-- 
		 Lionel Ulmer - http://www.bbrox.org/