ioctl argument type [Was Re: svn commit: r359968 - head/sys/kern]
Xin LI
delphij at gmail.com
Mon Sep 14 07:45:06 UTC 2020
Hi,
I have seen Chromium trigger the warning (I run -CURRENT with INVARIANTS)
and looked into the code history a little bit.
It seems that the command was changed to u_long in r36846
<https://svnweb.freebsd.org/base?view=revision&revision=36846> with a
follow up commit of r38517
<https://svnweb.freebsd.org/base?view=revision&revision=38517> , possibly
because ioctl was defined to take an unsigned long command before FreeBSD.
Internally, we have truncated it to 32-bit since 2005 (r140406
<https://svnweb.freebsd.org/base?view=revision&revision=140406>), and this
recent change made it a silent behavior. POSIX, on the other hand, defined
<https://pubs.opengroup.org/onlinepubs/9699919799/functions/ioctl.html>
ioctl as taking an int as its second parameter, but neither Linux (glibc in
particular, despite its documentation says
<https://www.gnu.org/software/libc/manual/html_node/IOCTLs.html>
differently) nor macOS appear to define it that way, but Solaris seems
<https://docs.oracle.com/cd/E23824_01/html/821-1463/ioctl-2.html> to be
defining it as an int.
What was the motivation to keep the prototype definition as
int
ioctl(int fd, unsigned long request, ...);
instead of:
int
ioctl(int fd, int request, ...);
Other than to make existing code happy? Alternatively, will it be a good
idea to give compiler some hints (e.g. by using __attribute__(enable_if))
to emit errors, if we insist keeping the existing signature?
On Wed, Apr 15, 2020 at 6:21 AM Hans Petter Selasky <hselasky at freebsd.org>
wrote:
> Author: hselasky
> Date: Wed Apr 15 13:20:51 2020
> New Revision: 359968
> URL: https://svnweb.freebsd.org/changeset/base/359968
>
> Log:
> Cast all ioctl command arguments through uint32_t internally.
>
> Hide debug print showing use of sign extended ioctl command argument
> under INVARIANTS. The print is available to all and can easily fill
> up the logs.
>
> No functional change intended.
>
> MFC after: 1 week
> Sponsored by: Mellanox Technologies
>
> Modified:
> head/sys/kern/sys_generic.c
>
> Modified: head/sys/kern/sys_generic.c
>
> ==============================================================================
> --- head/sys/kern/sys_generic.c Wed Apr 15 13:13:46 2020 (r359967)
> +++ head/sys/kern/sys_generic.c Wed Apr 15 13:20:51 2020 (r359968)
> @@ -652,18 +652,19 @@ int
> sys_ioctl(struct thread *td, struct ioctl_args *uap)
> {
> u_char smalldata[SYS_IOCTL_SMALL_SIZE]
> __aligned(SYS_IOCTL_SMALL_ALIGN);
> - u_long com;
> + uint32_t com;
> int arg, error;
> u_int size;
> caddr_t data;
>
> +#ifdef INVARIANTS
> if (uap->com > 0xffffffff) {
> printf(
> "WARNING pid %d (%s): ioctl sign-extension ioctl
> %lx\n",
> td->td_proc->p_pid, td->td_name, uap->com);
> - uap->com &= 0xffffffff;
> }
> - com = uap->com;
> +#endif
> + com = (uint32_t)uap->com;
>
> /*
> * Interpret high order word to find amount of data to be
>
More information about the freebsd-current
mailing list