Follow @Openwall on Twitter for new release announcements and other news
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date: Mon, 20 Jan 2020 13:09:30 +0100
From: Szabolcs Nagy <nsz@...t70.net>
To: Olaf Meeuwissen <paddy-hack@...ber.fsf.org>
Cc: musl@...ts.openwall.com
Subject: Re: [BUG] ioctl: overflow in implicit constant conversion

* Olaf Meeuwissen <paddy-hack@...ber.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.

>   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.
> 
>   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.

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).

> 
>   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.

Powered by blists - more mailing lists

Confused about mailing lists and their use? Read about mailing lists on Wikipedia and check out these guidelines on proper formatting of your messages.