On 2020-09-14 09:44, Xin LI wrote:
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 <hsela...@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
Hi,
Using unsigned long is not cross platform compatible, especially when
you have 32-bit compat shim layers.
On 64-bit platforms long is usually 64-bit and on 32-bit platforms long
is usually 32-bit.
You've brought up a good question with a good history line.
Maybe we should just "#if 0" the INVARIANTS check and remove that code?
--HPS
_______________________________________________
freebsd-current@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/freebsd-current
To unsubscribe, send any mail to "freebsd-current-unsubscr...@freebsd.org"
