Re: [ros-dev] [ros-diffs] [pschweitzer] 62675: [CHARMAP] Use rather wcsncpy(). A bit less safe, but at least, data are copied till possible

[Timo Kreuzer]

I don't really see the reason here. Whether using the secure function or
not is is a question of whether we need null-termination or not.
If we need it the secure function will do the right thing. If we do not
need null-termination the non-secure function does the right thing and
the secure function would not add any security, but simply do the wrong
thing.
MSDN says about the lfFaceName member of the LOGFONT structure "A
null-terminated string that specifies the typeface name of the font. "
So we want it to be null-terminated and adding one more character does
not add anything useful, but will result in a broken structure.
Another difference is that the non-secure function pads the destination
array with nulls, the secure function does - afaik - not. If that is the
reason, I'd prefer zeroing it out before copying the string.

Am 06.04.2014 22:20, schrieb pschweitzer(a)svn.reactos.org:
>   Author: pschweitzer
>  Date: Sun Apr  6 20:20:39 2014
>  New Revision: 62675
>
>  URL: http://svn.reactos.org/svn/reactos?rev=62675&view=rev
>  Log:
>  [CHARMAP]
>  Use rather wcsncpy(). A bit less safe, but at least, data are copied till possible
>
>  Modified:
>      trunk/reactos/base/applications/charmap/lrgcell.c
>      trunk/reactos/base/applications/charmap/map.c
>
>  Modified: trunk/reactos/base/applications/charmap/lrgcell.c
>  URL:
> http://svn.reactos.org/svn/reactos/trunk/reactos/base/applications/charmap/…
>  ==============================================================================
>  --- trunk/reactos/base/applications/charmap/lrgcell.c	[iso-8859-1] (original)
>  +++ trunk/reactos/base/applications/charmap/lrgcell.c	[iso-8859-1] Sun Apr  6 20:20:39
> 2014
>  @@ -48,9 +48,9 @@
>                         hdc);
>   
>               lf.lfCharSet =  DEFAULT_CHARSET;
>  -            wcscpy_s(lf.lfFaceName,
>  -                     sizeof(lf.lfFaceName) / sizeof(lf.lfFaceName[0]),
>  -                     lpFontName);
>  +            wcsncpy(lf.lfFaceName,
>  +                    lpFontName,
>  +                    sizeof(lf.lfFaceName) / sizeof(lf.lfFaceName[0]));
>   
>               hFont = CreateFontIndirectW(&lf);
>   
>
>  Modified: trunk/reactos/base/applications/charmap/map.c
>  URL:
> http://svn.reactos.org/svn/reactos/trunk/reactos/base/applications/charmap/…
>  ==============================================================================
>  --- trunk/reactos/base/applications/charmap/map.c	[iso-8859-1] (original)
>  +++ trunk/reactos/base/applications/charmap/map.c	[iso-8859-1] Sun Apr  6 20:20:39 2014
>  @@ -228,9 +228,9 @@
>       ReleaseDC(infoPtr->hMapWnd, hdc);
>   
>       infoPtr->CurrentFont.lfCharSet =  DEFAULT_CHARSET;
>  -    wcscpy_s(infoPtr->CurrentFont.lfFaceName,
>  -             sizeof(infoPtr->CurrentFont.lfFaceName) /
> sizeof(infoPtr->CurrentFont.lfFaceName[0]),
>  -             lpFontName);
>  +    wcsncpy(infoPtr->CurrentFont.lfFaceName,
>  +            lpFontName,
>  +            sizeof(infoPtr->CurrentFont.lfFaceName) /
> sizeof(infoPtr->CurrentFont.lfFaceName[0]));
>   
>       infoPtr->hFont = CreateFontIndirectW(&infoPtr->CurrentFont);
>   
>
>
>

[Pierre Schweitzer]

Actually, it was based on my observation of:
http://doxygen.reactos.org/dc/d77/wcs_8c_afce6ecc002302f621f1280446ee5ae77.…
http://doxygen.reactos.org/d1/d60/tcsncpy_8h_source.html

My point was not to break program behavior by producing empty strings.
Actually, with current functions, I do. So that's an open question.

My (naive) guess, would be as I did on CDFS: copy len - 1 and 0 last char.

