mailing list of musl libc
 help / color / mirror / code / Atom feed
* [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: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: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: 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).