Re: Fix for USB keyboards eating keys, a DDB story
On 5/10/17 5:19 PM, Hrvoje Popovski wrote: > On 10.5.2017. 15:22, Martin Pieuchot wrote: >> This big hammer of delaying every input via a timeout introduced a nasty >> side effect. Since only one element can be queued, we can lose inputs >> if the keyboard is too fast. >> >> Here are some bug reports: >> >> https://marc.info/?l=openbsd-bugs=147284987417838=2 >> https://marc.info/?l=openbsd-tech=142082432912041=1 > > > Hi all, > > i've applied this patch on boxes below and remote virtual console no > longer lose their characters: > > Dell R620 - iDRAC7 > Dell R630 - iDRAC8 > Supermicro 1018R-WR - iKVM > IBM x3550M4 - IMM2 - was working and working after patch > > i don't have HP ILO console to test on... > > mpi many thanks for this patch, it's a lifesaver > Thank you, it works on a R330 too!
Re: [patch]Modify the example code in write(2) manual
Hi Ingo, Thanks very much for your time and detailed explanation! I have one more question: If write(2) indeed returns 0, the write(2) won't set errno variable, ring? Because from the manual, the errno is set only when the return value is -1. If this is true, the errno's value should be set by last system call which failed, and has no relation with current write(2). So it is not guaranteed that following words will be print: program: write: Undefined error: 0 It may print a totally unrelated description which can mislead the user. Does my concern make sense? Please correct me if I am wrong, thanks very much in advance! On 9/26/2018 10:33 PM, Ingo Schwarze wrote: > Hi, > > Nan Xiao wrote on Wed, Sep 26, 2018 at 09:42:02PM +0800: > >> Any developer can comment on this patch? Thanks! > > I think this change is a bad idea and should not be committed. > > No matter whether or not it can happen on OpenBSD, *if* some > implementation of write(2) sometimes returns 0 even for nbytes > 0, > your change is likely to cause an infinite busy loop, which is > quite bad. > > In that case, the current code would print: > > program: write: Undefined error: 0 > > That's admittedly not totally informative, but we can't do much > better because it is indeed not quite clear which kind of problem > the return value of 0 might indicate. And in any case, that edge > case seems uncommon enough that complicating the example code to > handle the case of a return value of zero separately would also > seem like a bad idea. > > Besides, the idiom documented here is actually widely used in the > tree, and it is indeed recommended, see for example > > /usr/src/bin/cat/cat.c > > Yours, > Ingo > > >> I am reading write(2) manual, and come across the following example: >> >> for (off = 0; off < bsz; off += nw) >> if ((nw = write(d, buf + off, bsz - off)) == 0 || nw == -1) >> err(1, "write"); >> >> I am just wondering when the write(2) will return 0? If in some cases, >> it will indeed return 0, according to the manual: >> >> Upon successful completion the number of bytes which were written is >> returned. Otherwise, a -1 is returned and the global variable errno is >> set to indicate the error. >> >> Because the errno is only set when return value is -1, if write(2) >> returns 0, the errno should be an undefined value, and "err(1, >> "write");" also won't print correct information. >> >> If write(2) won't return 0, my following patch fixes the example code: >> >> diff --git write.2 write.2 >> index c1686b1a910..db134959002 100644 >> --- write.2 >> +++ write.2 >> @@ -164,7 +164,7 @@ ssize_t nw; >> int d; >> >> for (off = 0; off < bsz; off += nw) >> -if ((nw = write(d, buf + off, bsz - off)) == 0 || nw == -1) >> +if ((nw = write(d, buf + off, bsz - off)) == -1) >> err(1, "write"); >> .Ed >> .Sh ERRORS -- Best Regards Nan Xiao(肖楠)
More bif
Diff below rename the remaining "struct bridge_iflist" variables to
`bif', cohérence oblige!
Ok?
Index: net/bridgectl.c
===
RCS file: /cvs/src/sys/net/bridgectl.c,v
retrieving revision 1.8
diff -u -p -r1.8 bridgectl.c
--- net/bridgectl.c 5 Feb 2018 05:06:51 - 1.8
+++ net/bridgectl.c 26 Sep 2018 20:10:03 -
@@ -538,7 +538,7 @@ int
bridge_brlconf(struct bridge_softc *sc, struct ifbrlconf *bc)
{
struct ifnet *ifp;
- struct bridge_iflist *ifl;
+ struct bridge_iflist *bif;
struct brl_node *n;
struct ifbrlreq req;
int error = 0;
@@ -547,14 +547,14 @@ bridge_brlconf(struct bridge_softc *sc,
ifp = ifunit(bc->ifbrl_ifsname);
if (ifp == NULL)
return (ENOENT);
- ifl = (struct bridge_iflist *)ifp->if_bridgeport;
- if (ifl == NULL || ifl->bridge_sc != sc)
+ bif = (struct bridge_iflist *)ifp->if_bridgeport;
+ if (bif == NULL || bif->bridge_sc != sc)
return (ESRCH);
- SIMPLEQ_FOREACH(n, >bif_brlin, brl_next) {
+ SIMPLEQ_FOREACH(n, >bif_brlin, brl_next) {
total++;
}
- SIMPLEQ_FOREACH(n, >bif_brlout, brl_next) {
+ SIMPLEQ_FOREACH(n, >bif_brlout, brl_next) {
total++;
}
@@ -563,12 +563,12 @@ bridge_brlconf(struct bridge_softc *sc,
goto done;
}
- SIMPLEQ_FOREACH(n, >bif_brlin, brl_next) {
+ SIMPLEQ_FOREACH(n, >bif_brlin, brl_next) {
bzero(, sizeof req);
if (bc->ifbrl_len < sizeof(req))
goto done;
strlcpy(req.ifbr_name, sc->sc_if.if_xname, IFNAMSIZ);
- strlcpy(req.ifbr_ifsname, ifl->ifp->if_xname, IFNAMSIZ);
+ strlcpy(req.ifbr_ifsname, bif->ifp->if_xname, IFNAMSIZ);
req.ifbr_action = n->brl_action;
req.ifbr_flags = n->brl_flags;
req.ifbr_src = n->brl_src;
@@ -587,12 +587,12 @@ bridge_brlconf(struct bridge_softc *sc,
bc->ifbrl_len -= sizeof(req);
}
- SIMPLEQ_FOREACH(n, >bif_brlout, brl_next) {
+ SIMPLEQ_FOREACH(n, >bif_brlout, brl_next) {
bzero(, sizeof req);
if (bc->ifbrl_len < sizeof(req))
goto done;
strlcpy(req.ifbr_name, sc->sc_if.if_xname, IFNAMSIZ);
- strlcpy(req.ifbr_ifsname, ifl->ifp->if_xname, IFNAMSIZ);
+ strlcpy(req.ifbr_ifsname, bif->ifp->if_xname, IFNAMSIZ);
req.ifbr_action = n->brl_action;
req.ifbr_flags = n->brl_flags;
req.ifbr_src = n->brl_src;
Index: net/if_bridge.c
===
RCS file: /cvs/src/sys/net/if_bridge.c,v
retrieving revision 1.310
diff -u -p -r1.310 if_bridge.c
--- net/if_bridge.c 26 Sep 2018 11:50:42 - 1.310
+++ net/if_bridge.c 26 Sep 2018 20:10:03 -
@@ -725,7 +725,6 @@ bridge_output(struct ifnet *ifp, struct
struct ether_addr *dst;
struct bridge_softc *sc;
struct bridge_tunneltag *brtag;
- struct bridge_iflist *ifl;
#if NBPFILTER > 0
caddr_t if_bpf;
#endif
@@ -773,7 +772,7 @@ bridge_output(struct ifnet *ifp, struct
if ((dst_p = bridge_rtlookup(sc, dst)) != NULL)
dst_if = dst_p->brt_if;
if (dst_if == NULL || ETHER_IS_MULTICAST(eh->ether_dhost)) {
- struct bridge_iflist *bif;
+ struct bridge_iflist *bif, *bif0;
struct mbuf *mc;
int used = 0;
@@ -818,9 +817,9 @@ bridge_output(struct ifnet *ifp, struct
}
}
- ifl = (struct bridge_iflist *)dst_if->if_bridgeport;
- KASSERT(ifl != NULL);
- if (bridge_filterrule(>bif_brlout, eh, mc) ==
+ bif0 = (struct bridge_iflist *)dst_if->if_bridgeport;
+ KASSERT(bif0 != NULL);
+ if (bridge_filterrule(>bif_brlout, eh, mc) ==
BRL_ACTION_BLOCK)
continue;
@@ -883,7 +882,7 @@ void
bridgeintr_frame(struct bridge_softc *sc, struct ifnet *src_if, struct mbuf *m)
{
struct ifnet *dst_if;
- struct bridge_iflist *ifl;
+ struct bridge_iflist *bif;
struct bridge_rtnode *dst_p;
struct ether_addr *dst, *src;
struct ether_header eh;
@@ -894,8 +893,8 @@ bridgeintr_frame(struct bridge_softc *sc
sc->sc_if.if_ipackets++;
sc->sc_if.if_ibytes += m->m_pkthdr.len;
- ifl = (struct bridge_iflist *)src_if->if_bridgeport;
- KASSERT(ifl != NULL);
+ bif = (struct bridge_iflist *)src_if->if_bridgeport;
+ KASSERT(bif != NULL);
if (m->m_pkthdr.len < sizeof(eh)) {
m_freem(m);
@@ -909,15
syslogd line escaping
Hi, I have been asked twice whether syslogd(8) is escaping log data. Should we document it? bluhm Index: usr.sbin/syslogd/syslogd.8 === RCS file: /data/mirror/openbsd/cvs/src/usr.sbin/syslogd/syslogd.8,v retrieving revision 1.59 diff -u -p -r1.59 syslogd.8 --- usr.sbin/syslogd/syslogd.8 2 Sep 2018 14:32:12 - 1.59 +++ usr.sbin/syslogd/syslogd.8 26 Sep 2018 19:18:17 - @@ -225,6 +225,9 @@ and from The message sent to .Nm should consist of a single line. +Embedded new line characters are converted to spaces, binary data +is encoded by +.Xr vis 3 . The message can contain a priority code, which should be a preceding decimal number in angle braces, for example, .Dq <5> .
Re: unveil(2) tcpdump(8)
I'm not too worried about the crash, I was playing with removing rpath
from pledge in the case that /etc/ethers wasn't need which sure enough
it is. pledge was most likely the reason that made it crash.
But brynet@ pointed out that while he was working on tcpdump last year
he saw that it also needs to open /etc/rpc, and yes it calls
getrpcbynumber(3) so an updated diff is below.
Index: privsep.c
===
RCS file: /cvs/src/usr.sbin/tcpdump/privsep.c,v
retrieving revision 1.48
diff -u -p -u -r1.48 privsep.c
--- privsep.c 8 Aug 2018 22:57:12 - 1.48
+++ privsep.c 26 Sep 2018 17:04:25 -
@@ -207,7 +207,7 @@ __dead void
priv_exec(int argc, char *argv[])
{
int bpfd = -1;
- int i, sock, cmd, nflag = 0, Pflag = 0;
+ int i, sock, cmd, nflag = 0, oflag = 0, Pflag = 0;
char *cmdbuf, *infile = NULL;
char *RFileName = NULL;
char *WFileName = NULL;
@@ -229,6 +229,10 @@ priv_exec(int argc, char *argv[])
nflag++;
break;
+ case 'o':
+ oflag = 1;
+ break;
+
case 'r':
RFileName = optarg;
break;
@@ -305,6 +309,14 @@ priv_exec(int argc, char *argv[])
test_state(cmd, STATE_RUN);
impl_init_done(sock, );
+ if (oflag) {
+ if (unveil("/etc/pf.os", "r") == -1)
+ err(1, "unveil");
+ }
+ if (unveil("/etc/ethers", "r") == -1)
+ err(1, "unveil");
+ if (unveil("/etc/rpc", "r") == -1)
+ err(1, "unveil");
if (pledge("stdio rpath inet dns recvfd bpf", NULL) ==
-1)
err(1, "pledge");
On 08:58 Wed 26 Sep , Theo de Raadt wrote:
> Ricardo Mestre wrote:
>
> > Hi,
> >
> > This has been shown internally for some time, but deraadt@ asked me to show
> > it
> > to a bigger audience now so here it is!
> >
> > If we want OS fingerprinting by using -o flag then we can unveil /etc/pf.os
> > in
> > read mode, nevertheless in order to do this we need to inform the privsep
> > proc
> > that we are using -o so I added it to priv_exec().
>
> looks right
>
> > The other file needed to be unveiled is /etc/ethers in read mode, which I
> > tried
> > to make it conditional but after several successful tests I bumped into a
> > packet which made tcpdump crash after some time. Unfortunately I don't have
> > the
> > core nor the pcap files to investigate what happen so for now the unveil of
> > this file will be kept unconditional regardless of the flags or expression
> > used.
>
> It is very likely that a protocol parser will find a MAC address nested
> inside, and try to parse it via /etc/ethers. So makes sense, and there
> is little risk in exposing ethers
>
> There is far more risk that some other file is required, and will fail to
> open silently, and tcpdump willbehave differently
>
> You know well: unveil requires a different audit approach than pledge
>
> But it is rare for a missing file via unveil to crash a program, so something
> is fishy. A crash isn't good, perhaps try to reproduce and collect a corefile
> using the sysctl kern.nosuidcoredump=3 method.
>
Re: bgpd ROA validation
On Tue, Sep 25, 2018 at 12:23:48PM +0200, Claudio Jeker wrote:
> On Sat, Sep 22, 2018 at 09:48:24PM +, Job Snijders wrote:
> > Hi claudio,
> >
> > Seems we are getting very close. Some suggestions to simplify the
> > experience for the end user.
> >
> > Let's start with supporting just one (unnamed) roa-set, so far I've
> > really not come across a use case where multiple ROA tables are useful.
> > I say this having implemented origin validation on both ISPs and IXPs.
> >
> > The semantics of the Origin Validation procedure that apply to
> > "Validated ROA Payloads" are not compatible with how the ARIN WHOIS
> > OriginAS semantics, so roa-set will never be used for ARIN WHOIS data.
>
> Please explain further, because honestly both the ruleset that
> arouteserver generates for ARIN WHOIS OriginAS and ROA are doing
> the same. They connect a source-as with a prefix. This is what a
> roa-set is giving you. It implements a way to quickly lookup a 2 key
> element (AS + prefix). Depending on how this table is used it can be used
> for both cases. This is the technical view based on looking at rulesets and
> thinking on how to write them in a way that is fast and understandable.
> I understand that from an administration point of view RPKI ROA is much
> stronger and can therefor be used more strictly (e.g. with the simple rule
> deny quick from ebgp roa-set RPKI invalid). The generic origin validatiation
> as a supporting measure to IRR based prefixset filtering is only allowing
> additional prefixes based on the roa-set (e.g. match from ebgp \
> large-community $INTCOMM_ORIGIN_OK roa-set ARINDB valid \
> set large-community $INTCOMM_PREF_OK). They are two different things but
> can use the same filter building blocks.
> Maybe naming it roa-set is wrong (since it is too much connected with
> RPKI) but the lookup table is usable for more than just RPKI. This is why
> I think supporting multiple tables is benefitial. I also agree that a
> simplified user experience for RPKI is a good thing, it should be simple
> to use.
>
> > There currently is 1 RPKI, and therefor we should have just 1 roa-set.
>
> I would fully agree here if ARIN would make their trust anchor freely
> distributable. But yes in general only one RPKI table is needed.
> My point is that roa-set is more generic than RPKI. It is not an RPKI only
> thing. It can be used for more than that and we should support this as
> well since it makes for much better and efficient configs.
>
> > The advantage of having only one roa-set is that it does not need to be
> > referenced in the policies.
> >
> > I propose the patch is amended to accomodate the following:
> >
> > roa-set {
> > 165.254.255.0/24 source-as 15562
> > 193.0.0.0/21 source-as
> > }
> >
> > deny from any ovs invalid
> > match from any ovs valid set community local-as:42
> > match from any ovs unknown set community local-as:43
> >
> > I suggest the filter keyword 'ovs' (origin validation state) is
> > introduced to allow easy matching. I think this looks better. If the
> > roa-set is not defined or empty, all route announcements are 'unknown'.
>
> If we want to use ovs then the naming scheme should be the same as for the
> extended community. At least if we reuse this keyword it should work the
> same. Also the side-effect is that the two configs look fairly similar
> which can be good or bad:
> match from any ovs valid set community local-as:42
> match from any ext-community ovs valid set community local-as:42
>
> I'm not against this, just wanted to bring it up.
>
> Also I think unknown should be renamed not-found. I will do that since
> not-found is what the RFC is using.
>
> I will rework the diff considering the input.
Here we go. This changes the diff so there is only one roa-set like
mentioned above:
roa-set {
165.254.255.0/24 source-as 15562
193.0.0.0/21 source-as
}
deny from any ovs invalid
match from any ovs valid set community local-as:42
match from any ovs unknown set community local-as:43
Additionally I left the old sets around but used a bit different:
origin-set FOO {
...
}
match from any community $ORIGIN_AS_OK origin-set FOO valid \
set community $ORIGIN_PREF_OK
I may even consider to drop the 'valid' in the above rule since that is
the only check that kind of matters in this case.
--
:wq Claudio
Index: bgpd.c
===
RCS file: /cvs/src/usr.sbin/bgpd/bgpd.c,v
retrieving revision 1.202
diff -u -p -r1.202 bgpd.c
--- bgpd.c 25 Sep 2018 07:58:11 - 1.202
+++ bgpd.c 26 Sep 2018 16:14:05 -
@@ -506,15 +506,15 @@ reconfigure(char *conffile, struct bgpd_
return (-1);
/* prefixsets for filters in the RDE */
- while ((ps = SIMPLEQ_FIRST(conf->prefixsets)) != NULL) {
unveil DPRINTF()
Hello,
As done in other parts of the kernel, introduce DPRINTF() macro
to unveil. I think this is worth doing because the code is slightly
more readable. OK?
- Michael
Index: kern_unveil.c
===
RCS file: /cvs/src/sys/kern/kern_unveil.c,v
retrieving revision 1.15
diff -u -p -u -r1.15 kern_unveil.c
--- kern_unveil.c 25 Sep 2018 19:24:17 - 1.15
+++ kern_unveil.c 26 Sep 2018 15:02:51 -
@@ -36,6 +36,11 @@
#include
/* #define DEBUG_UNVEIL */
+#ifdef DEBUG_UNVEIL
+#define DPRINTF(x...) printf(x)
+#else
+#define DPRINTF(x...)
+#endif
#define UNVEIL_MAX_VNODES 128
#define UNVEIL_MAX_NAMES 128
@@ -111,9 +116,7 @@ unveil_delete_names(struct unveil *uv)
ret++;
}
rw_exit_write(>uv_lock);
-#ifdef DEBUG_UNVEIL
- printf("deleted %d names\n", ret);
-#endif
+ DPRINTF("deleted %d names\n", ret);
return ret;
}
@@ -126,9 +129,7 @@ unveil_add_name(struct unveil *uv, char
unvn = unvname_new(name, strlen(name) + 1, flags);
RBT_INSERT(unvname_rbt, >uv_names, unvn);
rw_exit_write(>uv_lock);
-#ifdef DEBUG_UNVEIL
- printf("added name %s underneath vnode %p\n", name, uv->uv_vp);
-#endif
+ DPRINTF("added name %s underneath vnode %p\n", name, uv->uv_vp);
}
struct unvname *
@@ -138,10 +139,8 @@ unveil_namelookup(struct unveil *uv, cha
rw_enter_read(>uv_lock);
-#ifdef DEBUG_UNVEIL
- printf("unveil_namelookup: looking up name %s (%p) in vnode %p\n",
+ DPRINTF("unveil_namelookup: looking up name %s (%p) in vnode %p\n",
name, name, uv->uv_vp);
-#endif
KASSERT(uv->uv_vp != NULL);
@@ -175,11 +174,9 @@ unveil_destroy(struct process *ps)
/* skip any vnodes zapped by unveil_removevnode */
if (vp != NULL) {
vp->v_uvcount--;
-#ifdef DEBUG_UNVEIL
- printf("unveil: %s(%d): removing vnode %p uvcount %d "
+ DPRINTF("unveil: %s(%d): removing vnode %p uvcount %d "
"in position %ld\n",
ps->ps_comm, ps->ps_pid, vp, vp->v_uvcount, i);
-#endif
vrele(vp);
}
ps->ps_uvncount -= unveil_delete_names(uv);
@@ -270,10 +267,8 @@ unveil_lookup(struct vnode *vp, struct p
/* clear the cwd unveil when we .. past it */
if (pr->ps_uvpcwd && (vp == pr->ps_uvpcwd->uv_vp)) {
-#ifdef DEBUG_UNVEIL
- printf("unveil: %s(%d): nuking cwd traversing vnode %p\n",
+ DPRINTF("unveil: %s(%d): nuking cwd traversing vnode %p\n",
p->p_p->ps_comm, p->p_p->ps_pid, vp);
-#endif
p->p_p->ps_uvpcwd = NULL;
p->p_p->ps_uvpcwdgone = 0;
}
@@ -292,10 +287,8 @@ unveil_lookup(struct vnode *vp, struct p
r = pr->ps_uvvcount - 1;
while (l <= r) {
size_t m = l + (r - l)/2;
-#ifdef DEBUG_UNVEIL
- printf("unveil: checking vnode %p vs. unveil vnode %p\n",
+ DPRINTF("unveil: checking vnode %p vs. unveil vnode %p\n",
vp, uv[m].uv_vp);
-#endif
if (vp == uv[m].uv_vp) {
KASSERT(uv[m].uv_vp->v_uvcount > 0);
KASSERT(uv[m].uv_vp->v_usecount > 0);
@@ -342,9 +335,7 @@ unveil_setflags(u_char *flags, u_char nf
{
#if 0
if (((~(*flags)) & nflags) != 0) {
-#ifdef DEBUG_UNVEIL
- printf("Flags escalation %llX -> %llX\n", *flags, nflags);
-#endif
+ DPRINTF("Flags escalation %llX -> %llX\n", *flags, nflags);
return 1;
}
#endif
@@ -451,11 +442,9 @@ unveil_add(struct proc *p, struct nameid
* unrestrict it.
*/
if (directory_add) {
-#ifdef DEBUG_UNVEIL
- printf("unveil: %s(%d): updating directory vnode %p"
+ DPRINTF("unveil: %s(%d): updating directory vnode %p"
" to unrestricted uvcount %d\n",
pr->ps_comm, pr->ps_pid, vp, vp->v_uvcount);
-#endif
if (!unveil_setflags(>uv_flags, flags))
ret = EPERM;
else
@@ -471,12 +460,10 @@ unveil_add(struct proc *p, struct nameid
struct unvname *tname;
if ((tname = unveil_namelookup(uv,
ndp->ni_cnd.cn_nameptr)) != NULL) {
-#ifdef DEBUG_UNVEIL
- printf("unveil: %s(%d): changing flags for %s"
+ DPRINTF("unveil: %s(%d): changing flags for %s"
"in vnode %p, uvcount %d\n",
pr->ps_comm, pr->ps_pid, tname->un_name, vp,
vp->v_uvcount);
-#endif
Re: unveil(2) tcpdump(8)
Ricardo Mestre wrote: > Hi, > > This has been shown internally for some time, but deraadt@ asked me to show it > to a bigger audience now so here it is! > > If we want OS fingerprinting by using -o flag then we can unveil /etc/pf.os in > read mode, nevertheless in order to do this we need to inform the privsep proc > that we are using -o so I added it to priv_exec(). looks right > The other file needed to be unveiled is /etc/ethers in read mode, which I > tried > to make it conditional but after several successful tests I bumped into a > packet which made tcpdump crash after some time. Unfortunately I don't have > the > core nor the pcap files to investigate what happen so for now the unveil of > this file will be kept unconditional regardless of the flags or expression > used. It is very likely that a protocol parser will find a MAC address nested inside, and try to parse it via /etc/ethers. So makes sense, and there is little risk in exposing ethers There is far more risk that some other file is required, and will fail to open silently, and tcpdump willbehave differently You know well: unveil requires a different audit approach than pledge But it is rare for a missing file via unveil to crash a program, so something is fishy. A crash isn't good, perhaps try to reproduce and collect a corefile using the sysctl kern.nosuidcoredump=3 method.
Re: [patch]Modify the example code in write(2) manual
Hi, Nan Xiao wrote on Wed, Sep 26, 2018 at 09:42:02PM +0800: > Any developer can comment on this patch? Thanks! I think this change is a bad idea and should not be committed. No matter whether or not it can happen on OpenBSD, *if* some implementation of write(2) sometimes returns 0 even for nbytes > 0, your change is likely to cause an infinite busy loop, which is quite bad. In that case, the current code would print: program: write: Undefined error: 0 That's admittedly not totally informative, but we can't do much better because it is indeed not quite clear which kind of problem the return value of 0 might indicate. And in any case, that edge case seems uncommon enough that complicating the example code to handle the case of a return value of zero separately would also seem like a bad idea. Besides, the idiom documented here is actually widely used in the tree, and it is indeed recommended, see for example /usr/src/bin/cat/cat.c Yours, Ingo > I am reading write(2) manual, and come across the following example: > > for (off = 0; off < bsz; off += nw) > if ((nw = write(d, buf + off, bsz - off)) == 0 || nw == -1) > err(1, "write"); > > I am just wondering when the write(2) will return 0? If in some cases, > it will indeed return 0, according to the manual: > > Upon successful completion the number of bytes which were written is > returned. Otherwise, a -1 is returned and the global variable errno is > set to indicate the error. > > Because the errno is only set when return value is -1, if write(2) > returns 0, the errno should be an undefined value, and "err(1, > "write");" also won't print correct information. > > If write(2) won't return 0, my following patch fixes the example code: > > diff --git write.2 write.2 > index c1686b1a910..db134959002 100644 > --- write.2 > +++ write.2 > @@ -164,7 +164,7 @@ ssize_t nw; > int d; > > for (off = 0; off < bsz; off += nw) > - if ((nw = write(d, buf + off, bsz - off)) == 0 || nw == -1) > + if ((nw = write(d, buf + off, bsz - off)) == -1) > err(1, "write"); > .Ed > .Sh ERRORS
getent: adjust alignment in hostsprint()
hostsprint() reserves only 16 columns for IPs and prints one whitespace too many afterwards: $ getent hosts 1.1.1.1 long ::::::: 1.1.1.1 one.one.one.one ::::::: long ::::::: long This diff cranks it up to 39 as per hostsaddrinfo() and aligns nicely: $ ./obj/getent hosts 1.1.1.1 long ::::::: 1.1.1.1 one.one.one.one ::::::: long ::::::: long OK? Index: getent.c === RCS file: /cvs/src/usr.bin/getent/getent.c,v retrieving revision 1.18 diff -u -p -r1.18 getent.c --- getent.c25 Sep 2018 19:51:39 - 1.18 +++ getent.c26 Sep 2018 13:48:49 - @@ -225,7 +225,7 @@ hostsprint(const struct hostent *he) if (inet_ntop(he->h_addrtype, he->h_addr, buf, sizeof(buf)) == NULL) strlcpy(buf, "# unknown", sizeof(buf)); - printfmtstrings(he->h_aliases, " ", " ", "%-16s %s", buf, he->h_name); + printfmtstrings(he->h_aliases, " ", " ", "%-39s %s", buf, he->h_name); } static int hostsaddrinfo(const char *name)
Re: [patch]Modify the example code in write(2) manual
ping tech@, Any developer can comment on this patch? Thanks! On 9/25/2018 10:10 PM, Nan Xiao wrote: > Hi tech@, > > I am reading write(2) manual, and come across the following example: > > for (off = 0; off < bsz; off += nw) > if ((nw = write(d, buf + off, bsz - off)) == 0 || nw == -1) > err(1, "write"); > > I am just wondering when the write(2) will return 0? If in some cases, > it will indeed return 0, according to the manual: > >> Upon successful completion the number of bytes which were written is > returned. Otherwise, a -1 is returned and the global variable errno is > set to indicate the error. > > Because the errno is only set when return value is -1, if write(2) > returns 0, the errno should be an undefined value, and "err(1, > "write");" also won't print correct information. > > If write(2) won't return 0, my following patch fixes the example code: > > diff --git write.2 write.2 > index c1686b1a910..db134959002 100644 > --- write.2 > +++ write.2 > @@ -164,7 +164,7 @@ ssize_t nw; > int d; > > for (off = 0; off < bsz; off += nw) > - if ((nw = write(d, buf + off, bsz - off)) == 0 || nw == -1) > + if ((nw = write(d, buf + off, bsz - off)) == -1) > err(1, "write"); > .Ed > .Sh ERRORS > > Thanks! > -- Best Regards Nan Xiao(肖楠)
Re: getent: use more appropiate types/limits around strtonum()
On Wed, Sep 26, 2018 at 06:48:07AM -0600, Todd C. Miller wrote:
> One comment inline, otherwise OK millert@
> > @@ -397,6 +397,9 @@ static int
> > services(int argc, char *argv[])
> > {
> > struct servent *se;
> > + const char *err;
> > + char*proto;
> > + int serv;
>
> This should be in_port_t, not int. I would also call the variable
> port instead of serv but the type is what is important.
Missed that one, thanks.
I'll commit it with `in_port_t port' later this evening unless someone
objects.
Re: getent: use more appropiate types/limits around strtonum()
On Wed, 26 Sep 2018 00:44:18 +0200, Klemens Nanni wrote:
> Replace `long long id' with appropiate types and names, use smaller
> limits where applicable and move variable declarations up out of loops.
>
> This makes the code clearer and a tad simpler while staying consistent
> across databases.
One comment inline, otherwise OK millert@
- todd
> Index: getent.c
> ===
> RCS file: /cvs/src/usr.bin/getent/getent.c,v
> retrieving revision 1.18
> diff -u -p -r1.18 getent.c
> --- getent.c 25 Sep 2018 19:51:39 - 1.18
> +++ getent.c 25 Sep 2018 22:21:22 -
> @@ -190,8 +190,10 @@ ethers(int argc, char *argv[])
> static int
> group(int argc, char *argv[])
> {
> - int i, rv = RV_OK;
> struct group*gr;
> + const char *err;
> + gid_t gid;
> + int i, rv = RV_OK;
>
> setgroupent(1);
> if (argc == 2) {
> @@ -199,11 +201,9 @@ group(int argc, char *argv[])
> GROUPPRINT;
> } else {
> for (i = 2; i < argc; i++) {
> - const char *err;
> - long long id = strtonum(argv[i], 0, UINT_MAX, );
> -
> + gid = strtonum(argv[i], 0, GID_MAX, );
> if (!err)
> - gr = getgrgid((gid_t)id);
> + gr = getgrgid(gid);
> else
> gr = getgrnam(argv[i]);
> if (gr != NULL)
> @@ -291,8 +291,10 @@ hosts(int argc, char *argv[])
> static int
> passwd(int argc, char *argv[])
> {
> - int i, rv = RV_OK;
> struct passwd *pw;
> + const char *err;
> + uid_t uid;
> + int i, rv = RV_OK;
>
> setpassent(1);
> if (argc == 2) {
> @@ -300,11 +302,9 @@ passwd(int argc, char *argv[])
> PASSWDPRINT;
> } else {
> for (i = 2; i < argc; i++) {
> - const char *err;
> - long long id = strtonum(argv[i], 0, UINT_MAX, );
> -
> + uid = strtonum(argv[i], 0, UID_MAX, );
> if (!err)
> - pw = getpwuid((uid_t)id);
> + pw = getpwuid(uid);
> else
> pw = getpwnam(argv[i]);
> if (pw != NULL)
> @@ -327,6 +327,8 @@ static int
> protocols(int argc, char *argv[])
> {
> struct protoent *pe;
> + const char *err;
> + int proto;
> int i, rv = RV_OK;
>
> setprotoent(1);
> @@ -335,11 +337,9 @@ protocols(int argc, char *argv[])
> PROTOCOLSPRINT;
> } else {
> for (i = 2; i < argc; i++) {
> - const char *err;
> - long long id = strtonum(argv[i], 0, UINT_MAX, );
> -
> + proto = strtonum(argv[i], 0, INT_MAX, );
> if (!err)
> - pe = getprotobynumber((int)id);
> + pe = getprotobynumber(proto);
> else
> pe = getprotobyname(argv[i]);
> if (pe != NULL)
> @@ -362,6 +362,8 @@ static int
> rpc(int argc, char *argv[])
> {
> struct rpcent *re;
> + const char *err;
> + int rpc;
> int i, rv = RV_OK;
>
> setrpcent(1);
> @@ -370,11 +372,9 @@ rpc(int argc, char *argv[])
> RPCPRINT;
> } else {
> for (i = 2; i < argc; i++) {
> - const char *err;
> - long long id = strtonum(argv[i], 0, UINT_MAX, );
> -
> + rpc = strtonum(argv[i], 0, INT_MAX, );
> if (!err)
> - re = getrpcbynumber((int)id);
> + re = getrpcbynumber(rpc);
> else
> re = getrpcbyname(argv[i]);
> if (re != NULL)
> @@ -397,6 +397,9 @@ static int
> services(int argc, char *argv[])
> {
> struct servent *se;
> + const char *err;
> + char*proto;
> + int serv;
This should be in_port_t, not int. I would also call the variable
port instead of serv but the type is what is important.
> int i, rv = RV_OK;
>
> setservent(1);
> @@ -405,15 +408,11 @@ services(int argc, char *argv[])
> SERVICESPRINT;
> } else {
> for (i = 2; i < argc; i++) {
> - const char *err;
> - long long id;
> - char *proto = strchr(argv[i], '/');
> -
> - if (proto != NULL)
> + if ((proto =
Re: fix usermod -l
On Wed, 26 Sep 2018 10:51:28 +0100, Ricardo Mestre wrote: > While doing something else here I noticed that changing the login name of an > existing user with usermod -l the program gets a segmentation fault. > > This looks like it was introduced when millert@ changed pwcache and the fix > is a matter of changing getpwnam(3) to uid_from_user(3). OK millert@ - todd
fix usermod -l
Hi,
While doing something else here I noticed that changing the login name of an
existing user with usermod -l the program gets a segmentation fault.
This looks like it was introduced when millert@ changed pwcache and the fix is
a matter of changing getpwnam(3) to uid_from_user(3).
OK?
Index: user.c
===
RCS file: /cvs/src/usr.sbin/user/user.c,v
retrieving revision 1.121
diff -u -p -u -r1.121 user.c
--- user.c 13 Sep 2018 15:23:32 - 1.121
+++ user.c 26 Sep 2018 09:34:07 -
@@ -1369,6 +1369,7 @@ moduser(char *login_name, char *newlogin
int ptmpfd;
int rval;
int i;
+ uid_t uid;
if (!valid_login(newlogin)) {
errx(EXIT_FAILURE, "`%s' is not a valid login name",
login_name);
@@ -1427,7 +1428,8 @@ moduser(char *login_name, char *newlogin
if (up != NULL) {
if (up->u_flags & F_USERNAME) {
/* if changing name, check new name isn't already in
use */
- if (strcmp(login_name, newlogin) != 0 &&
getpwnam(newlogin) != NULL) {
+ if (strcmp(login_name, newlogin) != 0 &&
+ uid_from_user(newlogin, ) != -1) {
close(ptmpfd);
pw_abort();
errx(EXIT_FAILURE, "already a `%s' user",
newlogin);
Re: imsg over network
On Fri, Sep 14, 2018 at 01:57:13PM -0700, Geoff Hill wrote: > The imsg_init(3) man page currently doesn't make it clear whether > this library can be used for remote communication. > > The current text reads: > > The imsg functions provide a simple mechanism for communication > between processes using sockets. Each transmitted message is > guaranteed to be presented to the receiving program whole. They > are commonly used in privilege separated processes, where > processes with different rights are required to cooperate. > > It also mentions socketpair(2) later but does not specify whether > this would work over tcp(4) connections. > > Looking at the libutil imsg code, it doesn't look like there's any > htonl(3) or byte order swapping when accessing the sizes, so this was > probably not intended for network use. > > So my questions are: > not that i know anything about this subject, but according to nicm: > (a) Am I correct in understanding imsg cannot be used to >communicate over networks in an architecture-independent way? > yes > (b) Should the manual page be updated to reflect this reality, > so users can better compare possible IPC mechanisms? > possibly (i did) > (c) Should the library itself be updated to use network byte > order, so it could be used between different architectures > over an established TCP connection? > pass (from me, not nicm) > Simplest possible patch for (b) below. > committed, thanks. jmc > Index: imsg_init.3 > === > RCS file: /cvs/src/lib/libutil/imsg_init.3,v > retrieving revision 1.21 > diff -u -p -u -r1.21 imsg_init.3 > --- imsg_init.3 16 Feb 2018 07:42:07 - 1.21 > +++ imsg_init.3 14 Sep 2018 20:53:10 - > @@ -106,7 +106,7 @@ > .Sh DESCRIPTION > The > .Nm imsg > -functions provide a simple mechanism for communication between processes > +functions provide a simple mechanism for communication between local > processes > using sockets. > Each transmitted message is guaranteed to be presented to the receiving > program > whole. >
unveil(2) tcpdump(8)
Hi,
This has been shown internally for some time, but deraadt@ asked me to show it
to a bigger audience now so here it is!
If we want OS fingerprinting by using -o flag then we can unveil /etc/pf.os in
read mode, nevertheless in order to do this we need to inform the privsep proc
that we are using -o so I added it to priv_exec().
The other file needed to be unveiled is /etc/ethers in read mode, which I tried
to make it conditional but after several successful tests I bumped into a
packet which made tcpdump crash after some time. Unfortunately I don't have the
core nor the pcap files to investigate what happen so for now the unveil of
this file will be kept unconditional regardless of the flags or expression
used.
Could you please test tcpdump on your network with this patch? You should test
several different flags and the different combinations between them just as I
did, and please also try different expressions then report back if you had any
issues or not and if this can go in.
Index: privsep.c
===
RCS file: /cvs/src/usr.sbin/tcpdump/privsep.c,v
retrieving revision 1.48
diff -u -p -u -r1.48 privsep.c
--- privsep.c 8 Aug 2018 22:57:12 - 1.48
+++ privsep.c 26 Sep 2018 06:57:20 -
@@ -207,7 +207,7 @@ __dead void
priv_exec(int argc, char *argv[])
{
int bpfd = -1;
- int i, sock, cmd, nflag = 0, Pflag = 0;
+ int i, sock, cmd, nflag = 0, oflag = 0, Pflag = 0;
char *cmdbuf, *infile = NULL;
char *RFileName = NULL;
char *WFileName = NULL;
@@ -229,6 +229,10 @@ priv_exec(int argc, char *argv[])
nflag++;
break;
+ case 'o':
+ oflag = 1;
+ break;
+
case 'r':
RFileName = optarg;
break;
@@ -305,6 +309,12 @@ priv_exec(int argc, char *argv[])
test_state(cmd, STATE_RUN);
impl_init_done(sock, );
+ if (oflag) {
+ if (unveil("/etc/pf.os", "r") == -1)
+ err(1, "unveil");
+ }
+ if (unveil("/etc/ethers", "r") == -1)
+ err(1, "unveil");
if (pledge("stdio rpath inet dns recvfd bpf", NULL) ==
-1)
err(1, "pledge");
Re: ral(4): add RT3290 support
On Mon, Sep 17, 2018 at 09:34:06PM -0400, James Hastings wrote: > > > Ported from original vendor driver. > RT3290 is similar to RT5390 but integrates WLAN + Bluetooth on single chip. > Bluetooth not supported. First off, thank you for working on this. I tested it on amd64, ifconfig ral0 up will cause system hang totally. The adapter is good since it works on Linux *shrug*. > New 4kb firmware at /etc/firmware/ral-rt3290 for this chip only. > New routines to read efuse rom and control wlan core. > Tested on RT3090 and RT5390. ^^ Typo? Did you test on RT3290? Thanks.
Re: unveil manpage tweak 2
On Wed, Sep 26, 2018 at 01:49:00PM +0800, Michael Mikonos wrote: > Re-reading the unveil manual I found a typo which isn't flagged > by a spell checker. Does anyone prefer just doing s/paths// though? > morning. ok for your diff - no opinion on the s/paths// suggestion. jmc > > Index: unveil.2 > === > RCS file: /cvs/src/lib/libc/sys/unveil.2,v > retrieving revision 1.10 > diff -u -p -u -r1.10 unveil.2 > --- unveil.2 26 Sep 2018 02:54:34 - 1.10 > +++ unveil.2 26 Sep 2018 05:44:09 - > @@ -108,7 +108,7 @@ This means that a directory that is remo > .Fn unveil > will appear to not exist. > .Pp > -Non-directories paths are remembered by name within their containing > +Non-directory paths are remembered by name within their containing > directory, and so may be created, removed, or re-created after a call to > .Fn unveil > and still appear to exist. >