On 07/04/2014 00:08, Timo Kreuzer wrote:
>   I don't really see the reason here. Whether using
> the secure function or
>  not is is a question of whether we need null-termination or not.
>  If we need it the secure function will do the right thing. If we do not
>  need null-termination the non-secure function does the right thing and
>  the secure function would not add any security, but simply do the wrong
>  thing.
>  MSDN says about the lfFaceName member of the LOGFONT structure "A
>  null-terminated string that specifies the typeface name of the font. "
>  So we want it to be null-terminated and adding one more character does
>  not add anything useful, but will result in a broken structure.
>  Another difference is that the non-secure function pads the destination
>  array with nulls, the secure function does - afaik - not. If that is the
>  reason, I'd prefer zeroing it out before copying the string.
>
>  Am 06.04.2014 22:20, schrieb pschweitzer(a)svn.reactos.org:
> >   Author: pschweitzer
> >  Date: Sun Apr  6 20:20:39 2014
> >  New Revision: 62675
> >
> >  URL: http://svn.reactos.org/svn/reactos?rev=62675&view=rev
> >  Log:
> >  [CHARMAP]
> >  Use rather wcsncpy(). A bit less safe, but at least, data are copied till possible
> >
> >  Modified:
> >      trunk/reactos/base/applications/charmap/lrgcell.c
> >      trunk/reactos/base/applications/charmap/map.c
> >
> >  Modified: trunk/reactos/base/applications/charmap/lrgcell.c
> >  URL:
> > http://svn.reactos.org/svn/reactos/trunk/reactos/base/applications/charmap/…
> >  ==============================================================================
> >  --- trunk/reactos/base/applications/charmap/lrgcell.c	[iso-8859-1] (original)
> >  +++ trunk/reactos/base/applications/charmap/lrgcell.c	[iso-8859-1] Sun Apr  6 20:20:39
> > 2014
> >  @@ -48,9 +48,9 @@
> >                         hdc);
> >   
> >               lf.lfCharSet =  DEFAULT_CHARSET;
> >  -            wcscpy_s(lf.lfFaceName,
> >  -                     sizeof(lf.lfFaceName) / sizeof(lf.lfFaceName[0]),
> >  -                     lpFontName);
> >  +            wcsncpy(lf.lfFaceName,
> >  +                    lpFontName,
> >  +                    sizeof(lf.lfFaceName) / sizeof(lf.lfFaceName[0]));
> >   
> >               hFont = CreateFontIndirectW(&lf);
> >   
> >
> >  Modified: trunk/reactos/base/applications/charmap/map.c
> >  URL:
> > http://svn.reactos.org/svn/reactos/trunk/reactos/base/applications/charmap/…
> >  ==============================================================================
> >  --- trunk/reactos/base/applications/charmap/map.c	[iso-8859-1] (original)
> >  +++ trunk/reactos/base/applications/charmap/map.c	[iso-8859-1] Sun Apr  6 20:20:39 2014
> >  @@ -228,9 +228,9 @@
> >       ReleaseDC(infoPtr->hMapWnd, hdc);
> >   
> >       infoPtr->CurrentFont.lfCharSet =  DEFAULT_CHARSET;
> >  -    wcscpy_s(infoPtr->CurrentFont.lfFaceName,
> >  -             sizeof(infoPtr->CurrentFont.lfFaceName) /
> > sizeof(infoPtr->CurrentFont.lfFaceName[0]),
> >  -             lpFontName);
> >  +    wcsncpy(infoPtr->CurrentFont.lfFaceName,
> >  +            lpFontName,
> >  +            sizeof(infoPtr->CurrentFont.lfFaceName) /
> > sizeof(infoPtr->CurrentFont.lfFaceName[0]));
> >   
> >       infoPtr->hFont = CreateFontIndirectW(&infoPtr->CurrentFont);
> >   
> >
> >
> >  
>  _______________________________________________
>  Ros-dev mailing list
>  Ros-dev(a)reactos.org
>  http://www.reactos.org/mailman/listinfo/ros-dev 

-- 
Pierre Schweitzer<pierre at reactos.org>
System Administrator
ReactOS Foundation

[Thomas Faber]

On 2014-04-07 07:58, Pierre Schweitzer wrote:
>   My (naive) guess, would be as I did on CDFS: copy len
> - 1 and 0 last char. 
StringCbCopyW will do just that :)

[Timo Kreuzer]

Oh right, I was wrongly assuming similar behavior, when thinking of
wcscpy_s.


Am 07.04.2014 08:46, schrieb Thomas Faber:
>   On 2014-04-07 07:58, Pierre Schweitzer wrote:
> >   My (naive) guess, would be as I did on CDFS: copy
> > len - 1 and 0 last char. 
>  StringCbCopyW will do just that :)
>
>  _______________________________________________
>  Ros-dev mailing list
>  Ros-dev(a)reactos.org
>  http://www.reactos.org/mailman/listinfo/ros-dev
>