Re: [patch] kern/exec_script: avoid invalid free() in a case of error

[Michael McConville]

> On Sun, Dec 13, 2015 at 9:45 PM, Maxim Pugachev  wrote:
> > Hi,
> >
> > In exec_script_makecmds function, when EXEC_HASFD flag was set, but
> > copystr/copyinstr returns an error, we need to set *tmpsap to NULL to
> > terminate a loop (under "fail" label) correctly.

I spent a while straining to see the forest through the trees, but this
makes sense to me. ok mmcc@

Is this allocation chain idiom used elsewhere in sys/kern? Maybe we
could break it out into ~3 functions. The current state of affairs seems
bug-prone.

> > Index: sys/kern/exec_script.c
> > ===
> > RCS file: /cvs/src/sys/kern/exec_script.c,v
> > retrieving revision 1.36
> > diff -u -p -r1.36 exec_script.c
> > --- sys/kern/exec_script.c  10 Sep 2015 18:10:35 -  1.36
> > +++ sys/kern/exec_script.c  13 Dec 2015 18:33:53 -
> > @@ -222,8 +222,10 @@ check_shell:
> >  #endif
> > error = copyinstr(epp->ep_name, *tmpsap++, 
> > MAXPATHLEN,
> > NULL);
> > -   if (error != 0)
> > +   if (error != 0) {
> > +   *tmpsap = NULL;
> > goto fail;
> > +   }
> > } else
> > snprintf(*tmpsap++, MAXPATHLEN, "/dev/fd/%d", epp->ep_fd);
> > *tmpsap = NULL;
> 

Re: [patch] kern/exec_script: avoid invalid free() in a case of error

[Michael McConville]

Michael McConville wrote:
> > On Sun, Dec 13, 2015 at 9:45 PM, Maxim Pugachev  
> > wrote:
> > > Hi,
> > >
> > > In exec_script_makecmds function, when EXEC_HASFD flag was set, but
> > > copystr/copyinstr returns an error, we need to set *tmpsap to NULL to
> > > terminate a loop (under "fail" label) correctly.
> 
> I spent a while straining to see the forest through the trees, but this
> makes sense to me. ok mmcc@
> 
> Is this allocation chain idiom used elsewhere in sys/kern? Maybe we
> could break it out into ~3 functions. The current state of affairs seems
> bug-prone.

It seems that all possible paths through this nested condition increment
tmpsap. Why not just increment it afterward so no one else ends up with
the headache I now have?

As always: I could be misinterpreting this.


