New static analysis results

New static analysis results

[Alexander Monakov]

Hello,

I'm happy to report a few new results from running static code analysis on
musl (from a tool developed where I work).

ctime.c:5
    localtime(t) may return NULL, but that will cause UB in asctime

regexec.c:253
    "return REG_NOMATCH;" in GET_NEXT_WCHAR leaks memory allocated for 'buf'

lookup_serv.c:55
getnameinfo.c:99
    pointless "if (!p) continue;" when "if (!*p) continue;" was probably
    intended

fpathconf.c
    off-by-one error in range check (if (name >= sizeof ...))

Alexander

Re: New static analysis results

[Rich Felker]

On Thu, Sep 04, 2014 at 08:45:45PM +0400, Alexander Monakov wrote:
> Hello,
> 
> I'm happy to report a few new results from running static code analysis on
> musl (from a tool developed where I work).
> 
> ctime.c:5
>     localtime(t) may return NULL, but that will cause UB in asctime

Yes, I need to look into what ctime should do in this case though...

> regexec.c:253
>     "return REG_NOMATCH;" in GET_NEXT_WCHAR leaks memory allocated for 'buf'

This should be checked, but it sounds likely.

> lookup_serv.c:55
> getnameinfo.c:99
>     pointless "if (!p) continue;" when "if (!*p) continue;" was probably
>     intended

I'd have to look at the code but it's possible the intent was leftover
from old code that was changed rather than being what you think. But I
think your proposed change is probably right for the current code.
Looks low-priority anyway (only affects parsing invalid hosts/services
files).

> fpathconf.c
>     off-by-one error in range check (if (name >= sizeof ...))

Indeed. This should be fixed.

Rich

Re: New static analysis results

[Rich Felker]

On Thu, Sep 04, 2014 at 01:13:58PM -0400, Rich Felker wrote:
> On Thu, Sep 04, 2014 at 08:45:45PM +0400, Alexander Monakov wrote:
> > Hello,
> > 
> > I'm happy to report a few new results from running static code analysis on
> > musl (from a tool developed where I work).
> > 
> > ctime.c:5
> >     localtime(t) may return NULL, but that will cause UB in asctime
> 
> Yes, I need to look into what ctime should do in this case though...

Found it:

  7.27.3.2 The ctime function

  2 The ctime function converts the calendar time pointed to by timer
  to local time in the form of a string. It is equivalent to

           asctime(localtime(timer))

The standard basically specifies the implementation, so it's clearly
UB if localtime(t) would return a null pointer. Looks like no action
is needed here; the most-desirable-behavior (crash) for UB happens
automatically anyway.

> > regexec.c:253
> >     "return REG_NOMATCH;" in GET_NEXT_WCHAR leaks memory allocated for 'buf'
> 
> This should be checked, but it sounds likely.

nsz is looking into fixing it.

> > lookup_serv.c:55
> > getnameinfo.c:99
> >     pointless "if (!p) continue;" when "if (!*p) continue;" was probably
> >     intended
> 
> I'd have to look at the code but it's possible the intent was leftover
> from old code that was changed rather than being what you think. But I
> think your proposed change is probably right for the current code.
> Looks low-priority anyway (only affects parsing invalid hosts/services
> files).

Digging up the history was confusing so I'm just fixing them based on
the current code.

For lookup_serv.c, the line was a nop and is not needed. For
getnameinfo.c it seems to be an actual bug that could cause reading
past the end of the line buffer (but not write).

> > fpathconf.c
> >     off-by-one error in range check (if (name >= sizeof ...))
> 
> Indeed. This should be fixed.

Fixing.

Rich

Re: New static analysis results

[Alexander Monakov]

On Fri, 5 Sep 2014, Rich Felker wrote:
> > > ctime.c:5
> > >     localtime(t) may return NULL, but that will cause UB in asctime
> > 
> > Yes, I need to look into what ctime should do in this case though...
> 
> Found it:
> 
>   7.27.3.2 The ctime function
> 
>   2 The ctime function converts the calendar time pointed to by timer
>   to local time in the form of a string. It is equivalent to
> 
>            asctime(localtime(timer))
> 
> The standard basically specifies the implementation, so it's clearly
> UB if localtime(t) would return a null pointer. Looks like no action
> is needed here; the most-desirable-behavior (crash) for UB happens
> automatically anyway.

I suspect what happened is, at some point localtime was not specified to
return NULL and set errno, and at that time it made perfect sense to specify
asctime as you quoted, and then at some later point localtime specification
was expanded with error cases, but asctime specification was not adjusted.
Is that possible?

It doesn't look very nice for a libc to invoke UB where it could easily
propagate error to the caller, but "that's exactly what the standard requires"
can't be argued with I guess.

Alexander

Re: New static analysis results

[Rich Felker]

On Fri, Sep 05, 2014 at 10:39:45PM +0400, Alexander Monakov wrote:
> On Fri, 5 Sep 2014, Rich Felker wrote:
> > > > ctime.c:5
> > > >     localtime(t) may return NULL, but that will cause UB in asctime
> > > 
> > > Yes, I need to look into what ctime should do in this case though...
> > 
> > Found it:
> > 
> >   7.27.3.2 The ctime function
> > 
> >   2 The ctime function converts the calendar time pointed to by timer
> >   to local time in the form of a string. It is equivalent to
> > 
> >            asctime(localtime(timer))
> > 
> > The standard basically specifies the implementation, so it's clearly
> > UB if localtime(t) would return a null pointer. Looks like no action
> > is needed here; the most-desirable-behavior (crash) for UB happens
> > automatically anyway.
> 
> I suspect what happened is, at some point localtime was not specified to
> return NULL and set errno, and at that time it made perfect sense to specify
> asctime as you quoted, and then at some later point localtime specification
> was expanded with error cases, but asctime specification was not adjusted.
> Is that possible?
> 
> It doesn't look very nice for a libc to invoke UB where it could easily
> propagate error to the caller, but "that's exactly what the standard requires"
> can't be argued with I guess.

