* [musl] [BUG] ioctl: overflow in implicit constant conversion @ 2020-01-20 10:39 Olaf Meeuwissen 2020-01-20 12:09 ` Szabolcs Nagy 0 siblings, 1 reply; 6+ messages in thread From: Olaf Meeuwissen @ 2020-01-20 10:39 UTC (permalink / raw) To: musl 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. 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, ...) ``` 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. 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [musl] [BUG] ioctl: overflow in implicit constant conversion 2020-01-20 10:39 [musl] [BUG] ioctl: overflow in implicit constant conversion Olaf Meeuwissen @ 2020-01-20 12:09 ` Szabolcs Nagy 2020-01-20 12:57 ` Alexander Monakov ` (2 more replies) 0 siblings, 3 replies; 6+ messages in thread From: Szabolcs Nagy @ 2020-01-20 12:09 UTC (permalink / raw) To: Olaf Meeuwissen; +Cc: musl * 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. > 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. ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [musl] [BUG] ioctl: overflow in implicit constant conversion 2020-01-20 12:09 ` Szabolcs Nagy @ 2020-01-20 12:57 ` Alexander Monakov 2020-01-20 17:14 ` Rich Felker 2020-01-20 13:08 ` Alexander Monakov 2020-01-21 12:27 ` Olaf Meeuwissen 2 siblings, 1 reply; 6+ messages in thread From: Alexander Monakov @ 2020-01-20 12:57 UTC (permalink / raw) To: musl; +Cc: Olaf Meeuwissen I'll reiterate points raised previously in https://www.openwall.com/lists/musl/2017/08/30/10 On Mon, 20 Jan 2020, Szabolcs Nagy wrote: > 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. But _why_ does POSIX specify 'int' there? Checking on freebsd.org man pages I see that all BSD flavours had 'unsigned long' and Linux followed that. Did POSIX fail to document prevailing practice in this case? I still think the warning points to a bigger problem that with musl the kernel receives a sign-extended value. So I think ioctl.c should be fixed to not sign-extend the value, _IO macros could be changed to use (int)sizeof(c) instead of sizeof(c), and if silencing -Wsign-conversion is also desired, #ifdef __GNUC__ #define ioctl(fd, req, ...) ioctl(fd, (int)(req), ##__VA_ARGS__) #endif Alexander ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [musl] [BUG] ioctl: overflow in implicit constant conversion 2020-01-20 12:57 ` Alexander Monakov @ 2020-01-20 17:14 ` Rich Felker 0 siblings, 0 replies; 6+ messages in thread From: Rich Felker @ 2020-01-20 17:14 UTC (permalink / raw) To: musl On Mon, Jan 20, 2020 at 03:57:33PM +0300, Alexander Monakov wrote: > I'll reiterate points raised previously in https://www.openwall.com/lists/musl/2017/08/30/10 > > On Mon, 20 Jan 2020, Szabolcs Nagy wrote: > > > 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. > > But _why_ does POSIX specify 'int' there? Checking on freebsd.org > man pages I see that all BSD flavours had 'unsigned long' and Linux > followed that. Did POSIX fail to document prevailing practice in > this case? I'm not sure. I'm guessing there were more influential proprietary unices at the time whose definitions won out in committee. After all, the POSIX ioctl definition was built around STREAMS, not other historic use which seems to have been deemed sufficiently system-specific not to standardize despite having widespread overlapping parts. :( But in any case, portable software already has to be tolerant of either version. > I still think the warning points to a bigger problem that with > musl the kernel receives a sign-extended value. If this is actually an issue it can be fixed with: - int r = __syscall(SYS_ioctl, fd, req, arg); + int r = __syscall(SYS_ioctl, fd, (unsigned)req, arg); It has no relation to the public interface of the function. Rich ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [musl] [BUG] ioctl: overflow in implicit constant conversion 2020-01-20 12:09 ` Szabolcs Nagy 2020-01-20 12:57 ` Alexander Monakov @ 2020-01-20 13:08 ` Alexander Monakov 2020-01-21 12:27 ` Olaf Meeuwissen 2 siblings, 0 replies; 6+ messages in thread From: Alexander Monakov @ 2020-01-20 13:08 UTC (permalink / raw) To: musl; +Cc: Olaf Meeuwissen On Mon, 20 Jan 2020, Szabolcs Nagy wrote: > 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). Doesn't the next release with time64 changes give a nice opportunity to make this change as well? Alexander ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [musl] [BUG] ioctl: overflow in implicit constant conversion 2020-01-20 12:09 ` Szabolcs Nagy 2020-01-20 12:57 ` Alexander Monakov 2020-01-20 13:08 ` Alexander Monakov @ 2020-01-21 12:27 ` Olaf Meeuwissen 2 siblings, 0 replies; 6+ messages in thread From: Olaf Meeuwissen @ 2020-01-21 12:27 UTC (permalink / raw) To: Szabolcs Nagy; +Cc: musl 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2020-01-21 12:29 UTC | newest] Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-01-20 10:39 [musl] [BUG] ioctl: overflow in implicit constant conversion Olaf Meeuwissen 2020-01-20 12:09 ` Szabolcs Nagy 2020-01-20 12:57 ` Alexander Monakov 2020-01-20 17:14 ` Rich Felker 2020-01-20 13:08 ` Alexander Monakov 2020-01-21 12:27 ` Olaf Meeuwissen
Code repositories for project(s) associated with this public inbox https://git.vuxu.org/mirror/musl/ This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).