Re: [musl] Potential bug in printf_core

["Érico Nogueira" <ericonr@disroot.org>]

On Fri Aug 6, 2021 at 4:29 PM -03, Pontus Jensen Karlsson wrote:
> On 8/6/21 4:20 PM, Rich Felker wrote:
> > On Fri, Aug 06, 2021 at 03:14:22PM +0200, Pontus Jensen Karlsson wrote:
> >> Hi,
> >>
> >> I've been trying to build audit-userspace tools for an ARMv7 SBC
> >> using musl 1.2.2 as libc.
> >> The tool auditd continously segfaults and I've traced it to a printf
> >> statement that
> >> I have isolated the issue to this piece of code (simplified for
> >> debugging purposes):
> >>
> >> #include <sys/time.h>
> >> #include <stdio.h>
> >>
> >> int main(int argc, char **argv)
> >> {
> >>      struct timeval tv = {
> >>          .tv_sec = 1000,
> >>          .tv_usec = 4177777
> >>      };
> >>      char *str = "Hello World";
> >>      unsigned num = 8062;
> >>
> >>      printf("%lu %03u %u %s", tv.tv_sec, (unsigned)(tv.tv_usec), num, str);
> >> }
> >>
> >> This code segfaults at memchr (s = 0x3fbf71 <error: Cannot access
> >> memory at address 0x3fbf71>)
> >> but three frames up we're at src/stdio/vfprintf.c:593.
> >>
> >> Here it attempts to read the string length from the arg.p address,
> >> the problem is that arg.p points
> >> to the int-value of (unsigned)(tv.tv_usec) and not the memory
> >> address of str.
> >>
> >> So, I'm confused as to why this happens? Is it something weird with
> >> the state-machine in printf_core,
> >> or am I misunderstanding something which needs to be patched into
> >> audit-userspace?
> > You're missing that %lu is not a valid format specifier for time_t.
> > You need to either do %jd and (intmax_t)tv.tv_sec or %lld and (long
> > long)tv.tv_sec.
>
> You are absolutely correct. After changing to %llu it worked flawlessly,
> well I had
> to do it for both tv_sec and tv_usec but after that it works. I also
> read the note on
> the frontpage of musl.libc.org which explained the reason why this had
> to be done.
>
> My question now is, have most C libraries moved to long long unsigned
> for tv_sec,
> i.e. is this portable?

There is only one portable way of dealing with this, and that is casting
the value. You shouldn't worry about the underlying type, only that the
type you're casting to can fit it. This way your program can work with
*all* C libraries, be they new or old (and will even keep working if
someone makes time_t an __int128_t, though you might be losing data in
that case, but a compiler would warn about the narrowing conversion).

As for the rest of your question, glibc 2.34 just released with support
for time64 on 32-bit targets, but it isn't the default yet (requires
some #defines). Hopefully 2.35 or 2.36 will make it the default. The
BSDs have fixed time64 concerns for a long time now.

>
> ~ PJK
>
> >
> > So yes, this seems to be a bug in audit-userspace.
> >
> > Rich