Index: sys/kern/exec_script.c
===
RCS file: /cvs/src/sys/kern/exec_script.c,v
retrieving revision 1.36
diff -u -p -r1.36 exec_script.c
--- sys/kern/exec_script.c  10 Sep 2015 18:10:35 -  1.36
+++ sys/kern/exec_script.c  29 Dec 2015 01:56:11 -
@@ -209,23 +209,26 @@ check_shell:
if (ISSET(p->p_flag, P_SYSTRACE)) {
error = systrace_scriptname(p, *tmpsap);
if (error == 0)
-   tmpsap++;
+   tmpsap;
else
/*
 * Since systrace_scriptname() provides a
 * convenience, not a security issue, we are
 * safe to do this.
 */
-   error = copystr(epp->ep_name, *tmpsap++,
+   error = copystr(epp->ep_name, *tmpsap,
MAXPATHLEN, NULL);
} else
 #endif
-   error = copyinstr(epp->ep_name, *tmpsap++, MAXPATHLEN,
+   error = copyinstr(epp->ep_name, *tmpsap, MAXPATHLEN,
NULL);
-   if (error != 0)
+   if (error != 0) {
+   *(tmpsap + 1) = NULL;
goto fail;
+   }
} else
-   snprintf(*tmpsap++, MAXPATHLEN, "/dev/fd/%d", epp->ep_fd);
+   snprintf(*tmpsap, MAXPATHLEN, "/dev/fd/%d", epp->ep_fd);
+   tmpsap++;
*tmpsap = NULL;
 
/*

Re: Fix size hints for x11-ssh-askpass

[Alexander Hall]

On Mon, Dec 28, 2015 at 08:10:49AM +0100, Matthieu Herrb wrote:
> On Sun, Dec 27, 2015 at 08:28:36PM +0100, Alexander Hall wrote:
> > Hi,
> > 
> > Recently, my window manager (i3) started making the ssh-askpass windows 
> > too small to be really usable. The problem seems to be that the size 
> > hints indicate that it provides a width and height, while those fields 
> > are set to 0. While looking at this, the same seems to be the case for 
> > position (x and y are 0). AFAICS, both of these hints are obsolete.
> 
> x y width and height are marked obsolete because the values are taken
> from the window parameters passed to XCreateWindow and not from here.
> 
> But the hints themselve are not obsolete. I would rather investigate
> why i3 behaves wrong than just removing the placement and size hints.
> 
> It seems to me that it can come from  different reasons (I don't use
> i3 so I've not taken the time to look at its code):
> 
> - it incorrectly tries to use the obsolete values, rather than the
>   window parameters
> 
> - it completely dropped the implementation of the "old" ICCCM
>   hints and only follows the extened wm hints. Since ssh-askpass
>   doesn't handle those it gets random values...
> 
> - the computed window parameters are either wrong or interpreted
>   wrong. This can be the case with Xinerama or pseudo-xinerama when
>   both ssh-ask pass and the window manager are trying to center the
>   window on a given monitor rather than having it centered globally
>   (where it will end up being split across 2 monitors if they are of
>   equal size).
> 
> > 
> > The diff below (even without the PPosition removal, but I added that 
> > for good measure) fixes this, at least for i3.
> 
> I would rather not remove those hints, even if other window manager
> seem to behave correctly. Maybe it's needed to pass the correct values
> for the obsolete fields. (ihmo it's still an i3 problem in that case,
> but it's a more acceptable workaround). ie set them from the d->w3.w
> structure.

Thanks a lot for the detailed answer. This is obviously not the right
fix. I previously tried adding those fields and that helped too IIRC.
I'll have a look at the i3 sources.

/Alexander

> 
> Also if someone is interested, support for ewmh compliant window
> managers could be added to ssh-askpass. It may help with complex
> multi-monitor layouts.
> 
> > 
> > Should this rather be taken upstream, if any, if it's not already 
> > there, in which case it might already be fixed?
> 
> Upstream is no longer maintained as far as I know.
> 
> > 
> > Anyway, the diff follows. Please test on your favourite window manager.
> > 
> > OK? Comments?
> > 
> > /Alexander
> > 
> > 
> > Index: x11-ssh-askpass.c
> > ===
> > RCS file: /cvs/xenocara/app/ssh-askpass/x11-ssh-askpass.c,v
> > retrieving revision 1.6
> > diff -u -p -r1.6 x11-ssh-askpass.c
> > --- x11-ssh-askpass.c   24 Apr 2015 02:19:41 -  1.6
> > +++ x11-ssh-askpass.c   27 Dec 2015 18:52:55 -
> > @@ -772,8 +772,6 @@ void createDialogWindow(AppInfo *app)
> >outOfMemory(app, __LINE__);
> > }
> > d->sizeHints->flags = 0;
> > -   d->sizeHints->flags |= PPosition;
> > -   d->sizeHints->flags |= PSize;
> > d->sizeHints->min_width = d->w3.w.width;
> > d->sizeHints->min_height = d->w3.w.height;
> > d->sizeHints->flags |= PMinSize;
> 
> > Index: x11-ssh-askpass.c
> > ===
> > RCS file: /cvs/xenocara/app/ssh-askpass/x11-ssh-askpass.c,v
> > retrieving revision 1.6
> > diff -u -p -r1.6 x11-ssh-askpass.c
> > --- x11-ssh-askpass.c   24 Apr 2015 02:19:41 -  1.6
> > +++ x11-ssh-askpass.c   27 Dec 2015 18:52:55 -
> > @@ -772,8 +772,6 @@ void createDialogWindow(AppInfo *app)
> >outOfMemory(app, __LINE__);
> > }
> > d->sizeHints->flags = 0;
> > -   d->sizeHints->flags |= PPosition;
> > -   d->sizeHints->flags |= PSize;
> > d->sizeHints->min_width = d->w3.w.width;
> > d->sizeHints->min_height = d->w3.w.height;
> > d->sizeHints->flags |= PMinSize;
> 
> 
> -- 
> Matthieu Herrb

mpsafe tx for re(4)

[David Gwynne]

this builds on jmatthew@'s last commit and adds mpsafe tx.

ive beat on it pretty hard, but more eyes/tests are appreciated.

ok?

Index: re.c
===
RCS file: /cvs/src/sys/dev/ic/re.c,v
retrieving revision 1.188
diff -u -p -r1.188 re.c
--- re.c28 Dec 2015 05:49:15 -  1.188
+++ re.c28 Dec 2015 07:51:23 -
@@ -1042,6 +1042,7 @@ re_attach(struct rl_softc *sc, const cha
ifp->if_softc = sc;
strlcpy(ifp->if_xname, sc->sc_dev.dv_xname, IFNAMSIZ);
ifp->if_flags = IFF_BROADCAST | IFF_SIMPLEX | IFF_MULTICAST;
+   ifp->if_xflags = IFXF_MPSAFE;
ifp->if_ioctl = re_ioctl;
ifp->if_start = re_start;
ifp->if_watchdog = re_watchdog;
@@ -1497,12 +1498,12 @@ re_txeof(struct rl_softc *sc)
 * to restart the channel here to flush them out. This only
 * seems to be required with the PCIe devices.
 */
-   if (tx_free < sc->rl_ldata.rl_tx_desc_cnt)
+   if (ifq_is_oactive(>if_snd))
+   ifq_restart(>if_snd);
+   else if (tx_free < sc->rl_ldata.rl_tx_desc_cnt)
CSR_WRITE_1(sc, sc->rl_txstart, RL_TXSTART_START);
-   else {
-   ifq_clr_oactive(>if_snd);
+   else
ifp->if_timer = 0;
-   }
 
return (1);
 }
@@ -1589,7 +1590,7 @@ re_intr(void *arg)
 * masks.
 */
