*scanf, wrong types in va_arg, and strict aliasing

*scanf, wrong types in va_arg, and strict aliasing

[Pascal Cuoq]

Hello again,


the scanf implementation does the same thing as the printf implementation, in vfscanf.c, line 110:

https://git.musl-libc.org/cgit/musl/tree/src/stdio/vfscanf.c?id=54807d47acecab778498ced88ce8f62bfa16e379#n110

This line is eventually reached, for instance, for the snippet:

int n; sscanf("0", "%d", &n);

And the argument being consumed has type int*. This is not a case that 7.16.1.1:2 allows.


Besides, since I am reviewing this file, and since I was originally interested in analyzing it for strict aliasing violations (although the analyzer is not ready to handle this file), I should point out the function store_int at line 22 and the way it is used at line 146:

https://git.musl-libc.org/cgit/musl/tree/src/stdio/vfscanf.c?id=54807d47acecab778498ced88ce8f62bfa16e379#n22

https://git.musl-libc.org/cgit/musl/tree/src/stdio/vfscanf.c?id=54807d47acecab778498ced88ce8f62bfa16e379#n146

This will not go down well with the strict aliasing analyzer, when it is finally ready for this sort of code. And indeed, compiling the previous scanf snippet together with musl's source code while enabling link-time optimization could plausibly produce non-working binary code because of the type-based alias analysis.

For this latter problem, use of types with the may_alias attribute will fix it for GCC and Clang. For the problem with va_arg, I do not see any easy way out. It might be possible to write a longer version that a modern compiler miraculously recognizes as doing the same thing over and over and folds back into a single sequence of instructions, but without having tried it yet, this seems a bit far-fetched.


Pascal

Re: *scanf, wrong types in va_arg, and strict aliasing

[Pascal Cuoq]

> On 10 Apr 2017, at 11:31, Pascal Cuoq <cuoq@trust-in-soft.com> wrote:
> 
> Besides, since I am reviewing this file, and since I was originally interested in analyzing it for strict aliasing violations (although the analyzer is not ready to handle this file), I should point out the function store_int at line 22 and the way it is used at line 146:
> 
> https://git.musl-libc.org/cgit/musl/tree/src/stdio/vfscanf.c?id=54807d47acecab778498ced88ce8f62bfa16e379#n22
> 
> https://git.musl-libc.org/cgit/musl/tree/src/stdio/vfscanf.c?id=54807d47acecab778498ced88ce8f62bfa16e379#n146
> 
> This will not go down well with the strict aliasing analyzer, when it is finally ready for this sort of code. And indeed, compiling the previous scanf snippet together with musl's source code while enabling link-time optimization could plausibly produce non-working binary code because of the type-based alias analysis.

Well I just got the analyzer to accept the file, and while emitting tons of false positives that it shouldn't, by not warning for store_int it reminded me that writing a signed or unsigned version of the intended integer type does not break “strict aliasing” as described in 6.5:7. The code assumes that size_t is typedef'd to long at line 139, but I no longer think there is has to be any strict aliasing problem here.

Please disregard the second half of my previous message.

Re: *scanf, wrong types in va_arg, and strict aliasing

[Rich Felker]

On Mon, Apr 10, 2017 at 09:31:48AM +0000, Pascal Cuoq wrote:
> Hello again,
> 
> 
> the scanf implementation does the same thing as the printf implementation, in vfscanf.c, line 110:
> 
> https://git.musl-libc.org/cgit/musl/tree/src/stdio/vfscanf.c?id=54807d47acecab778498ced88ce8f62bfa16e379#n110
> 
> This line is eventually reached, for instance, for the snippet:
> 
> int n; sscanf("0", "%d", &n);
> 
> And the argument being consumed has type int*. This is not a case that 7.16..1.1:2 allows.

Indeed, that's wrong, but POSIX makes it hard or even impossible to do
right because of the %n$ localization forms. Whereas printf's
requirement for these is:

    "When numbered argument specifications are used, specifying the
    Nth argument requires that all the leading arguments, from the
    first to the (N-1)th, are specified in the format string."

scanf's requirement is a bit different:

    "When numbered argument specifications are used, specifying the
    Nth argument requires that all the leading arguments, from the
    first to the (N-1)th, are pointers."

This means that use of %2$ without a %1$ is permitted as long as a
pointer (what kind?!) is passed in the first variadic position after
the format string.

I think the best we could do is to _try_ to do it right, by tracking
the types like we do for printf, and only fallback to generic void*
when a conversion specifier is missing. But that's a lot of extra code
needed for something with no user-visible improvements.

Alternatively, we could write a compiler-barrier in trivial asm to
wrap an external va_arg, which is ugly but would also be useful in
places like fcntl/ioctl that might read variadic args with unknown
type to pass to the kernel.

Any opinions on what we should do?

Rich

Re: *scanf, wrong types in va_arg, and strict aliasing

[Rich Felker]

On Mon, Apr 10, 2017 at 09:51:01AM +0000, Pascal Cuoq wrote:
> 
> > On 10 Apr 2017, at 11:31, Pascal Cuoq <cuoq@trust-in-soft.com> wrote:
> > 
> > Besides, since I am reviewing this file, and since I was originally interested in analyzing it for strict aliasing violations (although the analyzer is not ready to handle this file), I should point out the function store_int at line 22 and the way it is used at line 146:
> > 
> > https://git.musl-libc.org/cgit/musl/tree/src/stdio/vfscanf.c?id=54807d47acecab778498ced88ce8f62bfa16e379#n22
> > 
> > https://git.musl-libc.org/cgit/musl/tree/src/stdio/vfscanf.c?id=54807d47acecab778498ced88ce8f62bfa16e379#n146
> > 
> > This will not go down well with the strict aliasing analyzer, when it is finally ready for this sort of code. And indeed, compiling the previous scanf snippet together with musl's source code while enabling link-time optimization could plausibly produce non-working binary code because of the type-based alias analysis.
> 
> Well I just got the analyzer to accept the file, and while emitting
> tons of false positives that it shouldn't, by not warning for
> store_int it reminded me that writing a signed or unsigned version
> of the intended integer type does not break “strict aliasing” as
> described in 6.5:7. The code assumes that size_t is typedef'd to
> long at line 139, but I no longer think there is has to be any
> strict aliasing problem here.
> 
> Please disregard the second half of my previous message.

The assumption about size_t (and also ptrdiff_t and intmax_t, I think)
is an issue, though. I think I should just add extra size classes for
those rather than trying to make assumptions or deductions about which
underlying types they're defined as, much like the proposed printf fix.

Rich