bug? in getopt(3) / possible [PATCH]
$ cat > opts.c <<'EOT' && cc -Wall -W opts.c -o opts
#include
#include
int main(int ac, char **av){
int c, i;
while((c = getopt(ac, av, "q")) != -1)
if(c != '?' && c != ':') printf("OPT %c\n", c);
for(i = optind; i < ac; i++) printf("%s\n", av[i]);
}
EOT
$ ./opts -q- arg
OPT q
-q-
arg
On any other platform (except those where OpenBSD's getopt_long.c
made its way) this will complain about an unknown option "-", and
leave "optind" pointing to the next argument, not to "-q-":
linux$ ./opts -q- arg
OPT q
./opts: illegal option -- -
arg
[or "unrecognized option: -", "invalid option -- '-'", etc]
The code in getopt_long.c says:
/*
* If the user specified "-" and '-' isn't listed in
* options, return -1 (non-option) as per POSIX.
* Otherwise, it is an unknown option character (or ':').
*/
if (optchar == (int)'-' && *place == '\0')
return (-1);
If that is supposed to catch a "--" option end marker, it doesn't seem to,
because it's already caught by the code above it:
if (optreset || !*place) { /* update scanning pointer */
...
/*
* If we have "-" do nothing, if "--" we are done.
*/
if (place[1] != '\0' && *++place == '-' && place[1] == '\0') {
optind++;
place = EMSG;
...
return (-1);
}
If not, could you point me where the POSIX standard requires the
OpenBSD's behavior?
In case this is not a completely bogus report, maybe the patch below
could do -- not tested very much.
The extra "oli == options" condition is to handle the GNU getopt's
"optstring starting with '-' => \1=optarg" extension, also implemented
by OpenBSD's getopt_long.c.
If this is correct, these are sub-bugs in the current code, that this
patch is also fixing:
getopt("-p", ["prog", "-p--", "--", "arg"])
=> fails to skip the "--"
getopt("p-:", ["prog", "-p--", "--", "arg"])
=> complains about unknown option "-", but still sets
the "-" option to "--" (not to "-"!)
--- lib/libc/stdlib/getopt_long.c~
+++ lib/libc/stdlib/getopt_long.c
@@ -418,15 +418,8 @@
}
if ((optchar = (int)*place++) == (int)':' ||
- (optchar == (int)'-' && *place != '\0') ||
- (oli = strchr(options, optchar)) == NULL) {
- /*
-* If the user specified "-" and '-' isn't listed in
-* options, return -1 (non-option) as per POSIX.
-* Otherwise, it is an unknown option character (or ':').
-*/
- if (optchar == (int)'-' && *place == '\0')
- return (-1);
+ ((oli = strchr(options, optchar)) == NULL ||
+(optchar == (int)'-' && oli == options))) {
if (!*place)
++optind;
if (PRINT_ERROR)
Fix a panic with unlocked soclose()
jcs@ has reported the following panic which started appearing as a consequence of the recent file close unlocking: panic: kernel diagnostic assertion "timo || _kernel_lock_held()" failed: file "/usr/src/sys/kern/kern_synch.c", line 123 panic __assert tsleep usbd_transfer usbd_do_request_flags ure_iff ure_ioctl in6_delmulti in6_leavegroup in6_freemoptions in_pcbdetach udp_detach soclose The above shows that soclose() can call parts of the kernel that still need the kernel lock. This is now apparent because close(2) is unlocked. However, the issue has been present longer. If a thread raced with the closing of a socket file descriptor, the final FRELE() might be invoked in an unlocked context, triggering a similar situation. I think most of the socket handling is now under NET_LOCK(). However, calls to the driver layer from the socket code need to be serialized with the kernel lock. The following diff does that for SIOCDELMULTI ioctl requests that can be invoked from the socket close path. I wonder if this is actually enough and if most of soclose() should be put back under KERNEL_LOCK() as a precaution... Index: netinet/in.c === RCS file: src/sys/netinet/in.c,v retrieving revision 1.168 diff -u -p -r1.168 in.c --- netinet/in.c1 Dec 2019 21:12:42 - 1.168 +++ netinet/in.c11 Mar 2020 16:26:44 - @@ -929,7 +929,9 @@ in_delmulti(struct in_multi *inm) sizeof(struct sockaddr_in); satosin(_addr)->sin_family = AF_INET; satosin(_addr)->sin_addr = inm->inm_addr; + KERNEL_LOCK(); (*ifp->if_ioctl)(ifp, SIOCDELMULTI, (caddr_t)); + KERNEL_UNLOCK(); TAILQ_REMOVE(>if_maddrlist, >inm_ifma, ifma_list); Index: netinet/ip_mroute.c === RCS file: src/sys/netinet/ip_mroute.c,v retrieving revision 1.128 diff -u -p -r1.128 ip_mroute.c --- netinet/ip_mroute.c 2 Sep 2019 13:12:09 - 1.128 +++ netinet/ip_mroute.c 11 Mar 2020 16:26:44 - @@ -772,7 +772,9 @@ vif_delete(struct ifnet *ifp) satosin(_addr)->sin_len = sizeof(struct sockaddr_in); satosin(_addr)->sin_family = AF_INET; satosin(_addr)->sin_addr = zeroin_addr; + KERNEL_LOCK(); (*ifp->if_ioctl)(ifp, SIOCDELMULTI, (caddr_t)); + KERNEL_UNLOCK(); free(v, M_MRTABLE, sizeof(*v)); } Index: netinet6/in6.c === RCS file: src/sys/netinet6/in6.c,v retrieving revision 1.234 diff -u -p -r1.234 in6.c --- netinet6/in6.c 18 Nov 2019 22:08:59 - 1.234 +++ netinet6/in6.c 11 Mar 2020 16:26:44 - @@ -1100,7 +1100,9 @@ in6_delmulti(struct in6_multi *in6m) ifr.ifr_addr.sin6_len = sizeof(struct sockaddr_in6); ifr.ifr_addr.sin6_family = AF_INET6; ifr.ifr_addr.sin6_addr = in6m->in6m_addr; + KERNEL_LOCK(); (*ifp->if_ioctl)(ifp, SIOCDELMULTI, (caddr_t)); + KERNEL_UNLOCK(); TAILQ_REMOVE(>if_maddrlist, >in6m_ifma, ifma_list); Index: netinet6/ip6_mroute.c === RCS file: src/sys/netinet6/ip6_mroute.c,v retrieving revision 1.122 diff -u -p -r1.122 ip6_mroute.c --- netinet6/ip6_mroute.c 4 Sep 2019 16:13:49 - 1.122 +++ netinet6/ip6_mroute.c 11 Mar 2020 16:26:44 - @@ -565,7 +565,9 @@ ip6_mrouter_detach(struct ifnet *ifp) memset(, 0, sizeof(ifr)); ifr.ifr_addr.sin6_family = AF_INET6; ifr.ifr_addr.sin6_addr = in6addr_any; + KERNEL_LOCK(); (*ifp->if_ioctl)(ifp, SIOCDELMULTI, (caddr_t)); + KERNEL_UNLOCK(); free(m6, M_MRTABLE, sizeof(*m6)); }
Re: check disk_lookup return value
Jasper Lievisse Adriaanse wrote: > Check return value of disk_lookup before dereference as it can > return NULL. These are Coverity CID 1452925, 1452951, 1452967. In what case... nfs diskless root? But in that case, do we even get here because rootdev/swapdev will be -1 won't they? I don't see how it can fail. I think coverity doesn't see the full picture. However, I think it is fine to follow the API and check for an error which can't happen...
check disk_lookup return value
Hi,
Check return value of disk_lookup before dereference as it can
return NULL. These are Coverity CID 1452925, 1452951, 1452967.
Other functions using disk_lookup (or the re-defined versions like
wdlookup) check the return value already, including in the context
of hibernation (sdmmc_scsi_hibernate_io and wd_hibernate_io). Is there
something special about the instances below or was it merely oversight?
Index: dev/softraid.c
===
RCS file: /cvs/src/sys/dev/softraid.c,v
retrieving revision 1.399
diff -u -p -r1.399 softraid.c
--- dev/softraid.c 10 Mar 2020 08:41:19 - 1.399
+++ dev/softraid.c 11 Mar 2020 12:28:53 -
@@ -5068,6 +5068,8 @@ sr_hibernate_io(dev_t dev, daddr_t blkno
*/
if (op == HIB_INIT) {
dv = disk_lookup(_cd, DISKUNIT(dev));
+ if (dv == NULL)
+ return (EIO);
sd = (struct sd_softc *)dv;
sc = (struct sr_softc *)dv->dv_parent->dv_parent;
Index: dev/ic/nvme.c
===
RCS file: /cvs/src/sys/dev/ic/nvme.c,v
retrieving revision 1.72
diff -u -p -r1.72 nvme.c
--- dev/ic/nvme.c 10 Mar 2020 14:49:20 - 1.72
+++ dev/ic/nvme.c 11 Mar 2020 12:28:53 -
@@ -1498,6 +1498,8 @@ nvme_hibernate_io(dev_t dev, daddr_t blk
/* find nvme softc */
disk = disk_lookup(_cd, DISKUNIT(dev));
+ if (disk == NULL)
+ return (EIO);
scsibus = disk->dv_parent;
my->sc = (struct nvme_softc *)disk->dv_parent->dv_parent;
Index: dev/ic/ahci.c
===
RCS file: /cvs/src/sys/dev/ic/ahci.c,v
retrieving revision 1.34
diff -u -p -r1.34 ahci.c
--- dev/ic/ahci.c 8 Jul 2019 22:02:59 - 1.34
+++ dev/ic/ahci.c 11 Mar 2020 12:28:53 -
@@ -3265,6 +3265,8 @@ ahci_hibernate_io(dev_t dev, daddr_t blk
/* map dev to an ahci port */
disk = disk_lookup(_cd, DISKUNIT(dev));
+ if (disk == NULL)
+ return (EIO);
scsibus = disk->dv_parent;
sc = (struct ahci_softc *)disk->dv_parent->dv_parent;
--
jasper
Re: uplcom: remove dead code
> Date: Wed, 11 Mar 2020 13:38:12 +0100
> From: Jasper Lievisse Adriaanse
>
> Hi,
>
> Remove dead code which is actually duplicated a few lines above
> right after err is set. Coverity ID 975917
>
> OK?
ok kettenis@
> Index: dev/usb/uplcom.c
> ===
> RCS file: /cvs/src/sys/dev/usb/uplcom.c,v
> retrieving revision 1.73
> diff -u -p -U11 -r1.73 uplcom.c
> --- dev/usb/uplcom.c 18 Nov 2018 16:23:14 - 1.73
> +++ dev/usb/uplcom.c 11 Mar 2020 12:36:03 -
> @@ -619,27 +619,22 @@ uplcom_param(void *addr, int portno, str
> if (err) {
> DPRINTF(("uplcom_param: err=%s\n", usbd_errstr(err)));
> return (EIO);
> }
>
> if (ISSET(t->c_cflag, CRTSCTS))
> uplcom_set_crtscts(sc);
>
> if (sc->sc_rts == -1 || sc->sc_dtr == -1)
> uplcom_set_line_state(sc);
>
> - if (err) {
> - DPRINTF(("uplcom_param: err=%s\n", usbd_errstr(err)));
> - return (EIO);
> - }
> -
> return (0);
> }
>
> int
> uplcom_open(void *addr, int portno)
> {
> struct uplcom_softc *sc = addr;
> usb_device_request_t req;
> usbd_status uerr;
> int err;
>
>
> --
> jasper
>
>
uplcom: remove dead code
Hi,
Remove dead code which is actually duplicated a few lines above
right after err is set. Coverity ID 975917
OK?
Index: dev/usb/uplcom.c
===
RCS file: /cvs/src/sys/dev/usb/uplcom.c,v
retrieving revision 1.73
diff -u -p -U11 -r1.73 uplcom.c
--- dev/usb/uplcom.c18 Nov 2018 16:23:14 - 1.73
+++ dev/usb/uplcom.c11 Mar 2020 12:36:03 -
@@ -619,27 +619,22 @@ uplcom_param(void *addr, int portno, str
if (err) {
DPRINTF(("uplcom_param: err=%s\n", usbd_errstr(err)));
return (EIO);
}
if (ISSET(t->c_cflag, CRTSCTS))
uplcom_set_crtscts(sc);
if (sc->sc_rts == -1 || sc->sc_dtr == -1)
uplcom_set_line_state(sc);
- if (err) {
- DPRINTF(("uplcom_param: err=%s\n", usbd_errstr(err)));
- return (EIO);
- }
-
return (0);
}
int
uplcom_open(void *addr, int portno)
{
struct uplcom_softc *sc = addr;
usb_device_request_t req;
usbd_status uerr;
int err;
--
jasper
Re: Debugger, traced processes & exit status
On 29/02/20(Sat) 19:55, Martin Pieuchot wrote:
> On 28/02/20(Fri) 18:24, Martin Pieuchot wrote:
> > Diff below fixes multiple issues with traced process, exposed by the
> > regression test attached:
> >
> > - When a debugging process exit, give back the traced process to its
> > original parent, if it exists, instead of init(8).
> >
> > - When a traced process exit, make sure the original parent receives
> > the exit status only after the debugger has seen it. This is done
> > by keeping a list of 'orphaned' children in the original parent and
> > looking in it in dowait4().
> >
> > The logic and most of the code comes from FreeBSD as pointed out by
> > guenther@.
>
> Improved versions that:
>
> - Introduce process_clear_orphan() to remove a process from an orphan
>list.
>
> - Introduce process_untrace() to give back a process to its original
>parent or to init(8), used in both PT_DETACH & exit1().
>
> - Rename proc_reparent() into process_reparent() for coherency
Add a KASSERT() and respect the style of the file, prodded by visa@.
ok?
Index: kern/kern_exit.c
===
RCS file: /cvs/src/sys/kern/kern_exit.c,v
retrieving revision 1.185
diff -u -p -r1.185 kern_exit.c
--- kern/kern_exit.c1 Mar 2020 18:50:52 - 1.185
+++ kern/kern_exit.c6 Mar 2020 10:57:03 -
@@ -76,6 +76,7 @@
#endif
void proc_finish_wait(struct proc *, struct proc *);
+void process_clear_orphan(struct process *);
void process_zap(struct process *);
void proc_free(struct proc *);
void unveil_destroy(struct process *ps);
@@ -253,21 +254,22 @@ exit1(struct proc *p, int xexit, int xsi
}
/*
-* Give orphaned children to init(8).
+* Reparent children to their original parent, in case
+* they were being traced, or to init(8).
*/
qr = LIST_FIRST(>ps_children);
if (qr) /* only need this if any child is S_ZOMB */
wakeup(initprocess);
for (; qr != 0; qr = nqr) {
nqr = LIST_NEXT(qr, ps_sibling);
- proc_reparent(qr, initprocess);
/*
* Traced processes are killed since their
* existence means someone is screwing up.
*/
if (qr->ps_flags & PS_TRACED &&
!(qr->ps_flags & PS_EXITING)) {
- atomic_clearbits_int(>ps_flags, PS_TRACED);
+ process_untrace(qr);
+
/*
* If single threading is active,
* direct the signal to the active
@@ -278,8 +280,19 @@ exit1(struct proc *p, int xexit, int xsi
STHREAD);
else
prsignal(qr, SIGKILL);
+ } else {
+ process_reparent(qr, initprocess);
}
}
+
+ /*
+* Make sure orphans won't remember the exiting process.
+*/
+ while ((qr = LIST_FIRST(>ps_orphans)) != NULL) {
+ KASSERT(qr->ps_oppid == pr->ps_pid);
+ qr->ps_oppid = 0;
+ process_clear_orphan(qr);
+ }
}
/* add thread's accumulated rusage into the process's total */
@@ -310,7 +323,7 @@ exit1(struct proc *p, int xexit, int xsi
*/
if (pr->ps_flags & PS_NOZOMBIE) {
struct process *ppr = pr->ps_pptr;
- proc_reparent(pr, initprocess);
+ process_reparent(pr, initprocess);
wakeup(ppr);
}
@@ -562,6 +575,29 @@ loop:
return (0);
}
}
+ /*
+* Look in the orphans list too, to allow the parent to
+* collect it's child exit status even if child is being
+* debugged.
+*
+* Debugger detaches from the parent upon successful
+* switch-over from parent to child. At this point due to
+* re-parenting the parent loses the child to debugger and a
+* wait4(2) call would report that it has no children to wait
+* for. By maintaining a list of orphans we allow the parent
+* to successfully wait until the child becomes a zombie.
+*/
+ if (nfound == 0) {
+ LIST_FOREACH(pr, >p_p->ps_orphans, ps_orphan) {
+ if ((pr->ps_flags & PS_NOZOMBIE) ||
+ (pid != WAIT_ANY &&
+ pr->ps_pid != pid &&
+ pr->ps_pgid !=
Re: tweak how amd64 (not intel) cpu topology is calculated
> On 10 Mar 2020, at 00:04, Stuart Henderson wrote: > > On 2020/03/09 22:50, David Gwynne wrote: >> this works better on his epyc 2 box, and works right on my epyc 1, esxi >> on epyc 1, and on an apu1. > > Fine on apu2 (GX-412TC) and the old HP microserver (Turion N40L) also. > Diff makes sense and I'm happy you found an alternative to my dodgy > CPU_INFO_FOREACH :) i prefer to think of ESXi as the dodgy bit in this situation. does anyone want to ok this? dlg
