Re: [PATCH] inttostr.h: add compile-time buffer overrun checks

[Jim Meyering]

Bruno Haible wrote:
> Hi Jim,
>
>> @smallexample
>> #undef memcpy
>> #define bos0(dest) __builtin_object_size (dest, 0)
>> #define memcpy(dest, src, n) \
>>   __builtin___memcpy_chk (dest, src, n, bos0 (dest))
>> ...
>> Such built-in functions are provided for @code{memcpy}, @code{mempcpy},
>> @code{memmove}, @code{memset}, @code{strcpy}, @code{stpcpy}, @code{strncpy},
>> @code{strcat} and @code{strncat}.
>>
>> There are also checking built-in functions for formatted output functions.
>> @smallexample
>> int __builtin___sprintf_chk (char *s, int flag, size_t os, const char *fmt, 
>> ...);
>
> Indeed, this __builtin_object_size is useful here, because it works also
> on pointers, whereas sizeof() gives the information only on arrays. By 
> combining
> both, we can get checking in 5 out of 6 cases in the attached sample.
>
> glibc uses __builtin_object_size for its Fortify support, in the files
>   /usr/include/bits/socket2.h
>   /usr/include/bits/stdio2.h
>   /usr/include/bits/stdlib.h
>   /usr/include/bits/string3.h
>   /usr/include/bits/unistd.h
>   /usr/include/bits/wchar2.h
> and conditionalized by
>   #if __USE_FORTIFY_LEVEL > 0
>
> There are many functions to which 'char *' pointers are being passed. For
> memcpy, memset, memmove, and so on, there is already a size_t argument. The
> kinds of errors that the *_chk functions can check for these are only really
> dumb beginner mistakes. It's OK for glibc to do that, but I think it's not
> necessary for gnulib.
>
> But for functions that don't take a size_t argument, such as
>   strcat
>   readlink
>   realpath
>   forkpty
>   openpty
>   sprintf, u*_sprintf
>   vsprintf, u*_vsprintf
>   mkdtemp
>   mkostemp
>   mkstemp
>   mkstemps
>   mkstemp_safer
>   mkostemp_safer
>   mkostemps_safer
>   mkstemps_safer
>   inttostr
> these *_chk functions can point out some unobvious mistakes.
>
> Among these, gnulib doesn't replace most of them on glibc systems, and we need
> the warnings only on glibc systems. So what we need to handle in gnulib is 
> only
> essentially
>   u*_sprintf
>   u*_vsprintf
>   mkstemp_safer
>   mkostemp_safer
>   mkostemps_safer
>   mkstemps_safer
>   inttostr
>
> Let's start with inttostr. Using the same technique as glibc in its
> /usr/include/bits/unistd.h file, I arrive at the attached code, which produces
> a compile-time warning when possible and calls the _chk function when it 
> cannot
> prove that the bound it sufficient.
>
> It may look complicated, but it boils down to
>   - a bit of macrology and inline functions in inttostr.h,
>   - a function inttostr_chk that checks the bound,
>   - a use of gl_ASM_SYMBOL_PREFIX in the module description.
>
> Should we pursue this path?

Nice and thorough work.  Thanks for covering almost all of the
remaining cases.  Anyone who manages to sneak through the remaining
loophole is almost guaranteed to be doing it deliberately.