See also asctime: it's even worse, specified to be UB, via potential
buffer overflow, if the values are outside of the expected range.

These functions really just should not be used for anything. Short of
rolling your own, strftime is the only correct way to format time as a
string.

At some point it would be nice to make a big list of standard C
functions that are utterly unusable due to UB on errors. Unusable due
to lack of thread safety is another big area, too.

Rich

Re: New static analysis results

[Jens Gustedt]

[-- Attachment #1: Type: text/plain, Size: 1272 bytes --]

Am Freitag, den 05.09.2014, 14:53 -0400 schrieb Rich Felker:
> See also asctime: it's even worse, specified to be UB, via potential
> buffer overflow, if the values are outside of the expected range.
> 
> These functions really just should not be used for anything. Short of
> rolling your own, strftime is the only correct way to format time as a
> string.

the corresponding xxx_s functions from Annex K are a bit better, here.

> At some point it would be nice to make a big list of standard C
> functions that are utterly unusable due to UB on errors. Unusable due
> to lack of thread safety is another big area, too.

Annex K can basically be read as such a list (for C itself, not POSIX)
and gives replacements for them, I think.

Implementing these functions, using them with a constraint handler
that is set to ignore_handler_s, and checking for the return values of
the functions is a realistic alternative to all this UB stuff.

Jens

-- 
:: INRIA Nancy Grand Est ::: AlGorille ::: ICube/ICPS :::
:: ::::::::::::::: office Strasbourg : +33 368854536   ::
:: :::::::::::::::::::::: gsm France : +33 651400183   ::
:: ::::::::::::::: gsm international : +49 15737185122 ::
:: http://icube-icps.unistra.fr/index.php/Jens_Gustedt ::



[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

Re: New static analysis results

[Rich Felker]

On Fri, Sep 05, 2014 at 10:50:52PM +0200, Jens Gustedt wrote:
> Am Freitag, den 05.09.2014, 14:53 -0400 schrieb Rich Felker:
> > See also asctime: it's even worse, specified to be UB, via potential
> > buffer overflow, if the values are outside of the expected range.
> > 
> > These functions really just should not be used for anything. Short of
> > rolling your own, strftime is the only correct way to format time as a
> > string.
> 
> the corresponding xxx_s functions from Annex K are a bit better, here.
> 
> > At some point it would be nice to make a big list of standard C
> > functions that are utterly unusable due to UB on errors. Unusable due
> > to lack of thread safety is another big area, too.
> 
> Annex K can basically be read as such a list (for C itself, not POSIX)
> and gives replacements for them, I think.

I don't think so. Out of Annex K, here are the functions that replace
a fundamentally unusable (due to UB) function with the same name minus
the final _s (5):

sprintf_s (but snprintf already did it better)
vsprintf_s (ditto)
gets_s (but gets was removed, and fgets already did it better)
asctime_s (but strftime already did it better)
ctime_s (ditto)

So from this first group, I would call them all utterly useless
replacements -- they're replacing things that already had better
replacements.

Second, the ones which replace a function that's unusable due to lack
of thread-safety (5):

strtok_s
strerror_s
wcstok_s
gmtime_s
localtime_s

For all of these, POSIX already replaced them with _r versions; the _s
versions are mostly gratuitous duplicates with the same or worse
interfaces. So they're "useful" only for plain-C without POSIX.

And next, the ones which replace a function that's usable in a
thread-safe manner, but lacks a context, necessitating the use of TLS
for context (2):

bsearch_s
qsort_s

These seem like welcome additions.

And finally, the ones which have nothing to do with fixing a problem
in the function they 'replace' (52):

tmpfile_s
tmpnam_s
fopen_s
freopen_s
fprintf_s
fscanf_s
printf_s
scanf_s
snprintf_s
sscanf_s
vfprintf_s
vfscanf_s
vprintf_s
vscanf_s
vsnprintf_s
vsscanf_s
getenv_s
wctomb_s
mbstowcs_s
wcstombs_s
memcpy_s
memmove_s
strcpy_s
strncpy_s
strcat_s
strncat_s
memset_s
strnlen_s
fwprintf_s
fwscanf_s
snwprintf_s
swprintf_s
swscanf_s
vfwprintf_s
vfwscanf_s
vsnwprintf_s
vswprintf_s
vswscanf_s
vwprintf_s
vwscanf_s
wprintf_s
wscanf_s
wcscpy_s
wcsncpy_s
wmemcpy_s
wmemmove_s
wcscat_s
wcsncat_s
wcsnlen_s
wcrtomb_s
mbsrtowcs_s
wcsrtombs_s

> Implementing these functions, using them with a constraint handler
> that is set to ignore_handler_s, and checking for the return values of
> the functions is a realistic alternative to all this UB stuff.

Setting constraint handler is not thread-safe or library-safe, so it's
not a practical solution.

Rich