Date: Mon, 14 Sep 2020 00:44:51 -0700 From: Xin LI <delphij@gmail.com> To: FreeBSD Current <freebsd-current@freebsd.org> Cc: hselasky@freebsd.org, phk@freebsd.org, jilles@freebsd.org, Doug Rabson <dfr@rabson.org>, Xin Li <delphij@freebsd.org> Subject: ioctl argument type [Was Re: svn commit: r359968 - head/sys/kern] Message-ID: <CAGMYy3uQA5byFvu=Zyc9g2tgY4MhZ8WyRvBS795PcqNh6CO=fw@mail.gmail.com> In-Reply-To: <202004151320.03FDKqT7027080@repo.freebsd.org> References: <202004151320.03FDKqT7027080@repo.freebsd.org>
next in thread | previous in thread | raw e-mail | index | archive | help
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@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 >
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?CAGMYy3uQA5byFvu=Zyc9g2tgY4MhZ8WyRvBS795PcqNh6CO=fw>