re_rxeof(sc);
-   tx = re_txeof(sc);
+   re_txeof(sc);
} else
CSR_WRITE_4(sc, RL_TIMERCNT, 1); /* reload */
} else if (tx | rx) {
@@ -1602,12 +1603,6 @@ re_intr(void *arg)
}
}
 
-   if (!IFQ_IS_EMPTY(>if_snd)) {
-   KERNEL_LOCK();
-   re_start(ifp);
-   KERNEL_UNLOCK();
-   }
-
CSR_WRITE_2(sc, RL_IMR, sc->rl_intrs);
 
return (claimed);
@@ -1691,7 +1686,7 @@ re_encap(struct rl_softc *sc, struct mbu
 
/* FALLTHROUGH */
default:
-   return (ENOBUFS);
+   return (ENOMEM);
}
 
nsegs = map->dm_nsegs;
@@ -1715,8 +1710,8 @@ re_encap(struct rl_softc *sc, struct mbu
nsegs++;
}
 
-   if (sc->rl_ldata.rl_tx_free - (*used + nsegs) <= 1) {
-   error = EFBIG;
+   if (*used + nsegs + 1 >= sc->rl_ldata.rl_tx_free) {
+   error = ENOBUFS;
goto fail_unload;
}
 
@@ -1840,36 +1835,41 @@ re_start(struct ifnet *ifp)
struct mbuf *m;
int idx, used = 0, txq_free, error;
 
-   if (!(ifp->if_flags & IFF_RUNNING) || ifq_is_oactive(>if_snd))
-   return;
-   if ((sc->rl_flags & RL_FLAG_LINK) == 0)
+   if (!ISSET(sc->rl_flags, RL_FLAG_LINK)) {
+   IFQ_PURGE(>if_snd);
return;
+   }
 
txq_free = sc->rl_ldata.rl_txq_considx;
idx = sc->rl_ldata.rl_txq_prodidx;
-   if (idx >= txq_free)
+   if (txq_free <= idx)
txq_free += RL_TX_QLEN;
txq_free -= idx;
 
-   while (txq_free > 1) {
+   for (;;) {
+   if (txq_free <= 1) {
+   ifq_set_oactive(>if_snd);
+   break;
+   }
+
m = ifq_deq_begin(>if_snd);
if (m == NULL)
break;
 
error = re_encap(sc, m, >rl_ldata.rl_txq[idx], );
-   if (error != 0 && error != ENOBUFS) {
+   if (error == 0)
+   ifq_deq_commit(>if_snd, m);
+   else if (error == ENOBUFS) {
ifq_deq_rollback(>if_snd, m);
ifq_set_oactive(>if_snd);
break;
-   } else if (error != 0) {
+   } else {
ifq_deq_commit(>if_snd, m);
m_freem(m);
ifp->if_oerrors++;
continue;
}
 
-   ifq_deq_commit(>if_snd, m);
-
 #if NBPFILTER > 0
if (ifp->if_bpf)
bpf_mtap_ether(ifp->if_bpf, m, BPF_DIRECTION_OUT);
@@ -1882,10 +1882,11 @@ re_start(struct ifnet *ifp)
return;

ifp->if_timer = 5;
-   sc->rl_ldata.rl_txq_prodidx = idx;
atomic_sub_int(>rl_ldata.rl_tx_free, used);
KASSERT(sc->rl_ldata.rl_tx_free >= 0);
 
+   sc->rl_ldata.rl_txq_prodidx = idx;
+
CSR_WRITE_1(sc, sc->rl_txstart, RL_TXSTART_START);
 }
 
@@ -2112,7 +2113,6 @@ re_watchdog(struct ifnet *ifp)
sc = ifp->if_softc;
s = splnet();
printf("%s: watchdog timeout\n", sc->sc_dev.dv_xname);
-   ifp->if_oerrors++;
 
re_txeof(sc);
re_rxeof(sc);
@@ -2140,7 +2140,6 @@ re_stop(struct ifnet *ifp)
 

Re: Fix size hints for x11-ssh-askpass

[Alexander Hall]

On Mon, Dec 28, 2015 at 11:42:00AM +0100, Alexander Hall wrote:
> On Mon, Dec 28, 2015 at 08:10:49AM +0100, Matthieu Herrb wrote:
> > On Sun, Dec 27, 2015 at 08:28:36PM +0100, Alexander Hall wrote:
> > > Hi,
> > > 
> > > Recently, my window manager (i3) started making the ssh-askpass windows 
> > > too small to be really usable. The problem seems to be that the size 
> > > hints indicate that it provides a width and height, while those fields 
> > > are set to 0. While looking at this, the same seems to be the case for 
> > > position (x and y are 0). AFAICS, both of these hints are obsolete.
> > 
> > x y width and height are marked obsolete because the values are taken
> > from the window parameters passed to XCreateWindow and not from here.
> > 
> > But the hints themselve are not obsolete. I would rather investigate
> > why i3 behaves wrong than just removing the placement and size hints.
> > 
> > It seems to me that it can come from  different reasons (I don't use
> > i3 so I've not taken the time to look at its code):
> > 
> > - it incorrectly tries to use the obsolete values, rather than the
> >   window parameters

Yes that's what it seems like. See below.

> > 
> > - it completely dropped the implementation of the "old" ICCCM
> >   hints and only follows the extened wm hints. Since ssh-askpass
> >   doesn't handle those it gets random values...
> > 
> > - the computed window parameters are either wrong or interpreted
> >   wrong. This can be the case with Xinerama or pseudo-xinerama when
> >   both ssh-ask pass and the window manager are trying to center the
> >   window on a given monitor rather than having it centered globally
> >   (where it will end up being split across 2 monitors if they are of
> >   equal size).
> > 
> > > 
> > > The diff below (even without the PPosition removal, but I added that 
> > > for good measure) fixes this, at least for i3.
> > 
> > I would rather not remove those hints, even if other window manager
> > seem to behave correctly. Maybe it's needed to pass the correct values
> > for the obsolete fields. (ihmo it's still an i3 problem in that case,
> > but it's a more acceptable workaround). ie set them from the d->w3.w
> > structure.
> 
> Thanks a lot for the detailed answer. This is obviously not the right
> fix. I previously tried adding those fields and that helped too IIRC.
> I'll have a look at the i3 sources.

Ok, I can see this in the i3 code:

/* Plasma windows set their geometry in WM_SIZE_HINTS. */
if ((wm_size_hints.flags & XCB_ICCCM_SIZE_HINT_US_POSITION || 
wm_size_hints.flags & XCB_ICCCM_SIZE_HINT_P_POSITION) &&
(wm_size_hints.flags & XCB_ICCCM_SIZE_HINT_US_SIZE || 
wm_size_hints.flags & XCB_ICCCM_SIZE_HINT_P_SIZE)) {
DLOG("We are setting geometry according to wm_size_hints x=%d y=%d w=%d 
h=%d\n",
 wm_size_hints.x, wm_size_hints.y, wm_size_hints.width, 
wm_size_hints.height);
geom->x = wm_size_hints.x;
geom->y = wm_size_hints.y;
geom->width = wm_size_hints.width;
geom->height = wm_size_hints.height;
}   

which indeed indicate it's using the deprecated members, with no intention not 
to.

However, AFAICT, /usr/X11R6/include/xcb/xcb_icccm.h gives no
alternative, and as I'm not aiming to be an X hacker, ok to commit
the workaround, or is that only counterproductive in terms of i3?

/Alexander


Index: x11-ssh-askpass.c
===
RCS file: /cvs/xenocara/app/ssh-askpass/x11-ssh-askpass.c,v
retrieving revision 1.6
diff -u -p -r1.6 x11-ssh-askpass.c
--- x11-ssh-askpass.c   24 Apr 2015 02:19:41 -  1.6
+++ x11-ssh-askpass.c   28 Dec 2015 12:25:55 -
@@ -772,7 +772,11 @@ void createDialogWindow(AppInfo *app)
   outOfMemory(app, __LINE__);
}
d->sizeHints->flags = 0;
+   d->sizeHints->x = d->w3.w.x;
+   d->sizeHints->y = d->w3.w.y;
d->sizeHints->flags |= PPosition;
+   d->sizeHints->width = d->w3.w.width;
+   d->sizeHints->height = d->w3.w.height;
d->sizeHints->flags |= PSize;
d->sizeHints->min_width = d->w3.w.width;
d->sizeHints->min_height = d->w3.w.height;

Re: bugs in printf(3)

[Todd C. Miller]

On Fri, 25 Dec 2015 00:30:29 +0100, Ingo Schwarze wrote:

> Besides, i don't see the point in messing with FILE flags at all
> in case of encoding errors.  As opposed to fgetwc(3) and fputwc(3),
> the manual doesn't document this fiddling, and POSIX doesn't ask
> for it.  No other conversions in printf(3) set the error indicator.
> It isn't required because printf(3) provides a proper error return
> (-1) in the first place.  Has anybody ever seen any code calling
> ferror(3) after printf(3)?  That just wouldn't make sense.
> Of course, printf(3) can result in the error indicator getting set,
> but only if the underlying low-level write calls fail.

You are correct that neither our man page nor ISO C document
setting the error indicator.  I get your expected output on Linux
and Solaris so I think this diff is correct.  I wonder if it is
worth documenting that the error indicator is not set in fprintf(3)?

We do have code in our tree that checks ferror() after doing writes
via fprintf.  One example src/usr.sbin/smtpd, but I have not done
an exhaustive check.

 - todd

Re: bug in fputwc(3) error reporting

[Todd C. Miller]

OK millert@.

I believe there are also instances of the same problem in the
non-wide code.  In fact, we don't even document that the error
indicator is set in putc(3)!  Someone else may wish to tackle that
one :-)

 - todd

Re: [st...@openbsd.org: vlc, ld.so sigsegv/sigbus: _dl_cache_grpsym_list]

[Stuart Henderson]

On 2015/12/27 20:36, Philip Guenther wrote:
> On Sun, Dec 27, 2015 at 9:13 AM, Stuart Henderson  wrote:
> > Widening the audience to tech in case anyone with an idea missed it
> > on ports: it seems some people are having a lot more trouble than just
> > needing to restart build 2 or 3 times.
> 
> Can you drop the LD_DEBUG=1 output from a crash somewhere?
> 
> 
> Philip Guenther
> 

Yes, here are two examples:

https://junkpile.org/vlc-cache-gen-ld.so-crash1.txt 2117KB
https://junkpile.org/vlc-cache-gen-ld.so-crash2.txt 700KB

mpsafe tx for bge(4)

[David Gwynne]

this tweaks the bge tx code and marks it mpsafe.

ok?


Index: if_bge.c
===
RCS file: /cvs/src/sys/dev/pci/if_bge.c,v
retrieving revision 1.380
diff -u -p -r1.380 if_bge.c
--- if_bge.c29 Nov 2015 20:19:35 -  1.380
+++ if_bge.c28 Dec 2015 12:24:47 -
@@ -2994,6 +2994,7 @@ bge_attach(struct device *parent, struct
ifp = >arpcom.ac_if;
ifp->if_softc = sc;
ifp->if_flags = IFF_BROADCAST | IFF_SIMPLEX | IFF_MULTICAST;
+   ifp->if_xflags = IFXF_MPSAFE;
ifp->if_ioctl = bge_ioctl;
ifp->if_start = bge_start;
ifp->if_watchdog = bge_watchdog;
@@ -3665,12 +3666,12 @@ bge_txeof(struct bge_softc *sc)
 
txcnt = atomic_sub_int_nv(>bge_txcnt, freed);
 
-   if (txcnt < BGE_TX_RING_CNT - 16)
-   ifq_clr_oactive(>if_snd);
-   if (txcnt == 0)
-   ifp->if_timer = 0;
-
sc->bge_tx_saved_considx = cons;
+
+   if (ifq_is_oactive(>if_snd))
+   ifq_restart(>if_snd);
+   else if (txcnt == 0)
+   ifp->if_timer = 0;
 }
 
 int
@@ -3733,12 +3734,6 @@ bge_intr(void *xsc)
 
/* Check TX ring producer/consumer */
bge_txeof(sc);
-
-   if (!IFQ_IS_EMPTY(>if_snd)) {
-   KERNEL_LOCK();
-   bge_start(ifp);
-   KERNEL_UNLOCK();
-   }
}
 
return (1);
@@ -4099,12 +4094,12 @@ doit:
if (i < dmamap->dm_nsegs)
goto fail_unload;
 
-   bus_dmamap_sync(sc->bge_dmatag, dmamap, 0, dmamap->dm_mapsize,
-   BUS_DMASYNC_PREWRITE);
-
if (frag == sc->bge_tx_saved_considx)
goto fail_unload;
 
+   bus_dmamap_sync(sc->bge_dmatag, dmamap, 0, dmamap->dm_mapsize,
+   BUS_DMASYNC_PREWRITE);
+
sc->bge_rdata->bge_tx_ring[cur].bge_flags |= BGE_TXBDFLAG_END;
sc->bge_cdata.bge_tx_chain[cur] = m;
sc->bge_cdata.bge_tx_map[cur] = dmamap;
@@ -4126,16 +4121,14 @@ fail_unload:
 void
 bge_start(struct ifnet *ifp)
 {
-   struct bge_softc *sc;
+   struct bge_softc *sc = ifp->if_softc;
struct mbuf *m;
int txinc;
 
-   sc = ifp->if_softc;
-
-   if (!(ifp->if_flags & IFF_RUNNING) || ifq_is_oactive(>if_snd))
-   return;
-   if (!BGE_STS_BIT(sc, BGE_STS_LINK))
+   if (!BGE_STS_BIT(sc, BGE_STS_LINK)) {
+   IFQ_PURGE(>if_snd);
return;
+   }
 
txinc = 0;
while (1) {
@@ -4591,7 +4584,6 @@ bge_stop(struct bge_softc *sc, int softo
timeout_del(>bge_rxtimeout_jumbo);
 
ifp->if_flags &= ~IFF_RUNNING;
-   ifq_clr_oactive(>if_snd);
ifp->if_timer = 0;
 
if (!softonly) {
@@ -4653,6 +4645,9 @@ bge_stop(struct bge_softc *sc, int softo
}
 
intr_barrier(sc->bge_intrhand);
+   ifq_barrier(>if_snd);
+
+   ifq_clr_oactive(>if_snd);
 
/* Free the RX lists. */
bge_free_rx_ring_std(sc);

Re: [PATCH] vi remove custom perr from cl_main

[Martijn van Duren]

End there's also this monstrosity that removes localization support from 
vi, in contrast to nvi2, which switched from custom db2 functions to 
catgets(3): http://marc.info/?l=openbsd-tech=144908525123080=2


On 12/28/15 20:37, Michael McConville wrote:

Todd C. Miller wrote:

On Sun, 27 Dec 2015 10:12:23 +0100, Martijn van Duren wrote:


This patch has been accepted by the nvi2 project.[1] There are more
patches, but I'm waiting till these have been reviewed by nvi2.


Committed, thanks.


Thanks for taking care of this.

There's a lot more where that came from. Martijn, Michael Reed, and I
have been chipping away at nvi2 lately. I may be soon be looking for
ok's on diffs that we got committed upstream.

Re: [PATCH] vi remove custom perr from cl_main

[Todd C. Miller]

On Sun, 27 Dec 2015 10:12:23 +0100, Martijn van Duren wrote:

> This patch has been accepted by the nvi2 project.[1]
> There are more patches, but I'm waiting till these have been reviewed by 
> nvi2.

Committed, thanks.

 - todd

Re: Circular queues are gone

[Todd C. Miller]

On Sun, 27 Dec 2015 02:05:53 -0800, Chris Hettrick wrote:

> Circular queues are gone, so remove remaining references to them.

Thanks.  I've committed this and added a short bit about XOR simple
queues.  Those are currently not documented (hint hint tedu@).

 - todd

Re: [PATCH] vi remove custom perr from cl_main

[Michael McConville]

Todd C. Miller wrote:
> On Sun, 27 Dec 2015 10:12:23 +0100, Martijn van Duren wrote:
> 
> > This patch has been accepted by the nvi2 project.[1] There are more
> > patches, but I'm waiting till these have been reviewed by nvi2.
> 
> Committed, thanks.

Thanks for taking care of this.

There's a lot more where that came from. Martijn, Michael Reed, and I
have been chipping away at nvi2 lately. I may be soon be looking for
ok's on diffs that we got committed upstream.

ferror in ntpd (Re: bugs in printf(3))

[Sebastian Benoit]

Todd C. Miller(todd.mil...@courtesan.com) on 2015.12.28 10:46:08 -0700:
> On Fri, 25 Dec 2015 00:30:29 +0100, Ingo Schwarze wrote:
> 
> > Besides, i don't see the point in messing with FILE flags at all
> > in case of encoding errors.  As opposed to fgetwc(3) and fputwc(3),
> > the manual doesn't document this fiddling, and POSIX doesn't ask
> > for it.  No other conversions in printf(3) set the error indicator.
> > It isn't required because printf(3) provides a proper error return
> > (-1) in the first place.  Has anybody ever seen any code calling
> > ferror(3) after printf(3)?  That just wouldn't make sense.
> > Of course, printf(3) can result in the error indicator getting set,
> > but only if the underlying low-level write calls fail.
> 
> You are correct that neither our man page nor ISO C document
> setting the error indicator.  I get your expected output on Linux
> and Solaris so I think this diff is correct.  I wonder if it is
> worth documenting that the error indicator is not set in fprintf(3)?
> 
> We do have code in our tree that checks ferror() after doing writes
> via fprintf.  One example src/usr.sbin/smtpd, but I have not done
> an exhaustive check.

ntpd is one of these.

if the error is set, then ntpd has a bug i think, since it never calls
clearerr()

ok?

(sorry for hijacking the thread)

diff --git usr.sbin/ntpd/ntpd.c usr.sbin/ntpd/ntpd.c
index cf88fe8..df7dedb 100644
--- usr.sbin/ntpd/ntpd.c
+++ usr.sbin/ntpd/ntpd.c
@@ -558,6 +558,7 @@ writefreq(double d)
if (warnonce) {
log_warnx("can't write %s", DRIFTFILE);
warnonce = 0;
+   clearerr(freqfp);
}
return 0;
}

Re: [patch] nlist(3): out of bounds read

[Michael McConville]

Serguey Parkhomovsky wrote:
> Ping? This is the same sanity check that's done in nm(1)'s ELF handling.

Make sense to me. Tentative ok mmcc@

Alternatively, this check could be added to __elf_is_ok__, which is
called right above where you added it. However, the definition of the
function would have to change slightly; it's documented as checking
whether the ELF header matches the target platform.

> On Thu, Dec 10, 2015 at 09:40:11AM -0800, Serguey Parkhomovsky wrote:
> > When dealing with a malformed ELF header, e_shentsize may be 0. This
> > causes an out of bounds read while finding the symbol table on line 141.
> > 
> > Found using afl.
> > 
> > Index: nlist.c
> > ===
> > RCS file: /cvs/src/lib/libc/gen/nlist.c,v
> > retrieving revision 1.65
> > diff -u -p -r1.65 nlist.c
> > --- nlist.c 16 Oct 2015 16:54:38 -  1.65
> > +++ nlist.c 10 Dec 2015 16:36:26 -
> > @@ -102,6 +102,10 @@ __fdnlist(int fd, struct nlist *list)
> > !__elf_is_okay__() || fstat(fd, ) < 0)
> > return (-1);
> >  
> > +   /* Make sure section header size is not too small */
> > +   if (ehdr.e_shentsize < sizeof(Elf_Shdr))
> > +   return (-1);
> > +
> > /* calculate section header table size */
> > shdr_size = ehdr.e_shentsize * ehdr.e_shnum;
> >  
> 

Re: [patch] nlist(3): out of bounds read

[Michael McConville]

Michael McConville wrote:
> Serguey Parkhomovsky wrote:
> > Ping? This is the same sanity check that's done in nm(1)'s ELF handling.
> 
> Make sense to me. Tentative ok mmcc@
> 
> Alternatively, this check could be added to __elf_is_ok__, which is
> called right above where you added it. However, the definition of the
> function would have to change slightly; it's documented as checking
> whether the ELF header matches the target platform.

Here's a patch for that.

I used the cleanest manner of patching in the check. __elf_is_ok__'s
logic is pretty convoluted for a function that just returns the result
of &&-ing a bunch of boolean conditions. We could turn the entire thing
into a single return statement if we were so inclined.


Index: lib/libc/gen/nlist.c
===
RCS file: /cvs/src/lib/libc/gen/nlist.c,v
retrieving revision 1.65
diff -u -p -r1.65 nlist.c
--- lib/libc/gen/nlist.c16 Oct 2015 16:54:38 -  1.65
+++ lib/libc/gen/nlist.c29 Dec 2015 05:08:09 -
@@ -77,6 +77,9 @@ __elf_is_okay__(Elf_Ehdr *ehdr)
retval = 1;
}
 
+   if (ehdr->e_shentsize < sizeof(Elf_Shdr))
+   return (0);
+
return retval;
 }
 

Re: Fix support for early SATA drives on ahci(4)

[Jonathan Matthew]

On Tue, Dec 29, 2015 at 12:43:43AM +0100, Mark Kettenis wrote:
> I have an old Maxtor 7Y250M0 SATA drive that didn't quite work when
> attached to an Intel AHCI controller.  The drive was properly
> detected, but any attempt to read from the drive failed.  It worked
> fine if I switched the SATA controller into IDE mode in the BIOS, so
> the drive was fine.  Turns out our atascsi layer didn't set the
> transfer mode like our old wdc/pciide code does.  Fixing this
> omission makes the drive work.
> 
> It doesn't really make all that much sense doing this for a SATA drive
> since they don't really support different DMA modes.  But there is a
> PATA variant of the same drive, so I suppose somebody left an obselete
> check in the firmware code.
> 
> The diff below only checks for Ultra DMA modes.  All SATA drives
> should support those.  If we ever want to support ancient PATA drives
> over atascsi, some additional code will be needed.
> 
> ok?

I've tested an assortment of sata devices and nothing terrible happened.
ok by me.


> 
> 
> Index: atascsi.c
> ===
> RCS file: /cvs/src/sys/dev/ata/atascsi.c,v
> retrieving revision 1.125
> diff -u -p -r1.125 atascsi.c
> --- atascsi.c 28 Aug 2015 00:03:53 -  1.125
> +++ atascsi.c 28 Dec 2015 23:23:14 -
> @@ -266,6 +266,8 @@ atascsi_probe(struct scsi_link *link)
>   int port, type, qdepth;
>   int rv;
>   u_int16_t   cmdset;
> + u_int16_t   validinfo, ultradma;
> + int i, xfermode = -1;
>  
>   port = link->target;
>   if (port >= as->as_link.adapter_buswidth)
> @@ -363,6 +365,24 @@ atascsi_probe(struct scsi_link *link)
>  
>   if (type != ATA_PORT_T_DISK)
>   return (0);
> +
> + /*
> +  * Early SATA drivers (as well as PATA drives) need to have
> +  * their transfer mode set properly, otherwise commands that
> +  * use DMA will time out.
> +  */
> + validinfo = letoh16(ap->ap_identify.validinfo);
> + if (ISSET(validinfo, ATA_ID_VALIDINFO_ULTRADMA)) {
> + ultradma = letoh16(ap->ap_identify.ultradma);
> + for (i = 7; i >= 0; i--) {
> + if (ultradma & (1 << i)) {
> + xfermode = ATA_SF_XFERMODE_UDMA | i;
> + break;
> + }
> + }
> + }
> + if (xfermode != -1)
> + (void)atascsi_port_set_features(ap, ATA_SF_XFERMODE, xfermode);
>  
>   if (as->as_capability & ASAA_CAP_NCQ &&
>   ISSET(letoh16(ap->ap_identify.satacap), ATA_SATACAP_NCQ) &&
> Index: atascsi.h
> ===
> RCS file: /cvs/src/sys/dev/ata/atascsi.h,v
> retrieving revision 1.49
> diff -u -p -r1.49 atascsi.h
> --- atascsi.h 15 May 2015 10:54:26 -  1.49
> +++ atascsi.h 28 Dec 2015 23:23:14 -
> @@ -53,6 +53,8 @@ struct scsi_link;
>   * ATA SET FEATURES subcommands
>   */
>  #define ATA_SF_WRITECACHE_EN 0x02
> +#define ATA_SF_XFERMODE  0x03
> +#define  ATA_SF_XFERMODE_UDMA0x40
>  #define ATA_SF_LOOKAHEAD_EN  0xaa
>  
>  struct ata_identify {
> @@ -77,6 +79,7 @@ struct ata_identify {
>   u_int16_t   piomode;/*  51 */
>   u_int16_t   dmamode;/*  52 */
>   u_int16_t   validinfo;  /*  53 */
> +#define ATA_ID_VALIDINFO_ULTRADMA0x0004
>   u_int16_t   curcyls;/*  54 */
>   u_int16_t   curheads;   /*  55 */
>   u_int16_t   cursectrk;  /*  56 */
> 

Fix support for early SATA drives on ahci(4)

[Mark Kettenis]

I have an old Maxtor 7Y250M0 SATA drive that didn't quite work when
attached to an Intel AHCI controller.  The drive was properly
detected, but any attempt to read from the drive failed.  It worked
fine if I switched the SATA controller into IDE mode in the BIOS, so
the drive was fine.  Turns out our atascsi layer didn't set the
transfer mode like our old wdc/pciide code does.  Fixing this
omission makes the drive work.

It doesn't really make all that much sense doing this for a SATA drive
since they don't really support different DMA modes.  But there is a
PATA variant of the same drive, so I suppose somebody left an obselete
check in the firmware code.

The diff below only checks for Ultra DMA modes.  All SATA drives
should support those.  If we ever want to support ancient PATA drives
over atascsi, some additional code will be needed.

ok?


Index: atascsi.c
===
RCS file: /cvs/src/sys/dev/ata/atascsi.c,v
retrieving revision 1.125
diff -u -p -r1.125 atascsi.c
--- atascsi.c   28 Aug 2015 00:03:53 -  1.125
+++ atascsi.c   28 Dec 2015 23:23:14 -
@@ -266,6 +266,8 @@ atascsi_probe(struct scsi_link *link)
int port, type, qdepth;
int rv;
u_int16_t   cmdset;
+   u_int16_t   validinfo, ultradma;
+   int i, xfermode = -1;
 
port = link->target;
if (port >= as->as_link.adapter_buswidth)
@@ -363,6 +365,24 @@ atascsi_probe(struct scsi_link *link)
 
if (type != ATA_PORT_T_DISK)
return (0);
+
+   /*
+* Early SATA drivers (as well as PATA drives) need to have
+* their transfer mode set properly, otherwise commands that
+* use DMA will time out.
+*/
+   validinfo = letoh16(ap->ap_identify.validinfo);
+   if (ISSET(validinfo, ATA_ID_VALIDINFO_ULTRADMA)) {
+   ultradma = letoh16(ap->ap_identify.ultradma);
+   for (i = 7; i >= 0; i--) {
+   if (ultradma & (1 << i)) {
+   xfermode = ATA_SF_XFERMODE_UDMA | i;
+   break;
+   }
+   }
+   }
+   if (xfermode != -1)
+   (void)atascsi_port_set_features(ap, ATA_SF_XFERMODE, xfermode);
 
if (as->as_capability & ASAA_CAP_NCQ &&
ISSET(letoh16(ap->ap_identify.satacap), ATA_SATACAP_NCQ) &&
Index: atascsi.h
===
RCS file: /cvs/src/sys/dev/ata/atascsi.h,v
retrieving revision 1.49
diff -u -p -r1.49 atascsi.h
--- atascsi.h   15 May 2015 10:54:26 -  1.49
+++ atascsi.h   28 Dec 2015 23:23:14 -
@@ -53,6 +53,8 @@ struct scsi_link;
  * ATA SET FEATURES subcommands
  */
 #define ATA_SF_WRITECACHE_EN   0x02
+#define ATA_SF_XFERMODE0x03
+#define  ATA_SF_XFERMODE_UDMA  0x40
 #define ATA_SF_LOOKAHEAD_EN0xaa
 
 struct ata_identify {
@@ -77,6 +79,7 @@ struct ata_identify {
u_int16_t   piomode;/*  51 */
u_int16_t   dmamode;/*  52 */
u_int16_t   validinfo;  /*  53 */
+#define ATA_ID_VALIDINFO_ULTRADMA  0x0004
u_int16_t   curcyls;/*  54 */
u_int16_t   curheads;   /*  55 */
u_int16_t   cursectrk;  /*  56 */

Re: ksh.1: Remove $ERRNO reference

[Michael McConville]

Michael Reed wrote:
> It's had the ``Not implemented'' notice since ksh.1's initial import
> 19 years ago [1], so it's probably not going to be implemented.
> 
> [1]: 
> http://cvsweb.openbsd.org/cgi-bin/cvsweb/src/bin/ksh/ksh.1?rev=1.1=text/x-cvsweb-markup

Committed. Thanks!

bin/ksh/NOTES suggests that the sole motive for $ERRNO may have been its
presence in AT ksh.

> Index: ksh.1
> ===
> RCS file: /cvs/src/bin/ksh/ksh.1,v
> retrieving revision 1.171
> diff -u -p -r1.171 ksh.1
> --- ksh.1 24 Nov 2015 21:07:31 -  1.171
> +++ ksh.1 25 Dec 2015 18:32:46 -
> @@ -1386,12 +1386,6 @@ is set, it overrides
>  If this parameter is found to be set after any profile files are executed, 
> the
>  expanded value is used as a shell startup file.
>  It typically contains function and alias definitions.
> -.It Ev ERRNO
> -Integer value of the shell's
> -.Va errno
> -variable.
> -It indicates the reason the last system call failed.
> -Not yet implemented.
>  .It Ev EXECSHELL
>  If set, this parameter is assumed to contain the shell that is to be used to
>  execute commands that
>