Re: [musl] [BUG] ioctl: overflow in implicit constant conversion

[Olaf Meeuwissen <paddy-hack@member.fsf.org>]

Hi,

Szabolcs Nagy writes:

> * Olaf Meeuwissen <paddy-hack@member.fsf.org> [2020-01-20 19:39:22 +0900]:
>> Hi all,
>>
>> I've been asked[1] to upstream an issue I initially submitted[2] to the
>> Alpinelinux project.
>>
>>  [1]: https://gitlab.alpinelinux.org/alpine/aports/issues/7580#note_63663
>>  [2]: https://gitlab.alpinelinux.org/alpine/aports/issues/7580
>>
>> You can find all the details in that issue[2], but to save you the trip,
>> here's the gist of it.
>>
>>   I get `overflow in implicit constant conversion` compiler warnings on
>>   some `ioctl()` calls in my code.
>
> the overflow warning is valid, but harmless.

Yes, but it is caused by the implementation below, IIUC, not by *my*
code.  AFAICS, there's nothing in my code that I can change to make the
warning go away, no matter how harmless it is.  If it is truly harmless,
then I think the musl code that triggers it ought to be fixed so I don't
end up going down a rabbit hole trying to fix my code when I cannot.

>>   I found
>>
>>   ``` c
>>   int ioctl (int, int, ...)
>>   ```
>>
>>   in `/usr/include/sys/ioctl.h` and the following in
>>   `/usr/include/bits/ioctl.h` (included by the former)
>>
>>   ``` c
>>   #define _IOC(a,b,c,d) ( ((a)<<30) | ((b)<<8) | (c) | ((d)<<16) )
>>   #define _IOC_NONE  0U
>>   #define _IOC_WRITE 1U
>>   #define _IOC_READ  2U
>>
>>   #define _IO(a,b) _IOC(_IOC_NONE,(a),(b),0)
>>   #define _IOW(a,b,c) _IOC(_IOC_WRITE,(a),(b),sizeof(c))
>>   #define _IOR(a,b,c) _IOC(_IOC_READ,(a),(b),sizeof(c))
>>   #define _IOWR(a,b,c) _IOC(_IOC_READ|_IOC_WRITE,(a),(b),sizeof(c))
>>   ```
>>
>>   If I am not mistaken and assuming a 32-bit int, that means that any
>>   second argument passed to `ioctl()` that is created using the `_IOR`
>>   or `_IOWR` macros will be an *unsigned* 32-bit integer with its most
>>   significant bit set and will trigger the warning I observe.

No matter what (non-negative) values for a, b and c you pick, use of
_IOR(a,b,c) will trigger the overflow for 32-bit ints.  That happens
because it expands to

  _IOC( ((2U)<<30) | ((a)<<8) | (b) | ((sizeof(c))<<16) )

as _IOC_READ is "injected" as the first argument to _IOC.  The
((2U<<30)) yields 2147483648 which is already larger than INT_MAX for
32-bit ints.  The remaining bitwise-ORs only make the final value larger
still.

Now, if it were my code that triggered it, fine.  I'd look for a fix in
my code.  However, this is in the musl header files.  I'd expect them to
not trigger spurious warnings (even with a -pedantic flag ;-).

>>   BTW, most other distributions I compile on define ioctl() as
>>
>>   ``` c
>>   int ioctl (int, unsigned long, ...)
>>   ```
>
> this is a conformance bug, since posix specifies int.
>
> one could argue that linux requires unsigned long, but
> the correct way to introduce linux specific apis is to
> give them a different name, not something that clashes
> with standard names that have specified semantics.

If I compile my code on a Linux platform, I'd expect the C library to
adhere to Linux conventions, be they POSIX or otherwise.  If I compile
on a BSD platform, I'd expect the C library to adhere to BSD
conventions, be they POSIX or otherwise.

# As Alexander Monakov pointed out in another follow-up, all BSDs seem
# to do the same thing as Linux ...

> changing the type is an api and abi break in musl.
> we could try to silence the warning instead (e.g via
> explicit cast in _IOC or in an ioctl macro).

That would be much appreciated :bow:

However, given that on a Linux or BSD platform I can pass an unsigned
long, I doubt that the explicit cast will work in the general case if
you were to cast in an ioctl macro.  Putting an explicit cast in _IOC
would be fine though.

>>   and chaging your declaration to take an `unsigned int` “fixes” this
>>   for me.
>>
>> This happens when I compile with `-Wall -pedantic` on x86_64/amd64.
>> When compiling without `-pedantic` the warning goes away.
>>
>> Since my initial report (against musl-1.1.16) the warning has changed to
>> something like
>>
>>   overflow in conversion from 'long unsigned int' to 'int' changes value
>>   from '3221771554' to '-1073195742' [-Woverflow]
>>
>> and I have not checked what happens when `-pedantic` is not given.  Nor
>> have I checked whether my `unsigned int` still “fixes” this.
>>
>> I still see this with 1.1.24.
>>
>> I think this is a bug and would like to see this fixed.
>
> it is worth fixing if the fix does not cause bigger
> problems than the warning we are fixing.

From my point of view, the current musl implementation is logically
incorrect for 32-bit ints.  If this is not a problem in practice, go
ahead an plaster it over with a cast in _IOC.  Long term though, I think
you need to have your ioctl declaration cater to the platform that musl
is used on.

Hope this helps,
--
Olaf Meeuwissen, LPIC-2            FSF Associate Member since 2004-01-27
 GnuPG key: F84A2DD9/B3C0 2F47 EA19 64F4 9F13  F43E B8A4 A88A F84A 2DD9
 Support Free Software                        https://my.fsf.org/donate
 Join the Free Software Foundation              https://my.fsf.org/join