Re: signify: -z implies -q
On 2019-07-26 03.02.43 +, Mike Burns wrote: > Poking around signify and learned that `verifyzdata` calls `verifymsg` > with `1` hardcoded in the `quiet` parameter. > > I do appreciate that there is a distinction between case 'z' setting > `quiet = 1` vs `verifyzdata` passing `1` as an argument, so maybe this > diff doesn't quiet capture a truth: Now that I've played with signify more, it's clear that mentioning -q is quite far from from a complete statement of what -z is all about, Forget about this diff, apologies.
Re: Remove some sigvec Xr's
On Thu, Jul 25, 2019 at 1:04 PM Todd C. Miller wrote: > We should only cross-reference the obsolete sigvec(3) function from > the signal compat manuals and sigaction(2). > > This also syncs the SEE ALSO section in ualarm(3) match that of > alarm(3). > ok guenther@ We could reference signal(3) in csh instead of sigaction(2) if > that's what people prefer. I see no reason to prefer signal(3) over sigaction(2) for pages that aren't about C-standard functions. Hmm: sh(1) and ksh(1) have *nothing* from sections 2 or 3 in their SEE ALSO. That doesn't seem like a wrong choice, albeit inconsistent with csh(1). Part of me feels like _if_ they're going to mention umask(2), setrlimit(2), and sigaction(2), then they should mention chdir(2), as the other classic "must be in the shell" syscall. Philip Guenther
Re: remove duplicate definitions of LAPIC_ID_MASK and LAPIC_ID_SHIFT
On Thu, Jul 25, 2019 at 4:44 PM Kevin Lo wrote: > ok? > Yes, please. guenther@
signify: -z implies -q
Poking around signify and learned that `verifyzdata` calls `verifymsg` with `1` hardcoded in the `quiet` parameter. I do appreciate that there is a distinction between case 'z' setting `quiet = 1` vs `verifyzdata` passing `1` as an argument, so maybe this diff doesn't quiet capture a truth: Index: signify.1 === RCS file: /cvs/src/usr.bin/signify/signify.1,v retrieving revision 1.47 diff -u -p -r1.47 signify.1 --- signify.1 8 May 2019 17:55:41 - 1.47 +++ signify.1 26 Jul 2019 02:56:10 - @@ -129,7 +129,8 @@ Sign and verify archives, where the signing data is embedded in the .Xr gzip 1 -header. +header. Implies +.Fl q . .El .Pp The key and signature files created by
remove duplicate definitions of LAPIC_ID_MASK and LAPIC_ID_SHIFT
ok? Index: sys/arch/amd64/include/i82489reg.h === RCS file: /cvs/src/sys/arch/amd64/include/i82489reg.h,v retrieving revision 1.4 diff -u -p -u -p -r1.4 i82489reg.h --- sys/arch/amd64/include/i82489reg.h 21 Jul 2015 04:48:33 - 1.4 +++ sys/arch/amd64/include/i82489reg.h 26 Jul 2019 01:36:01 - @@ -105,8 +105,6 @@ #define LAPIC_ICRHI0x310 /* Int. cmd. RW */ -# define LAPIC_ID_MASK0x0f00 -# define LAPIC_ID_SHIFT 24 #define LAPIC_LVTT 0x320 /* Loc.vec.(timer) RW */ # define LAPIC_LVTT_VEC_MASK 0x00ff Index: sys/arch/i386/include/i82489reg.h === RCS file: /cvs/src/sys/arch/i386/include/i82489reg.h,v retrieving revision 1.4 diff -u -p -u -p -r1.4 i82489reg.h --- sys/arch/i386/include/i82489reg.h 21 Jul 2015 06:19:50 - 1.4 +++ sys/arch/i386/include/i82489reg.h 26 Jul 2019 01:36:01 - @@ -105,8 +105,6 @@ #define LAPIC_ICRHI0x310 /* Int. cmd. RW */ -# define LAPIC_ID_MASK0x0f00 -# define LAPIC_ID_SHIFT 24 #define LAPIC_LVTT 0x320 /* Loc.vec.(timer) RW */ # define LAPIC_LVTT_VEC_MASK 0x00ff
Remove some sigvec Xr's
We should only cross-reference the obsolete sigvec(3) function from the signal compat manuals and sigaction(2). This also syncs the SEE ALSO section in ualarm(3) match that of alarm(3). We could reference signal(3) in csh instead of sigaction(2) if that's what people prefer. - todd Index: lib/libc/gen/ualarm.3 === RCS file: /cvs/src/lib/libc/gen/ualarm.3,v retrieving revision 1.16 diff -u -p -u -r1.16 ualarm.3 --- lib/libc/gen/ualarm.3 5 Jun 2013 03:39:22 - 1.16 +++ lib/libc/gen/ualarm.3 25 Jul 2019 21:57:07 - @@ -71,13 +71,11 @@ The maximum value for allowed is 2147483647. .Sh SEE ALSO -.Xr getitimer 2 , .Xr setitimer 2 , .Xr sigaction 2 , +.Xr sigsuspend 2 , .Xr alarm 3 , .Xr signal 3 , -.Xr sigpause 3 , -.Xr sigvec 3 , .Xr sleep 3 , .Xr usleep 3 .Sh STANDARDS Index: usr.bin/lastcomm/lastcomm.1 === RCS file: /cvs/src/usr.bin/lastcomm/lastcomm.1,v retrieving revision 1.21 diff -u -p -u -r1.21 lastcomm.1 --- usr.bin/lastcomm/lastcomm.1 25 Jul 2019 19:05:22 - 1.21 +++ usr.bin/lastcomm/lastcomm.1 25 Jul 2019 21:51:19 - @@ -132,7 +132,7 @@ default accounting file .El .Sh SEE ALSO .Xr last 1 , -.Xr sigvec 3 , +.Xr sigaction 2 , .Xr acct 5 , .Xr core 5 , .Xr accton 8 Index: bin/csh/csh.1 === RCS file: /cvs/src/bin/csh/csh.1,v retrieving revision 1.81 diff -u -p -u -r1.81 csh.1 --- bin/csh/csh.1 2 Jun 2019 06:53:11 - 1.81 +++ bin/csh/csh.1 25 Jul 2019 21:51:37 - @@ -2708,10 +2708,10 @@ source of home directories for .Xr fork 2 , .Xr pipe 2 , .Xr setrlimit 2 , +.Xr sigaction 2 , .Xr umask 2 , .Xr wait 2 , .Xr killpg 3 , -.Xr sigvec 3 , .Xr tty 4 , .Xr environ 7 , .Xr script 7
Re: itimerdecr(): simplify code
On Thu, 25 Jul 2019 11:00:03 -0500, Scott Cheloha wrote: > > However, if the "itp->it_value.tv_sec >= 0" is true, that means > > that "itp->it_value.tv_sec" must also be true. > > which is not correct, as itp->it_value.tv_sec could be zero. Sigh, still too early I guess. > What we really want to check is: > > if (itp->it_value.tv_sec > 0 || > (itp->it_value.tv_sec == 0 && itp->it_value.tv_usec > 0)) > > which is nearly equivalent to the expansion above. The expansion > above ignores the sign of itp->it_value.tv_usec, so, not exactly the > same. Right. - todd
Re: itimerdecr(): simplify code
On Thu, Jul 25, 2019 at 09:37:20AM -0600, Todd C. Miller wrote: > On Thu, 25 Jul 2019 09:53:42 -0500, Scott Cheloha wrote: > > > Woohoo, slow morning, nearly missed that bug. Your conditional > > is incorrect because it rejects timevals of whole seconds, e.g. > > tv_sec == 1 and tv_usec == 0. > > Whoops, too early for me I guess. However, as I think about this > further, can't this be simplified even more? > > Given: > > if (itp->it_value.tv_sec >= 0 && timerisset(>it_value)) > > that expands to: > > if (itp->it_value.tv_sec >= 0 && > (itp->it_value.tv_sec || itp->it_value.tv_usec)) > > However, if the "itp->it_value.tv_sec >= 0" is true, that means > that "itp->it_value.tv_sec" must also be true. So logically it > is the equivalent: > > if (itp->it_value.tv_sec >= 0 && (1 || itp->it_value.tv_usec)) > > Which is can be simplied to just: > > if (itp->it_value.tv_sec >= 0) > > So the timerisset() call appears to be superfluous. I see no case > where "itp->it_value.tv_sec >= 0" is true that timerisset(>it_value) > is not also true. > > Am I missing something? This is indeed the expansion: > if (itp->it_value.tv_sec >= 0 && > (itp->it_value.tv_sec || itp->it_value.tv_usec)) But then you say: > However, if the "itp->it_value.tv_sec >= 0" is true, that means > that "itp->it_value.tv_sec" must also be true. which is not correct, as itp->it_value.tv_sec could be zero. What we really want to check is: if (itp->it_value.tv_sec > 0 || (itp->it_value.tv_sec == 0 && itp->it_value.tv_usec > 0)) which is nearly equivalent to the expansion above. The expansion above ignores the sign of itp->it_value.tv_usec, so, not exactly the same.
Re: itimerdecr(): simplify code
On Thu, 25 Jul 2019 09:53:42 -0500, Scott Cheloha wrote: > Woohoo, slow morning, nearly missed that bug. Your conditional > is incorrect because it rejects timevals of whole seconds, e.g. > tv_sec == 1 and tv_usec == 0. Whoops, too early for me I guess. However, as I think about this further, can't this be simplified even more? Given: if (itp->it_value.tv_sec >= 0 && timerisset(>it_value)) that expands to: if (itp->it_value.tv_sec >= 0 && (itp->it_value.tv_sec || itp->it_value.tv_usec)) However, if the "itp->it_value.tv_sec >= 0" is true, that means that "itp->it_value.tv_sec" must also be true. So logically it is the equivalent: if (itp->it_value.tv_sec >= 0 && (1 || itp->it_value.tv_usec)) Which is can be simplied to just: if (itp->it_value.tv_sec >= 0) So the timerisset() call appears to be superfluous. I see no case where "itp->it_value.tv_sec >= 0" is true that timerisset(>it_value) is not also true. Am I missing something? - todd
Re: itimerdecr(): simplify code
On Thu, 25 Jul 2019 09:47:53 -0500, Scott Cheloha wrote: > I've been thinking for a while about adding another macro to sys/time.h > to handle this very common case, "timerisexpired": > > #define timerisexpired(tv) ((tv)->tv_sec < 0 || !timerisset((tv))) > > for chipping away at relative timeouts. Then we could write > > if (!timerisexpired(>it_value)) > > ... which is precisely what we want to express. That would make it easier to read for sure. - todd
add pkg-config files for readline, editline, ncurses
mrsh[1], a cross-platform shell, can use readline in interactive mode. It's configure script detects the presence of readline using pkg-config(1). Thus, this patch adds a pkg-config file for our readline. I just copied over the generate_pkgconfig.sh script/make rules found in other libraries and edited to fit. I also added pkg-config files for libedit and libcurses, since libedit requires linking with libcurses. I started getting build errors before I realised that there are a couple editline libraries floating around, each differing slightly with each other: NetBSD editline - Provides , . - Uses ncurses. OpenBSD editline - Provides . - Uses ncurses. Jess Thrysøe editline[2] - Cross platform autotool port of NetBSD editline. - Provides and - Uses ncurses. Joachim Nilsson editline[3] - Updated version of Rich Salz/Simmule Turner editline. - Call compatible with GNU Readline. - Provides . - Doesn't require ncurses. A very confusing mess. Some Linux systems (e.g. Arch) generate pkg-config files for the different "types" of ncurses: /usr/lib/pkgconfig/ncurses++.pc -> ncurses++w.pc /usr/lib/pkgconfig/ncurses++w.pc /usr/lib/pkgconfig/ncurses.pc -> ncursesw.pc /usr/lib/pkgconfig/ncursesw.pc /usr/lib/pkgconfig/tic.pc -> ncursesw.pc /usr/lib/pkgconfig/tinfo.pc -> ncursesw.pc Not sure if this is something I should do here. [1] https://git.sr.ht/~emersion/mrsh [2] https://thrysoee.dk/editline/ [3] https://github.com/troglobit/editline Index: lib/libedit/Makefile === RCS file: /cvs/src/lib/libedit/Makefile,v retrieving revision 1.32 diff -u -p -r1.32 Makefile --- lib/libedit/Makefile15 Jan 2019 01:54:00 - 1.32 +++ lib/libedit/Makefile25 Jul 2019 14:00:00 - @@ -19,6 +19,9 @@ LIBEDITDIR?=${.CURDIR} INCS= histedit.h INCSDIR=/usr/include +PC_FILES=libedit.pc +CLEANFILES+=${PC_FILES} + CLEANFILES+=common.h.tmp emacs.h.tmp fcns.h.tmp func.h.tmp CLEANFILES+=help.h.tmp vi.h.tmp tc1.o tc1 @@ -78,6 +81,14 @@ includes: /dev/null 2>&1 || \ ${INSTALL} ${INSTALL_COPY} -o ${BINOWN} -g ${BINGRP} -m 444 histedit.h \ ${DESTDIR}/usr/include + +all: ${PC_FILES} +${PC_FILES}: histedit.h + /bin/sh ${.CURDIR}/generate_pkgconfig.sh -c ${.CURDIR} -o ${.OBJDIR} + +beforeinstall: + ${INSTALL} ${INSTALL_COPY} -o root -g ${SHAREGRP} \ + -m ${SHAREMODE} ${.OBJDIR}/${PC_FILES} ${DESTDIR}/usr/lib/pkgconfig/ .include .include Index: lib/libedit/generate_pkgconfig.sh === RCS file: lib/libedit/generate_pkgconfig.sh diff -N lib/libedit/generate_pkgconfig.sh --- /dev/null 1 Jan 1970 00:00:00 - +++ lib/libedit/generate_pkgconfig.sh 25 Jul 2019 14:00:00 - @@ -0,0 +1,71 @@ +#!/bin/sh +# +# $OpenBSD$ +# +# Copyright (c) 2010,2011 Jasper Lievisse Adriaanse +# +# Permission to use, copy, modify, and distribute this software for any +# purpose with or without fee is hereby granted, provided that the above +# copyright notice and this permission notice appear in all copies. +# +# THE SOFTWARE IS PROVIDED "AS IS" AND THE AUTHOR DISCLAIMS ALL WARRANTIES +# WITH REGARD TO THIS SOFTWARE INCLUDING ALL IMPLIED WARRANTIES OF +# MERCHANTABILITY AND FITNESS. IN NO EVENT SHALL THE AUTHOR BE LIABLE FOR +# ANY SPECIAL, DIRECT, INDIRECT, OR CONSEQUENTIAL DAMAGES OR ANY DAMAGES +# WHATSOEVER RESULTING FROM LOSS OF USE, DATA OR PROFITS, WHETHER IN AN +# ACTION OF CONTRACT, NEGLIGENCE OR OTHER TORTIOUS ACTION, ARISING OUT OF +# OR IN CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE. +# +# Generate pkg-config file for libedit. + +usage() { + echo "usage: ${0##*/} -c current_directory -o obj_directory" + exit 1 +} + +curdir= +objdir= +while getopts "c:o:" flag; do + case "$flag" in + c) + curdir=$OPTARG + ;; + o) + objdir=$OPTARG + ;; + *) + usage + ;; + esac +done + +[ -n "${curdir}" ] || usage +if [ ! -d "${curdir}" ]; then + echo "${0##*/}: ${curdir}: not found" + exit 1 +fi +[ -n "${objdir}" ] || usage +if [ ! -w "${objdir}" ]; then + echo "${0##*/}: ${objdir}: not found or not writable" + exit 1 +fi + +version_major_re="s/^#define[[:blank:]]LIBEDIT_MAJOR[[:blank:]]([0-9]+).*/\1/p" +version_minor_re="s/^#define[[:blank:]]LIBEDIT_MINOR[[:blank:]]([0-9]+).*/\1/p" +version_file=${curdir}/histedit.h +lib_version=$(sed -nE ${version_major_re} ${version_file}).$(sed -nE ${version_minor_re} ${version_file}) + +pc_file="${objdir}/libedit.pc" +cat > ${pc_file} << __EOF__ +prefix=/usr +exec_prefix=\${prefix} +libdir=\${exec_prefix}/lib +includedir=\${prefix}/include + +Name: editline +Description: line editor, history and tokenization library +Version:
bgpd, change order of filter_set imsgs
This changes the way filtersets are passed on config reloads (and also for bgpctl network commands). Now the sets are sent first and then moved into the object that follows the filter_set with TAILQ_CONCAT(). The benefit of this is that IMSG_RECONF_FILTER will have the full rule ready when inserting it, allowing to build a per peer output filter. The integration tests still pass and bgpctl network also works :) OK? -- :wq Claudio Index: bgpd.c === RCS file: /cvs/src/usr.sbin/bgpd/bgpd.c,v retrieving revision 1.222 diff -u -p -r1.222 bgpd.c --- bgpd.c 24 Jul 2019 20:25:27 - 1.222 +++ bgpd.c 24 Jul 2019 21:15:29 - @@ -644,11 +644,11 @@ reconfigure(char *conffile, struct bgpd_ /* filters for the RDE */ while ((r = TAILQ_FIRST(conf->filters)) != NULL) { TAILQ_REMOVE(conf->filters, r, entry); + if (send_filterset(ibuf_rde, >set) == -1) + return (-1); if (imsg_compose(ibuf_rde, IMSG_RECONF_FILTER, 0, 0, -1, r, sizeof(struct filter_rule)) == -1) return (-1); - if (send_filterset(ibuf_rde, >set) == -1) - return (-1); filterset_free(>set); free(r); } @@ -669,18 +669,18 @@ reconfigure(char *conffile, struct bgpd_ return (-1); /* export targets */ + if (send_filterset(ibuf_rde, >export) == -1) + return (-1); if (imsg_compose(ibuf_rde, IMSG_RECONF_VPN_EXPORT, 0, 0, -1, NULL, 0) == -1) return (-1); - if (send_filterset(ibuf_rde, >export) == -1) - return (-1); filterset_free(>export); /* import targets */ + if (send_filterset(ibuf_rde, >import) == -1) + return (-1); if (imsg_compose(ibuf_rde, IMSG_RECONF_VPN_IMPORT, 0, 0, -1, NULL, 0) == -1) - return (-1); - if (send_filterset(ibuf_rde, >import) == -1) return (-1); filterset_free(>import); Index: rde.c === RCS file: /cvs/src/usr.sbin/bgpd/rde.c,v retrieving revision 1.479 diff -u -p -r1.479 rde.c --- rde.c 24 Jul 2019 20:25:27 - 1.479 +++ rde.c 25 Jul 2019 10:16:31 - @@ -354,9 +354,10 @@ rde_main(int debug, int verbose) exit(0); } -struct network_config netconf_s, netconf_p; -struct filterstate netconf_state; -struct filter_set_head *session_set, *parent_set; +struct network_config netconf_s, netconf_p; +struct filterstate netconf_state; +struct filter_set_head session_set = TAILQ_HEAD_INITIALIZER(session_set); +struct filter_set_head parent_set = TAILQ_HEAD_INITIALIZER(parent_set); void rde_dispatch_imsg_session(struct imsgbuf *ibuf) @@ -461,7 +462,6 @@ rde_dispatch_imsg_session(struct imsgbuf } memcpy(_s, imsg.data, sizeof(netconf_s)); TAILQ_INIT(_s.attrset); - session_set = _s.attrset; rde_filterstate_prep(_state, NULL, NULL, NULL, 0); asp = _state.aspath; @@ -518,7 +518,7 @@ rde_dispatch_imsg_session(struct imsgbuf log_warnx("rde_dispatch: wrong imsg len"); break; } - session_set = NULL; + TAILQ_CONCAT(_s.attrset, _set, entry); switch (netconf_s.prefix.aid) { case AID_INET: if (netconf_s.prefixlen > 32) @@ -582,17 +582,12 @@ badnetdel: log_warnx("rde_dispatch: wrong imsg len"); break; } - if (session_set == NULL) { - log_warnx("rde_dispatch: " - "IMSG_FILTER_SET unexpected"); - break; - } if ((s = malloc(sizeof(struct filter_set))) == NULL) fatal(NULL); memcpy(s, imsg.data, sizeof(struct filter_set)); if (s->type == ACTION_SET_NEXTHOP) s->action.nh = nexthop_get(>action.nexthop); - TAILQ_INSERT_TAIL(session_set, s, entry); + TAILQ_INSERT_TAIL(_set, s, entry); break; case IMSG_CTL_SHOW_NETWORK: case IMSG_CTL_SHOW_RIB: @@ -752,10 +747,9 @@ rde_dispatch_imsg_parent(struct imsgbuf
Re: itimerdecr(): simplify code
On Thu, Jul 25, 2019 at 09:47:53AM -0500, Scott Cheloha wrote: > On Thu, Jul 25, 2019 at 08:33:25AM -0600, Todd C. Miller wrote: > > On Wed, 24 Jul 2019 19:05:16 -0500, Scott Cheloha wrote: > > > > > We can simplify the itimerdecr() code with the sys/time.h macros. > > > I think this is a lot easier to decipher. > > > > > > With the macros we don't need special logic to correctly handle the > > > reload if the decrement exceeds the time remaining in the itimer. > > > > > > With the loop we now correctly handle decrements larger than our > > > interval. Maybe someone can make use of that. Using the loop instead > > > of just an if-clause doesn't complicate our code so I figure we might > > > as well do the right thing in that case. > > > > This looks correct and is much shorter. I do find mixing explicit > > checks of tv_sec with timerisset a little confusing, but maybe that > > is just me. > > > > For example, instead of this: > > > > if (itp->it_value.tv_sec >= 0 && timerisset(>it_value)) > > > > I find the following easier to read: > > > > if (itp->it_value.tv_sec >= 0 && itp->it_value.tv_usec > 0) > > > > (yes, I know the tv_usec could just be != 0). *ahem* Woohoo, slow morning, nearly missed that bug. Your conditional is incorrect because it rejects timevals of whole seconds, e.g. tv_sec == 1 and tv_usec == 0. So I will not be using it. This actually lends credence to the utility of a "timerisexpired" macro, if only because people look at conditionals like that and think they might be equivalent. Food for thought.
Re: itimerdecr(): simplify code
On Thu, Jul 25, 2019 at 08:33:25AM -0600, Todd C. Miller wrote: > On Wed, 24 Jul 2019 19:05:16 -0500, Scott Cheloha wrote: > > > We can simplify the itimerdecr() code with the sys/time.h macros. > > I think this is a lot easier to decipher. > > > > With the macros we don't need special logic to correctly handle the > > reload if the decrement exceeds the time remaining in the itimer. > > > > With the loop we now correctly handle decrements larger than our > > interval. Maybe someone can make use of that. Using the loop instead > > of just an if-clause doesn't complicate our code so I figure we might > > as well do the right thing in that case. > > This looks correct and is much shorter. I do find mixing explicit > checks of tv_sec with timerisset a little confusing, but maybe that > is just me. > > For example, instead of this: > > if (itp->it_value.tv_sec >= 0 && timerisset(>it_value)) > > I find the following easier to read: > > if (itp->it_value.tv_sec >= 0 && itp->it_value.tv_usec > 0) > > (yes, I know the tv_usec could just be != 0). I've been thinking for a while about adding another macro to sys/time.h to handle this very common case, "timerisexpired": #define timerisexpired(tv) ((tv)->tv_sec < 0 || !timerisset((tv))) for chipping away at relative timeouts. Then we could write if (!timerisexpired(>it_value)) ... which is precisely what we want to express. But there aren't loads of bugs caused by people miswriting this check so it's still on the drawing board. Absent that I think yours reads better.
Re: itimerdecr(): simplify code
On Wed, 24 Jul 2019 19:05:16 -0500, Scott Cheloha wrote: > We can simplify the itimerdecr() code with the sys/time.h macros. > I think this is a lot easier to decipher. > > With the macros we don't need special logic to correctly handle the > reload if the decrement exceeds the time remaining in the itimer. > > With the loop we now correctly handle decrements larger than our > interval. Maybe someone can make use of that. Using the loop instead > of just an if-clause doesn't complicate our code so I figure we might > as well do the right thing in that case. This looks correct and is much shorter. I do find mixing explicit checks of tv_sec with timerisset a little confusing, but maybe that is just me. For example, instead of this: if (itp->it_value.tv_sec >= 0 && timerisset(>it_value)) I find the following easier to read: if (itp->it_value.tv_sec >= 0 && itp->it_value.tv_usec > 0) (yes, I know the tv_usec could just be != 0). OK millert@ either way, - todd
Re: unveil in process accounting and lastcomm
On Thu, Jul 25, 2019 at 10:06:52AM -0400, Bryan Steele wrote: > On Thu, Jul 18, 2019 at 05:44:21PM -0400, Bryan Steele wrote: > > On Thu, Jul 18, 2019 at 11:46:46AM -0400, Bryan Steele wrote: > > > On Thu, Jul 18, 2019 at 04:13:10PM +0200, Alexander Bluhm wrote: > > > > Hi, > > > > > > > > Can we track unveil(2) violators in process accounting lastcomm(1)? > > > > This makes it easier to find them. > > > > > > > > $ lastcomm | grep -e '-[A-Z]U' > > > > pflogd -FU root__ 0.00 secs Thu Jul 18 14:19 > > > > (2:33:22.00) > > > > > > > > Seems that pflogd(8) has to be investigated. > > > > > > Interesting. > > > > > > This appears to be a false positive, libpcap will first attempt to open > > > /dev/bpf as O_RDWR and then falls back to O_RDONLY on EACCES. I'm fairly > > > confident that pflogd(8) does not need write access to bpf. If anything, > > > unveil(2) appears to have found an old bug here, as before pflogd was > > > always opening the device with both read/write permissions. > > > > > > tcpdump avoids this by internalizing more parts of libpcap, and also > > > opening /dev/bpf O_RDONLY itself. > > > > > > spamlogd appears to be the only other user of pcap_open_live() in base, > > > unfortunately it calls unveil after, so /dev/bpf is opened O_RDWR. I > > > don't use spamlogd, though. > > > > Here's a potential diff for both pflogd and spamlogd, I've tested it > > works with pflogd. I only compile tested spamlogd. But it should avoid > > the unveil accounting errors. > > > > This duplicates a lot of code into both, but I don't feel like trying > > to extent the libpcap API for this. > > > > comments? ok? > > With bluhm@'s unveil accounting diff, is anyone ok wit this this > approach to fixing the issue? These are the only pcap_open_live(3) > consumers in base, but I don't feel comfortable trying to extend > the libpcap API, and pretty much everything is already poking at > libpcap internals due to bad API design for privsep. > > -Bryan. Here's a new diff that renames the local function to avoid conflicting with future attempts at improving the libpcap API. Requested by deraadt@ -Bryan. Index: pflogd.c === RCS file: /cvs/src/sbin/pflogd/pflogd.c,v retrieving revision 1.59 diff -u -p -u -r1.59 pflogd.c --- sbin/pflogd/pflogd.c26 Aug 2018 18:24:46 - 1.59 +++ sbin/pflogd/pflogd.c25 Jul 2019 14:23:14 - @@ -73,6 +73,7 @@ void dump_packet(u_char *, const struct void dump_packet_nobuf(u_char *, const struct pcap_pkthdr *, const u_char *); int flush_buffer(FILE *); int if_exists(char *); +pcap_t *pflog_read_live(const char *, int, int, int, char *); void logmsg(int, const char *, ...); void purge_buffer(void); int reset_dump(void); @@ -194,10 +195,95 @@ if_exists(char *ifname) return (if_nametoindex(ifname) != 0); } +pcap_t * +pflog_read_live(const char *source, int slen, int promisc, int to_ms, +char *ebuf) +{ + int fd; + struct bpf_version bv; + struct ifreqifr; + u_int v, dlt = DLT_PFLOG; + pcap_t *p; + + if (source == NULL || slen <= 0) + return (NULL); + + p = pcap_create(source, ebuf); + if (p == NULL) + return (NULL); + + /* Open bpf(4) read only */ + if ((fd = open("/dev/bpf", O_RDONLY)) == -1) + return (NULL); + + if (ioctl(fd, BIOCVERSION, ) == -1) { + snprintf(ebuf, PCAP_ERRBUF_SIZE, "BIOCVERSION: %s", + pcap_strerror(errno)); + goto bad; + } + + if (bv.bv_major != BPF_MAJOR_VERSION || + bv.bv_minor < BPF_MINOR_VERSION) { + snprintf(ebuf, PCAP_ERRBUF_SIZE, + "kernel bpf filter out of date"); + goto bad; + } + + strlcpy(ifr.ifr_name, source, sizeof(ifr.ifr_name)); + if (ioctl(fd, BIOCSETIF, ) == -1) { + snprintf(ebuf, PCAP_ERRBUF_SIZE, "BIOCSETIF: %s", + pcap_strerror(errno)); + goto bad; + } + + if (dlt != (u_int) -1 && ioctl(fd, BIOCSDLT, )) { + snprintf(ebuf, PCAP_ERRBUF_SIZE, "BIOCSDLT: %s", + pcap_strerror(errno)); + goto bad; + } + + p->fd = fd; + p->snapshot = slen; + p->linktype = dlt; + + /* set timeout */ + if (to_ms != 0) { + struct timeval to; + to.tv_sec = to_ms / 1000; + to.tv_usec = (to_ms * 1000) % 100; + if (ioctl(p->fd, BIOCSRTIMEOUT, ) == -1) { + snprintf(ebuf, PCAP_ERRBUF_SIZE, "BIOCSRTIMEOUT: %s", + pcap_strerror(errno)); + } + } + if (promisc) + /* this is allowed to fail */ + ioctl(fd, BIOCPROMISC, NULL); + + if (ioctl(fd, BIOCGBLEN, ) == -1) { +
Re: unveil in process accounting and lastcomm
On Thu, Jul 18, 2019 at 05:44:21PM -0400, Bryan Steele wrote: > On Thu, Jul 18, 2019 at 11:46:46AM -0400, Bryan Steele wrote: > > On Thu, Jul 18, 2019 at 04:13:10PM +0200, Alexander Bluhm wrote: > > > Hi, > > > > > > Can we track unveil(2) violators in process accounting lastcomm(1)? > > > This makes it easier to find them. > > > > > > $ lastcomm | grep -e '-[A-Z]U' > > > pflogd -FU root__ 0.00 secs Thu Jul 18 14:19 > > > (2:33:22.00) > > > > > > Seems that pflogd(8) has to be investigated. > > > > Interesting. > > > > This appears to be a false positive, libpcap will first attempt to open > > /dev/bpf as O_RDWR and then falls back to O_RDONLY on EACCES. I'm fairly > > confident that pflogd(8) does not need write access to bpf. If anything, > > unveil(2) appears to have found an old bug here, as before pflogd was > > always opening the device with both read/write permissions. > > > > tcpdump avoids this by internalizing more parts of libpcap, and also > > opening /dev/bpf O_RDONLY itself. > > > > spamlogd appears to be the only other user of pcap_open_live() in base, > > unfortunately it calls unveil after, so /dev/bpf is opened O_RDWR. I > > don't use spamlogd, though. > > Here's a potential diff for both pflogd and spamlogd, I've tested it > works with pflogd. I only compile tested spamlogd. But it should avoid > the unveil accounting errors. > > This duplicates a lot of code into both, but I don't feel like trying > to extent the libpcap API for this. > > comments? ok? With bluhm@'s unveil accounting diff, is anyone ok wit this this approach to fixing the issue? These are the only pcap_open_live(3) consumers in base, but I don't feel comfortable trying to extend the libpcap API, and pretty much everything is already poking at libpcap internals due to bad API design for privsep. I'd appreciate if someone could test spamlogd(8) still works. -Bryan. Index: pflogd.c === RCS file: /cvs/src/sbin/pflogd/pflogd.c,v retrieving revision 1.59 diff -u -p -r1.59 pflogd.c --- sbin/pflogd/pflogd.c26 Aug 2018 18:24:46 - 1.59 +++ sbin/pflogd/pflogd.c25 Jul 2019 14:00:02 - @@ -73,6 +73,7 @@ void dump_packet(u_char *, const struct void dump_packet_nobuf(u_char *, const struct pcap_pkthdr *, const u_char *); int flush_buffer(FILE *); int if_exists(char *); +pcap_t *pcap_live(const char *, int, int, int, char *); void logmsg(int, const char *, ...); void purge_buffer(void); int reset_dump(void); @@ -194,10 +195,95 @@ if_exists(char *ifname) return (if_nametoindex(ifname) != 0); } +pcap_t * +pcap_live(const char *source, int slen, int promisc, int to_ms, +char *ebuf) +{ + int fd; + struct bpf_version bv; + struct ifreqifr; + u_int v, dlt = DLT_PFLOG; + pcap_t *p; + + if (source == NULL || slen <= 0) + return (NULL); + + p = pcap_create(source, ebuf); + if (p == NULL) + return (NULL); + + /* Open bpf(4) read only */ + if ((fd = open("/dev/bpf", O_RDONLY)) == -1) + return (NULL); + + if (ioctl(fd, BIOCVERSION, ) == -1) { + snprintf(ebuf, PCAP_ERRBUF_SIZE, "BIOCVERSION: %s", + pcap_strerror(errno)); + goto bad; + } + + if (bv.bv_major != BPF_MAJOR_VERSION || + bv.bv_minor < BPF_MINOR_VERSION) { + snprintf(ebuf, PCAP_ERRBUF_SIZE, + "kernel bpf filter out of date"); + goto bad; + } + + strlcpy(ifr.ifr_name, source, sizeof(ifr.ifr_name)); + if (ioctl(fd, BIOCSETIF, ) == -1) { + snprintf(ebuf, PCAP_ERRBUF_SIZE, "BIOCSETIF: %s", + pcap_strerror(errno)); + goto bad; + } + + if (dlt != (u_int) -1 && ioctl(fd, BIOCSDLT, )) { + snprintf(ebuf, PCAP_ERRBUF_SIZE, "BIOCSDLT: %s", + pcap_strerror(errno)); + goto bad; + } + + p->fd = fd; + p->snapshot = slen; + p->linktype = dlt; + + /* set timeout */ + if (to_ms != 0) { + struct timeval to; + to.tv_sec = to_ms / 1000; + to.tv_usec = (to_ms * 1000) % 100; + if (ioctl(p->fd, BIOCSRTIMEOUT, ) == -1) { + snprintf(ebuf, PCAP_ERRBUF_SIZE, "BIOCSRTIMEOUT: %s", + pcap_strerror(errno)); + } + } + if (promisc) + /* this is allowed to fail */ + ioctl(fd, BIOCPROMISC, NULL); + + if (ioctl(fd, BIOCGBLEN, ) == -1) { + snprintf(ebuf, PCAP_ERRBUF_SIZE, "BIOCGBLEN: %s", + pcap_strerror(errno)); + goto bad; + } + p->bufsize = v; + p->buffer = malloc(p->bufsize); + if (p->buffer == NULL) { +
Re: unveil in process accounting and lastcomm
Todd C. Miller wrote: > On Thu, 25 Jul 2019 12:00:48 +0200, Alexander Bluhm wrote: > > > Do we want unveil violators in the daily mail? We can turn it off > > if we get too many false positives. > > I think so. If it becomes annoying we can turn it off by default. I'm not sure how any of these could be false positives. They will all show that the software is trying to access something it shouldn't.
Re: unveil in process accounting and lastcomm
On Thu, 25 Jul 2019 12:00:48 +0200, Alexander Bluhm wrote: > Do we want unveil violators in the daily mail? We can turn it off > if we get too many false positives. I think so. If it becomes annoying we can turn it off by default. - todd
Re: zero tmpkeyiv in openssl enc
Sure, looks fine in this micro context so maybe someone copying code from here as an example will do better. Though note that wider-scoped key/iv still contain the key material after EVP_CipherInit_ex and so on. Doesn't appear we've sprinkled many explicit_bzero's into openssl(1) in general given its short lifetime for most operations. I'm not sure how paranoid we need to be in this context, but I'll apply this. On Tue, Jul 23, 2019 at 3:54 PM Steven Roberts wrote: > Hi, > > This patch for openssl enc will zero out tmpkeyiv which contains key > information. > > Thanks. > > Index: enc.c > === > RCS file: /cvs/src/usr.bin/openssl/enc.c,v > retrieving revision 1.21 > diff -u -p -u -r1.21 enc.c > --- enc.c 14 Jul 2019 03:30:45 - 1.21 > +++ enc.c 22 Jul 2019 16:53:20 - > @@ -633,6 +633,8 @@ enc_main(int argc, char **argv) > /* split and move data back to global > buffer */ > memcpy(key, tmpkeyiv, iklen); > memcpy(iv, tmpkeyiv+iklen, ivlen); > + /* zero the tmpkeyiv buffer */ > + explicit_bzero(tmpkeyiv, sizeof(tmpkeyiv)); > } else { > EVP_BytesToKey(enc_config.cipher, dgst, > sptr, > (unsigned char *)enc_config.keystr, > >
Re: unveil in process accounting and lastcomm
On Thu, Jul 25, 2019 at 12:00:48PM +0200, Alexander Bluhm wrote: > Do we want unveil violators in the daily mail? We can turn it off > if we get too many false positives. Janne Johansson recommend to mention lastcomm(1) in unveil(2) man page. Diff for daily, lastcomm(1), unveil(2). Kernel has been commited already. bluhm Index: etc/daily === RCS file: /data/mirror/openbsd/cvs/src/etc/daily,v retrieving revision 1.91 diff -u -p -r1.91 daily --- etc/daily 6 Feb 2018 19:57:37 - 1.91 +++ etc/daily 25 Jul 2019 09:56:20 - @@ -74,7 +74,7 @@ if [ -f /var/account/acct ]; then mv -f /var/account/acct.0 /var/account/acct.1 cp -f /var/account/acct /var/account/acct.0 sa -sq - lastcomm -f /var/account/acct.0 | grep -e ' -[A-Z]*[PT]' + lastcomm -f /var/account/acct.0 | grep -e ' -[A-Z]*[PTU]' fi # If ROOTBACKUP is set to 1 in the environment, and Index: usr.bin/lastcomm/lastcomm.1 === RCS file: /data/mirror/openbsd/cvs/src/usr.bin/lastcomm/lastcomm.1,v retrieving revision 1.19 diff -u -p -r1.19 lastcomm.1 --- usr.bin/lastcomm/lastcomm.1 27 Feb 2018 07:58:29 - 1.19 +++ usr.bin/lastcomm/lastcomm.1 25 Jul 2019 09:42:15 - @@ -115,10 +115,13 @@ indicates the command was terminated wit .Sq P indicates the command was terminated due to a .Xr pledge 2 -violation, and +violation, .Sq T indicates the command did a memory access violation detected by a -processor trap. +processor trap, and +.Sq U +indicates the command tried a file access that was prevented by +.Xr unveil 2 . .Sh FILES .Bl -tag -width /var/account/acct -compact .It Pa /var/account/acct Index: usr.bin/lastcomm/lastcomm.c === RCS file: /data/mirror/openbsd/cvs/src/usr.bin/lastcomm/lastcomm.c,v retrieving revision 1.27 diff -u -p -r1.27 lastcomm.c --- usr.bin/lastcomm/lastcomm.c 27 Feb 2018 07:58:29 - 1.27 +++ usr.bin/lastcomm/lastcomm.c 25 Jul 2019 09:41:34 - @@ -174,6 +174,7 @@ flagbits(int f) BIT(AXSIG, 'X'); BIT(APLEDGE, 'P'); BIT(ATRAP, 'T'); + BIT(AUNVEIL, 'U'); *p = '\0'; return (flags); } Index: lib/libc/sys/unveil.2 === RCS file: /data/mirror/openbsd/cvs/src/lib/libc/sys/unveil.2,v retrieving revision 1.17 diff -u -p -r1.17 unveil.2 --- lib/libc/sys/unveil.2 24 Mar 2019 19:55:31 - 1.17 +++ lib/libc/sys/unveil.2 25 Jul 2019 11:12:15 - @@ -132,6 +132,12 @@ use can be tricky because programs misbe unexpectedly disappear. In many cases it is easier to unveil the directories in which an application makes use of files. +After a process has terminated, +.Xr lastcomm 1 +will mark it with the +.Sq U +flag if file access was prevented by +.Nm unveil . .Sh RETURN VALUES .Rv -std .Sh ERRORS
Re: unveil in process accounting and lastcomm
On Thu, Jul 18, 2019 at 04:13:10PM +0200, Alexander Bluhm wrote: > $ lastcomm | grep -e '-[A-Z]U' > pflogd -FU root__ 0.00 secs Thu Jul 18 14:19 (2:33:22.00) Oops, I have forgotten to show the userland part of my diff. Do we want unveil violators in the daily mail? We can turn it off if we get too many false positives. ok? bluhm Index: etc/daily === RCS file: /data/mirror/openbsd/cvs/src/etc/daily,v retrieving revision 1.91 diff -u -p -r1.91 daily --- etc/daily 6 Feb 2018 19:57:37 - 1.91 +++ etc/daily 25 Jul 2019 09:56:20 - @@ -74,7 +74,7 @@ if [ -f /var/account/acct ]; then mv -f /var/account/acct.0 /var/account/acct.1 cp -f /var/account/acct /var/account/acct.0 sa -sq - lastcomm -f /var/account/acct.0 | grep -e ' -[A-Z]*[PT]' + lastcomm -f /var/account/acct.0 | grep -e ' -[A-Z]*[PTU]' fi # If ROOTBACKUP is set to 1 in the environment, and Index: usr.bin/lastcomm/lastcomm.1 === RCS file: /data/mirror/openbsd/cvs/src/usr.bin/lastcomm/lastcomm.1,v retrieving revision 1.19 diff -u -p -r1.19 lastcomm.1 --- usr.bin/lastcomm/lastcomm.1 27 Feb 2018 07:58:29 - 1.19 +++ usr.bin/lastcomm/lastcomm.1 25 Jul 2019 09:42:15 - @@ -115,10 +115,13 @@ indicates the command was terminated wit .Sq P indicates the command was terminated due to a .Xr pledge 2 -violation, and +violation, .Sq T indicates the command did a memory access violation detected by a -processor trap. +processor trap, and +.Sq U +indicates the command tried a file access that was prevented by +.Xr unveil 2 . .Sh FILES .Bl -tag -width /var/account/acct -compact .It Pa /var/account/acct Index: usr.bin/lastcomm/lastcomm.c === RCS file: /data/mirror/openbsd/cvs/src/usr.bin/lastcomm/lastcomm.c,v retrieving revision 1.27 diff -u -p -r1.27 lastcomm.c --- usr.bin/lastcomm/lastcomm.c 27 Feb 2018 07:58:29 - 1.27 +++ usr.bin/lastcomm/lastcomm.c 25 Jul 2019 09:41:34 - @@ -174,6 +174,7 @@ flagbits(int f) BIT(AXSIG, 'X'); BIT(APLEDGE, 'P'); BIT(ATRAP, 'T'); + BIT(AUNVEIL, 'U'); *p = '\0'; return (flags); }
bgpd RIB rework part 4
This is a bit bigger then planned but the change is a bit more substantial. First of all this removes 'route-collector yes|no' from the config. The old route-collector mode is removed because it causes confusion and the same can be done by using 'rde rib Loc-RIB no evaluate'. On top of this it is now possible to change those flags during runtime and the softreload process will take care and make sure all is applied correctly. Turning the evaluation process off and on again turned out to be a bit more tricky than expected. So a few additional changes had to be added to the decision process (making sure that an update received during the reload run does not leave behind pointers to freed objects). Please test and review :) -- :wq Claudio Index: bgpd.conf.5 === RCS file: /cvs/src/usr.sbin/bgpd/bgpd.conf.5,v retrieving revision 1.191 diff -u -p -r1.191 bgpd.conf.5 --- bgpd.conf.5 17 Jun 2019 14:00:45 - 1.191 +++ bgpd.conf.5 25 Jul 2019 09:07:22 - @@ -334,16 +334,6 @@ In this case the decision process is no The default is .Ic ignore . .Pp -.It Xo -.Ic route-collector -.Pq Ic yes Ns | Ns Ic no -.Xc -If set to -.Ic yes , -the route selection process is turned off. -The default is -.Ic no . -.Pp .It Ic router-id Ar address Set the router ID to the given IP address, which must be local to the machine. Index: bgpd.h === RCS file: /cvs/src/usr.sbin/bgpd/bgpd.h,v retrieving revision 1.390 diff -u -p -r1.390 bgpd.h --- bgpd.h 23 Jul 2019 06:26:44 - 1.390 +++ bgpd.h 25 Jul 2019 09:26:02 - @@ -58,10 +58,9 @@ #defineBGPD_OPT_NOACTION 0x0004 #defineBGPD_OPT_FORCE_DEMOTE 0x0008 -#defineBGPD_FLAG_NO_EVALUATE 0x0002 #defineBGPD_FLAG_REFLECTOR 0x0004 -#defineBGPD_FLAG_NEXTHOP_BGP 0x0080 -#defineBGPD_FLAG_NEXTHOP_DEFAULT 0x1000 +#defineBGPD_FLAG_NEXTHOP_BGP 0x0010 +#defineBGPD_FLAG_NEXTHOP_DEFAULT 0x0020 #defineBGPD_FLAG_DECISION_MASK 0x0f00 #defineBGPD_FLAG_DECISION_ROUTEAGE 0x0100 #defineBGPD_FLAG_DECISION_TRANS_AS 0x0200 Index: parse.y === RCS file: /cvs/src/usr.sbin/bgpd/parse.y,v retrieving revision 1.396 diff -u -p -r1.396 parse.y --- parse.y 24 Jul 2019 20:25:27 - 1.396 +++ parse.y 25 Jul 2019 09:01:04 - @@ -203,7 +203,7 @@ typedef struct { %token ANNOUNCE CAPABILITIES REFRESH AS4BYTE CONNECTRETRY %token DEMOTE ENFORCE NEIGHBORAS ASOVERRIDE REFLECTOR DEPEND DOWN %token DUMP IN OUT SOCKET RESTRICTED -%token LOG ROUTECOLL TRANSPARENT +%token LOG TRANSPARENT %token TCP MD5SIG PASSWORD KEY TTLSECURITY %token ALLOW DENY MATCH %token QUICK @@ -614,12 +614,6 @@ conf_main : AS as4number { else rr->flags &= ~F_RIB_NOFIBSYNC; } - | ROUTECOLL yesno { - if ($2 == 1) - conf->flags |= BGPD_FLAG_NO_EVALUATE; - else - conf->flags &= ~BGPD_FLAG_NO_EVALUATE; - } | TRANSPARENT yesno { if ($2 == 1) conf->flags |= BGPD_FLAG_DECISION_TRANS_AS; @@ -2843,7 +2837,6 @@ lookup(char *s) { "restricted", RESTRICTED}, { "rib",RIB}, { "roa-set",ROASET }, - { "route-collector",ROUTECOLL}, { "route-reflector",REFLECTOR}, { "router-id", ROUTERID}, { "rtable", RTABLE}, Index: printconf.c === RCS file: /cvs/src/usr.sbin/bgpd/printconf.c,v retrieving revision 1.138 diff -u -p -r1.138 printconf.c --- printconf.c 24 Jul 2019 20:25:27 - 1.138 +++ printconf.c 25 Jul 2019 09:02:30 - @@ -386,9 +386,6 @@ print_mainconf(struct bgpd_config *conf) if (conf->connectretry != INTERVAL_CONNECTRETRY) printf("connect-retry %u\n", conf->connectretry); - if (conf->flags & BGPD_FLAG_NO_EVALUATE) - printf("route-collector yes\n"); - if (conf->flags & BGPD_FLAG_DECISION_ROUTEAGE) printf("rde route-age evaluate\n"); Index: rde.c === RCS file: /cvs/src/usr.sbin/bgpd/rde.c,v retrieving revision 1.479 diff -u -p -r1.479 rde.c --- rde.c 24 Jul 2019 20:25:27 - 1.479 +++ rde.c 25 Jul 2019 09:02:09 - @@ -81,6 +81,9 @@ static voidrde_softreconfig_out_done(v static void rde_softreconfig_done(void); static void
Re: apmd: fix error message
On Wed, Jul 24, 2019 at 11:21:00PM +0200, Alexander Bluhm wrote: > There is one more %m: > > logmsg(LOG_ERR, "cannot fetch power status: %m"); Thanks, fixed to logmsg(LOG_ERR, "cannot fetch power status: %s", strerror(errno)); This leaves one occurence of "%m" in error() which is only used in the syslog(3) case.