Re: [dlls/msi/*] Strncpy elimination.

[Jakob Eriksson]

Good thing you sent these patches.  I was just thinking, should I dig 
in? :-)

Peter Berg Larsen wrote:
> I have been checking the usage of strncpy, replacing where apropriate with
> a memcpy or lstrcpyn[AW]. The first raw diff was 100kb, so there is bound
> to be one or two slips. These are the first batch which I found to be
> obvious, correct, and didnt need a special comment. Note with correct I
> mean if there was a \0 bug before then it still there.
>
> Changelog:
>    Janitorial Task: Check the usage of strncpy/strncpyW.
> Index: dlls/msi/action.c
>
> RCS file: /home/wine/wine/dlls/msi/action.c,v
> retrieving revision 1.104
> diff -u -r1.104 action.c
> --- dlls/msi/action.c	24 Mar 2005 21:01:38 -0000	1.104
> +++ dlls/msi/action.c	26 Mar 2005 09:40:49 -0000
> @@ -922,7 +922,7 @@
>                 while (*ptr == ' ') ptr++;
>                 len = ptr2-ptr;
>                 prop = HeapAlloc(GetProcessHeap(),0,(len+1)*sizeof(WCHAR));
> -
>            strncpyW(prop,ptr,len);
> -
>            memcpy(prop,ptr,len*sizeof(WCHAR));
>           prop[len]=0;
>           ptr2++;
>
> @@ -942,7 +942,7 @@
>                     len -= 2;
>                 }
>                 val = HeapAlloc(GetProcessHeap(),0,(len+1)*sizeof(WCHAR));
> -
>            strncpyW(val,ptr2,len);
> -
>            memcpy(val,ptr2,len*sizeof(WCHAR));
>           val[len] = 0;
>
>           if (strlenW(prop) > 0)
> Index: dlls/msi/dialog.c
>
> RCS file: /home/wine/wine/dlls/msi/dialog.c,v
> retrieving revision 1.9
> diff -u -r1.9 dialog.c
> --- dlls/msi/dialog.c	24 Mar 2005 15:09:31 -0000	1.9
> +++ dlls/msi/dialog.c	26 Mar 2005 09:40:51 -0000
> @@ -131,7 +131,7 @@
>     ret = HeapAlloc( GetProcessHeap(), 0, len*sizeof(WCHAR) );
>     if( !ret )
>         return ret;
> - strncpyW( ret, p, len );
> - memcpy( ret, p, len*sizeof(WCHAR) );
> ret[len-1] = 0;
> return ret;
>
> }
> Index: dlls/msi/format.c
> ===================================================================
> RCS file: /home/wine/wine/dlls/msi/format.c,v
> retrieving revision 1.8
> diff -u -r1.8 format.c
> --- dlls/msi/format.c	16 Mar 2005 11:31:35 -0000	1.8
> +++ dlls/msi/format.c	26 Mar 2005 09:40:51 -0000
> @@ -252,7 +252,7 @@
>     *key = HeapAlloc(GetProcessHeap(),0,i*sizeof(WCHAR));
>     /* do not have the [] in the key */
>     i -= 1;
> - strncpyW(*key,&(*mark)[1],i);
> -
>
> memcpy(*key,&(*mark)[1],i*sizeof(WCHAR));
> (*key)[i] = 0;
>
> TRACE("Found Key %s\n",debugstr_w(*key));

[dlls/msi/*] Strncpy elimination.

[Peter Berg Larsen]

On Mon, 28 Mar 2005, Jakob Eriksson wrote:
> Good thing you sent these patches.  I was just thinking, should I dig
> in? :-)

You could recheck them? I tend to get code blind, when having so many
cases that nearly are identical. As I said there are probably one two
places where I didnt get the sematic of the code. Especially look for
off-by-one (\0) when the replacement is a memcpy. I tend to do this,
because of codestyle, like:
> > +++ dlls/msi/dialog.c	26 Mar 2005 09:40:51 -0000
> > @@ -131,7 +131,7 @@
> >     ret = HeapAlloc( GetProcessHeap(), 0, len*sizeof(WCHAR) );
> >     if( !ret )
> >         return ret;
> > - strncpyW( ret, p, len );
> > - memcpy( ret, p, len*sizeof(WCHAR) );
> > ret[len-1] = 0;
> > return ret;
> >
> > }

This memcpyies one element more than needed.. but seemed much nicer than
memcpy( ret, p, (len - 1)*sizeof(WHCAR);

I probably should have subtracted one from len, making it obvious
that this is a strdupW, like:
  ret = HeapAlloc( GetProcessHeap(), 0, (len+1)*sizeof(WCHAR) );
  memcpy( ret, p, len*sizeof(WCHAR) );
  ret[len] = 0;

Peter

[dlls/msi/*] Strncpy elimination.

[Alexandre Julliard]

Peter Berg Larsen  writes:
> This memcpyies one element more than needed.. but seemed much nicer than
> memcpy( ret, p, (len - 1)*sizeof(WHCAR);
>
> I probably should have subtracted one from len, making it obvious
> that this is a strdupW, like:
>   ret = HeapAlloc( GetProcessHeap(), 0, (len+1)*sizeof(WCHAR) );
>   memcpy( ret, p, len*sizeof(WCHAR) );
>   ret[len] = 0;

If it's really a strdup it's cleaner to copy one more char and get rid
of the ret[len] = 0.

[dlls/msi/*] Strncpy elimination.

[Peter Berg Larsen]

On 28 Mar 2005, Alexandre Julliard wrote:
> > This memcpyies one element more than needed.. but seemed much nicer than
> > memcpy( ret, p, (len - 1)*sizeof(WHCAR);
> >
> > I probably should have subtracted one from len, making it obvious
> > that this is a strdupW, like:
> >   ret = HeapAlloc( GetProcessHeap(), 0, (len+1)*sizeof(WCHAR) );
> >   memcpy( ret, p, len*sizeof(WCHAR) );
> >   ret[len] = 0;
>
> If it's really a strdup it's cleaner to copy one more char and get rid
> of the ret[len] = 0.

Yes, and I would have done that, but in this case p is not \0 terminated.
I probably left it like that because I didnt understand why it copied one
byte to much in the strncpy case.

That is exactly the reason for sending the patches one dll at the time:
For authers/maintainers have a quick look at the replaces code. As I said
in the in the beginning; if there was a bug, then it still there (but
more obvious).

Peter