Re: [ros-dev] [ros-diffs] 01/01: [WINLOGON] Clean up part 2 - Replace the UNICODE_STRING usMessage by a PWSTR pszMessage. - Use the "%02d:%02d:%02d" time format and get rid of the safe string printf because the string will NEVER be longer than 8 characters. - Rename the timer id constant because it is NOT a resource id. - Rename variables according to the coding style.

[Eric Kohl]

Hello Thomas,

you're right, using the run-time size checks are a good way to keep
application from crashing because of buffer overflows. They'll just keep
on using corrupt data instead! If you want to fix this problem: Don't
use C! Use C++, C#, Java etc. instead!

I prefer to see an application crash because of a buffer overflow rather
than seeing it store truncated phone numbers in a database.

PS: If the timeout is longer than a day, winlogon uses the "%d days"
format. In the end, a buffer of 10 characters is still large enough.

PPS: I'll keep using the old functions until you remove them from the
runtime code.


Regards
Eric

Am 02.04.2018 um 14:12 schrieb Thomas Faber:
>   Hey Eric,
>  
>  On 2018-04-02 12:58, Eric Kohl wrote:
> >   -    RtlStringCbPrintfW(strbuf, sizeof(strbuf),
> > L"%d:%d:%d", hours,
> >  minutes, seconds);
> >  +    swprintf(szBuffer, L"%02d:%02d:%02d", iHours, iMinutes, iSeconds); 
>
>  Unfortunately I must disagree with this change.
>  
>  Buffer overflows are a big enough threat that code review and
>  static analysis are not generally considered sufficient to protect
>  against them.
>  So it's best practice for new code to always verify sizes at run-time,
>  and never to use s(w)print.
>  
>  Best regards,
>  Thomas
>  
>  PS: from what I see, iHours can be as large as 1193046, which won't
>      fit in 2 digits
>  
>  _______________________________________________
>  Ros-dev mailing list
>  Ros-dev(a)reactos.org
>  http://www.reactos.org/mailman/listinfo/ros-dev