Re: netcat: IPv6 address support for proxy
On Sun, Feb 05, 2017 at 12:27:19AM +0100, Jeremie Courreges-Anglas wrote: The colons used in IPv6 addresses conflicts with the proxy port specification. Do the right thing for -x ::1:8080, [::1] and [::1]:8080. With this patch '-x ::1' is still broken. I think we should either require/enforce square brackets for IPv6 addresses or allow all possible notations. Otherwise having '-x 127.0.0.1' working but not '-x ::1' seems both confusing and broken to me.
Re: pfctl: Fix function name in errx(3) message
On Mon, Mar 27, 2017 at 07:39:09PM +0200, Sebastian Benoit wrote: i commited a err(1, "calloc") instead. What about the other calls to calloc(3); shouldn't we keep their respective error messages consistent?
pfctl: Fix function name in errx(3) message
Index: pfctl.c
===
RCS file: /cvs/src/sbin/pfctl/pfctl.c,v
retrieving revision 1.338
diff -u -p -r1.338 pfctl.c
--- pfctl.c 26 Jan 2017 08:24:34 - 1.338
+++ pfctl.c 25 Mar 2017 11:37:01 -
@@ -753,7 +753,7 @@ pfctl_show_rules(int dev, char *path, in
memset(, 0, sizeof(pr));
if (anchorname[0] == '/') {
if ((npath = calloc(1, PATH_MAX)) == NULL)
- errx(1, "pfctl_rules: calloc");
+ errx(1, "pfctl_show_rules: calloc");
strlcpy(npath, anchorname, PATH_MAX);
} else {
if (path[0])
ifconfig.8: Small improvement, typofix
Hey, blocknonip's description is a tad clearer that way, the rest is mostly cosmetical but still. Index: sbin/ifconfig/ifconfig.8 === RCS file: /cvs/src/sbin/ifconfig/ifconfig.8,v retrieving revision 1.280 diff -u -p -r1.280 ifconfig.8 --- sbin/ifconfig/ifconfig.820 Mar 2017 14:06:26 - 1.280 +++ sbin/ifconfig/ifconfig.84 Apr 2017 09:33:37 - @@ -400,7 +400,7 @@ output from .Cm hwfeatures shows the maximum supported MTU. .It Cm netmask Ar mask -(inet and inet6) +(inet and inet6 only) Specify how much of the address to reserve for subdividing networks into subnetworks. The mask includes the network part of the local address @@ -508,7 +508,7 @@ Disable automatic point-to-point link de .It Cm blocknonip Ar interface Mark .Ar interface -so that no non-IPv4, IPv6, ARP, or Reverse +so that only IPv4, IPv6, ARP, and Reverse ARP packets are accepted from it or forwarded to it from other bridge member interfaces. .It Cm -blocknonip Ar interface @@ -824,7 +824,7 @@ The formula is + .Pf ( Cm advskew / 256). -If the master does not advertise within three times this interval, this host +If the master does not advertise this interval within three times, this host will begin advertising as master. .Sh IEEE 802.11 (WIRELESS DEVICES) .nr nS 1 @@ -1520,7 +1520,7 @@ Remove the trunk port Set the trunk protocol. Refer to .Xr trunk 4 -for a complete list of the available protocols, +for a complete list of the available protocols. .El .Sh TUNNEL .nr nS 1
sysctl.3: Fix file path
Index: lib/libc/gen/sysctl.3 === RCS file: /cvs/src/lib/libc/gen/sysctl.3,v retrieving revision 1.277 diff -u -p -r1.277 sysctl.3 --- lib/libc/gen/sysctl.3 16 Mar 2017 14:57:15 - 1.277 +++ lib/libc/gen/sysctl.3 14 Apr 2017 00:43:44 - @@ -1056,7 +1056,7 @@ Most architectures define at least the f .El .Pp Consult the example file -.Pa /etc/example/sysctl.conf +.Pa /etc/examples/sysctl.conf for a non-exhaustive list of .Li machdep variables.
sshd: Remove authorized_keys2 file
Now that protocol version 1 was finally dropped in sshd(8), get rid of
this file completely. Our default sshd_config(5) overwrites
AuthorizedKeysFile to ignore it anyway and sshd(8)'s FILES section
doesn't mention it either.
Index: etc/changelist
===
RCS file: /cvs/src/etc/changelist,v
retrieving revision 1.116
diff -u -p -r1.116 changelist
--- etc/changelist 27 Feb 2017 21:53:11 - 1.116
+++ etc/changelist 17 Apr 2017 19:26:47 -
@@ -147,7 +147,6 @@
/root/.rhosts
/root/.shosts
/root/.ssh/authorized_keys
-/root/.ssh/authorized_keys2
/var/cron/at.allow
/var/cron/at.deny
/var/cron/cron.allow
Index: usr.bin/ssh/pathnames.h
===
RCS file: /cvs/src/usr.bin/ssh/pathnames.h,v
retrieving revision 1.25
diff -u -p -r1.25 pathnames.h
--- usr.bin/ssh/pathnames.h 31 Mar 2016 05:24:06 - 1.25
+++ usr.bin/ssh/pathnames.h 17 Apr 2017 19:26:47 -
@@ -79,7 +79,7 @@
#define _PATH_SSH_USER_CONFFILE_PATH_SSH_USER_DIR "/config"
/*
- * File containing a list of those rsa keys that permit logging in as this
+ * File containing a list of those keys that permit logging in as this
* user. This file need not be readable by anyone but the user him/herself,
* but does not contain anything particularly secret. If the user's home
* directory resides on an NFS volume where root is mapped to nobody, this
@@ -87,9 +87,6 @@
* running as root.)
*/
#define _PATH_SSH_USER_PERMITTED_KEYS _PATH_SSH_USER_DIR "/authorized_keys"
-
-/* backward compat for protocol v2 */
-#define _PATH_SSH_USER_PERMITTED_KEYS2 _PATH_SSH_USER_DIR "/authorized_keys2"
/*
* Per-user and system-wide ssh "rc" files. These files are executed with
Index: usr.bin/ssh/servconf.c
===
RCS file: /cvs/src/usr.bin/ssh/servconf.c,v
retrieving revision 1.306
diff -u -p -r1.306 servconf.c
--- usr.bin/ssh/servconf.c 14 Mar 2017 07:19:07 - 1.306
+++ usr.bin/ssh/servconf.c 17 Apr 2017 19:26:47 -
@@ -294,12 +294,9 @@ fill_default_server_options(ServerOption
options->client_alive_interval = 0;
if (options->client_alive_count_max == -1)
options->client_alive_count_max = 3;
- if (options->num_authkeys_files == 0) {
+ if (options->num_authkeys_files == 0)
options->authorized_keys_files[options->num_authkeys_files++] =
xstrdup(_PATH_SSH_USER_PERMITTED_KEYS);
- options->authorized_keys_files[options->num_authkeys_files++] =
- xstrdup(_PATH_SSH_USER_PERMITTED_KEYS2);
- }
if (options->permit_tun == -1)
options->permit_tun = SSH_TUNMODE_NO;
if (options->ip_qos_interactive == -1)
Index: usr.bin/ssh/sshd.8
===
RCS file: /cvs/src/usr.bin/ssh/sshd.8,v
retrieving revision 1.288
diff -u -p -r1.288 sshd.8
--- usr.bin/ssh/sshd.8 30 Jan 2017 23:27:39 - 1.288
+++ usr.bin/ssh/sshd.8 17 Apr 2017 19:26:47 -
@@ -390,9 +390,7 @@ does not exist either, xauth is used to
specifies the files containing public keys for
public key authentication;
if this option is not specified, the default is
-.Pa ~/.ssh/authorized_keys
-and
-.Pa ~/.ssh/authorized_keys2 .
+.Pa ~/.ssh/authorized_keys .
Each line of the file contains one
key (empty lines and lines starting with a
.Ql #
Index: usr.bin/ssh/sshd_config
===
RCS file: /cvs/src/usr.bin/ssh/sshd_config,v
retrieving revision 1.101
diff -u -p -r1.101 sshd_config
--- usr.bin/ssh/sshd_config 14 Mar 2017 07:19:07 - 1.101
+++ usr.bin/ssh/sshd_config 17 Apr 2017 19:26:47 -
@@ -35,9 +35,7 @@
#PubkeyAuthentication yes
-# The default is to check both .ssh/authorized_keys and .ssh/authorized_keys2
-# but this is overridden so installations will only check .ssh/authorized_keys
-AuthorizedKeysFile .ssh/authorized_keys
+#AuthorizedKeysFile.ssh/authorized_keys
#AuthorizedPrincipalsFile none
Index: usr.bin/ssh/sshd_config.5
===
RCS file: /cvs/src/usr.bin/ssh/sshd_config.5,v
retrieving revision 1.243
diff -u -p -r1.243 sshd_config.5
--- usr.bin/ssh/sshd_config.5 14 Mar 2017 07:19:07 - 1.243
+++ usr.bin/ssh/sshd_config.5 17 Apr 2017 19:26:47 -
@@ -283,7 +283,7 @@ Alternately this option may be set to
.Cm none
to skip checking for user keys in files.
The default is
-.Qq .ssh/authorized_keys .ssh/authorized_keys2 .
+.Qq .ssh/authorized_keys .
.It Cm AuthorizedPrincipalsCommand
Specifies a program to be used to generate the list of allowed
certificate principals as per
Re: [patch] Add -z and -Z to apmd for automatic suspend/hibernate
On Mon, Aug 14, 2017 at 11:21:03AM -0400, Ted Unangst wrote:
> Klemens Nanni wrote:
> > > + case 'z':
> > > + autoaction = AUTO_SUSPEND;
> > > + autolimit = strtonum(optarg, 1, 100, );
> > > + if (errstr != NULL)
> > > + error("invalid percent: %s", errstr);
> > > + break;
> > You should pass optarg instead of errstr to error(). Either ways error()
> > will still append since it uses err(3). This leads to
> >
> > $ obj/apmd -dz0
> > apmd: invalid percent: too small: Result too large
>
> actually, both, but you should use something like errc() instead of errno.
Sure
error("%s percentage: %s", errstr, optarg)
would be the idiomatic way but that requires changing error()'s
signature and usage all over apmd.c. Do you suggest replacing err(3)
with errc(3) in error()?
Re: [patch] Add -z and -Z to apmd for automatic suspend/hibernate
On Sun, Aug 13, 2017 at 02:13:42PM +0200, Jesper Wallin wrote:
> On Sun, Aug 13, 2017 at 09:52:22AM +0200, Martijn van Duren wrote:
> > I've also been bitten by this a couple of times, but you can also solve
> > this via the sensorsd framework, which is how I've done it.
>
> Yeah, someone on IRC also suggested sensorsd or even ksh and a cronjob.
> I personally find it a bit too ducttapey though, especially for a
> feature one would expect on a laptop. This also saves me from running an
> extra daemon just in case my battery runs out.
>
> > I'm no expert in this area and I'm not going to make any statements on
> > whether we should add this or not, but two (non functionality) nits
> > inline for future reference.
>
> Thanks for the feedback, greatly appreciated! I've done a few changes
> and also rewrote some of the manual, as my previous patch was lying.
>
>
> Jesper Wallin
>
>
> Index: usr.sbin/apmd/apmd.8
> ===
> RCS file: /cvs/src/usr.sbin/apmd/apmd.8,v
> retrieving revision 1.47
> diff -u -p -r1.47 apmd.8
> --- usr.sbin/apmd/apmd.8 12 Feb 2015 14:03:49 - 1.47
> +++ usr.sbin/apmd/apmd.8 13 Aug 2017 11:50:16 -
> @@ -113,6 +113,20 @@ The polling rate defaults to
> once per 10 minutes, but may be specified using the
> .Fl t
> command-line flag.
> +.It Fl z Ar percent
> +Automatically suspend the system if no AC is connected and the
> +estimated battery life is equal or below
> +.Ar percent .
> +.It Fl Z Ar percent
> +Automatically hibernate the system if no AC is connected and the
> +estimated battery life is equal or below
> +.Ar percent .
> +.Pp
> +If both
> +.Fl Z
> +and
> +.Fl z
> +are specified, the first one will supersede the other.
> .El
> .Pp
> When a client requests a suspend or stand-by state,
> Index: usr.sbin/apmd/apmd.c
> ===
> RCS file: /cvs/src/usr.sbin/apmd/apmd.c,v
> retrieving revision 1.79
> diff -u -p -r1.79 apmd.c
> --- usr.sbin/apmd/apmd.c 16 Nov 2015 17:35:05 - 1.79
> +++ usr.sbin/apmd/apmd.c 13 Aug 2017 11:50:16 -
> @@ -56,6 +56,9 @@
> #define TRUE 1
> #define FALSE 0
>
> +#define AUTO_SUSPEND 1
> +#define AUTO_HIBERNATE 2
> +
> const char apmdev[] = _PATH_APM_CTLDEV;
> const char sockfile[] = _PATH_APM_SOCKET;
>
> @@ -94,8 +97,8 @@ void
> usage(void)
> {
> fprintf(stderr,
> - "usage: %s [-AadHLs] [-f devname] [-S sockname] [-t seconds]\n",
> - __progname);
> + "usage: %s [-AadHLs] [-f devname] [-S sockname] [-t seconds] "
> + "[-z percent] [-Z percent]\n", __progname);
> exit(1);
> }
>
> @@ -348,6 +351,8 @@ main(int argc, char *argv[])
> {
> const char *fname = apmdev;
> int ctl_fd, sock_fd, ch, suspends, standbys, hibernates, resumes;
> + int autoaction = 0;
> + int autolimit = 0;
> int statonly = 0;
> int powerstatus = 0, powerbak = 0, powerchange = 0;
> int noacsleep = 0;
> @@ -355,13 +360,14 @@ main(int argc, char *argv[])
> struct apm_power_info pinfo;
> time_t apmtimeout = 0;
> const char *sockname = sockfile;
> + const char *errstr;
> int kq, nchanges;
> struct kevent ev[2];
> int ncpu_mib[2] = { CTL_HW, HW_NCPU };
> int ncpu;
> size_t ncpu_sz = sizeof(ncpu);
>
> - while ((ch = getopt(argc, argv, "aACdHLsf:t:S:")) != -1)
> + while ((ch = getopt(argc, argv, "aACdHLsf:t:S:z:Z:")) != -1)
> switch(ch) {
> case 'a':
> noacsleep = 1;
> @@ -402,6 +408,18 @@ main(int argc, char *argv[])
> doperf = PERF_MANUAL;
> setperfpolicy("high");
> break;
> + case 'z':
> + autoaction = AUTO_SUSPEND;
> + autolimit = strtonum(optarg, 1, 100, );
> + if (errstr != NULL)
> + error("invalid percent: %s", errstr);
> + break;
> + case 'Z':
> + autoaction = AUTO_HIBERNATE;
> + autolimit = strtonum(optarg, 1, 100, );
> + if (errstr != NULL)
> + error("invalid percent: %s", errstr);
> + break;
> case '?':
> default:
> usage();
> @@ -479,6 +497,20 @@ main(int argc, char *argv[])
> if (powerstatus != powerbak) {
> powerstatus = powerbak;
> powerchange = 1;
> + }
> +
> + if (!powerstatus && autoaction &&
> + autolimit > (int)pinfo.battery_life) {
> + syslog(LOG_NOTICE, "estimated battery life
> %d%%, "
> + "autoaction limit set to %d%% .",
> +
Re: rc: reorder_libs: [2/2] Pick archive versions more efficiently
On Mon, Jul 17, 2017 at 11:57:02AM +0300, Vadim Zhukov wrote:
> > + for _liba in /usr/lib/lib{c,crypto}; do
> > + _libas="$_libas $(ls $_liba.so.+([0-9.]).a | sort -V | tail
> > -1)"
> > done
> > + _libas=${_libas# }
> >
> > # Remount read-write, if /usr/lib is on a read-only ffs filesystem.
> > if [[ $_mp == *' type ffs '*'read-only'* ]]; then
> >
>
> As a matter of microoptimization we could use "sort -rV | head -1"
> instead of "sort -V | tail -1". I'm okay with current version of this
> diff already, though.
Sorting requires to read the entire input, otherwise you'd only sort a
subset of the input. I don't see anything being optimized here.
Re: rc: reorder_libs: [2/2] Pick archive versions more efficiently
On Mon, Jul 17, 2017 at 02:14:26PM +0300, Vadim Zhukov wrote:
> 2017-07-17 14:03 GMT+03:00 Klemens Nanni <k...@posteo.org>:
> > On Mon, Jul 17, 2017 at 11:57:02AM +0300, Vadim Zhukov wrote:
> >> > + for _liba in /usr/lib/lib{c,crypto}; do
> >> > + _libas="$_libas $(ls $_liba.so.+([0-9.]).a | sort -V |
> >> > tail -1)"
> >> > done
> >> > + _libas=${_libas# }
> >> >
> >> > # Remount read-write, if /usr/lib is on a read-only ffs
> >> > filesystem.
> >> > if [[ $_mp == *' type ffs '*'read-only'* ]]; then
> >> >
> >>
> >> As a matter of microoptimization we could use "sort -rV | head -1"
> >> instead of "sort -V | tail -1". I'm okay with current version of this
> >> diff already, though.
> > Sorting requires to read the entire input, otherwise you'd only sort a
> > subset of the input. I don't see anything being optimized here.
>
> head -1 returns earlier than tail -1. ;)
Generally speaking, yes. But here, even for thousands of lines to be
sorted this won't make a difference. For me it's just changing logic
without benefit.
Re: rc: reorder_libs: [1/2] Drop unused _l, exit early on failure
On Sun, Jul 16, 2017 at 09:09:44AM +, Robert Peichaer wrote:
> The rationale to picking the library versions before remounting was
> to keep the time window having rw /usr as small as possible.
> If that's deemed ok, I'm too OK with switching the steps.
Considering the fact that the now simplified version picking routine
takes virtually no time, I'd like finish this up.
Here's the updated diff checking r/w status beforehand.
Index: rc
===
RCS file: /cvs/src/etc/rc,v
retrieving revision 1.508
diff -u -p -r1.508 rc
--- rc 17 Jul 2017 12:02:53 - 1.508
+++ rc 17 Jul 2017 12:56:07 -
@@ -170,12 +170,6 @@ reorder_libs() {
echo -n 'reordering libraries:'
- # Only choose the latest version of the libraries.
- for _liba in /usr/lib/lib{c,crypto}; do
- _libas="$_libas $(ls $_liba.so.+([0-9.]).a | sort -V | tail -1)"
- done
- _libas=${_libas# }
-
# Remount read-write, if /usr/lib is on a read-only ffs filesystem.
if [[ $_mp == *' type ffs '*'read-only'* ]]; then
if mount -u -w $_dkdev; then
@@ -185,6 +179,12 @@ reorder_libs() {
return
fi
fi
+
+ # Only choose the latest version of the libraries.
+ for _liba in /usr/lib/lib{c,crypto}; do
+ _libas="$_libas $(ls $_liba.so.+([0-9.]).a | sort -V | tail -1)"
+ done
+ _libas=${_libas# }
for _liba in $_libas; do
_tmpdir=$(mktemp -dq /tmp/_librebuild.) && (
Re: rc: Use here document for temporary pf rule set
On Sun, Jul 16, 2017 at 12:41:09PM +, Robert Peichaer wrote:
> On Sun, Jul 16, 2017 at 02:28:59PM +0200, Klemens Nanni wrote:
> > On Sun, Jul 16, 2017 at 12:11:55PM +, Robert Peichaer wrote:
> > > On Sun, Jul 16, 2017 at 01:37:56PM +0200, Klemens Nanni wrote:
> > > > This removes on level of indent, avoids the ugly RULES="$RULES ..."
> > > > repitition and spares a print.
> > > >
> > > > We could do a 'pfctl -ef -' right away but I kept changing and enabling
> > > > clearly seperated. Regarding the leading newlines and tabs of the inner
> > > > echo: pf perfectly munges those, no need to clear them.
> > > >
> > > > The "don't" -> "do not" is neccessary since otherwise the shell would
> > > > choke on it as opening quote.
> > > >
> > > >
> > > > Feedback? Comments?
> > >
> > > Nice idea. The only maby irrelevant concern I have is, that using the
> > > here-document approach uses a temporary file and if that for some reason
> > > fails, we end up without this or mangled rules.
> > sh reads the temporary file in 512 bytes chunks, the here document is
> > about 2.0K in size.
> >
> > I didn't bother intercepting sh with gdb and simulating a scenario where
> > the temporary file cannot be written but in case the user has no disk
> > space left I'd expect it to not be created at all since.
> >
> > In general I'd say that if /tmp doesn't have 2.0K left users probably
> > have more serious problems anyway.
>
> Have you thought about diskless(8) setups?
If /tmp is served via NFS and the here document's IO fails, 'pfctl -f -'
won't see any rule as it's not being executed.
I still think running out of space would cause more problems than just
this one but I shall be glad to be corrected.
Here's another approach still using $RULES but with saner quoting and
without the potentially dangerous here document.
Index: rc
===
RCS file: /cvs/src/etc/rc,v
retrieving revision 1.507
diff -u -p -r1.507 rc
--- rc 4 Jul 2017 19:02:11 - 1.507
+++ rc 16 Jul 2017 13:23:02 -
@@ -402,31 +399,35 @@ wsconsctl_conf
# Set initial temporary pf rule set.
if [[ $pf != NO ]]; then
- RULES="block all"
- RULES="$RULES\npass on lo0"
- RULES="$RULES\npass in proto tcp from any to any port ssh keep state"
- RULES="$RULES\npass out proto { tcp, udp } from any to any port domain
keep state"
- RULES="$RULES\npass out inet proto icmp all icmp-type echoreq keep
state"
- RULES="$RULES\npass out inet proto udp from any port bootpc to any port
bootps"
- RULES="$RULES\npass in inet proto udp from any port bootps to any port
bootpc"
- if ifconfig lo0 inet6 >/dev/null 2>&1; then
- RULES="$RULES\npass out inet6 proto icmp6 all icmp6-type
neighbrsol"
- RULES="$RULES\npass in inet6 proto icmp6 all icmp6-type
neighbradv"
- RULES="$RULES\npass out inet6 proto icmp6 all icmp6-type
routersol"
- RULES="$RULES\npass in inet6 proto icmp6 all icmp6-type
routeradv"
- RULES="$RULES\npass out inet6 proto udp from any port
dhcpv6-client to any port dhcpv6-server"
- RULES="$RULES\npass in inet6 proto udp from any port
dhcpv6-server to any port dhcpv6-client"
- fi
- RULES="$RULES\npass in proto carp keep state (no-sync)"
- RULES="$RULES\npass out proto carp !received-on any keep state
(no-sync)"
- if [[ $(sysctl vfs.mounts.nfs 2>/dev/null) == *[1-9]* ]]; then
- # Don't kill NFS.
- RULES="set reassemble yes no-df\n$RULES"
- RULES="$RULES\npass in proto { tcp, udp } from any port {
sunrpc, nfsd } to any"
- RULES="$RULES\npass out proto { tcp, udp } from any to any port
{ sunrpc, nfsd } !received-on any"
- fi
- print -- "$RULES" | pfctl -f -
- pfctl -e
+ RULES='
+ block all
+ pass on lo0
+ pass in proto tcp from any to any port ssh keep state
+ pass out proto { tcp, udp } from any to any port domain keep state
+ pass out inet proto icmp all icmp-type echoreq keep state
+ pass out inet proto udp from any port bootpc to any port bootps
+ pass in inet proto udp from any port bootps to any port bootpc'
+
+ ifconfig lo0 inet6 >/dev/null 2>&1 &&
+ RULES="$RULES"'
+ pass out inet6 proto icmp6 all icmp6-type neighbrsol
+ pass in inet6 proto
Re: rc: Use here document for temporary pf rule set
On Sun, Jul 16, 2017 at 03:24:19PM +0200, Klemens Nanni wrote: > + print -- "$RULES" | pfctl -nf - That should be 'pfctl -ef -' of course.
rc: Use here document for temporary pf rule set
This removes on level of indent, avoids the ugly RULES="$RULES ..."
repitition and spares a print.
We could do a 'pfctl -ef -' right away but I kept changing and enabling
clearly seperated. Regarding the leading newlines and tabs of the inner
echo: pf perfectly munges those, no need to clear them.
The "don't" -> "do not" is neccessary since otherwise the shell would
choke on it as opening quote.
Feedback? Comments?
Index: rc
===
RCS file: /cvs/src/etc/rc,v
retrieving revision 1.507
diff -u -p -r1.507 rc
--- rc 4 Jul 2017 19:02:11 - 1.507
+++ rc 16 Jul 2017 11:34:09 -
@@ -402,30 +402,33 @@ wsconsctl_conf
# Set initial temporary pf rule set.
if [[ $pf != NO ]]; then
- RULES="block all"
- RULES="$RULES\npass on lo0"
- RULES="$RULES\npass in proto tcp from any to any port ssh keep state"
- RULES="$RULES\npass out proto { tcp, udp } from any to any port domain
keep state"
- RULES="$RULES\npass out inet proto icmp all icmp-type echoreq keep
state"
- RULES="$RULES\npass out inet proto udp from any port bootpc to any port
bootps"
- RULES="$RULES\npass in inet proto udp from any port bootps to any port
bootpc"
- if ifconfig lo0 inet6 >/dev/null 2>&1; then
- RULES="$RULES\npass out inet6 proto icmp6 all icmp6-type
neighbrsol"
- RULES="$RULES\npass in inet6 proto icmp6 all icmp6-type
neighbradv"
- RULES="$RULES\npass out inet6 proto icmp6 all icmp6-type
routersol"
- RULES="$RULES\npass in inet6 proto icmp6 all icmp6-type
routeradv"
- RULES="$RULES\npass out inet6 proto udp from any port
dhcpv6-client to any port dhcpv6-server"
- RULES="$RULES\npass in inet6 proto udp from any port
dhcpv6-server to any port dhcpv6-client"
- fi
- RULES="$RULES\npass in proto carp keep state (no-sync)"
- RULES="$RULES\npass out proto carp !received-on any keep state
(no-sync)"
- if [[ $(sysctl vfs.mounts.nfs 2>/dev/null) == *[1-9]* ]]; then
- # Don't kill NFS.
- RULES="set reassemble yes no-df\n$RULES"
- RULES="$RULES\npass in proto { tcp, udp } from any port {
sunrpc, nfsd } to any"
- RULES="$RULES\npass out proto { tcp, udp } from any to any port
{ sunrpc, nfsd } !received-on any"
- fi
- print -- "$RULES" | pfctl -f -
+ pfctl -f - <<- __EOF__
+block all
+pass on lo0
+pass in proto tcp from any to any port ssh keep state
+pass out proto { tcp, udp } from any to any port domain keep state
+pass out inet proto icmp all icmp-type echoreq keep state
+pass out inet proto udp from any port bootpc to any port bootps
+pass in inet proto udp from any port bootps to any port bootpc
+$(if ifconfig lo0 inet6 >/dev/null 2>&1; then
+ echo '
+ pass out inet6 proto icmp6 all icmp6-type neighbrsol
+ pass in inet6 proto icmp6 all icmp6-type neighbradv
+ pass out inet6 proto icmp6 all icmp6-type routersol
+ pass in inet6 proto icmp6 all icmp6-type routeradv
+ pass out inet6 proto udp from any port dhcpv6-client to any port
dhcpv6-server
+ pass in inet6 proto udp from any port dhcpv6-server to any port
dhcpv6-client'
+fi)
+pass in proto carp keep state (no-sync)
+pass out proto carp !received-on any keep state (no-sync)
+$(if [[ $(sysctl vfs.mounts.nfs 2>/dev/null) == *[1-9]* ]]; then
+ # Do not kill NFS.
+ echo '
+ set reassemble yes no-df
+ pass in proto { tcp, udp } from any port { sunrpc, nfsd } to any
+ pass out proto { tcp, udp } from any to any port { sunrpc, nfsd }
!received-on any'
+fi)
+__EOF__
pfctl -e
fi
Re: rc: Use here document for temporary pf rule set
On Sun, Jul 16, 2017 at 12:11:55PM +, Robert Peichaer wrote: > On Sun, Jul 16, 2017 at 01:37:56PM +0200, Klemens Nanni wrote: > > This removes on level of indent, avoids the ugly RULES="$RULES ..." > > repitition and spares a print. > > > > We could do a 'pfctl -ef -' right away but I kept changing and enabling > > clearly seperated. Regarding the leading newlines and tabs of the inner > > echo: pf perfectly munges those, no need to clear them. > > > > The "don't" -> "do not" is neccessary since otherwise the shell would > > choke on it as opening quote. > > > > > > Feedback? Comments? > > Nice idea. The only maby irrelevant concern I have is, that using the > here-document approach uses a temporary file and if that for some reason > fails, we end up without this or mangled rules. sh reads the temporary file in 512 bytes chunks, the here document is about 2.0K in size. I didn't bother intercepting sh with gdb and simulating a scenario where the temporary file cannot be written but in case the user has no disk space left I'd expect it to not be created at all since. In general I'd say that if /tmp doesn't have 2.0K left users probably have more serious problems anyway.
Re: rc: reorder_libs: [2/2] Pick archive versions more efficiently
On Sun, Jul 16, 2017 at 10:26:25AM +, Robert Peichaer wrote:
> On Sun, Jul 16, 2017 at 03:37:15AM +0200, Klemens Nanni wrote:
> > Why looping over all existing archives, picking the latest version of
> > the current archive, skipping it in case it's already in our list of
> > selected latest versions or adding it otherwise?
> >
> > The current code runs ls|sort|tail about n * (v - 1) times for n
> > different libraries and v versions respectively since the globbed list
> > is almost always sorted already, effectively adding the latest versions
> > after skipping all others.
>
> Yeah well, until the globbing isn't sorting as expected.
>
> $ ls -1 libc.so.*.a
> libc.so.89.10.a
> libc.so.89.6.a
> libc.so.89.7.a
> libc.so.89.8.a
> libc.so.89.9.a
Exactly, that's why sort(1) is used.
>
> But that's not the point anyway in your otherwise perfectly valid change.
>
> > This diff makes it much clearer and simpler by sorting and picking
> > only as many versions as there are libraries to reorder (two). Globbing
> > is done within the loop so future libraries with different naming
> > schemes comes at no cost.
> >
> > Applies cleanly to both the current revision as well as my previous diff
> > but the previous one will fail on top of this one.
> >
> > Feedback? Comments?
> >
> > Index: rc
> > ===
> > RCS file: /cvs/src/etc/rc,v
> > retrieving revision 1.507
> > diff -u -p -r1.507 rc
> > --- rc 4 Jul 2017 19:02:11 - 1.507
> > +++ rc 16 Jul 2017 01:15:43 -
> > @@ -171,13 +171,10 @@ reorder_libs() {
> > echo -n 'reordering libraries:'
> >
> > # Only choose the latest version of the libraries.
> > - for _liba in /usr/lib/libc.so.*.a /usr/lib/libcrypto.so.*.a; do
> > - _liba=$(ls ${_liba%%.[0-9]*}*.a | sort -V | tail -1)
> > - for _l in $_libas; do
> > - [[ $_l == $_liba ]] && continue 2
> > - done
> > - _libas="$_libas $_liba"
> > + for _liba in 'libc.so.*.a' 'libcrypto.so.*.a'; do
> > + _libas="$_libas $(ls /usr/lib/$_liba | sort -V | tail -1)"
> > done
>
> That's definitely a much better approach.
>
> But I'd like to stay strict matching the filenames.
>
> + for _liba in /usr/lib/lib{c,crypto}; do
> + _libas="$_libas $(ls $_liba.so.+([0-9.]).a | sort -V | tail -1)"
> + done
>
> > + _libas=${_libas# }
>
> This would be another way of suppressing the blank right away.
> But it's not really necessary anyway, because the leading blank is
> removed in the list of the other for-loop.
>
> _libas="${_libas:+$_libas }$(ls $_liba.so.+([0-9.]).a | sort -V | tail
> -1)"
I explicitly avoided this since stripping the leading space in the end
seemed clearer about what's going on. Also, this check is done for each
library whereas only the first one is true.
I also thought about leaving it in but stripping it makes clear I know
what I'm doing and future changes where $_libas might get quoted will
be working as expected.
Re: rc: reorder_libs: [1/2] Drop unused _l, exit early on failure
On Sun, Jul 16, 2017 at 07:18:00AM +0200, Theo Buehler wrote: > On Sun, Jul 16, 2017 at 03:34:07AM +0200, Klemens Nanni wrote: > > $_l is not used and picking the latest archive versions is of no use > > if /usr/lib cannot be written to. > > > > This patch applies cleanly before my next one but not vice versa. > > > > Feedback? OK? Oh, that OK should've been gone. > _l is only unused after your second patch :) Indeed, a bit ugly now that I'm looking at it again but that's why those two should only go together. > hoisting the remount over picking the library version makes sense, > but you should keep it after the "echo -n 'reordering libraries:'" Fair point.
Re: rc: reorder_libs: [2/2] Pick archive versions more efficiently
On Sun, Jul 16, 2017 at 11:55:09AM +, Robert Peichaer wrote: > To the contrary what my previous answer might indicate, I don't care > that much either. If you want to explicitely remove the blank, go for > it. Most problems I ever encountered while writing shell code were related to (nested) quoting and unexpected characters (mostly whitespaces). Leaving the resulting $_libas exactly as one would expect it to be doesn't hurt plus the code's intent is perfectly clear while reading it. See the updated diff.
rc: Use IFS when looking for carp interfaces
The Internal Field Seperator is meant for this so use it instead of
reading and stripping ':' again.
Feedback? Comments?
Index: rc
===
RCS file: /cvs/src/etc/rc,v
retrieving revision 1.508
diff -u -p -r1.508 rc
--- rc 17 Jul 2017 12:02:53 - 1.508
+++ rc 17 Jul 2017 13:33:54 -
@@ -351,8 +350,8 @@ if [[ $1 == shutdown ]]; then
[[ -f /etc/rc.shutdown ]] && sh /etc/rc.shutdown
fi
- ifconfig | while read _if _junk; do
- [[ $_if == carp+([0-9]): ]] && ifconfig ${_if%:} down
+ ifconfig | while IFS=: read _if _junk; do
+ [[ $_if == carp+([0-9]) ]] && ifconfig $_if down
done
exit 0
Re: tee: Replace hand-rolled list with SLIST_*
On Mon, Jul 10, 2017 at 07:49:10PM +0200, Alexander Bluhm wrote:
> On Sun, Jul 02, 2017 at 04:29:39AM +0200, Klemens Nanni wrote:
> > No functional change or bugfix but queue(3) is the for a reason. That
> > way the code even is a tad clearer to me (and two lines shorter).
> >
> > Feedback/OK?
>
> You do too much in this diff:
> 1. convert list to SLIST
> 2. make fp a global variable
> 3. rename p to fp
> 4. rename struct list to struct file
> 5. replace malloc(sizeof(*p)) with malloc(sizeof(struct file))
> 6. Remove braces from last SLIST_FOREACH loop
>
> 1. is good.
> 2. is bad. Local variables are almost always better and easier
>to maintain. Using global loop variables is especially error
>prone.
> 3.-6. is just personal taste. Unless you more or less own a source
>file, do not touch it. If everyone would change everything to
>his personal taste, our history would be full of useles back and
>forth.
Points taken, thanks for your review/feedback!
> Just do 1, then chances are higher that it gets commited.
Here's the updated diff handling the list->SLIST conversion alone.
Index: tee.c
===
RCS file: /cvs/src/usr.bin/tee/tee.c,v
retrieving revision 1.11
diff -u -p -r1.11 tee.c
--- tee.c 28 Oct 2016 07:22:59 - 1.11
+++ tee.c 10 Jul 2017 18:41:46 -
@@ -32,6 +32,7 @@
#include
#include
+#include
#include
#include
@@ -43,11 +44,11 @@
#include
struct list {
- struct list *next;
+ SLIST_ENTRY(list) next;
int fd;
char *name;
};
-struct list *head;
+SLIST_HEAD(, list) head;
static void
add(int fd, char *name)
@@ -58,8 +59,7 @@ add(int fd, char *name)
err(1, NULL);
p->fd = fd;
p->name = name;
- p->next = head;
- head = p;
+ SLIST_INSERT_HEAD(, p, next);
}
int
@@ -75,6 +75,8 @@ main(int argc, char *argv[])
if (pledge("stdio wpath cpath", NULL) == -1)
err(1, "pledge");
+ SLIST_INIT();
+
append = 0;
while ((ch = getopt(argc, argv, "ai")) != -1) {
switch(ch) {
@@ -109,7 +111,7 @@ main(int argc, char *argv[])
err(1, "pledge");
while ((rval = read(STDIN_FILENO, buf, sizeof(buf))) > 0) {
- for (p = head; p; p = p->next) {
+ SLIST_FOREACH(p, , next) {
n = rval;
bp = buf;
do {
@@ -127,7 +129,7 @@ main(int argc, char *argv[])
exitval = 1;
}
- for (p = head; p; p = p->next) {
+ SLIST_FOREACH(p, , next) {
if (close(p->fd) == -1) {
warn("%s", p->name);
exitval = 1;
intel.4: Typofix
Add missing verb and period. Index: driver/xf86-video-intel/man/intel.man === RCS file: /cvs/xenocara/driver/xf86-video-intel/man/intel.man,v retrieving revision 1.9 diff -u -p -r1.9 intel.man --- driver/xf86-video-intel/man/intel.man 12 Apr 2015 19:42:05 - 1.9 +++ driver/xf86-video-intel/man/intel.man 18 Jul 2017 00:45:37 - @@ -321,8 +321,8 @@ Default: 0 .BI "Option \*qZaphodHeads\*q \*q" string \*q .IP Specify the randr output(s) to use with zaphod mode for a particular driver -instance. If you this option you must use it with all instances of the -driver +instance. If you use this option you must use it with all instances of the +driver. .br For example: .B
Re: intel.4: Typofix
On Mon, Jul 17, 2017 at 10:38:22PM -0400, Bryan Steele wrote: > This is an upstream Xorg manual. > > https://lists.x.org/mailman/listinfo/xorg-devel Reported upstream, thanks.
innovations: Typofix
Is tech@ the right place for www diffs? Index: innovations.html === RCS file: /cvs/www/innovations.html,v retrieving revision 1.51 diff -u -p -r1.51 innovations.html --- innovations.html3 Jul 2017 23:47:21 - 1.51 +++ innovations.html9 Jul 2017 08:50:15 - @@ -464,7 +464,7 @@ explained in detail in our https://man.openbsd.org/OpenBSD-current/man8/sshd.8;>sshd(8) 2004), Claudio Jeker (https://man.openbsd.org/OpenBSD-current/man8/bgpd.8;>bgpd(8), 2015), Eric Faurot (https://man.openbsd.org/OpenBSD-current/man8/smtpd.8;>smtpd(8), 2016), Rafael Zalamena (various, 2016), and others. trapsleds: Reduction of incidental nop instructions/sequences in the - instruction stream which could be be useful potentially for ROP attack methods + instruction stream which could be useful potentially for ROP attack methods to innaccurately target gadgets. These nops sequences are converted into trap sequences where possible. Todd Mortimer and Theo de Raadt, June 2017. The .o files of the kernel are relinked in random order from a link-kit,
Re: install.sub: Fix scrambled address list in v6_defroute()
On Wed, Jun 14, 2017 at 03:00:11AM +0200, Klemens Nanni wrote:
> Installing -current the other day showed a broken list when picking
> the IPv6 default route just like reported on bugs@ five days ago[1].
1: http://marc.info/?l=openbsd-bugs=149704197715791
>
> This behaviour can be reproduced manually running the code from
> v6_defroute():
>
> $ # source/define bsort()
> $ _if=trunk0
> $ bsort $(ping6 -n -c 2 ff02::2%$_if 2>/dev/null |
> sed -n '/bytes from/{s/^.*from //;s/,.*$//;p;}' |
> sed -n 'G;s/\n/&&/;/^\(.*\n\).*\n\1/d;h;P')
> fe80:::__ff:fe__:___%trunk0: hlim=64 icmp_seq=0 icmp_seq=1 ms
> time=0.431 time=0.802
>
> The first sed filters for those lines containing the router addresses
> and does two things:
> 1. strip leading text up to the address
> 2. strip trailing text after the first ','
>
> However, as far as i can see 'ping6 -nc2' will never output lines that
> would match the second criteria thus making this part of the command
> obsolete.
>
> The second sed removes duplicate addresses (not neccessarily consecutive
> ones). This works fine for itself, but as seen above its input contains
> more than just addresses. Besides that, sorting through sed just before
> having bsort() cleaning the list is duplicate effort here.
>
> With this patch bsort() gets passed possibly duplicate IPv6 addresses
> one per line so _routers will eventually contain a sorted list of unique
> router addresses; this fixes the list shown during the installer.
>
>
> While debugging this, I noticed that stopping after two echo replies
> would sometimes show only one router; increasing this limit to three
> made both of my network's routers reply/show up reliably.
I'm somewhat sursprised noone else has fixed or complained about this
so far. This still annoys me when setting up boxes in (IPv6-only)
networks with more than one router.
Feedback? Comments?
Marginally updated diff dropping ^ and $ from the sed command.
Index: install.sub
===
RCS file: /cvs/src/distrib/miniroot/install.sub,v
retrieving revision 1.1020
diff -u -p -r1.1020 install.sub
--- install.sub 7 Jul 2017 16:21:34 - 1.1020
+++ install.sub 9 Jul 2017 07:36:21 -
@@ -997,9 +997,8 @@ v6_defroute() {
route -n show -inet6 | egrep -q '^default[[:space:]]' && return
- _routers=$(bsort $(ping6 -n -c 2 ff02::2%$_if 2>/dev/null |
- sed -n '/bytes from/{s/^.*from //;s/,.*$//;p;}' |
- sed -n 'G;s/\n/&&/;/^\(.*\n\).*\n\1/d;h;P'))
+ _routers=$(bsort $(ping6 -n -c 3 ff02::2%$_if 2>/dev/null |
+ sed -n 's/.*from \(.*\): .*/\1/p'))
_prompt="IPv6 default router?"
Re: rc: Use here document for temporary pf rule set
On Sun, Jul 16, 2017 at 08:15:38PM +, Robert Peichaer wrote:
> > + ifconfig lo0 inet6 >/dev/null 2>&1 &&
>
> Please leave the if-then-fi construct intact. This short form is
> mostly used for on-line commands (with only a few exceptions).
OK.
> > + RULES="$RULES"'
>
> What is the reason to use double quotes and single quotes here?
> Why not just use double quotes like this?
Personal preference to make clear nothing inside the rules gets
substituted. Using double quotes only will work just fine here.
> This is not equivalent to the existing code.
>
> > + pass in proto { tcp, udp } from any port { sunrpc, nfsd } to any
> > + pass out proto { tcp, udp } from any to any port { sunrpc, nfsd
> > } !received-on any'
> > + print -- "$RULES" | pfctl -nf -
Of course, fixed. Thanks!
> Unless one of the pf people speaks up in favour of combining it,
> I'd like to leave the two steps separated as you noted in your
> original email too.
Sure.
This is hopefully the final version of my diff. After all it now only
merges consecutive assignments of RULE into single ones.
Feedback?
Index: rc
===
RCS file: /cvs/src/etc/rc,v
retrieving revision 1.507
diff -u -p -r1.507 rc
--- rc 4 Jul 2017 19:02:11 - 1.507
+++ rc 16 Jul 2017 21:10:48 -
@@ -402,28 +399,35 @@ wsconsctl_conf
# Set initial temporary pf rule set.
if [[ $pf != NO ]]; then
- RULES="block all"
- RULES="$RULES\npass on lo0"
- RULES="$RULES\npass in proto tcp from any to any port ssh keep state"
- RULES="$RULES\npass out proto { tcp, udp } from any to any port domain
keep state"
- RULES="$RULES\npass out inet proto icmp all icmp-type echoreq keep
state"
- RULES="$RULES\npass out inet proto udp from any port bootpc to any port
bootps"
- RULES="$RULES\npass in inet proto udp from any port bootps to any port
bootpc"
+ RULES='
+ block all
+ pass on lo0
+ pass in proto tcp from any to any port ssh keep state
+ pass out proto { tcp, udp } from any to any port domain keep state
+ pass out inet proto icmp all icmp-type echoreq keep state
+ pass out inet proto udp from any port bootpc to any port bootps
+ pass in inet proto udp from any port bootps to any port bootpc'
+
if ifconfig lo0 inet6 >/dev/null 2>&1; then
- RULES="$RULES\npass out inet6 proto icmp6 all icmp6-type
neighbrsol"
- RULES="$RULES\npass in inet6 proto icmp6 all icmp6-type
neighbradv"
- RULES="$RULES\npass out inet6 proto icmp6 all icmp6-type
routersol"
- RULES="$RULES\npass in inet6 proto icmp6 all icmp6-type
routeradv"
- RULES="$RULES\npass out inet6 proto udp from any port
dhcpv6-client to any port dhcpv6-server"
- RULES="$RULES\npass in inet6 proto udp from any port
dhcpv6-server to any port dhcpv6-client"
+ RULES="$RULES
+ pass out inet6 proto icmp6 all icmp6-type neighbrsol
+ pass in inet6 proto icmp6 all icmp6-type neighbradv
+ pass out inet6 proto icmp6 all icmp6-type routersol
+ pass in inet6 proto icmp6 all icmp6-type routeradv
+ pass out inet6 proto udp from any port dhcpv6-client to any
port dhcpv6-server
+ pass in inet6 proto udp from any port dhcpv6-server to any port
dhcpv6-client"
fi
- RULES="$RULES\npass in proto carp keep state (no-sync)"
- RULES="$RULES\npass out proto carp !received-on any keep state
(no-sync)"
+
+ RULES="$RULES
+ pass in proto carp keep state (no-sync)
+ pass out proto carp !received-on any keep state (no-sync)"
+
+ # Don't kill NFS.
if [[ $(sysctl vfs.mounts.nfs 2>/dev/null) == *[1-9]* ]]; then
- # Don't kill NFS.
- RULES="set reassemble yes no-df\n$RULES"
- RULES="$RULES\npass in proto { tcp, udp } from any port {
sunrpc, nfsd } to any"
- RULES="$RULES\npass out proto { tcp, udp } from any to any port
{ sunrpc, nfsd } !received-on any"
+ RULES="set reassemble yes no-df
+ $RULES
+ pass in proto { tcp, udp } from any port { sunrpc, nfsd } to any
+ pass out proto { tcp, udp } from any to any port { sunrpc, nfsd
} !received-on any"
fi
print -- "$RULES" | pfctl -f -
pfctl -e
Re: rc: reorder_libs: [2/2] Pick archive versions more efficiently
On Sun, Jul 16, 2017 at 10:26:25AM +, Robert Peichaer wrote:
> But I'd like to stay strict matching the filenames.
>
> + for _liba in /usr/lib/lib{c,crypto}; do
> + _libas="$_libas $(ls $_liba.so.+([0-9.]).a | sort -V | tail -1)"
> + done
>
> > + _libas=${_libas# }
Agreed, I had a similiar approach first but then tried to reduce the
differences to the essentials.
Here's an updated diff taking this into while also dropping $_l together
with this hunk instead of the other one.
Feedback?
Index: rc
===
RCS file: /cvs/src/etc/rc,v
retrieving revision 1.507
diff -u -p -r1.507 rc
--- rc 4 Jul 2017 19:02:11 - 1.507
+++ rc 16 Jul 2017 11:54:36 -
@@ -158,7 +158,7 @@ make_keys() {
# Re-link libraries, placing the objects in a random order.
reorder_libs() {
- local _dkdev _l _liba _libas _mp _tmpdir _remount=false _error=false
+ local _dkdev _liba _libas _mp _tmpdir _remount=false _error=false
[[ $library_aslr == NO ]] && return
@@ -171,13 +171,10 @@ reorder_libs() {
echo -n 'reordering libraries:'
# Only choose the latest version of the libraries.
- for _liba in /usr/lib/libc.so.*.a /usr/lib/libcrypto.so.*.a; do
- _liba=$(ls ${_liba%%.[0-9]*}*.a | sort -V | tail -1)
- for _l in $_libas; do
- [[ $_l == $_liba ]] && continue 2
- done
- _libas="$_libas $_liba"
+ for _liba in /usr/lib/lib{c,crypto}; do
+ _libas="$_libas $(ls $_liba.so.+([0-9.]).a | sort -V | tail -1)"
done
+ _libas=${_libas# }
# Remount read-write, if /usr/lib is on a read-only ffs filesystem.
if [[ $_mp == *' type ffs '*'read-only'* ]]; then
rc: reorder_libs: [1/2] Drop unused _l, exit early on failure
$_l is not used and picking the latest archive versions is of no use
if /usr/lib cannot be written to.
This patch applies cleanly before my next one but not vice versa.
Feedback? OK?
Index: rc
===
RCS file: /cvs/src/etc/rc,v
retrieving revision 1.507
diff -u -p -r1.507 rc
--- rc 4 Jul 2017 19:02:11 - 1.507
+++ rc 16 Jul 2017 01:25:27 -
@@ -158,7 +158,7 @@ make_keys() {
# Re-link libraries, placing the objects in a random order.
reorder_libs() {
- local _dkdev _l _liba _libas _mp _tmpdir _remount=false _error=false
+ local _dkdev _liba _libas _mp _tmpdir _remount=false _error=false
[[ $library_aslr == NO ]] && return
@@ -168,6 +168,16 @@ reorder_libs() {
# Skip if /usr/lib is on a nfs mounted filesystem.
[[ $_mp == *' type nfs '* ]] && return
+ # Remount read-write, if /usr/lib is on a read-only ffs filesystem.
+ if [[ $_mp == *' type ffs '*'read-only'* ]]; then
+ if mount -u -w $_dkdev; then
+ _remount=true
+ else
+ echo ' failed.'
+ return
+ fi
+ fi
+
echo -n 'reordering libraries:'
# Only choose the latest version of the libraries.
@@ -178,16 +188,6 @@ reorder_libs() {
done
_libas="$_libas $_liba"
done
-
- # Remount read-write, if /usr/lib is on a read-only ffs filesystem.
- if [[ $_mp == *' type ffs '*'read-only'* ]]; then
- if mount -u -w $_dkdev; then
- _remount=true
- else
- echo ' failed.'
- return
- fi
- fi
for _liba in $_libas; do
_tmpdir=$(mktemp -dq /tmp/_librebuild.) && (
rc: reorder_libs: [2/2] Pick archive versions more efficiently
Why looping over all existing archives, picking the latest version of
the current archive, skipping it in case it's already in our list of
selected latest versions or adding it otherwise?
The current code runs ls|sort|tail about n * (v - 1) times for n
different libraries and v versions respectively since the globbed list
is almost always sorted already, effectively adding the latest versions
after skipping all others.
This diff makes it much clearer and simpler by sorting and picking
only as many versions as there are libraries to reorder (two). Globbing
is done within the loop so future libraries with different naming
schemes comes at no cost.
Applies cleanly to both the current revision as well as my previous diff
but the previous one will fail on top of this one.
Feedback? Comments?
Index: rc
===
RCS file: /cvs/src/etc/rc,v
retrieving revision 1.507
diff -u -p -r1.507 rc
--- rc 4 Jul 2017 19:02:11 - 1.507
+++ rc 16 Jul 2017 01:15:43 -
@@ -171,13 +171,10 @@ reorder_libs() {
echo -n 'reordering libraries:'
# Only choose the latest version of the libraries.
- for _liba in /usr/lib/libc.so.*.a /usr/lib/libcrypto.so.*.a; do
- _liba=$(ls ${_liba%%.[0-9]*}*.a | sort -V | tail -1)
- for _l in $_libas; do
- [[ $_l == $_liba ]] && continue 2
- done
- _libas="$_libas $_liba"
+ for _liba in 'libc.so.*.a' 'libcrypto.so.*.a'; do
+ _libas="$_libas $(ls /usr/lib/$_liba | sort -V | tail -1)"
done
+ _libas=${_libas# }
# Remount read-write, if /usr/lib is on a read-only ffs filesystem.
if [[ $_mp == *' type ffs '*'read-only'* ]]; then
Re: magic.5: Add missing types
On Mon, Jul 03, 2017 at 05:36:52PM +0100, Nicholas Marriott wrote: > Hi > > On Thu, Jun 29, 2017 at 09:29:57PM +0200, Klemens Nanni wrote: > > While reading file(1)'s code in #openbsd-daily mulander noted that the > > 'name' and 'use' types were missing from magic(5). > > > > I'm not entirely sure yet whether this is complete, so here's what I > > did: > > > > magic(5) provided by devel/magic documents version 5.31 while base's > > magic(5) is at 4.24. Here are the types found in 5.31 but not ours as > > well as those of the missing ones that are actually implemented but > > undocumented as of now: > > > > $ grep -i "TYPE_($(grep -F 'It Dv' $(man -w magic) | > > cut -d' ' -f3 | sort | uniq -u | paste -sd\| - | > > tee /dev/stderr))" magic.h > > beid3|beqwdate|clear|indirect|leid3|leqwdate|name|qwdate|use > > MAGIC_TYPE_CLEAR, > > MAGIC_TYPE_NAME, > > MAGIC_TYPE_USE, > > > > What about the current version being 4.21? We're clearly ahead of this, > > it seems magic(5) wasn't updated when nicm@ reimplemented things. > > > > This patch documents the respective types. > > Did you copy this text from upstream or write it? > > Some comments inline. > > > Feedback/OK? > > > > Index: magic.5 > > === > > RCS file: /cvs/src/usr.bin/file/magic.5,v > > retrieving revision 1.17 > > diff -u -p -r1.17 magic.5 > > --- magic.5 24 Apr 2016 07:02:07 - 1.17 > > +++ magic.5 29 Jun 2017 17:41:56 - > > @@ -218,6 +218,31 @@ This is intended to be used with the tes > > .Em x > > (which is always true) and a message that is to be used if there are > > no other matches. > > +.It Dv clear > > +This test is always true and clears the match flag for that continuation > > +level. > > I would just say "that level" or "the test's level" not "that > continuation level" because there is no previous mention of > "continuation". > > > +It is intended to be used with the default test. > > +.It Dv name > > +Define a > > +.Dq named > > I don't think quotation marks are necessary here. > > > +magic instance that can be called from another > > +.Dv use > > +magic entry, like a subroutine call. > > +Named instance direct magic offsets are relative to the offset of the > > +previous matched entry, but indirect offsets are relative to the > > +beginning of the file as usual. > > +Named magic entries always match. > > +.It Dv use > > +Recursively call the named magic starting from the current offset. > > +If the name of the referenced begins with a > > "the referenced instance" would be better than "the referenced". Here's an updated diff incorporating your suggestions. I didn't touch the manual's first sentence mentioning file(1)'s version since nicm@ reimplemented file(1) with revision 1.11 on 2015/04/24. He or someone else more knowledgable in this regard might want to bump it accordingly. Feedback? Comments? Index: magic.5 === RCS file: /cvs/src/usr.bin/file/magic.5,v retrieving revision 1.17 diff -u -p -r1.17 magic.5 --- magic.5 24 Apr 2016 07:02:07 - 1.17 +++ magic.5 25 Jul 2017 18:56:40 - @@ -30,7 +30,7 @@ .\" OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF .\" SUCH DAMAGE. .\" -.Dd $Mdocdate: April 24 2016 $ +.Dd $Mdocdate: July 25 2017 $ .Dt MAGIC 5 .Os .\" install as magic.4 on USG, magic.5 on V7, Berkeley and Linux systems. @@ -218,6 +218,28 @@ This is intended to be used with the tes .Em x (which is always true) and a message that is to be used if there are no other matches. +.It Dv clear +This test is always true and clears the match flag for that level. +It is intended to be used with the default test. +.It Dv name +Define a named magic instance that can be called from another +.Dv use +magic entry, like a subroutine call. +Named instance direct magic offsets are relative to the offset of the +previous matched entry, but indirect offsets are relative to the +beginning of the file as usual. +Named magic entries always match. +.It Dv use +Recursively call the named magic starting from the current offset. +If the name of the referenced instance begins with a +.Dv ^ +then the endianness of the magic is switched; if the magic mentioned +.Dv leshort +for example, +it is treated as +.Dv beshort +and vice versa. +This is useful to avoid duplicating the rules for different endianness. .El .Pp Each top-level magic pattern (see below for an explanation of levels)
Re: magic.5: Add missing types
On Tue, Jul 25, 2017 at 09:43:48PM +0100, Stuart Henderson wrote: > On 2017/07/25 20:57, Klemens Nanni wrote: > > I didn't touch the manual's first sentence mentioning file(1)'s version > > since nicm@ reimplemented file(1) with revision 1.11 on 2015/04/24. He > > or someone else more knowledgable in this regard might want to bump it > > accordingly. > > OpenBSD's /etc/magic doesn't match that provided with any particular > version of file, parts have been updated as people have run into a need > or bug, but there's been no wholesale update. Perhaps this would do? > > diff --git usr.bin/file/magic.5 usr.bin/file/magic.5 > index 812c4c31095..f9ea22c3cc4 100644 > --- usr.bin/file/magic.5 > +++ usr.bin/file/magic.5 > @@ -41,10 +41,9 @@ > This manual page documents the format of the magic file as > used by the > .Xr file 1 > -command, version 4.24. > -The > +command. > .Xr file 1 > -command identifies the type of a file using, > +identifies the type of a file using, > among other tests, > a test for whether the file contains certain > .Dq "magic patterns" . Already thought as much, looks fine to me. > > -.Dd $Mdocdate: April 24 2016 $ > > +.Dd $Mdocdate: July 25 2017 $ > > btw, it's better to leave the CVS keywords ($Mdocdate$, $OpenBSD$, etc) > alone, changing them guarantees a conflict if commits are made to the file. Ah, sure (I knew about this in my first diff).
calendar: Remove support for non-UTF-8 locales
5.9 is out, KOI8 isn't supported anymore.
Index: io.c
===
RCS file: /cvs/src/usr.bin/calendar/io.c,v
retrieving revision 1.44
diff -u -p -r1.44 io.c
--- io.c31 Aug 2016 09:38:47 - 1.44
+++ io.c25 Jul 2017 21:35:50 -
@@ -89,13 +89,9 @@ cal(void)
if (strncmp(buf, "LANG=", 5) == 0) {
(void) setlocale(LC_ALL, buf + 5);
setnnames();
- /* XXX remove KOI8 lines after 5.9 is out */
if (!strcmp(buf + 5, "ru_RU.UTF-8") ||
!strcmp(buf + 5, "uk_UA.UTF-8") ||
- !strcmp(buf + 5, "by_BY.UTF-8") ||
- !strcmp(buf + 5, "ru_RU.KOI8-R") ||
- !strcmp(buf + 5, "uk_UA.KOI8-U") ||
- !strcmp(buf + 5, "by_BY.KOI8-B")) {
+ !strcmp(buf + 5, "by_BY.UTF-8") ) {
bodun_maybe++;
bodun = 0;
free(prefix);
Re: calendar: Remove support for non-UTF-8 locales
On Tue, Jul 25, 2017 at 11:56:42PM +0200, Theo Buehler wrote:
> On Tue, Jul 25, 2017 at 11:38:59PM +0200, Klemens Nanni wrote:
> > 5.9 is out, KOI8 isn't supported anymore.
>
> It's at least the third time I see this diff. Pretty sure that Jan Stary
> sent the same quite a while back.
>
> ok -- without the '))' -> ') )' change.
Overlooked, here you go.
Index: io.c
===
RCS file: /cvs/src/usr.bin/calendar/io.c,v
retrieving revision 1.44
diff -u -p -r1.44 io.c
--- io.c31 Aug 2016 09:38:47 - 1.44
+++ io.c25 Jul 2017 21:35:50 -
@@ -89,13 +89,9 @@ cal(void)
if (strncmp(buf, "LANG=", 5) == 0) {
(void) setlocale(LC_ALL, buf + 5);
setnnames();
- /* XXX remove KOI8 lines after 5.9 is out */
if (!strcmp(buf + 5, "ru_RU.UTF-8") ||
!strcmp(buf + 5, "uk_UA.UTF-8") ||
- !strcmp(buf + 5, "by_BY.UTF-8") ||
- !strcmp(buf + 5, "ru_RU.KOI8-R") ||
- !strcmp(buf + 5, "uk_UA.KOI8-U") ||
- !strcmp(buf + 5, "by_BY.KOI8-B")) {
+ !strcmp(buf + 5, "by_BY.UTF-8")) {
bodun_maybe++;
bodun = 0;
free(prefix);
Re: kill: Use __dead, declare functions static
On Mon, Jul 24, 2017 at 06:11:32PM -0400, Ted Unangst wrote: > Klemens Nanni wrote: > > usage() never returns, all functions are to be used within this unit > > only. > > > > Since changes are conflicting, I'll wait for this diff first, but I'd > > like to remove the void casts for fprintf and use getprogname(3) over > > __progname as well. > > > > Feedback? Comments? > > I enjoy doing these little cleanups myself, but usually (or always even) it's > because I'm reading and studying the code for some other reason. There's not > much benefit to going through all the code and making such changes > mechanically. In the worst case, I think it makes old code look maintained > when it's not. Having old code look old can be useful. > > I know, this sounds silly. Isn't bringing the code up to modern standards part > of maintaining it? Yes, that's true. I'm not sure there's a good explanation. > But a diff to add __dead everywhere in usr.bin feels more like a just a fresh > coat of paint than actual improvement. This pretty much nails it, I'm convinced. Thank you.
Re: kill: Use __dead, declare functions static
On Mon, Jul 24, 2017 at 03:27:05PM -0600, Theo de Raadt wrote:
> this addiction to static is entirely pointless.
Consider it a matter of taste and leave it out, then. I assume you're
fine with __dead, though?
Index: kill.c
===
RCS file: /cvs/src/bin/kill/kill.c,v
retrieving revision 1.14
diff -u -p -r1.14 kill.c
--- kill.c 29 Mar 2017 22:40:15 - 1.14
+++ kill.c 24 Jul 2017 21:46:24 -
@@ -42,10 +42,10 @@
extern char *__progname;
-void nosig(char *);
+void __dead nosig(char *);
void printsignals(FILE *);
int signame_to_signum(char *);
-void usage(void);
+void __dead usage(void);
int
main(int argc, char *argv[])
@@ -148,7 +148,7 @@ signame_to_signum(char *sig)
return (-1);
}
-void
+void __dead
nosig(char *name)
{
@@ -171,7 +171,7 @@ printsignals(FILE *fp)
}
}
-void
+void __dead
usage(void)
{
(void)fprintf(stderr, "usage: %s [-s signal_name] pid ...\n",
kill: Use __dead, declare functions static
usage() never returns, all functions are to be used within this unit
only.
Since changes are conflicting, I'll wait for this diff first, but I'd
like to remove the void casts for fprintf and use getprogname(3) over
__progname as well.
Feedback? Comments?
Index: kill.c
===
RCS file: /cvs/src/bin/kill/kill.c,v
retrieving revision 1.14
diff -u -p -r1.14 kill.c
--- kill.c 29 Mar 2017 22:40:15 - 1.14
+++ kill.c 24 Jul 2017 21:03:03 -
@@ -42,10 +42,10 @@
extern char *__progname;
-void nosig(char *);
-void printsignals(FILE *);
-int signame_to_signum(char *);
-void usage(void);
+static void __dead nosig(char *);
+static voidprintsignals(FILE *);
+static int signame_to_signum(char *);
+static void __dead usage(void);
int
main(int argc, char *argv[])
@@ -148,7 +148,7 @@ signame_to_signum(char *sig)
return (-1);
}
-void
+static void __dead
nosig(char *name)
{
@@ -171,7 +171,7 @@ printsignals(FILE *fp)
}
}
-void
+static void __dead
usage(void)
{
(void)fprintf(stderr, "usage: %s [-s signal_name] pid ...\n",
Re: kill: Use __dead, declare functions static
On Mon, Jul 24, 2017 at 04:17:46PM -0600, Theo de Raadt wrote: > > I know, this sounds silly. Isn't bringing the code up to modern standards > > part > > of maintaining it? > > I suppose that's my question: > > What is it about __dead that makes it part of "modern standards", when it > isn't dead, and an actual keyword. __no_return isn't a standards mandated > keyword either. > > What makes this modern, if adoption if this is still MAYBE in the future? > > In tiny programs like this, what's the gain? > > I say zero, I think it is a waste of time, because this is all the > lipstick. Just consistency with other programs, style(9) conformance and "lipstick" indeed. > I do weird things like whitespace removals all the time, but the > process is different. I audit an entire program manually, cleaning it > up and making annotations and notes as I go through, and then some of > the changes are commited and many others are discarded. > > Just saying that what I'm seeing here doesn't feel like the same > valuable process. That's where I still thought of these clean ups as worth to be committed, I guess. Thanks.
Re: test: Add "<" and ">" to grammar comment, adjust alignment
On Mon, Jul 24, 2017 at 08:31:26PM +0200, Klemens Nanni wrote: > Add missing operators and make the grammar more readable while using > spaces and tabs consistently. Missing bits: operands can be more than "any legacy UNIX file name" of course, correct that as well. One might just say or but I think nicely catches files and directories.
test: Catch integer overflow, fail on trailing whitespaces
test's internal getn() makes integers out of strings although boundaries
are checked for long which leads to wrong test results when operands are
greater than INT_MAX but smaller than LONG_MAX:
$ t() { /bin/test "$1" -lt 0 && : overflow; }
$ t 1
+ /bin/test 1 -lt 0
$ t $((1 << 31))# INT32_MAX + 1
+ /bin/test 2147483648 -lt 0
+ : overflow
n=$((0x7fff))
n=${n%7}8
$ t $n # INT64_MAX + 1
+ /bin/test 9223372036854775808 -lt 0
test: 9223372036854775808: out of range
The INT64_MAX case is expected behaviour, just wanted to illustrate it.
Note how $n is crafted manually since $((1 << 63)) overflows in our KSH
already.
$ t "0 "
+ /bin/test 0 -lt 0
With this diff overflows are properly catched:
$ t $((1 << 31))
+ /usr/obj/bin/test 2147483648 -lt 0
test: 2147483648: too large
Another intended side effect is that trailing whitespaces in integer
operands will now cause test(1) to fail:
$ t "0 "
+ /usr/obj/bin/test/test 0 -lt 0
test: 0 : invalid
Expecting some people to oppose, I'd like to discuss this particular
change in behaviour. Here are my four cents:
- When used in shell scripts, integer operands should usually
not require quoting, whether they're passed verbatim, through
variables or subshells or the like. If they do contain
whitespaces, the shell already takes care of them when
omitting quotes since after variable substitution/expansion
and before passing arguments to the executable or built-in.
If quotes are used (as in "take this as is"), I generally
prefer to be warned instead of letting unexpected characters
slip through, especially when it comes to arithmetic
operations.
- While properly returing integers, strtol(3) still considers
trailing non-digits as error, stripping them in test(1) as of
now seems more like a convenience hack (possibly problematic).
- Using the more strict strtonum(3) is not only safer but also
way simpler and shorter. The manual pages tell us that leading
whitespaces are ok but trailing ones are not.
- Behaviour across binaries and built-ins of various shells
already differs so changing/improving our test(1) won't break
consistency. This was tested with `test "0 " -lt 0':
* test(1) from GNU coreutils 8.25 returns 0
* bash, dash and ksh return 0
* zsh returns 2 and warns "integer expression expected: 0 "
* fish silently returns 1
Feedback and comments, please.
Index: test.c
===
RCS file: /cvs/src/bin/test/test.c,v
retrieving revision 1.17
diff -u -p -r1.17 test.c
--- test.c 21 Jan 2017 11:03:42 - 1.17
+++ test.c 24 Jul 2017 15:02:03 -
@@ -13,6 +13,7 @@
#include
#include
+#include
#include
#include
#include
@@ -451,26 +448,17 @@ t_lex(char *s)
return OPERAND;
}
-/* atoi with error detection */
static int
getn(const char *s)
{
- char *p;
- long r;
-
- errno = 0;
- r = strtol(s, , 10);
-
- if (errno != 0)
- errx(2, "%s: out of range", s);
-
- while (isspace((unsigned char)*p))
- p++;
+ const char *e;
+ int r;
- if (*p)
- errx(2, "%s: bad number", s);
+ r = strtonum(s, INT_MIN, INT_MAX, );
+ if (e)
+ errx(2, "%s: %s", s, e);
- return (int) r;
+ return r;
}
static int
test: Add "<" and ">" to grammar comment, adjust alignment
Add missing operators and make the grammar more readable while using
spaces and tabs consistently.
Feedback? Comments?
Index: test.c
===
RCS file: /cvs/src/bin/test/test.c,v
retrieving revision 1.17
diff -u -p -r1.17 test.c
--- test.c 21 Jan 2017 11:03:42 - 1.17
+++ test.c 24 Jul 2017 15:02:03 -
@@ -22,20 +23,18 @@
#include
/* test(1) accepts the following grammar:
- oexpr ::= aexpr | aexpr "-o" oexpr ;
- aexpr ::= nexpr | nexpr "-a" aexpr ;
- nexpr ::= primary | "!" primary
- primary ::= unary-operator operand
- | operand binary-operator operand
- | operand
- | "(" oexpr ")"
- ;
- unary-operator ::= "-r"|"-w"|"-x"|"-f"|"-d"|"-c"|"-b"|"-p"|
- "-u"|"-g"|"-k"|"-s"|"-t"|"-z"|"-n"|"-o"|"-O"|"-G"|"-L"|"-S";
-
- binary-operator ::= "="|"!="|"-eq"|"-ne"|"-ge"|"-gt"|"-le"|"-lt"|
- "-nt"|"-ot"|"-ef";
- operand ::=
+ oexpr ::= aexpr | aexpr "-o" oexpr
+ aexpr ::= nexpr | nexpr "-a" aexpr
+ nexpr ::= primary | "!" primary
+ primary ::= unary-operator operand |
+ operand binary-operator operand |
+ operand |
+ "(" oexpr ")"
+ unary-operator ::= "-r"|"-w"|"-x"|"-f"|"-d"|"-c"|"-b"|"-p"|"-u"|"-g"|
+ "-k"|"-s"|"-t"|"-z"|"-n"|"-o"|"-O"|"-G"|"-L"|"-S"
+ binary-operator ::= "="|"!="|"<"|">"|"-eq"|"-ne"|"-ge"|"-gt"|"-le"|
+ "-lt"|"-nt"|"-ot"|"-ef"
+ operand ::= | |
*/
enum token {
tech: use getprogname(3)
As done in other places already, replace __progname with getprogname(3).
Feedback? Comments?
Index: test.c
===
RCS file: /cvs/src/bin/test/test.c,v
retrieving revision 1.17
diff -u -p -r1.17 test.c
--- test.c 21 Jan 2017 11:03:42 - 1.17
+++ test.c 24 Jul 2017 15:02:03 -
@@ -155,13 +154,12 @@ static __dead void syntax(const char *op
int
main(int argc, char *argv[])
{
- extern char *__progname;
- int res;
+ int res;
if (pledge("stdio rpath", NULL) == -1)
err(2, "pledge");
- if (strcmp(__progname, "[") == 0) {
+ if (strcmp(getprogname(), "[") == 0) {
if (strcmp(argv[--argc], "]"))
errx(2, "missing ]");
argv[argc] = NULL;
Re: less(1) - segmentation fault with '-g'
On Thu, Jul 27, 2017 at 10:31:51AM +0100, Larry Hynes wrote: > $ env | grep LESS > LESSHISTFILE=- > LESS="-i -M -R -g -c" > LESSCHARSET=utf-8 > > $ unset LESS > $ unset LESSCHARSET > $ unset LESSHISTFILE > > $ LESS="-g" > $ echo 'foo\nbar\nfoo\nbar\nfoo\nbar' | less > > While in less, '/' to search, then '^N', or '!', to search for lines > which do NOT match the pattern, entering foo results in a seg > fault. > > This is on amd64. I can put the core file somewhere if anyone wants it. > > $ uname -a > OpenBSD foo.larryhynes.com 6.1 GENERIC.MP#8 amd64 > > $ less -V > less (OpenBSD) (POSIX regular expressions) > > $ echo $TERM > rxvt-unicode > > $ echo $SHELL > /bin/ksh > > $ dmesg > OpenBSD 6.1 (GENERIC.MP) #8: Tue Jun 27 08:50:26 CEST 2017 > > rob...@syspatch-61-amd64.openbsd.org:/usr/src/sys/arch/amd64/compile/GENERIC.MP > real mem = 4278042624 (4079MB) > avail mem = 4143710208 (3951MB) > mpath0 at root > scsibus0 at mpath0: 256 targets > mainbus0 at root > bios0 at mainbus0: SMBIOS rev. 2.8 @ 0xf68d0 (10 entries) > bios0: vendor SeaBIOS version > "rel-1.10.1-0-g8891697-prebuilt.qemu-project.org" date 04/01/2014 > bios0: QEMU Standard PC (i440FX + PIIX, 1996) I cannot reproduce this behaviour an real amd64 hardware (ThinkPad X230) running a snapshot from Jul. 12th. All of xterm, rxvt-unicode, st and tmux work for the following: $ env | fgrep LESS $ LESS=-g $ type less less is /usr/bin/less $ echo 'foo\nbar\nfoo\nbar\nfoo\nbar' | less Pressing /!fooq matches and quits fine without segmentation fault.
Re: w: Remove XXX'ed check
On Thu, Jul 27, 2017 at 02:03:05PM +0200, Jeremie Courreges-Anglas wrote: > On Thu, Jul 27 2017, Klemens Nanni <k...@posteo.org> wrote: > > Only main() calls pr_args() in L330 with ep->kp as argument which in > > turn is set in L257 or L266 for every utmp entry. kp is checked against > > NULL already in L229. > > > > Even if kp was somehow NULL chances are high we'd fail before pr_args() > > was called anyway since L244, L256 and L265 would then cause a NULL pointer > > dereference. In fact, proc_compare() even expects its second argument to > > to be not NULL (only the first one is checked explicitly). > > > > Not that crashing is guaranteed upon undefined behaviour but noone > > seems to have reported failure within the last 13 years, so I think it's > > safe to remove that check. > > > > Feedback? Comments? > > I don't think this is correct. ep->kp is set only if we find a matching > live process. If we don't, the diff results in a crash. I don't know > all the details of utmp handling, but I really doubt that we can assume > a 1->1 mapping between utmp entries and live processes (stale entries, > race conditions, etc). > > Here's an example with a stale ssh entry: > > ritchie /usr/src/usr.bin/w$ w > 1:59PM up 58 mins, 2 users, load averages: 0.27, 0.16, 0.14 > USERTTY FROM LOGIN@ IDLE WHAT > jca p0 :01:03PM 0 tmux: client > (/tmp/tmux-1000/default) > jca pg 127.0.0.1 1:57PM 1 - > ritchie /usr/src/usr.bin/w$ ./obj/w > 1:59PM up 58 mins, 2 users, load averages: 0.21, 0.16, 0.13 > USERTTY FROM LOGIN@ IDLE WHAT > jca p0 :01:03PM 0 tmux: client > (/tmp/tmux-1000/default) > Segmentation fault (core dumped) > > > So the answer is "yes, this can happen". Ah, I stand corrected about the 1->1 mapping, thanks a lot.
w: Remove XXX'ed check
Only main() calls pr_args() in L330 with ep->kp as argument which in turn is set in L257 or L266 for every utmp entry. kp is checked against NULL already in L229. Even if kp was somehow NULL chances are high we'd fail before pr_args() was called anyway since L244, L256 and L265 would then cause a NULL pointer dereference. In fact, proc_compare() even expects its second argument to to be not NULL (only the first one is checked explicitly). Not that crashing is guaranteed upon undefined behaviour but noone seems to have reported failure within the last 13 years, so I think it's safe to remove that check. Feedback? Comments? Index: w.c === RCS file: /cvs/src/usr.bin/w/w.c,v retrieving revision 1.62 diff -u -p -r1.62 w.c --- w.c 30 May 2017 15:10:48 - 1.62 +++ w.c 26 Jul 2017 00:20:04 - @@ -383,8 +383,6 @@ pr_args(struct kinfo_proc *kp) char **argv, *str; int left; - if (kp == NULL) - goto nothing; /* XXX - can this happen? */ left = argwidth; argv = kvm_getargv(kd, kp, argwidth+60); /* +60 for ftpd snip */ if (argv == NULL)
magic.5: Add missing types
While reading file(1)'s code in #openbsd-daily mulander noted that the 'name' and 'use' types were missing from magic(5). I'm not entirely sure yet whether this is complete, so here's what I did: magic(5) provided by devel/magic documents version 5.31 while base's magic(5) is at 4.24. Here are the types found in 5.31 but not ours as well as those of the missing ones that are actually implemented but undocumented as of now: $ grep -i "TYPE_($(grep -F 'It Dv' $(man -w magic) | cut -d' ' -f3 | sort | uniq -u | paste -sd\| - | tee /dev/stderr))" magic.h beid3|beqwdate|clear|indirect|leid3|leqwdate|name|qwdate|use MAGIC_TYPE_CLEAR, MAGIC_TYPE_NAME, MAGIC_TYPE_USE, What about the current version being 4.21? We're clearly ahead of this, it seems magic(5) wasn't updated when nicm@ reimplemented things. This patch documents the respective types. Feedback/OK? Index: magic.5 === RCS file: /cvs/src/usr.bin/file/magic.5,v retrieving revision 1.17 diff -u -p -r1.17 magic.5 --- magic.5 24 Apr 2016 07:02:07 - 1.17 +++ magic.5 29 Jun 2017 17:41:56 - @@ -218,6 +218,31 @@ This is intended to be used with the tes .Em x (which is always true) and a message that is to be used if there are no other matches. +.It Dv clear +This test is always true and clears the match flag for that continuation +level. +It is intended to be used with the default test. +.It Dv name +Define a +.Dq named +magic instance that can be called from another +.Dv use +magic entry, like a subroutine call. +Named instance direct magic offsets are relative to the offset of the +previous matched entry, but indirect offsets are relative to the +beginning of the file as usual. +Named magic entries always match. +.It Dv use +Recursively call the named magic starting from the current offset. +If the name of the referenced begins with a +.Dv ^ +then the endianness of the magic is switched; if the magic mentioned +.Dv leshort +for example, +it is treated as +.Dv beshort +and vice versa. +This is useful to avoid duplicating the rules for different endianness. .El .Pp Each top-level magic pattern (see below for an explanation of levels)
Re: ktrace: Following symlinks
On Thu, Jun 29, 2017 at 09:50:25PM -0700, Philip Guenther wrote: On Thu, Jun 22, 2017 at 7:17 PM, Klemens Nanni <k...@posteo.org> wrote: So I just wrapped my head around vfs(9) with regard to making ktrace following symlinks again, however I don't quite understand what problems may occur when doing so. May anyone enlighten me on this? IMHO, it makes more sense to add fktrace(2) aka NetBSD where an open fd is passed in. To have a more generic interface? Why not letting ktrace(2) handle this just like it already does for regular files?
file: Simplify Makefile
No need for multiple echos or xargs (wich runs cat only once anyway)
here. The {post-,}magic files stay unchanged.
In magic target don't specify dependencies twice.
OK?
Index: Makefile
===
RCS file: /cvs/src/usr.bin/file/Makefile,v
retrieving revision 1.17
diff -u -p -r1.17 Makefile
--- Makefile28 Jun 2017 13:37:56 - 1.17
+++ Makefile28 Jun 2017 23:59:33 -
@@ -23,12 +23,10 @@ MAG1= $(.CURDIR)/magdir/Header \
MAGFILES= $(.CURDIR)/magdir/[0-9a-z]*
post-magic: $(MAGFILES)
- for i in ${.ALLSRC:N*.orig}; do \
- echo $$i; \
- done|sort|xargs -n 1024 cat >$(.TARGET)
+ cat $$(echo ${.ALLSRC:N*.orig} | tr ' ' '\n' | sort) >$(.TARGET)
magic: $(MAG1) post-magic
- cat ${MAG1} post-magic >$(.TARGET)
+ cat $(.ALLSRC) >$(.TARGET)
afterinstall:
${INSTALL} ${INSTALL_COPY} -o $(MAGICOWN) -g $(MAGICGRP) \
Re: brconfig: Unify/fix strtoul(3) handling
On Sun, Jul 02, 2017 at 01:09:19AM +0100, Ricardo Mestre wrote: You're blindly replacing strtoul with strtonum without taking the base 0 into account. I'd recommend you take a look at strtoul's manpage first and check what using base 0 means (strtonum always use base 10). Don't worry, I can read; this is fully intentional. Ted already mentioned the base "issue" weeks ago so I checked and updated the diff accordingly.
tee: Replace hand-rolled list with SLIST_*
No functional change or bugfix but queue(3) is the for a reason. That
way the code even is a tad clearer to me (and two lines shorter).
Feedback/OK?
Index: tee.c
===
RCS file: /cvs/src/usr.bin/tee/tee.c,v
retrieving revision 1.11
diff -u -p -r1.11 tee.c
--- tee.c 28 Oct 2016 07:22:59 - 1.11
+++ tee.c 2 Jul 2017 02:14:54 -
@@ -32,6 +32,7 @@
#include
#include
+#include
#include
#include
@@ -42,30 +43,26 @@
#include
#include
-struct list {
- struct list *next;
+struct file {
+ SLIST_ENTRY(file) next;
int fd;
char *name;
-};
-struct list *head;
+} *fp;
+SLIST_HEAD(, file) head;
static void
add(int fd, char *name)
{
- struct list *p;
-
- if ((p = malloc(sizeof(*p))) == NULL)
+ if ((fp = malloc(sizeof(struct file))) == NULL)
err(1, NULL);
- p->fd = fd;
- p->name = name;
- p->next = head;
- head = p;
+ fp->fd = fd;
+ fp->name = name;
+ SLIST_INSERT_HEAD(, fp, next);
}
int
main(int argc, char *argv[])
{
- struct list *p;
int fd;
ssize_t n, rval, wval;
char *bp;
@@ -75,6 +72,8 @@ main(int argc, char *argv[])
if (pledge("stdio wpath cpath", NULL) == -1)
err(1, "pledge");
+ SLIST_INIT();
+
append = 0;
while ((ch = getopt(argc, argv, "ai")) != -1) {
switch(ch) {
@@ -109,12 +108,12 @@ main(int argc, char *argv[])
err(1, "pledge");
while ((rval = read(STDIN_FILENO, buf, sizeof(buf))) > 0) {
- for (p = head; p; p = p->next) {
+ SLIST_FOREACH(fp, , next) {
n = rval;
bp = buf;
do {
- if ((wval = write(p->fd, bp, n)) == -1) {
- warn("%s", p->name);
+ if ((wval = write(fp->fd, bp, n)) == -1) {
+ warn("%s", fp->name);
exitval = 1;
break;
}
@@ -127,12 +126,11 @@ main(int argc, char *argv[])
exitval = 1;
}
- for (p = head; p; p = p->next) {
- if (close(p->fd) == -1) {
- warn("%s", p->name);
+ SLIST_FOREACH(fp, , next)
+ if (close(fp->fd) == -1) {
+ warn("%s", fp->name);
exitval = 1;
}
- }
return exitval;
}
Re: brconfig: Unify/fix strtoul(3) handling
On Fri, Jun 09, 2017 at 08:33:16PM +0200, Klemens Nanni wrote:
No need for temporary variables either, strtonum guarantees through
maxval that its return value won't overflow when casted.
Index: brconfig.c
===
RCS file: /cvs/src/sbin/ifconfig/brconfig.c,v
retrieving revision 1.15
diff -u -p -r1.15 brconfig.c
--- brconfig.c 7 Jun 2017 16:47:29 - 1.15
+++ brconfig.c 9 Jun 2017 17:33:10 -
@@ -220,7 +220,6 @@ bridge_ifsetflag(const char *ifsname, u_
err(1, "%s: ioctl SIOCBRDGGIFFLGS %s", name, ifsname);
req.ifbr_ifsflags |= flag & ~IFBIF_RO_MASK;
-
if (ioctl(s, SIOCBRDGSIFFLGS, (caddr_t)) < 0)
err(1, "%s: ioctl SIOCBRDGSIFFLGS %s", name, ifsname);
}
@@ -232,12 +231,10 @@ bridge_ifclrflag(const char *ifsname, u_
strlcpy(req.ifbr_name, name, sizeof(req.ifbr_name));
strlcpy(req.ifbr_ifsname, ifsname, sizeof(req.ifbr_ifsname));
-
if (ioctl(s, SIOCBRDGGIFFLGS, (caddr_t)) < 0)
err(1, "%s: ioctl SIOCBRDGGIFFLGS %s", name, ifsname);
req.ifbr_ifsflags &= ~(flag | IFBIF_RO_MASK);
-
if (ioctl(s, SIOCBRDGSIFFLGS, (caddr_t)) < 0)
err(1, "%s: ioctl SIOCBRDGSIFFLGS %s", name, ifsname);
}
@@ -400,18 +397,13 @@ void
bridge_timeout(const char *arg, int d)
{
struct ifbrparam bp;
- long newtime;
- char *endptr;
+ const char *errstr;
- errno = 0;
- newtime = strtol(arg, , 0);
- if (arg[0] == '\0' || endptr[0] != '\0' ||
- (newtime & ~INT_MAX) != 0L ||
- (errno == ERANGE && newtime == LONG_MAX))
- errx(1, "invalid arg for timeout: %s", arg);
+ bp.ifbrp_ctime = strtonum(arg, 0, INT_MAX, );
+ if (errstr != NULL)
+ errx(1, "%s arg for timeout: %s", errstr, arg);
strlcpy(bp.ifbrp_name, name, sizeof(bp.ifbrp_name));
- bp.ifbrp_ctime = newtime;
if (ioctl(s, SIOCBRDGSTO, (caddr_t)) < 0)
err(1, "%s", name);
}
@@ -420,17 +412,13 @@ void
bridge_maxage(const char *arg, int d)
{
struct ifbrparam bp;
- unsigned long v;
- char *endptr;
+ const char *errstr;
- errno = 0;
- v = strtoul(arg, , 0);
- if (arg[0] == '\0' || endptr[0] != '\0' || v > 0xffUL ||
- (errno == ERANGE && v == ULONG_MAX))
- errx(1, "invalid arg for maxage: %s", arg);
+ bp.ifbrp_maxage = strtonum(arg, 0, UINT8_MAX, );
+ if (errstr != NULL)
+ errx(1, "%s arg for maxage: %s", errstr, arg);
strlcpy(bp.ifbrp_name, name, sizeof(bp.ifbrp_name));
- bp.ifbrp_maxage = v;
if (ioctl(s, SIOCBRDGSMA, (caddr_t)) < 0)
err(1, "%s", name);
}
@@ -439,17 +427,13 @@ void
bridge_priority(const char *arg, int d)
{
struct ifbrparam bp;
- unsigned long v;
- char *endptr;
+ const char *errstr;
- errno = 0;
- v = strtoul(arg, , 0);
- if (arg[0] == '\0' || endptr[0] != '\0' || v > 0xUL ||
- (errno == ERANGE && v == ULONG_MAX))
- errx(1, "invalid arg for spanpriority: %s", arg);
+ bp.ifbrp_prio = strtonum(arg, 0, UINT16_MAX, );
+ if (errstr != NULL)
+ errx(1, "%s arg for spanpriority: %s", errstr, arg);
strlcpy(bp.ifbrp_name, name, sizeof(bp.ifbrp_name));
- bp.ifbrp_prio = v;
if (ioctl(s, SIOCBRDGSPRI, (caddr_t)) < 0)
err(1, "%s", name);
}
@@ -478,17 +462,13 @@ void
bridge_fwddelay(const char *arg, int d)
{
struct ifbrparam bp;
- unsigned long v;
- char *endptr;
+ const char *errstr;
- errno = 0;
- v = strtoul(arg, , 0);
- if (arg[0] == '\0' || endptr[0] != '\0' || v > 0xffUL ||
- (errno == ERANGE && v == ULONG_MAX))
- errx(1, "invalid arg for fwddelay: %s", arg);
+ bp.ifbrp_fwddelay = strtonum(arg, 0, UINT8_MAX, );
+ if (errstr != NULL)
+ errx(1, "%s arg for fwddelay: %s", errstr, arg);
strlcpy(bp.ifbrp_name, name, sizeof(bp.ifbrp_name));
- bp.ifbrp_fwddelay = v;
if (ioctl(s, SIOCBRDGSFD, (caddr_t)) < 0)
err(1, "%s", name);
}
@@ -497,17 +477,13 @@ void
bridge_hellotime(const char *arg, int d)
{
struct ifbrparam bp;
- unsigned long v;
- char *endptr;
+ const char *errstr;
- errno = 0;
- v = strtoul(arg, , 0);
- if (arg[0] == '\0' || endptr[0] != '\0' || v > 0xffUL ||
- (errno == ERANGE && v == ULONG_MAX))
- errx(1, "invalid arg for hellotime: %s", arg);
+ bp.ifbrp_hellotime = strtonum(arg, 0, UINT8_MAX, );
+ if (errstr != NULL)
+
Re: install.sub: Clean v[46]_info() ouput
On Mon, Jul 03, 2017 at 10:47:31PM +, Robert Peichaer wrote:
Dokument explicitely possible outputs and tweak the sed expressions
to remove the superfluous whitespaces. I guess that does the trick.
Index: install.sub
===
RCS file: /cvs/src/distrib/miniroot/install.sub,v
retrieving revision 1.1019
diff -u -p -p -u -r1.1019 install.sub
--- install.sub 2 Jul 2017 12:45:43 - 1.1019
+++ install.sub 3 Jul 2017 22:34:04 -
@@ -896,13 +896,16 @@ __EOT
}
# Obtain and output the inet information related to interface $1.
-# Should output ' '.
+# Outputs one of:
+#
+#
+# \n [\n]
v4_info() {
ifconfig $1 inet | sed -n '
1s/.*
install.sub: Typo/whitespace nit
Remove duplicate full stop and add space after function name.
Feedback/OK?
Index: install.sub
===
RCS file: /cvs/src/distrib/miniroot/install.sub,v
retrieving revision 1.1019
diff -u -p -r1.1019 install.sub
--- install.sub 2 Jul 2017 12:45:43 - 1.1019
+++ install.sub 4 Jul 2017 00:11:59 -
@@ -275,7 +275,7 @@ diskinfo() {
_i=${_i##+([[:space:],])}
_i=${_i%%+([[:space:],])}
- # Extract Network Address Authority information from dmesg..
+ # Extract Network Address Authority information from dmesg.
_n=$(dmesg | sed -En '/^'$_d' at /h;${g;s/^.*
([a-z0-9]+\.[a-zA-Z0-9_]+)$/\1/p;}')
# Extract disk size from disklabel output.
@@ -2968,7 +2970,7 @@ do_install() {
finish_up
}
-do_upgrade(){
+do_upgrade() {
# Get $ROOTDISK and $ROOTDEV
get_rootinfo
Re: magic.5: Add missing types
On Mon, Jul 03, 2017 at 05:36:52PM +0100, Nicholas Marriott wrote: Hi On Thu, Jun 29, 2017 at 09:29:57PM +0200, Klemens Nanni wrote: [...] What about the current version being 4.21? We're clearly ahead of this, it seems magic(5) wasn't updated when nicm@ reimplemented things. This patch documents the respective types. Did you copy this text from upstream or write it? It's copied from magic.5 provided by devel/libmagic-5.31. Some comments inline. Feedback/OK? Index: magic.5 === RCS file: /cvs/src/usr.bin/file/magic.5,v retrieving revision 1.17 diff -u -p -r1.17 magic.5 --- magic.5 24 Apr 2016 07:02:07 - 1.17 +++ magic.5 29 Jun 2017 17:41:56 - @@ -218,6 +218,31 @@ This is intended to be used with the tes .Em x (which is always true) and a message that is to be used if there are no other matches. +.It Dv clear +This test is always true and clears the match flag for that continuation +level. I would just say "that level" or "the test's level" not "that continuation level" because there is no previous mention of "continuation". +It is intended to be used with the default test. +.It Dv name +Define a +.Dq named I don't think quotation marks are necessary here. +magic instance that can be called from another +.Dv use +magic entry, like a subroutine call. +Named instance direct magic offsets are relative to the offset of the +previous matched entry, but indirect offsets are relative to the +beginning of the file as usual. +Named magic entries always match. +.It Dv use +Recursively call the named magic starting from the current offset. +If the name of the referenced begins with a "the referenced instance" would be better than "the referenced". Sounds good to me.
Re: ping: Style fixes/cleanups
Purely cosmetic/style(9) fixes: Remove unneeded indent in prototypes,
add space after return keyword.
Feedback/OK?
Index: ping.c
===
RCS file: /cvs/src/sbin/ping/ping.c,v
retrieving revision 1.220
diff -u -p -r1.220 ping.c
--- ping.c 4 Jul 2017 15:55:22 - 1.220
+++ ping.c 4 Jul 2017 21:43:12 -
@@ -208,34 +208,34 @@ volatile sig_atomic_t seenalrm;
volatile sig_atomic_t seenint;
volatile sig_atomic_t seeninfo;
-voidfill(char *, char *);
-voidsummary(void);
-voidonsignal(int);
-voidretransmit(int);
-int pinger(int);
-const char *pr_addr(struct sockaddr *, socklen_t);
-voidpr_pack(u_char *, int, struct msghdr *);
-__dead void usage(void);
+voidfill(char *, char *);
+voidsummary(void);
+voidonsignal(int);
+voidretransmit(int);
+int pinger(int);
+const char *pr_addr(struct sockaddr *, socklen_t);
+voidpr_pack(u_char *, int, struct msghdr *);
+__dead void usage(void);
/* IPv4 specific functions */
-voidpr_ipopt(int, u_char *);
-int in_cksum(u_short *, int);
-voidpr_icmph(struct icmp *);
-voidpr_retip(struct ip *);
-voidpr_iph(struct ip *);
+voidpr_ipopt(int, u_char *);
+int in_cksum(u_short *, int);
+voidpr_icmph(struct icmp *);
+voidpr_retip(struct ip *);
+voidpr_iph(struct ip *);
#ifndef SMALL
-int map_tos(char *, int *);
+int map_tos(char *, int *);
#endif /* SMALL */
/* IPv6 specific functions */
-int get_hoplim(struct msghdr *);
-int get_pathmtu(struct msghdr *, struct sockaddr_in6 *);
-voidpr_icmph6(struct icmp6_hdr *, u_char *);
-voidpr_iph6(struct ip6_hdr *);
-voidpr_exthdrs(struct msghdr *);
-voidpr_ip6opt(void *);
-voidpr_rthdr(void *);
-voidpr_retip6(struct ip6_hdr *, u_char *);
+int get_hoplim(struct msghdr *);
+int get_pathmtu(struct msghdr *, struct sockaddr_in6 *);
+voidpr_icmph6(struct icmp6_hdr *, u_char *);
+voidpr_iph6(struct ip6_hdr *);
+voidpr_exthdrs(struct msghdr *);
+voidpr_ip6opt(void *);
+voidpr_rthdr(void *);
+voidpr_retip6(struct ip6_hdr *, u_char *);
int
main(int argc, char *argv[])
@@ -572,7 +572,7 @@ main(int argc, char *argv[])
timing = 1;
if (v6flag) {
- /* in F_VERBOSE case, we may get non-echoreply packets*/
+ /* in F_VERBOSE case, we may get non-echoreply packets */
if (options & F_VERBOSE && datalen < 2048) /* XXX 2048? */
packlen = 2048 + IP6LEN + ECHOLEN + EXTRA;
else
@@ -965,7 +965,7 @@ pr_addr(struct sockaddr *addr, socklen_t
if (getnameinfo(addr, addrlen, buf, sizeof(buf), NULL, 0, flag) == 0)
return (buf);
else
- return "?";
+ return ("?");
}
/*
@@ -1023,7 +1023,7 @@ pinger(int s)
u_int16_t seq;
if (npackets && ntransmitted >= npackets)
- return(-1); /* no more transmission */
+ return (-1);/* no more transmission */
seq = htons(ntransmitted++);
@@ -1459,7 +1459,7 @@ in_cksum(u_short *addr, int len)
sum = (sum >> 16) + (sum & 0x); /* add hi 16 to low 16 */
sum += (sum >> 16); /* add carry */
answer = ~sum; /* truncate to 16 bits */
- return(answer);
+ return (answer);
}
/*
@@ -1849,15 +1849,15 @@ get_hoplim(struct msghdr *mhdr)
for (cm = (struct cmsghdr *)CMSG_FIRSTHDR(mhdr); cm;
cm = (struct cmsghdr *)CMSG_NXTHDR(mhdr, cm)) {
if (cm->cmsg_len == 0)
- return(-1);
+ return (-1);
if (cm->cmsg_level == IPPROTO_IPV6 &&
cm->cmsg_type == IPV6_HOPLIMIT &&
cm->cmsg_len == CMSG_LEN(sizeof(int)))
- return(*(int *)CMSG_DATA(cm));
+ return (*(int *)CMSG_DATA(cm));
}
- return(-1);
+ return (-1);
}
int
@@ -1869,7 +1869,7 @@ get_pathmtu(struct msghdr *mhdr, struct
for (cm = (struct cmsghdr *)CMSG_FIRSTHDR(mhdr); cm;
cm = (struct cmsghdr *)CMSG_NXTHDR(mhdr, cm)) {
if (cm->cmsg_len == 0)
- return(0);
+ return (0);
if (cm->cmsg_level == IPPROTO_IPV6 &&
cm->cmsg_type == IPV6_PATHMTU &&
@@ -1897,7 +1897,7 @@
Re: install.sub: ieee80211_{scan,config}: Allow quoted SSIDs
On Mon, Jul 03, 2017 at 12:45:32AM +0200, Klemens Nanni wrote:
Thanks for looking into it.
On Sun, Jul 02, 2017 at 04:32:43PM +, Robert Peichaer wrote:
ieee80211_scan()
- Extract the needed information (nwid, bssid) using a very specific
sed expression. Any line, not matching this expr is ignored.
- Remove leading and trailing double-quotes in case of nwids with
spaces.
I had the ugly case of an empty SSID in reach while testing this so I
intentionally left double quotes in place within WLANLIST so that
the list presented to the user wouldn't look broken, e.g.
"my wifi" chan 1 bssid ...
"" chan 2 bssid ...
as opposed to
my wifi chan 1 bssid ...
chan 2 bssid ...
I'd also leave unqouting to the routine that actually requires it
instead of the function that just provides the list.
- Write nwid and bssid into WLANLIST as '()'.
Writing the simple format directly to cache seems like a good idea
instead of just cutting ^nwid first here and .*$ somewhere else.
ieee80211_config()
- just print WLANLIST using ieee80211_scan() if the user chooses
'?' which has the right format already
- in case the user selects an entry from WLANLIST using a number,
remove the '()' part from the line, resulting in
the nwid (without double-quotes)
- using the quote() function with the ifconfig command ensures,
that the nwid is quoted properly with single-quotes in case it
contains spaces
This is not needed as "$_nwid" will even work if _nwid='my "wifi'.
- using the quote() function when writing the nwid to the hostname.if
files ensures that the nwid is quoted properly with single-quotes
in case it contains spaces
The parse_hn_line() function in netstart does handle quoted nwids
properly when processing the hostname.if config lines as far as I
can see.
Yes, it does. But it chokes on SSIDs containing a literal " for example.
Here is an updated diff taking above considerations into account.
Note how ([[:xdigit:]:]*)$ when picking the answer must not be
simplified to (.*)$ as this would fail on SSIDs like "my (hidden) wifi".
Feedback/OK?
That patch was mangled, sorry. Here it goes again.
Index: install.sub
===
RCS file: /cvs/src/distrib/miniroot/install.sub,v
retrieving revision 1.1019
diff -u -p -r1.1019 install.sub
--- install.sub 2 Jul 2017 12:45:43 - 1.1019
+++ install.sub 4 Jul 2017 20:43:56 -
@@ -1060,10 +1060,9 @@ v6_config() {
# Perform an 802.11 network scan on interface $1.
# The result is cached in $WLANLIST.
ieee80211_scan() {
- # N.B. Skipping quoted nwid's for now.
[[ -f $WLANLIST ]] ||
ifconfig $1 scan |
- sed -n 's/^ nwid \([^"]\)/\1/p' >$WLANLIST
+ sed -n 's/^[[:space:]]*nwid \(.*\) chan [0-9]* bssid
\([[:xdigit:]:]*\).*/\1 (\2)/p' >$WLANLIST
cat $WLANLIST
}
@@ -1082,12 +1081,12 @@ ieee80211_config() {
ask_until "Access point? (ESSID, 'any', list# or '?')" "any"
case "$resp" in
+([0-9]))
- _nwid=$(ieee80211_scan $_if | sed -n "${resp}s/ .*//p")
+ _nwid=$(ieee80211_scan $_if |
+ sed -n ${resp}'{s/ ([[:xdigit:]:]*)$//p;q;}')
[[ -z $_nwid ]] && echo "There is no line $resp."
+ [[ $_nwid = \"*\" ]] && _nwid=${_nwid#\"}
_nwid=${_nwid%\"}
;;
- \?) ieee80211_scan $_if |
- sed -n 's/^\([^ ]*\) chan .* bssid \([^ ]*\)
.*$/ \1 (\2)/p' |
- cat -n | more -c
+ \?) ieee80211_scan $_if | cat -n | more -c
;;
*) _nwid=$resp
;;
Re: ping: Style fixes/cleanups
On Tue, Jul 04, 2017 at 03:58:13PM +, Florian Obser wrote:
I like most (all? of the correct ones), can you have another go
at trying to fix the pointed out changes in behaviour?
Thanks natano for pointing out these stupid mistakes.
Unify option checking, check for F_RROUTE only if F_HDRINCL is not set
already (they're incompatible and checked together already beforehand).
Feedback/OK?
Index: ping.c
===
RCS file: /cvs/src/sbin/ping/ping.c,v
retrieving revision 1.220
diff -u -p -r1.220 ping.c
--- ping.c 4 Jul 2017 15:55:22 - 1.220
+++ ping.c 4 Jul 2017 21:18:36 -
@@ -562,10 +562,10 @@ main(int argc, char *argv[])
(void)setsockopt(s, SOL_SOCKET, SO_DEBUG, ,
sizeof(optval));
- if ((options & F_FLOOD) && (options & F_INTERVAL))
+ if (options & F_FLOOD && options & F_INTERVAL)
errx(1, "-f and -i options are incompatible");
- if ((options & F_FLOOD) && (options & (F_AUD_RECV | F_AUD_MISS)))
+ if (options & F_FLOOD && options & (F_AUD_RECV | F_AUD_MISS))
warnx("No audible output for flood pings");
if (datalen >= sizeof(struct payload)) /* can we time transfer */
@@ -621,7 +621,7 @@ main(int argc, char *argv[])
* let the kernel pass extension headers of incoming packets,
* for privileged socket options
*/
- if ((options & F_VERBOSE) != 0) {
+ if (options & F_VERBOSE) {
int opton = 1;
if (setsockopt(s, IPPROTO_IPV6, IPV6_RECVHOPOPTS,
@@ -717,10 +717,8 @@ main(int argc, char *argv[])
else
ip->ip_src.s_addr = INADDR_ANY;
ip->ip_dst = dst4.sin_addr;
- }
-
/* record route option */
- if (options & F_RROUTE) {
+ } else if (options & F_RROUTE) {
if (IN_MULTICAST(ntohl(dst4.sin_addr.s_addr)))
errx(1, "record route not valid to multicast"
" destinations");
@@ -771,7 +769,7 @@ main(int argc, char *argv[])
(void)signal(SIGINT, onsignal);
(void)signal(SIGINFO, onsignal);
- if ((options & F_FLOOD) == 0) {
+ if (!(options & F_FLOOD)) {
(void)signal(SIGALRM, onsignal);
itimer.it_interval = interval;
itimer.it_value = interval;
@@ -805,7 +803,7 @@ main(int argc, char *argv[])
if (ntransmitted - nreceived - 1 > nmissedmax) {
nmissedmax = ntransmitted - nreceived - 1;
if (!(options & F_FLOOD) &&
- (options & F_AUD_MISS))
+ options & F_AUD_MISS)
(void)fputc('\a', stderr);
}
continue;
@@ -859,7 +857,7 @@ main(int argc, char *argv[])
* a path MTU notification.)
*/
if ((mtu = get_pathmtu(, )) > 0) {
- if ((options & F_VERBOSE) != 0) {
+ if (options & F_VERBOSE) {
printf("new path MTU (%d) is "
"notified\n", mtu);
}
@@ -959,7 +957,7 @@ pr_addr(struct sockaddr *addr, socklen_t
static char buf[NI_MAXHOST];
int flag = 0;
- if ((options & F_HOSTNAME) == 0)
+ if (!(options & F_HOSTNAME))
flag |= NI_NUMERICHOST;
if (getnameinfo(addr, addrlen, buf, sizeof(buf), NULL, 0, flag) == 0)
@@ -1890,7 +1888,7 @@ get_pathmtu(struct msghdr *mhdr, struct
dst->sin6_scope_id &&
mtuctl->ip6m_addr.sin6_scope_id !=
dst->sin6_scope_id)) {
- if ((options & F_VERBOSE) != 0) {
+ if (options & F_VERBOSE) {
printf("path MTU for %s is notified. "
"(ignored)\n",
pr_addr((struct sockaddr *)
Re: ping: Style fixes/cleanups
On Tue, Jul 04, 2017 at 04:00:43PM +, Florian Obser wrote:
yeah, this is arse backwards, I'm willing to commit the oposite though,
i.e. get rid of the void casts for printf
Casts removed, cosecutive calls merged where suitable.
Feedback/OK?
Index: ping.c
===
RCS file: /cvs/src/sbin/ping/ping.c,v
retrieving revision 1.220
diff -u -p -r1.220 ping.c
--- ping.c 4 Jul 2017 15:55:22 - 1.220
+++ ping.c 4 Jul 2017 19:39:35 -
@@ -914,19 +914,19 @@ fill(char *bp, char *patp)
for (jj = 0; jj < ii; ++jj)
bp[jj + kk] = pat[jj];
if (!(options & F_QUIET)) {
- (void)printf("PATTERN: 0x");
+ printf("PATTERN: 0x");
for (jj = 0; jj < ii; ++jj)
- (void)printf("%02x", bp[jj] & 0xFF);
- (void)printf("\n");
+ printf("%02x", bp[jj] & 0xFF);
+ printf("\n");
}
}
void
summary(void)
{
- printf("\n--- %s ping statistics ---\n", hostname);
- printf("%lld packets transmitted, ", ntransmitted);
- printf("%lld packets received, ", nreceived);
+ printf("\n--- %s ping statistics ---\n"
+ "%lld packets transmitted, %lld packets received, ",
+ hostname, ntransmitted, nreceived);
if (nrepeats)
printf("%lld duplicates, ", nrepeats);
@@ -944,8 +944,8 @@ summary(void)
double num = nreceived + nrepeats;
double avg = tsum / num;
double dev = sqrt(fmax(0, tsumsq / num - avg * avg));
- printf("round-trip min/avg/max/std-dev = %.3f/%.3f/%.3f/%.3f
ms\n",
- tmin, avg, tmax, dev);
+ printf("round-trip min/avg/max/std-dev = "
+ "%.3f/%.3f/%.3f/%.3f ms\n", tmin, avg, tmax, dev);
}
}
@@ -1209,7 +1209,7 @@ pr_pack(u_char *buf, int cc, struct msgh
if (timingsafe_memcmp(mac, ,
sizeof(mac)) != 0) {
- (void)printf("signature mismatch!\n");
+ printf("signature mismatch!\n");
return;
}
timinginfo=1;
@@ -1245,19 +1245,19 @@ pr_pack(u_char *buf, int cc, struct msgh
if (options & F_FLOOD)
(void)write(STDOUT_FILENO, , 1);
else {
- (void)printf("%d bytes from %s: icmp_seq=%u", cc,
+ printf("%d bytes from %s: icmp_seq=%u", cc,
pr_addr(from, fromlen), ntohs(seq));
if (v6flag)
- (void)printf(" hlim=%d", hoplim);
+ printf(" hlim=%d", hoplim);
else
- (void)printf(" ttl=%d", ip->ip_ttl);
+ printf(" ttl=%d", ip->ip_ttl);
if (cc >= ECHOLEN + ECHOTMLEN)
- (void)printf(" time=%.3f ms", triptime);
+ printf(" time=%.3f ms", triptime);
if (dupflag)
- (void)printf(" (DUP!)");
+ printf(" (DUP!)");
/* check the data */
if (cc - ECHOLEN < datalen)
- (void)printf(" (TRUNC!)");
+ printf(" (TRUNC!)");
if (v6flag)
cp = buf + ECHOLEN + ECHOTMLEN;
else
@@ -1267,7 +1267,7 @@ pr_pack(u_char *buf, int cc, struct msgh
i < cc && i < datalen;
++i, ++cp, ++dp) {
if (*cp != *dp) {
- (void)printf("\nwrong data byte #%d "
+ printf("\nwrong data byte #%d "
"should be 0x%x but was 0x%x",
i - ECHOLEN, *dp, *cp);
if (v6flag)
@@ -1278,8 +1278,8 @@ pr_pack(u_char *buf, int cc, struct msgh
for (i = ECHOLEN; i < cc && i < datalen;
++i, ++cp) {
if ((i % 32) == 8)
- (void)printf("\n\t");
- (void)printf("%x ", *cp);
+ printf("\n\t");
+ printf("%x ", *cp);
}
break;
}
@@ -1289,7 +1289,7 @@
Re: install.sub: ieee80211_{scan,config}: Allow quoted SSIDs
Thanks for looking into it.
On Sun, Jul 02, 2017 at 04:32:43PM +, Robert Peichaer wrote:
ieee80211_scan()
- Extract the needed information (nwid, bssid) using a very specific
sed expression. Any line, not matching this expr is ignored.
- Remove leading and trailing double-quotes in case of nwids with
spaces.
I had the ugly case of an empty SSID in reach while testing this so I
intentionally left double quotes in place within WLANLIST so that
the list presented to the user wouldn't look broken, e.g.
"my wifi" chan 1 bssid ...
"" chan 2 bssid ...
as opposed to
my wifi chan 1 bssid ...
chan 2 bssid ...
I'd also leave unqouting to the routine that actually requires it
instead of the function that just provides the list.
- Write nwid and bssid into WLANLIST as '()'.
Writing the simple format directly to cache seems like a good idea
instead of just cutting ^nwid first here and .*$ somewhere else.
ieee80211_config()
- just print WLANLIST using ieee80211_scan() if the user chooses
'?' which has the right format already
- in case the user selects an entry from WLANLIST using a number,
remove the '()' part from the line, resulting in
the nwid (without double-quotes)
- using the quote() function with the ifconfig command ensures,
that the nwid is quoted properly with single-quotes in case it
contains spaces
This is not needed as "$_nwid" will even work if _nwid='my "wifi'.
- using the quote() function when writing the nwid to the hostname.if
files ensures that the nwid is quoted properly with single-quotes
in case it contains spaces
The parse_hn_line() function in netstart does handle quoted nwids
properly when processing the hostname.if config lines as far as I
can see.
Yes, it does. But it chokes on SSIDs containing a literal " for example.
Here is an updated diff taking above considerations into account.
Note how ([[:xdigit:]:]*)$ when picking the answer must not be
simplified to (.*)$ as this would fail on SSIDs like "my (hidden) wifi".
Feedback/OK?
I'd say taking nasty characters in $_nwid further into account can be
put into a seperate diff.
Index: install.sub
===
RCS file: /cvs/src/distrib/miniroot/install.sub,v
retrieving revision 1.1019
diff -u -p -r1.1019 install.sub
--- install.sub 2 Jul 2017 12:45:43 - 1.1019
+++ install.sub 2 Jul 2017 22:42:38 -
@@ -896,13 +896,14 @@ __EOT
}
@ -1060,10 +1061,9 @@ v6_config() {
# Perform an 802.11 network scan on interface $1.
# The result is cached in $WLANLIST.
ieee80211_scan() {
- # N.B. Skipping quoted nwid's for now.
[[ -f $WLANLIST ]] ||
ifconfig $1 scan |
- sed -n 's/^ nwid \([^"]\)/\1/p' >$WLANLIST
+ sed -n 's/^[[:space:]]*nwid \(.*\) chan [0-9]* bssid
\([[:xdigit:]:]*\).*/\1 (\2)/p' >$WLANLIST
cat $WLANLIST
}
@@ -1082,12 +1082,12 @@ ieee80211_config() {
ask_until "Access point? (ESSID, 'any', list# or '?')" "any"
case "$resp" in
+([0-9]))
- _nwid=$(ieee80211_scan $_if | sed -n "${resp}s/ .*//p")
+ _nwid=$(ieee80211_scan $_if |
+ sed -n ${resp}'{s/ ([[:xdigit:]:]*)$//p;q;}')
[[ -z $_nwid ]] && echo "There is no line $resp."
+ [[ $_nwid = \"*\" ]] && _nwid=${_nwid#\"}
_nwid=${_nwid%\"}
;;
- \?) ieee80211_scan $_if |
- sed -n 's/^\([^ ]*\) chan .* bssid \([^ ]*\)
.*$/ \1 (\2)/p' |
- cat -n | more -c
+ \?) ieee80211_scan $_if | cat -n | more -c
;;
*) _nwid=$resp
;;
install.sub: No need for subshell in feed_random()
A simple command list suffices as this is just about redirecting all
output at once.
Feedback/OK?
Index: install.sub
===
RCS file: /cvs/src/distrib/miniroot/install.sub,v
retrieving revision 1.1016
diff -u -p -r1.1016 install.sub
--- install.sub 30 Jun 2017 16:46:02 - 1.1016
+++ install.sub 2 Jul 2017 04:02:08 -
@@ -2432,8 +2433,8 @@ mount_fs() {
# Feed the random pool some entropy before we read from it.
feed_random() {
- (dmesg; cat $CGI_INFO /*.conf; sysctl; route -n show; df;
- ifconfig -A; hostname) >/dev/random 2>&1
+ {dmesg; cat $CGI_INFO /*.conf; sysctl; route -n show; df;
+ ifconfig -A; hostname;} >/dev/random 2>&1
if [[ -e /mnt/var/db/host.random ]]; then
dd if=/mnt/var/db/host.random of=/dev/random bs=65536 count=1 \
status=none
Re: install.sub: No need for subshell in feed_random()
On Sat, Jul 01, 2017 at 09:35:09PM -0700, Philip Guenther wrote:
On Sat, Jul 1, 2017 at 9:27 PM, Klemens Nanni <k...@posteo.org> wrote:
A simple command list suffices as this is just about redirecting all
output at once.
Feedback/OK?
...
+ {dmesg; cat $CGI_INFO /*.conf; sysctl; route -n show; df;
This code has not been tested (and does not work).
The code has of course been tested but that diff indeed lacks a
whitespace. My bad, sorry.
The trailing semicolon after df may be omitted.
Index: install.sub
===
RCS file: /cvs/src/distrib/miniroot/install.sub,v
retrieving revision 1.1016
diff -u -p -r1.1016 install.sub
--- install.sub 30 Jun 2017 16:46:02 - 1.1016
+++ install.sub 2 Jul 2017 04:02:08 -
@@ -2432,8 +2433,8 @@ mount_fs() {
# Feed the random pool some entropy before we read from it.
feed_random() {
- (dmesg; cat $CGI_INFO /*.conf; sysctl; route -n show; df;
- ifconfig -A; hostname) >/dev/random 2>&1
+ { dmesg; cat $CGI_INFO /*.conf; sysctl; route -n show; df;
+ ifconfig -A; hostname;} >/dev/random 2>&1
if [[ -e /mnt/var/db/host.random ]]; then
dd if=/mnt/var/db/host.random of=/dev/random bs=65536 count=1 \
status=none
Re: install.sub: No need for subshell in feed_random()
On Sat, Jul 01, 2017 at 11:01:00PM -0600, Theo de Raadt wrote: Don't understand your point. What is this fixing? Nothing gets "fixed", it just avoids unnecessary forking and makes clear that the inner commands do not require a subshell - there are no side effects on the environment so a command list will do just fine for the sake of bundling output.
Re: install.sub: No need for subshell in feed_random()
On Sat, Jul 01, 2017 at 10:13:45PM -0700, Philip Guenther wrote: I've lost track of how many times at work and in OpenBSD I've tested a diff, made a final modification and then committed...a change broken by that final modification. Once you've tested a diff, either don't make a "this is easy!" change or retest with the change... Point taken.
Re: install.sub: No need for subshell in feed_random()
On Sat, Jul 01, 2017 at 11:17:55PM -0600, Theo de Raadt wrote:
On Sat, Jul 01, 2017 at 11:01:00PM -0600, Theo de Raadt wrote:
>Don't understand your point. What is this fixing?
Nothing gets "fixed", it just avoids unnecessary forking and makes clear
that the inner commands do not require a subshell - there are no side
effects on the environment so a command list will do just fine for the
sake of bundling output.
So you fixed nothing, and you don't have an explanation.
The explanation was meant as "it's simpler code".
That's not good enough.
Let me explain the reality. These scripts are written in idiomatic
shell. So that many people can understand them. I barely understand
the distinction you are making, and must presume many others in the
future won't understand it either.
From sh(1):
Command lists, as described above, can be enclosed within ‘()’
to have them executed in a subshell, or within ‘{}’ to have them
executed in the current environment:
(command ...)
{ command ...;}
To be fair both are used frequently here so I assume people do know the
difference.
The change doesn't make sense.
It agree that it my not seem reasonable enough (to you), but there is a
technical point to it.
strmode.3: Remove return values section
strmode(3) is void and thus never returns anything. Feedback/OK? Index: strmode.3 === RCS file: /cvs/src/lib/libc/string/strmode.3,v retrieving revision 1.16 diff -u -p -r1.16 strmode.3 --- strmode.3 5 Jun 2013 03:39:23 - 1.16 +++ strmode.3 5 Jul 2017 11:28:41 - @@ -140,10 +140,6 @@ The last character is a plus sign if there are any alternate or additional access control methods associated with the inode, otherwise it will be a space. -.Sh RETURN VALUES -The -.Fn strmode -function always returns 0. .Sh SEE ALSO .Xr chmod 1 , .Xr find 1 ,
swab: Swap bytes directly, simplify
No need for buffers t0, t1 here. This way we only have to step/move the
current position instead of the total bytes left as well as both source
and destination position.
Always swap an even number of bytes by clearing the length's last bit
and never write past it.
The , operator can be used safely here as the order of execution is
irrelevant (in case it's not guaranteed).
POSIX leaves treatment of the last odd byte unspecified but we don't
touch it at all so why not documenting this behaviour?
Feedback? Comments?
Index: swab.c
===
RCS file: /cvs/src/lib/libc/string/swab.c,v
retrieving revision 1.9
diff -u -p -r1.9 swab.c
--- swab.c 11 Dec 2014 23:05:38 - 1.9
+++ swab.c 5 Jul 2017 14:12:34 -
@@ -21,15 +21,8 @@ swab(const void *__restrict from, void *
{
const unsigned char *src = from;
unsigned char *dst = to;
- unsigned char t0, t1;
+ ssize_t i;
- while (len > 1) {
- t0 = src[0];
- t1 = src[1];
- dst[0] = t1;
- dst[1] = t0;
- src += 2;
- dst += 2;
- len -= 2;
- }
+ for (i = 0; i < (len & ~1) - 1; i += 2)
+ dst[i] = src[i + 1], dst[i + 1] = src[i];
}
Index: swab.3
===
RCS file: /cvs/src/lib/libc/string/swab.3,v
retrieving revision 1.9
diff -u -p -r1.9 swab.3
--- swab.3 12 Dec 2014 20:06:13 - 1.9
+++ swab.3 5 Jul 2017 14:12:34 -
@@ -57,7 +57,9 @@ If
is zero or less,
.Nm
does nothing.
-If it is odd, what happens to the last byte is unspecified.
+If it is odd,
+.Fa len Ns \-1
+bytes are swapped and the last byte is ignored.
If
.Fa src
and
Re: swab: Swap bytes directly, simplify
On Wed, Jul 05, 2017 at 05:27:18PM +0200, Ingo Schwarze wrote: Hi Klemens, Klemens Nanni wrote on Wed, Jul 05, 2017 at 05:02:05PM +0200: No need for buffers t0, t1 here. Your patch changes behaviour in some cases where the buffers do overlap. For example, if src == dst, right now, the code swaps bytes. With your patch, i'm not sure it is even deterministic any longer, and whatever it does exactly, it definitely destroys information. But the restrict keyword tells the compiler to expect distinct addresses and POSIX explicitly says that overlapping regions cause undefined behaviour. So I'd say for cases like src == dst we don't have to guarantee that bytes are swapped. Even if behaviour is explicitly unspecified, changing it still requires a rationale and a careful assessment of potential damage to existing software, even if software may only be affected when carelessly written. Agreed, I haven't checked for bad/dangerous usage in existing code for reasons explained above. POSIX leaves treatment of the last odd byte unspecified but we don't touch it at all so why not documenting this behaviour? I strongly oppose your patch to the manual page. Documenting implementation details is not among the goals of manual pages, in particular when they are not portable and should not be relied upon. Documenting what is specified, and what is not, is among the goals of manual pages. In that sense, your patch to the manual page introduces a bug and turns the STANDARDS section into a lie. Point taken, as I already mentioned I wasn't sure about the manual diff. No need to fix it because the patch is not likely to go anywhere, but once again you mangled the patch such that it won't even apply. Hm, the diff taken from my mail as is applies cleanly here.
Re: swab: Swap bytes directly, simplify
On Wed, Jul 05, 2017 at 09:47:11AM -0600, Theo de Raadt wrote: > > So I'd say for cases like src == dst we don't have to guarantee that > > bytes are swapped. > > and you've audited all the callers to this function? > > > Agreed, I haven't checked for bad/dangerous usage in existing code for > > reasons explained above. > > No you haven't. To be fair I glanced through sys and base for callers to swab() before patching but didn't find any: $ grep -slRF swab\( /usr/src /usr/src/gnu/gcc/gcc/sys-protos.h /usr/src/gnu/usr.bin/gcc/gcc/sys-protos.h /usr/src/include/unistd.h /usr/src/lib/libc/string/Makefile.inc /usr/src/lib/libc/string/swab.c > Completely irresponsible. > > What a waste of time. But you're right about irresponsibly not thinking this through. Definitely noted, thanks.
Re: dhcp-options(5) conflict with dhcpd.conf(5)
On Mon, Jun 26, 2017 at 08:26:03PM -0500, Edgar Pettijohn wrote: I found the following conflict between dhcp-options(5) and dhcpd.conf(5). From dhcpd.conf: As you can see in Example 2, it's legal to specify host addresses in parameters as hostnames rather than as numeric IP addresses. If a given hostname resolves to more than one IP address (for example, if that host has two Ethernet interfaces), both addresses are supplied to the client. From dhcp-options: The ip-address data type can be entered either as an explicit IP address (e.g., 239.254.197.10) or as a domain name (e.g., haagen.isc.org). A domain name must resolve to a single IP address. Does anyone know which is correct? Must it resolve to a single IP address or not? I'm not an expert but from what I know: Almost all parameters taking an ip-address argument allow multiple arguments, thus making multiple IP addresses per hostname usable. For those that except a single or strictly pairwise arguments such as dhcp-requested-address ip-address and static-routes ip-address ip-address [, ip-address ip-address ...] one may always use the first IP address a given hostname resolves to. Generally speaking there's nothing wrong with hosntames resolving to multiple IP addresses. RFC 2131, Section 3.6 (Use of DHCP in clients with multiple interfaces) for example states that A client with multiple network interfaces must use DHCP through each interface independently to obtain configuration information parameters for those separate interfaces. Since clients will only accept a single DHCP offer which in turn is always bound to the client's MAC address, DHCP can work just fine with multiple interfaces/leases per host. Thinking of techniques like round-robin DNS, expecting hostnames for the ip-address type to resolve to a single IP address only could actually cause problems.
Re: ktrace: Following symlinks
On Thu, Jun 29, 2017 at 11:33:36PM -0700, Philip Guenther wrote: On Thu, Jun 29, 2017 at 10:42 PM, Klemens Nanni <k...@posteo.org> wrote: On Thu, Jun 29, 2017 at 09:50:25PM -0700, Philip Guenther wrote: On Thu, Jun 22, 2017 at 7:17 PM, Klemens Nanni <k...@posteo.org> wrote: So I just wrapped my head around vfs(9) with regard to making ktrace following symlinks again, however I don't quite understand what problems may occur when doing so. May anyone enlighten me on this? IMHO, it makes more sense to add fktrace(2) aka NetBSD where an open fd is passed in. To have a more generic interface? Yes. ktrace -f - some_command | kdump kdump expects ktrace.out here and complains. Hmm, I wonder what happens if the fd involved is a pipe to the process being traced and if that will deadlock the kernel. Uh, hmm, could that happen as well with your suggestion to support FIFO if the traced process is the only reader, ala: mkfifio kt ktrace -f kt kdump -f /dev/stdin < kt That one's interesting: kdump won't see data from kt on stdin since you're redirecting into ktrace. Here's an equivalent inocation that's more obvious: $ < kt ktrace -f kt -- kdump -f /dev/stdin ^Cksh: cannot open kt: Interrupted system call Neither ktrace nor kdump will actually be executed since the shell already chokes on it. You can check for that while it's still running (doing nothing): $ pgrep -fl k[td] $ Here's some more fun, trace yourself endlessly while eating CPU: $ ktrace -f - kdump -f - 58275 kle 54 0 648K 684K onproc/3 - 0:20 56.54% kdump -f - Or let ktrace wait forever (without -a the FIFO gets replaced and you'll see dumps just like above): $ mkfifo kt $ ktrace -af kt -- kdump -f kt 95073 root 2 0 128K 160K idle fifow 0:00 0.00% ktrace -af kt -- kdump -f kt ? It's okay if that just blocks, but it's not okay if it blocks processes that aren't being traced or if it eats the CPU. Playing around with it hasn't blocked other programs or crashed the kernel so far but I'll further look into this. (VREG vnodes are exactly what the kernel can write to without having to worry about looping internally or userspace blocking it for arbitrary lengths of time. Well, at least if we ignore FUSE, which is basically ignored for this sort of discussion anyway, being a security nightmare. Anyone tried to ktrace a fuse-serving process, directing the ktrace to the fuse'd filesystem? Same question applies to acct() to a fuse'd filesystem, but at least that's root-only.) Why not letting ktrace(2) handle this just like it already does for regular files? *If* if it's safe (see above for an *example* consideration), then fd / struct file base access is much more general than filename / vnode based access. ktrace(1) *always* open()s the target filename, so would arguably remove a TOCTOU.
faq/ports/guide: Remove stray comment
Index: guide.html === RCS file: /cvs/www/faq/ports/guide.html,v retrieving revision 1.73 diff -u -p -r1.73 guide.html --- guide.html 8 Aug 2017 15:48:56 - 1.73 +++ guide.html 9 Aug 2017 00:13:22 - @@ -808,7 +808,7 @@ to fiddle with MULTI_PACKAGES a Once you've separated the files properly, you will need to check dependencies: LIB_DEPENDS, WANTLIB, and RUN_DEPENDS will be split for each subpackage. -It is usually time to check that your multi-packaging "works," and that +It is usually time to check that your multi-packaging "works" and that those nasty dependencies you don't want to force on the user are indeed relegated to a specific subpackage.
faq/ports/guide: Missing space
Index: faq/ports/guide.html === RCS file: /cvs/www/faq/ports/guide.html,v retrieving revision 1.71 diff -u -p -r1.71 guide.html --- faq/ports/guide.html26 Jun 2017 17:18:58 - 1.71 +++ faq/ports/guide.html7 Aug 2017 22:17:22 - @@ -789,7 +789,7 @@ In this case, try setting the MULTI_ -sub packages, add corresponding COMMENTS, and look at your packaging. Basically, MULTI_PACKAGES only affects the packaging: if you have MULTI_PACKAGES=-s1 -s2 all stuff relevant to the package will exist -intwo variants: +in two variants: COMMENT-s1 for the first package, COMMENT-s2 for the second package,
faq/ports/guide: Improve wording
This way it's feels much more natural to say. Feedback? Index: guide.html === RCS file: /cvs/www/faq/ports/guide.html,v retrieving revision 1.72 diff -u -p -r1.72 guide.html --- guide.html 7 Aug 2017 22:27:51 - 1.72 +++ guide.html 8 Aug 2017 15:29:54 - @@ -795,8 +795,8 @@ in two variants: COMMENT-s2 for the second package, PLIST-s1, PLIST-s2, DESCR-s1, DESCR-s2. You need to write those COMMENT-s1 and COMMENT-s2 in the -Makefile, and to split your PLIST into two parts, and to -create DESCR-s1/DESCR-s2. +Makefile, split your PLIST into two parts, and create +DESCR-s1/DESCR-s2. You will also need to specify separate PKGNAMEs for all subpackages.
xenocara readme: xdm -> xenodm
Found while looking for the core dump bits. Feedback? Index: README === RCS file: /cvs/xenocara/README,v retrieving revision 1.39 diff -u -p -r1.39 README --- README 24 Feb 2017 03:07:03 - 1.39 +++ README 18 Aug 2017 23:10:26 - @@ -177,9 +177,9 @@ Several things are needed: anywhere in the configuration file. 3) start the X server as root, with the -keepPriv option. A regular - user is not allowed to use this option. If you use xdm, you can add - the option in /etc/X11/xdm/Xservers. If you want to use startx, you - need to run it as root, like this: + user is not allowed to use this option. If you use xenodm, you can + add the option in /etc/X11/xenodm/Xservers. If you want to use + startx, you need to run it as root, like this: startx -- /usr/X11R6/bin/X -keepPriv
Re: ksh(1): custom completion for command containing hyphens
On Fri, Jun 02, 2017 at 05:07:42PM +0200, Anton Lindqvist wrote: Custom completions in ksh is currently limited to commands that does not contain hyphens since such a character cannot be part of an identifier. We could cheat and replace hyphens with underscores upon performing completions. The motivation behind this is that I want to add completions for ssh-add(1). With the attached diff, I can achieve the following: $ set -A complete_ssh_add -- $(find ~/.ssh -name '*_rsa') Comments? OK? While this works and we (obviously) don't have any binaries in base that would run into this problem, keep in mind that this workaround munges the real names: complete_ssh_add can now stand for and would in fact complete for both ssh_add and ssh-add.
Re: ktrace: Allow appending to FIFOs
On Tue, Jun 20, 2017 at 09:10:17PM -0400, Ted Unangst wrote: Klemens Nanni wrote: I wanted to quickly debug some program without actually dumping to disk by using FIFOs, however ktrace(2) wouldn't accept anything but regular files. Are there any pitfalls or limitations I am currently not aware of that justify this strict behaviour? oh, neat, i've actually really wanted something like this for a while. i was trying to find a way to make it an arbitrary fd, but this seems like a much smaller diff. i don't think there should be problems. i haven't checked the code recently, but it should already handle disk full, etc. situations, so if the reader stops it just means events get thrown away. the traced process shouldn't block. but that's something to confirm and test. The motivation was not to prevent full disks but rather to have an more intuitive workflow; ideally something like 'ktrace | kdump' or even some unified way/tool. I'm just not that familiar with VFS yet, hence my question. If this is really it, I'd like update the diff to make symbolic links as well. Besides that, getting EACCES on "wrong" files was pretty irritating: I spent some minutes looking at the actual permissions before digging deeper into the code.
ktrace: Allow appending to FIFOs
I wanted to quickly debug some program without actually dumping to disk
by using FIFOs, however ktrace(2) wouldn't accept anything but regular
files.
Are there any pitfalls or limitations I am currently not aware of that
justify this strict behaviour?
$ ln -s some.file link
$ ktrace -a -f link echo foo
ktrace: link: Too many levels of symbolic links
$ mkfifo fifo && cat fifo >/dev/null &
$ ktrace -a -f fifo echo foo
ktrace: fifo: Permission denied
With this tiny patch ktrace(2) allows appending to FIFOs, which enables
me to do the following:
$ mkfifo ktrace.out && kdump -l &
$ ktrace -a echo foo
foo
1903 ktrace RET ktrace 0
1903 ktrace CALL
execve(0x7f7e6d50,0x7f7e72e8,0x7f7e7300)
[...]
$ ktrace -a echo bar
bar
94065 ktrace RET ktrace 0
94065 ktrace CALL
execve(0x7f7e2280,0x7f7e2818,0x7f7e2830)
[...]
Feedback? OK?
Index: kern_ktrace.c
===
RCS file: /cvs/src/sys/kern/kern_ktrace.c,v
retrieving revision 1.91
diff -u -p -r1.91 kern_ktrace.c
--- kern_ktrace.c 14 Feb 2017 10:31:15 - 1.91
+++ kern_ktrace.c 20 Jun 2017 22:01:56 -
@@ -428,7 +428,7 @@ sys_ktrace(struct proc *p, void *v, regi
vp = nd.ni_vp;
VOP_UNLOCK(vp, p);
- if (vp->v_type != VREG) {
+ if (vp->v_type != VREG && vp->v_type != VFIFO) {
error = EACCES;
goto done;
}
ktrace: Following symlinks
So I just wrapped my head around vfs(9) with regard to making ktrace following symlinks again, however I don't quite understand what problems may occur when doing so. May anyone enlighten me on this? This feature was explicitly disabled 17 years ago[1] appearently because FreeBSD did so[2] but I couldn't find any further explanation. 1: http://cvsweb.openbsd.org/cgi-bin/cvsweb/src/sys/kern/kern_ktrace.c.diff?r1=1.19=1.20 2: https://svnweb.freebsd.org/base?diff_format=u=revision=50668
install.sub: Remove redundant check for NIFS
NIFS is checked inside start_cgiinfo() already, this nicely aligns with
the rest of the list in do_install().
Index: install.sub
===
RCS file: /cvs/src/distrib/miniroot/install.sub,v
retrieving revision 1.1015
diff -u -p -r1.1015 install.sub
--- install.sub 21 Jun 2017 23:54:19 - 1.1015
+++ install.sub 24 Jun 2017 01:09:43 -
@@ -2704,9 +2704,8 @@ do_install() {
# Configure the network.
donetconfig
- # If there's network connectivity, fetch list of mirror servers and
- # installer choices from previous runs.
- ((NIFS)) && start_cgiinfo
+ # Fetch list of mirror servers and installer choices from previous runs.
+ start_cgiinfo
echo
Re: Finish the link-kit job
===
RCS file: /cvs/src/distrib/miniroot/install.sub,v
retrieving revision 1.1014
diff -u -p -u -r1.1014 install.sub
--- distrib/miniroot/install.sub3 Jun 2017 22:27:41 - 1.1014
+++ distrib/miniroot/install.sub21 Jun 2017 12:36:06 -
@@ -2633,6 +2633,14 @@ finish_up() {
mv /mnt/bsd.mp /mnt/bsd
fi
+ # Create/update kernel.SHA256 matching the just installed kernel.
+ # Fix path in kernel.SHA256 to ensure it references the kernel as /bsd.
+ (
+ umask 077
+ sha256 -h /mnt/var/db/kernel.SHA256 /mnt/bsd
+ sed -i 's,(/mnt,(,' /mnt/var/db/kernel.SHA256
+ )
Minor nit but still: How about
(
umask 077
sha256 /mnt/bsd | sed s,/mnt,, > /mnt/var/db/kernel.SHA256
)
or even this (no need for subshell)
sha256 /mnt/bsd | sed s,/mnt,, > /mnt/var/db/kernel.SHA256
chmod 600 /mnt/var/db/kernel.SHA256
Re: [PATCH] ffs: always assign random inode generation numbers
On Sun, Jun 25, 2017 at 10:47:08PM -0600, Theo de Raadt wrote: :-) Speaking of signed integers, does it really need to be signed? Perhaps not. Anyone know for sure? Of course this number should probably exclude 0 in it's range. Probably [0, 3] completely as those are reserved for unallocated entries, bad blocks, / and lost+found respectively.
Re: [PATCH] ffs: always assign random inode generation numbers
On Sun, Jun 25, 2017 at 11:21:50PM -0600, Theo de Raadt wrote: On Sun, Jun 25, 2017 at 10:47:08PM -0600, Theo de Raadt wrote: >> :-) Speaking of signed integers, does it really need to be signed? > >Perhaps not. Anyone know for sure? > >Of course this number should probably exclude 0 in it's range. Probably [0, 3] completely as those are reserved for unallocated entries, bad blocks, / and lost+found respectively. it is the generation #... not the inode.. Right... *still trying to get the hang of this code/subsystem*
brconfig: Unify/fix strtoul(3) handling
Besides fixing a tautological 'v < 0' and using more descriptive/less errorprone sizeof(target) this patch unifies strtoul(3) handling both logically and cosmetically. Index: brconfig.c === RCS file: /cvs/src/sbin/ifconfig/brconfig.c,v retrieving revision 1.14 diff -u -p -r1.14 brconfig.c --- brconfig.c 28 Nov 2016 10:12:50 - 1.14 +++ brconfig.c 2 Jun 2017 15:55:29 - @@ -220,7 +220,6 @@ bridge_ifsetflag(const char *ifsname, u_ err(1, "%s: ioctl SIOCBRDGGIFFLGS %s", name, ifsname); req.ifbr_ifsflags |= flag & ~IFBIF_RO_MASK; - if (ioctl(s, SIOCBRDGSIFFLGS, (caddr_t)) < 0) err(1, "%s: ioctl SIOCBRDGSIFFLGS %s", name, ifsname); } @@ -232,12 +231,10 @@ bridge_ifclrflag(const char *ifsname, u_ strlcpy(req.ifbr_name, name, sizeof(req.ifbr_name)); strlcpy(req.ifbr_ifsname, ifsname, sizeof(req.ifbr_ifsname)); - if (ioctl(s, SIOCBRDGGIFFLGS, (caddr_t)) < 0) err(1, "%s: ioctl SIOCBRDGGIFFLGS %s", name, ifsname); req.ifbr_ifsflags &= ~(flag | IFBIF_RO_MASK); - if (ioctl(s, SIOCBRDGSIFFLGS, (caddr_t)) < 0) err(1, "%s: ioctl SIOCBRDGSIFFLGS %s", name, ifsname); } @@ -425,7 +422,8 @@ bridge_maxage(const char *arg, int d) errno = 0; v = strtoul(arg, , 0); - if (arg[0] == '\0' || endptr[0] != '\0' || v > 0xffUL || + if (arg[0] == '\0' || endptr[0] != '\0' || + v == 0 || v > sizeof(bp.ifbrp_maxage) || (errno == ERANGE && v == ULONG_MAX)) errx(1, "invalid arg for maxage: %s", arg); @@ -444,7 +442,8 @@ bridge_priority(const char *arg, int d) errno = 0; v = strtoul(arg, , 0); - if (arg[0] == '\0' || endptr[0] != '\0' || v > 0xUL || + if (arg[0] == '\0' || endptr[0] != '\0' || + v == 0 || v > sizeof(bp.ifbrp_prio) || (errno == ERANGE && v == ULONG_MAX)) errx(1, "invalid arg for spanpriority: %s", arg); @@ -483,7 +482,8 @@ bridge_fwddelay(const char *arg, int d) errno = 0; v = strtoul(arg, , 0); - if (arg[0] == '\0' || endptr[0] != '\0' || v > 0xffUL || + if (arg[0] == '\0' || endptr[0] != '\0' || + v == 0 || v > sizeof(bp.ifbrp_fwddelay) || (errno == ERANGE && v == ULONG_MAX)) errx(1, "invalid arg for fwddelay: %s", arg); @@ -502,7 +502,8 @@ bridge_hellotime(const char *arg, int d) errno = 0; v = strtoul(arg, , 0); - if (arg[0] == '\0' || endptr[0] != '\0' || v > 0xffUL || + if (arg[0] == '\0' || endptr[0] != '\0' || + v == 0 || v > sizeof(bp.ifbrp_hellotime) || (errno == ERANGE && v == ULONG_MAX)) errx(1, "invalid arg for hellotime: %s", arg); @@ -521,7 +522,8 @@ bridge_maxaddr(const char *arg, int d) errno = 0; newsize = strtoul(arg, , 0); - if (arg[0] == '\0' || endptr[0] != '\0' || newsize > 0xUL || + if (arg[0] == '\0' || endptr[0] != '\0' || + newsize == 0 || newsize > sizeof(bp.ifbrp_csize) || (errno == ERANGE && newsize == ULONG_MAX)) errx(1, "invalid arg for maxaddr: %s", arg); @@ -537,13 +539,12 @@ bridge_deladdr(const char *addr, int d) struct ifbareq ifba; struct ether_addr *ea; - strlcpy(ifba.ifba_name, name, sizeof(ifba.ifba_name)); ea = ether_aton(addr); if (ea == NULL) err(1, "Invalid address: %s", addr); + strlcpy(ifba.ifba_name, name, sizeof(ifba.ifba_name)); bcopy(ea, _dst, sizeof(struct ether_addr)); - if (ioctl(s, SIOCBRDGDADDR, ) < 0) err(1, "%s: %s", name, addr); } @@ -555,16 +556,16 @@ bridge_ifprio(const char *ifname, const unsigned long v; char *endptr; - strlcpy(breq.ifbr_name, name, sizeof(breq.ifbr_name)); - strlcpy(breq.ifbr_ifsname, ifname, sizeof(breq.ifbr_ifsname)); - errno = 0; v = strtoul(val, , 0); - if (val[0] == '\0' || endptr[0] != '\0' || v > 0xffUL || + if (val[0] == '\0' || endptr[0] != '\0' || + v == 0 || v > sizeof(breq.ifbr_priority) || (errno == ERANGE && v == ULONG_MAX)) - err(1, "invalid arg for ifpriority: %s", val); - breq.ifbr_priority = v; + errx(1, "invalid arg for ifpriority: %s", val); + strlcpy(breq.ifbr_name, name, sizeof(breq.ifbr_name)); + strlcpy(breq.ifbr_ifsname, ifname, sizeof(breq.ifbr_ifsname)); + breq.ifbr_priority = v; if (ioctl(s, SIOCBRDGSIFPRIO, (caddr_t)) < 0) err(1, "%s: %s", name, val); } @@ -576,18 +577,16 @@ bridge_ifcost(const char *ifname, const unsigned long v; char *endptr; - strlcpy(breq.ifbr_name, name, sizeof(breq.ifbr_name)); - strlcpy(breq.ifbr_ifsname, ifname, sizeof(breq.ifbr_ifsname)); - errno = 0; v = strtoul(val,
Makefile: Use -exec not xargs(1) with find(1)
-exec has been around since 2012 is cleaner and shorter, I don't see why
xargs(1) should be used for such simple cases. Besides that don't walk
the tree twice.
Index: include/Makefile
===
RCS file: /cvs/src/include/Makefile,v
retrieving revision 1.219
diff -u -p -r1.219 Makefile
--- include/Makefile17 Apr 2017 15:53:21 - 1.219
+++ include/Makefile3 Jun 2017 12:48:19 -
@@ -100,10 +100,9 @@ includes:
cd ${.CURDIR}/$$i && ${RUN_MAKE}; \
done
chown -RP ${BINOWN}:${BINGRP} ${DESTDIR}/usr/include
- find ${DESTDIR}/usr/include -type f -print0 | \
- xargs -0r chmod a=r
- find ${DESTDIR}/usr/include \( -type d -o -type l \) -print0 | \
- xargs -0r chmod -h u=rwx,go=rx
+ find ${DESTDIR}/usr/include \
+ \( -type f -exec chmod a=r {} + \) -o \
+ \( \( -type d -o -type l \) -exec chmod -h u=rwx,go=rx {} + \)
copies:
@echo copies: ${LDIRS}
Index: share/zoneinfo/Makefile
===
RCS file: /cvs/src/share/zoneinfo/Makefile,v
retrieving revision 1.11
diff -u -p -r1.11 Makefile
--- share/zoneinfo/Makefile 24 Nov 2016 14:38:30 - 1.11
+++ share/zoneinfo/Makefile 3 Jun 2017 12:48:19 -
@@ -81,8 +81,9 @@ realinstall: ${DATA} ${REDO} ${YEARISTYP
(cd ${.CURDIR}/datfiles; \
${ZIC} -y ${YEARISTYPECOPY} -d ${TZDIR} -p ${POSIXRULES})
chown -R ${BINOWN}:${BINGRP} ${TZDIR}
- find ${TZDIR} -type f -print0 | xargs -0r chmod a=r
- find ${TZDIR} -type d -print0 | xargs -0r chmod a=rx
+ find ${TZDIR} \
+ \( -type f -exec chmod a=r {} + \) -o \
+ \( -type d -exec chmod a=rx {} + \)
${INSTALL} -c -o root -g bin -m 644 ${.CURDIR}/datfiles/iso3166.tab \
${DESTDIR}/usr/share/misc
${INSTALL} -c -o root -g bin -m 644 ${.CURDIR}/datfiles/zone.tab \
chmod.1: Correct =X behaviour
=X actually works quite fine despite the mentioned condition: $ touch f; mkdir d $ chmod =X f d $ ls -ld f d d--x--x--x 2 kle wheel 512 Jun 3 15:11 d/ -- 1 kle wheel0 Jun 3 15:11 f One of the given examples has been updated to reflect this. Whether to implement/fix the desired(?) limitation is another question. Index: chmod.1 === RCS file: /cvs/src/bin/chmod/chmod.1,v retrieving revision 1.41 diff -u -p -r1.41 chmod.1 --- chmod.1 31 Dec 2015 23:38:16 - 1.41 +++ chmod.1 3 Jun 2017 13:10:02 - @@ -287,15 +287,6 @@ Execute/search bits. .It X The execute/search bits if the file is a directory or any of the execute/search bits are set in the original (unmodified) mode. -Operations with the -.Ar perm -symbol -.Sq X -are only meaningful in conjunction with the -.Ar op -symbol -.Sq + , -and are ignored in all other cases. .It u User permission bits in the mode of the original file. .It g @@ -333,7 +324,7 @@ Deny write permission to group and other Set the read and write permissions to the usual defaults, but retain any execute permissions that are currently set: .Pp -.Dl $ chmod =rw,+X file +.Dl $ chmod =rwX file .Pp Make a directory or file searchable/executable by everyone if it is already searchable/executable by anyone:
Re: sshd: Remove authorized_keys2 file
Bump: Feeback? OK?
On Mon, Apr 17, 2017 at 09:28:29PM +0200, Klemens Nanni wrote:
Now that protocol version 1 was finally dropped in sshd(8), get rid of
this file completely. Our default sshd_config(5) overwrites
AuthorizedKeysFile to ignore it anyway and sshd(8)'s FILES section
doesn't mention it either.
Index: etc/changelist
===
RCS file: /cvs/src/etc/changelist,v
retrieving revision 1.116
diff -u -p -r1.116 changelist
--- etc/changelist 27 Feb 2017 21:53:11 - 1.116
+++ etc/changelist 17 Apr 2017 19:26:47 -
@@ -147,7 +147,6 @@
/root/.rhosts
/root/.shosts
/root/.ssh/authorized_keys
-/root/.ssh/authorized_keys2
/var/cron/at.allow
/var/cron/at.deny
/var/cron/cron.allow
Index: usr.bin/ssh/pathnames.h
===
RCS file: /cvs/src/usr.bin/ssh/pathnames.h,v
retrieving revision 1.25
diff -u -p -r1.25 pathnames.h
--- usr.bin/ssh/pathnames.h 31 Mar 2016 05:24:06 - 1.25
+++ usr.bin/ssh/pathnames.h 17 Apr 2017 19:26:47 -
@@ -79,7 +79,7 @@
#define _PATH_SSH_USER_CONFFILE _PATH_SSH_USER_DIR "/config"
/*
- * File containing a list of those rsa keys that permit logging in as this
+ * File containing a list of those keys that permit logging in as this
* user. This file need not be readable by anyone but the user him/herself,
* but does not contain anything particularly secret. If the user's home
* directory resides on an NFS volume where root is mapped to nobody, this
@@ -87,9 +87,6 @@
* running as root.)
*/
#define _PATH_SSH_USER_PERMITTED_KEYS _PATH_SSH_USER_DIR "/authorized_keys"
-
-/* backward compat for protocol v2 */
-#define _PATH_SSH_USER_PERMITTED_KEYS2 _PATH_SSH_USER_DIR "/authorized_keys2"
/*
* Per-user and system-wide ssh "rc" files. These files are executed with
Index: usr.bin/ssh/servconf.c
===
RCS file: /cvs/src/usr.bin/ssh/servconf.c,v
retrieving revision 1.306
diff -u -p -r1.306 servconf.c
--- usr.bin/ssh/servconf.c 14 Mar 2017 07:19:07 - 1.306
+++ usr.bin/ssh/servconf.c 17 Apr 2017 19:26:47 -
@@ -294,12 +294,9 @@ fill_default_server_options(ServerOption
options->client_alive_interval = 0;
if (options->client_alive_count_max == -1)
options->client_alive_count_max = 3;
- if (options->num_authkeys_files == 0) {
+ if (options->num_authkeys_files == 0)
options->authorized_keys_files[options->num_authkeys_files++] =
xstrdup(_PATH_SSH_USER_PERMITTED_KEYS);
- options->authorized_keys_files[options->num_authkeys_files++] =
- xstrdup(_PATH_SSH_USER_PERMITTED_KEYS2);
- }
if (options->permit_tun == -1)
options->permit_tun = SSH_TUNMODE_NO;
if (options->ip_qos_interactive == -1)
Index: usr.bin/ssh/sshd.8
===
RCS file: /cvs/src/usr.bin/ssh/sshd.8,v
retrieving revision 1.288
diff -u -p -r1.288 sshd.8
--- usr.bin/ssh/sshd.8 30 Jan 2017 23:27:39 - 1.288
+++ usr.bin/ssh/sshd.8 17 Apr 2017 19:26:47 -
@@ -390,9 +390,7 @@ does not exist either, xauth is used to
specifies the files containing public keys for
public key authentication;
if this option is not specified, the default is
-.Pa ~/.ssh/authorized_keys
-and
-.Pa ~/.ssh/authorized_keys2 .
+.Pa ~/.ssh/authorized_keys .
Each line of the file contains one
key (empty lines and lines starting with a
.Ql #
Index: usr.bin/ssh/sshd_config
===
RCS file: /cvs/src/usr.bin/ssh/sshd_config,v
retrieving revision 1.101
diff -u -p -r1.101 sshd_config
--- usr.bin/ssh/sshd_config 14 Mar 2017 07:19:07 - 1.101
+++ usr.bin/ssh/sshd_config 17 Apr 2017 19:26:47 -
@@ -35,9 +35,7 @@
#PubkeyAuthentication yes
-# The default is to check both .ssh/authorized_keys and .ssh/authorized_keys2
-# but this is overridden so installations will only check .ssh/authorized_keys
-AuthorizedKeysFile .ssh/authorized_keys
+#AuthorizedKeysFile.ssh/authorized_keys
#AuthorizedPrincipalsFile none
Index: usr.bin/ssh/sshd_config.5
===
RCS file: /cvs/src/usr.bin/ssh/sshd_config.5,v
retrieving revision 1.243
diff -u -p -r1.243 sshd_config.5
--- usr.bin/ssh/sshd_config.5 14 Mar 2017 07:19:07 - 1.243
+++ usr.bin/ssh/sshd_config.5 17 Apr 2017 19:26:47 -
@@ -283,7 +283,7 @@ Alternately this option may be set to
.Cm none
to skip checking for user keys in files.
The default is
-.Qq .ssh/authorized_keys .ssh/authorized_keys2 .
+.Qq .ssh/authorized_keys .
.It Cm AuthorizedPrincipalsCommand
Specifies a program to be used to generate the list of allowed
certificate principals as per
Re: brconfig: Unify/fix strtoul(3) handling
On Wed, Jun 07, 2017 at 01:17:10PM -0400, Ted Unangst wrote:
Klemens Nanni wrote:
Besides fixing a tautological 'v < 0' and using more descriptive/less
errorprone sizeof(target) this patch unifies strtoul(3) handling both
logically and cosmetically.
A great deal of these look like exactly the situation strtonum was desinged to
cope with. this means dropping support for hex bases, but i'm happy telling
people they need to learn decimal. :)
Leave error checks to strtonum(3) and use data types properly reflecting
those of the target's struct member. This also fixes my previously
introduced systematic flaw:
sizeof(int) != INT_MAX
sizeof is clearly smaller; in fact reliably finding a variable's maximum
value at runtime is impossible in C.
We might also pass the limits to strtonum directly so ifconfig can tell
us what's wrong, e.g.
- v = strtonum(value, 0, UINT8_MAX, );
+ v = strtonum(value, 1, 10, );
# ifconfig bridge0 holdcnt 0
- ifconfig: too small arg for holdcnt: 0 # strtonum()
+ ifconfig: bridge0: Invalid argument # ioctl()
This is left unchanged for now since it would also mean having our
limits defined twice which isn't neccessarily a good idea with regard to
future changes.
Feedback/OK?
Index: brconfig.c
===
RCS file: /cvs/src/sbin/ifconfig/brconfig.c,v
retrieving revision 1.15
diff -u -p -r1.15 brconfig.c
--- brconfig.c 7 Jun 2017 16:47:29 - 1.15
+++ brconfig.c 7 Jun 2017 21:30:56 -
@@ -220,7 +220,6 @@ bridge_ifsetflag(const char *ifsname, u_
err(1, "%s: ioctl SIOCBRDGGIFFLGS %s", name, ifsname);
req.ifbr_ifsflags |= flag & ~IFBIF_RO_MASK;
-
if (ioctl(s, SIOCBRDGSIFFLGS, (caddr_t)) < 0)
err(1, "%s: ioctl SIOCBRDGSIFFLGS %s", name, ifsname);
}
@@ -232,12 +231,10 @@ bridge_ifclrflag(const char *ifsname, u_
strlcpy(req.ifbr_name, name, sizeof(req.ifbr_name));
strlcpy(req.ifbr_ifsname, ifsname, sizeof(req.ifbr_ifsname));
-
if (ioctl(s, SIOCBRDGGIFFLGS, (caddr_t)) < 0)
err(1, "%s: ioctl SIOCBRDGGIFFLGS %s", name, ifsname);
req.ifbr_ifsflags &= ~(flag | IFBIF_RO_MASK);
-
if (ioctl(s, SIOCBRDGSIFFLGS, (caddr_t)) < 0)
err(1, "%s: ioctl SIOCBRDGSIFFLGS %s", name, ifsname);
}
@@ -400,15 +397,12 @@ void
bridge_timeout(const char *arg, int d)
{
struct ifbrparam bp;
- long newtime;
- char *endptr;
+ int newtime;
+ const char *errstr;
- errno = 0;
- newtime = strtol(arg, , 0);
- if (arg[0] == '\0' || endptr[0] != '\0' ||
- (newtime & ~INT_MAX) != 0L ||
- (errno == ERANGE && newtime == LONG_MAX))
- errx(1, "invalid arg for timeout: %s", arg);
+ newtime = strtonum(arg, 0, INT_MAX, );
+ if (errstr != NULL)
+ errx(1, "%s arg for timeout: %s", errstr, arg);
strlcpy(bp.ifbrp_name, name, sizeof(bp.ifbrp_name));
bp.ifbrp_ctime = newtime;
@@ -420,14 +414,12 @@ void
bridge_maxage(const char *arg, int d)
{
struct ifbrparam bp;
- unsigned long v;
- char *endptr;
+ uint8_t v;
+ const char *errstr;
- errno = 0;
- v = strtoul(arg, , 0);
- if (arg[0] == '\0' || endptr[0] != '\0' || v > 0xffUL ||
- (errno == ERANGE && v == ULONG_MAX))
- errx(1, "invalid arg for maxage: %s", arg);
+ v = strtonum(arg, 0, UINT8_MAX, );
+ if (errstr != NULL)
+ errx(1, "%s arg for maxage: %s", errstr, arg);
strlcpy(bp.ifbrp_name, name, sizeof(bp.ifbrp_name));
bp.ifbrp_maxage = v;
@@ -439,14 +431,12 @@ void
bridge_priority(const char *arg, int d)
{
struct ifbrparam bp;
- unsigned long v;
- char *endptr;
+ uint16_t v;
+ const char *errstr;
- errno = 0;
- v = strtoul(arg, , 0);
- if (arg[0] == '\0' || endptr[0] != '\0' || v > 0xUL ||
- (errno == ERANGE && v == ULONG_MAX))
- errx(1, "invalid arg for spanpriority: %s", arg);
+ v = strtonum(arg, 0, UINT16_MAX, );
+ if (errstr != NULL)
+ errx(1, "%s arg for spanpriority: %s", errstr, arg);
strlcpy(bp.ifbrp_name, name, sizeof(bp.ifbrp_name));
bp.ifbrp_prio = v;
@@ -478,14 +468,12 @@ void
bridge_fwddelay(const char *arg, int d)
{
struct ifbrparam bp;
- unsigned long v;
- char *endptr;
+ uint8_t v;
+ const char *errstr;
- errno = 0;
- v = strtoul(arg, , 0);
- if (arg[0] == '\0' || endptr[0] != '\0' || v > 0xffUL ||
- (errno == ERANGE && v == ULONG_MAX))
- errx(1, "invalid arg for fwddelay: %s", arg);
+ v = strtonu
ifconfig: Fix/improve settimeslot(), simplify get_ts_map() out
This fixes the primitive parsing route of settimeslot() to allow any
possible list of slots and/or ranges, see the update manual section.
The old code would happily mask "1,2-" into 0b11, settimeslot() now fails
on such broken ranges and also checks for inconsistencies like "4-1",
etc.
get_ts_map() has been heavily simplified to fit into a small macro
TS_RANGE_MASK().
strsep(3) is used instead of the deprecated strtok(3).
While here, remove trailing spaces and align function prototypes for
better readability.
Feedback/OK?
Index: ifconfig.8
===
RCS file: /cvs/src/sbin/ifconfig/ifconfig.8,v
retrieving revision 1.285
diff -u -p -r1.285 ifconfig.8
--- ifconfig.8 8 Jun 2017 00:46:42 - 1.285
+++ ifconfig.8 8 Jun 2017 15:23:54 -
@@ -450,9 +450,18 @@ and
.Xr pf.conf 5 .
.It Cm -rtlabel
Clear the route label.
-.It Cm timeslot Ar timeslot_range
+.It Cm timeslot Ar n-m,n-m,...
Set the timeslot range map, which is used to control which channels
an interface device uses.
+.Ar n
+and
+.Ar m
+must range from 0 to 31. Multiple ranges may overlap.
+.Ar m
+must be
+greater than n if given.
+.Dq all
+can be used as alias to denote all timeslots.
.It Cm up
Mark an interface
.Dq up .
Index: ifconfig.c
===
RCS file: /cvs/src/sbin/ifconfig/ifconfig.c,v
retrieving revision 1.343
diff -u -p -r1.343 ifconfig.c
--- ifconfig.c 6 Jun 2017 04:52:40 - 1.343
+++ ifconfig.c 8 Jun 2017 15:23:54 -
@@ -326,13 +326,13 @@ int actions;/* Actions
performed */
#define NEXTARG20xfd
const structcmd {
- char*c_name;
- int c_parameter;/* NEXTARG means next argv */
- int c_action; /* defered action */
+ char *c_name;
+ int c_parameter; /* NEXTARG means next argv */
+ int c_action; /* defered action */
void(*c_func)(const char *, int);
void(*c_func2)(const char *, const char *);
} cmds[] = {
- { "up", IFF_UP, 0, setifflags } ,
+ { "up", IFF_UP, 0, setifflags },
{ "down", -IFF_UP,0, setifflags },
{ "arp", -IFF_NOARP, 0, setifflags },
{ "-arp", IFF_NOARP, 0, setifflags },
@@ -427,11 +427,11 @@ const struct cmd {
{ "defer",1, 0, setpfsync_defer },
{ "-defer", 0, 0, setpfsync_defer },
/* giftunnel is for backward compat */
- { "giftunnel", NEXTARG2, 0, NULL, settunnel } ,
- { "tunnel", NEXTARG2, 0, NULL, settunnel } ,
- { "deletetunnel", 0, 0, deletetunnel } ,
- { "tunneldomain", NEXTARG,0, settunnelinst } ,
- { "tunnelttl",NEXTARG,0, settunnelttl } ,
+ { "giftunnel", NEXTARG2, 0, NULL, settunnel },
+ { "tunnel", NEXTARG2, 0, NULL, settunnel },
+ { "deletetunnel", 0, 0, deletetunnel },
+ { "tunneldomain", NEXTARG,0, settunnelinst },
+ { "tunnelttl",NEXTARG,0, settunnelttl },
{ "pppoedev", NEXTARG,0, setpppoe_dev },
{ "pppoesvc", NEXTARG,0, setpppoe_svc },
{ "-pppoesvc",1, 0, setpppoe_svc },
@@ -537,15 +537,15 @@ const struct cmd {
#endif /* SMALL */
#if 0
/* XXX `create' special-cased below */
- { "create", 0, 0, clone_create } ,
+ { "create", 0, 0, clone_create },
#endif
- { "destroy", 0, 0, clone_destroy } ,
- { "link0",IFF_LINK0, 0, setifflags } ,
- { "-link0", -IFF_LINK0, 0, setifflags } ,
- { "link1",IFF_LINK1, 0, setifflags } ,
- { "-link1", -IFF_LINK1, 0, setifflags } ,
- { "link2",IFF_LINK2, 0, setifflags } ,
- { "-link2", -IFF_LINK2, 0, setifflags } ,
+ { "destroy", 0, 0, clone_destroy },
+ { "link0",IFF_LINK0, 0, setifflags },
+ { "-link0", -IFF_LINK0, 0, setifflags },
+ { "link1",IFF_LINK1, 0, setifflags },
+ { "-link1", -IFF_LINK1, 0, setifflags },
+ { "link2",IFF_LINK2, 0, setifflags },
+ { "-link2", -IFF_LINK2, 0, setifflags },
{ "media",NEXTARG0, A_MEDIA,setmedia },
{ "mediaopt",
install.sub: Clean v[46]_info() ouput
With this patch, v[46]_info() both output exactly what their description
says.
As of now, these functions are only used through
set -- $(v4_info $_if)
which gracefully handles any constellation of whitespaces in the output
just fine. However future usage might change (or not) and being precise
about a function's output doesn't hurt.
The sed command itself is a tad clearer/smarter that way as well, imho.
Here's what I mean (whitespaces visualised):
$ v4_info trunk0
UP
› ·192.168.1.2··0xff00·broadcast·192.168.1.255
$ v4_info_clean trunk0
UP
192.168.1.2·0xff00·broadcast·192.168.1.255
Feeback/OK?
Index: install.sub
===
RCS file: /cvs/src/distrib/miniroot/install.sub,v
retrieving revision 1.1014
diff -u -p -r1.1014 install.sub
--- install.sub 3 Jun 2017 22:27:41 - 1.1014
+++ install.sub 14 Jun 2017 15:13:05 -
@@ -896,13 +896,14 @@ __EOT
}
# Obtain and output the inet information related to interface $1.
-# Should output ' '.
+# Outputs '\n '.
v4_info() {
ifconfig $1 inet | sed -n '
- 1s/.*
Re: install.sub: Fix scrambled address list in v6_defroute()
On Wed, Jun 14, 2017 at 03:00:11AM +0200, Klemens Nanni wrote: Installing -current the other day showed a broken list when picking the IPv6 default route just like reported on bugs@ five days ago[1]. Missed the link. 1: http://marc.info/?l=openbsd-bugs=149704197715791
install.sub: Fix scrambled address list in v6_defroute()
Installing -current the other day showed a broken list when picking
the IPv6 default route just like reported on bugs@ five days ago[1].
This behaviour can be reproduced manually running the code from
v6_defroute():
$ # source/define bsort()
$ _if=trunk0
$ bsort $(ping6 -n -c 2 ff02::2%$_if 2>/dev/null |
sed -n '/bytes from/{s/^.*from //;s/,.*$//;p;}' |
sed -n 'G;s/\n/&&/;/^\(.*\n\).*\n\1/d;h;P')
fe80:::__ff:fe__:___%trunk0: hlim=64 icmp_seq=0 icmp_seq=1 ms
time=0.431 time=0.802
The first sed filters for those lines containing the router addresses
and does two things:
1. strip leading text up to the address
2. strip trailing text after the first ','
However, as far as i can see 'ping6 -nc2' will never output lines that
would match the second criteria thus making this part of the command
obsolete.
The second sed removes duplicate addresses (not neccessarily consecutive
ones). This works fine for itself, but as seen above its input contains
more than just addresses. Besides that, sorting through sed just before
having bsort() cleaning the list is duplicate effort here.
With this patch bsort() gets passed possibly duplicate IPv6 addresses
one per line so _routers will eventually contain a sorted list of unique
router addresses; this fixes the list shown during the installer.
While debugging this, I noticed that stopping after two echo replies
would sometimes show only one router; increasing this limit to three
made both of my network's routers reply/show up reliably.
Feedback/OK?
Index: install.sub
===
RCS file: /cvs/src/distrib/miniroot/install.sub,v
retrieving revision 1.1014
diff -u -p -r1.1014 install.sub
--- install.sub 3 Jun 2017 22:27:41 - 1.1014
+++ install.sub 14 Jun 2017 00:59:28 -
@@ -997,9 +997,8 @@ v6_defroute() {
route -n show -inet6 | egrep -q '^default[[:space:]]' && return
- _routers=$(bsort $(ping6 -n -c 2 ff02::2%$_if 2>/dev/null |
- sed -n '/bytes from/{s/^.*from //;s/,.*$//;p;}' |
- sed -n 'G;s/\n/&&/;/^\(.*\n\).*\n\1/d;h;P'))
+ _routers=$(bsort $(ping6 -n -c 3 ff02::2%$_if 2>/dev/null |
+ sed -n 's/^.*from \(.*\): .*$/\1/p'))
_prompt="IPv6 default router?"
Re: Add support for rdonly keyword in fstab
On Wed, Jun 14, 2017 at 07:54:48PM +0200, Jérôme FRGACIC wrote: I recently realized that the word "rdonly" is not an alias for "ro" in the fstab file, which is inconsistent with the mount(1) command. I would suggest to add support for it with this small patch. fstab(5) has always used ro/rw for this exclusively, which is sufficient enough imho. I dare say mount(8) actually has too many ways to say the same thing: -r -o ro -r norw -o rdonly How about removing some of those?
install.sub: ieee80211_{scan,config}: Allow quoted SSIDs
Instead of ignoring SSIDs containing whitespaces, slightly adjust the
commands to take everything in between 'nwid ' and ' chan' as SSID; if
it has double quotes at start *and* end, simply remove those.
This enables users to select networks such as "Unitymedia WifiSpot"
"FRITZ!Box 7490" for example which are common among the quoted ones at
least here in germany.
The only SSIDs known to break this are those containing ' chan ' as this
substring is used as delimiter. Picking "some chan 4 me" would therefore
result in _nwid being assigned '"some' (literal double quote), but than
reasonably acceptable (compared to the current behaviour).
Since cat's -n flag already takes care of the space between numbers and
input lines, remove the leading tab to avoid excessive widths for lines
with long SSIDs.
Feedback/OK?
Index: install.sub
===
RCS file: /cvs/src/distrib/miniroot/install.sub,v
retrieving revision 1.1014
diff -u -p -r1.1014 install.sub
--- install.sub 3 Jun 2017 22:27:41 - 1.1014
+++ install.sub 14 Jun 2017 22:06:47 -
@@ -1060,10 +1060,9 @@ v6_config() {
# Perform an 802.11 network scan on interface $1.
# The result is cached in $WLANLIST.
ieee80211_scan() {
- # N.B. Skipping quoted nwid's for now.
[[ -f $WLANLIST ]] ||
ifconfig $1 scan |
- sed -n 's/^ nwid \([^"]\)/\1/p' >$WLANLIST
+ sed -n 's/^[[:space:]]*nwid //p' >$WLANLIST
cat $WLANLIST
}
@@ -1078,15 +1077,16 @@ ieee80211_config() {
# Empty scan cache.
rm -f $WLANLIST
- while [[ -z $_nwid ]]; do
+ while [[ -z "$_nwid" ]]; do
ask_until "Access point? (ESSID, 'any', list# or '?')" "any"
case "$resp" in
+([0-9]))
- _nwid=$(ieee80211_scan $_if | sed -n "${resp}s/ .*//p")
+ _nwid=$(ieee80211_scan $_if | sed -n ${resp}'{s/ chan
.*//p;q;}')
[[ -z $_nwid ]] && echo "There is no line $resp."
+ [[ $_nwid = \"*\" ]] && _nwid=${_nwid#\"}
_nwid=${_nwid%\"}
;;
\?) ieee80211_scan $_if |
- sed -n 's/^\([^ ]*\) chan .* bssid \([^ ]*\)
.*$/ \1 (\2)/p' |
+ sed -n 's/^\(.*\) chan .* bssid \([^ ]*\)
.*$/\1 (\2)/p' |
cat -n | more -c
;;
*) _nwid=$resp
Re: install.sub: ieee80211_{scan,config}: Allow quoted SSIDs
No need for quoting $_nwid within [[ ... ]] since field splitting is not
applied.
Index: install.sub
===
RCS file: /cvs/src/distrib/miniroot/install.sub,v
retrieving revision 1.1014
diff -u -p -r1.1014 install.sub
--- install.sub 3 Jun 2017 22:27:41 - 1.1014
+++ install.sub 15 Jun 2017 14:48:56 -
@@ -1060,10 +1060,9 @@ v6_config() {
# Perform an 802.11 network scan on interface $1.
# The result is cached in $WLANLIST.
ieee80211_scan() {
- # N.B. Skipping quoted nwid's for now.
[[ -f $WLANLIST ]] ||
ifconfig $1 scan |
- sed -n 's/^ nwid \([^"]\)/\1/p' >$WLANLIST
+ sed -n 's/^[[:space:]]*nwid //p' >$WLANLIST
cat $WLANLIST
}
@@ -1082,11 +1081,12 @@ ieee80211_config() {
ask_until "Access point? (ESSID, 'any', list# or '?')" "any"
case "$resp" in
+([0-9]))
- _nwid=$(ieee80211_scan $_if | sed -n "${resp}s/ .*//p")
+ _nwid=$(ieee80211_scan $_if | sed -n ${resp}'{s/ chan
.*//p;q;}')
[[ -z $_nwid ]] && echo "There is no line $resp."
+ [[ $_nwid = \"*\" ]] && _nwid=${_nwid#\"}
_nwid=${_nwid%\"}
;;
\?) ieee80211_scan $_if |
- sed -n 's/^\([^ ]*\) chan .* bssid \([^ ]*\)
.*$/ \1 (\2)/p' |
+ sed -n 's/^\(.*\) chan .* bssid \([^ ]*\)
.*$/\1 (\2)/p' |
cat -n | more -c
;;
*) _nwid=$resp
arp: Fix synopsis
'arp -[d]a' do not require hostname as told in the manual page.
A single fprintf() is both shorter and cleaner.
Index: arp.8
===
RCS file: /cvs/src/usr.sbin/arp/arp.8,v
retrieving revision 1.39
diff -u -p -r1.39 arp.8
--- arp.8 5 Apr 2016 18:18:42 - 1.39
+++ arp.8 19 Jun 2017 10:15:10 -
@@ -40,7 +40,7 @@
.Nm arp
.Op Fl adn
.Op Fl V Ar rdomain
-.Ar hostname
+.Op Ar hostname
.Nm arp
.Op Fl F
.Op Fl f Ar file
Index: arp.c
===
RCS file: /cvs/src/usr.sbin/arp/arp.c,v
retrieving revision 1.79
diff -u -p -r1.79 arp.c
--- arp.c 19 Apr 2017 05:36:12 - 1.79
+++ arp.c 19 Jun 2017 10:15:10 -
@@ -601,11 +601,10 @@ ether_str(struct sockaddr_dl *sdl)
void
usage(void)
{
- fprintf(stderr, "usage: arp [-adn] [-V rdomain] hostname\n");
- fprintf(stderr, " arp [-F] [-f file] [-V rdomain] "
- "-s hostname ether_addr\n"
- " [temp | permanent] [pub]\n");
- fprintf(stderr, " arp -W ether_addr [iface]\n");
+ fprintf(stderr, "usage: arp [-adn] [-V rdomain] [hostname]\n"
+ " arp [-F] [-f file] [-V rdomain] -s hostname ether_addr\n"
+ " [temp | permanent] [pub]\n"
+ " arp -W ether_addr [iface]\n");
exit(1);
}
Re: ping: Style fixes/cleanups
On Tue, Jun 13, 2017 at 10:45:57AM -0600, Theo de Raadt wrote:
Sorry, but that type of diff is a no-go. You've made a large variety
of different decisions on your own and mixed them up with ones which
are personal taste, and then touched so many lines of the code that
future study of historical changes in the code will be confusing.
Points taken.
The right approach is seperate changes into seperate catagories with
seperate explanations. So I suggest you start with the ones of most
importance, and don't mix it with fluff (that's what we call it).
Each of the following patches applies cleanly on its own.
Starting with this one replacing perror(3) with err(1).
Index: ping.c
===
RCS file: /cvs/src/sbin/ping/ping.c,v
retrieving revision 1.218
diff -u -p -r1.218 ping.c
--- ping.c 22 Feb 2017 13:43:35 - 1.218
+++ ping.c 13 Jun 2017 17:28:46 -
@@ -729,10 +729,8 @@ main(int argc, char *argv[])
rspace[IPOPT_OLEN] = sizeof(rspace)-1;
rspace[IPOPT_OFFSET] = IPOPT_MINOFF;
if (setsockopt(s, IPPROTO_IP, IP_OPTIONS, rspace,
- sizeof(rspace)) < 0) {
- perror("ping: record route");
- exit(1);
- }
+ sizeof(rspace)) < 0)
+ err(1, "record route");
}
if ((moptions & MULTICAST_NOLOOP) &&
Re: ping: Style fixes/cleanups
Do not clear the unneeded dst[46] structure. Index: ping.c === RCS file: /cvs/src/sbin/ping/ping.c,v retrieving revision 1.218 diff -u -p -r1.218 ping.c --- ping.c 22 Feb 2017 13:43:35 - 1.218 +++ ping.c 13 Jun 2017 17:32:13 - @@ -434,8 +434,10 @@ main(int argc, char *argv[]) if (argc != 1) usage(); - memset(, 0, sizeof(dst4)); - memset(, 0, sizeof(dst6)); + if (v6flag) + memset(, 0, sizeof(dst6)); + else + memset(, 0, sizeof(dst4)); target = *argv;
Re: ping: Style fixes/cleanups
printf's return value is ignored allmost all the times so do the same
for the rest of them as well for consistency.
Subsequent calls got merged where I think is appropiate and readability
improves.
The usage format string now looks like it's actual put.
Index: ping.c
===
RCS file: /cvs/src/sbin/ping/ping.c,v
retrieving revision 1.218
diff -u -p -r1.218 ping.c
--- ping.c 22 Feb 2017 13:43:35 - 1.218
+++ ping.c 13 Jun 2017 18:08:42 -
@@ -756,10 +756,10 @@ main(int argc, char *argv[])
arc4random_buf(_offset, sizeof(tv64_offset));
arc4random_buf(_key, sizeof(mac_key));
- printf("PING %s (", hostname);
+ (void)printf("PING %s (", hostname);
if (options & F_VERBOSE)
- printf("%s --> ", pr_addr(from, from->sa_len));
- printf("%s): %d data bytes\n", pr_addr(dst, dst->sa_len), datalen);
+ (void)printf("%s --> ", pr_addr(from, from->sa_len));
+ (void)printf("%s): %d data bytes\n", pr_addr(dst, dst->sa_len),
datalen);
smsghdr.msg_name = dst;
smsghdr.msg_namelen = dst->sa_len;
@@ -862,7 +862,7 @@ main(int argc, char *argv[])
*/
if ((mtu = get_pathmtu(, )) > 0) {
if ((options & F_VERBOSE) != 0) {
- printf("new path MTU (%d) is "
+ (void)printf("new path MTU (%d) is "
"notified\n", mtu);
}
}
@@ -926,28 +926,28 @@ fill(char *bp, char *patp)
void
summary(void)
{
- printf("\n--- %s ping statistics ---\n", hostname);
- printf("%lld packets transmitted, ", ntransmitted);
- printf("%lld packets received, ", nreceived);
+ (void)printf("\n--- %s ping statistics ---\n"
+ "%lld packets transmitted, %lld packets received, ",
+ hostname, ntransmitted, nreceived);
if (nrepeats)
- printf("%lld duplicates, ", nrepeats);
+ (void)printf("%lld duplicates, ", nrepeats);
if (ntransmitted) {
if (nreceived > ntransmitted)
- printf("-- somebody's duplicating packets!");
+ (void)printf("-- somebody's duplicating packets!");
else
- printf("%.1f%% packet loss",
- double)ntransmitted - nreceived) * 100) /
- ntransmitted));
+ (void)printf("%.1f%% packet loss",
+ (double)(ntransmitted - nreceived)
+ * 100 / ntransmitted);
}
- printf("\n");
+ (void)printf("\n");
if (timinginfo) {
/* Only display average to microseconds */
double num = nreceived + nrepeats;
double avg = tsum / num;
double dev = sqrt(fmax(0, tsumsq / num - avg * avg));
- printf("round-trip min/avg/max/std-dev = %.3f/%.3f/%.3f/%.3f
ms\n",
- tmin, avg, tmax, dev);
+ (void)printf("round-trip min/avg/max/std-dev = "
+ "%.3f/%.3f/%.3f/%.3f ms\n", tmin, avg, tmax, dev);
}
}
@@ -1092,7 +1092,7 @@ pinger(int s)
if (i < 0 || i != cc) {
if (i < 0)
warn("sendmsg");
- printf("ping: wrote %s %d chars, ret=%d\n", hostname, cc, i);
+ (void)printf("ping: wrote %s %d chars, ret=%d\n", hostname, cc,
i);
}
if (!(options & F_QUIET) && options & F_FLOOD)
(void)write(STDOUT_FILENO, , 1);
@@ -1278,11 +1278,9 @@ pr_pack(u_char *buf, int cc, struct msgh
cp = (u_char *)
>icmp_data[0];
for (i = ECHOLEN; i < cc && i < datalen;
- ++i, ++cp) {
- if ((i % 32) == 8)
- (void)printf("\n\t");
- (void)printf("%x ", *cp);
- }
+ ++i, ++cp)
+ (void)printf(i % 32 == 8 ?
+ "\n\t%x " : "%x ", *cp);
break;
}
}
@@ -1560,8 +1558,7 @@ pr_icmph(struct icmp *icp)
(void)printf("Redirect, Unknown Code: %d",
icp->icmp_code);
break;
}
- (void)printf("(New addr: %s)\n",
- inet_ntoa(icp->icmp_gwaddr));
+ (void)printf("(New
Re: ping: Style fixes/cleanups
The intent here is to get the highest multiple of four smaller or equal
than i + 3. Instead of relying on integer division to get rid of the
remainder just to "undo" everything, simply clear the lowest two bits
(0b11 = 3) leaving multiples of four.
Index: ping.c
===
RCS file: /cvs/src/sbin/ping/ping.c,v
retrieving revision 1.218
diff -u -p -r1.218 ping.c
--- ping.c 22 Feb 2017 13:43:35 - 1.218
+++ ping.c 13 Jun 2017 17:29:19 -
@@ -1379,7 +1379,7 @@ pr_ipopt(int hlen, u_char *buf)
!memcmp(cp, old_rr, i) &&
!(options & F_FLOOD)) {
(void)printf("\t(same route)");
- i = ((i + 3) / 4) * 4;
+ i = (i + 3) & ~0x3;
hlen -= i;
cp += i;
break;
Re: ping: Style fixes/cleanups
Unify option checking and simply logic.
F_HDRINCL and F_ROUTE are mutually exclusive, thus check the latter only
if the former one is not set.
Index: ping.c
===
RCS file: /cvs/src/sbin/ping/ping.c,v
retrieving revision 1.218
diff -u -p -r1.218 ping.c
--- ping.c 22 Feb 2017 13:43:35 - 1.218
+++ ping.c 13 Jun 2017 17:38:22 -
@@ -562,17 +562,17 @@ main(int argc, char *argv[])
(void)setsockopt(s, SOL_SOCKET, SO_DEBUG, ,
sizeof(optval));
- if ((options & F_FLOOD) && (options & F_INTERVAL))
+ if (options & (F_FLOOD | F_INTERVAL))
errx(1, "-f and -i options are incompatible");
- if ((options & F_FLOOD) && (options & (F_AUD_RECV | F_AUD_MISS)))
+ if (options & (F_FLOOD | F_AUD_RECV | F_AUD_MISS))
warnx("No audible output for flood pings");
if (datalen >= sizeof(struct payload)) /* can we time transfer */
timing = 1;
if (v6flag) {
- /* in F_VERBOSE case, we may get non-echoreply packets*/
+ /* in F_VERBOSE case, we may get non-echoreply packets */
if (options & F_VERBOSE && datalen < 2048) /* XXX 2048? */
packlen = 2048 + IP6LEN + ECHOLEN + EXTRA;
else
@@ -621,7 +621,7 @@ main(int argc, char *argv[])
* let the kernel pass extension headers of incoming packets,
* for privileged socket options
*/
- if ((options & F_VERBOSE) != 0) {
+ if (options & F_VERBOSE) {
int opton = 1;
if (setsockopt(s, IPPROTO_IPV6, IPV6_RECVHOPOPTS,
@@ -696,7 +696,7 @@ main(int argc, char *argv[])
options |= F_HDRINCL;
}
- if (options & F_RROUTE && options & F_HDRINCL)
+ if (options & (F_RROUTE | F_HDRINCL))
errx(1, "-R option and -D or -T, or -t to unicast"
" destinations are incompatible");
@@ -717,10 +717,8 @@ main(int argc, char *argv[])
else
ip->ip_src.s_addr = INADDR_ANY;
ip->ip_dst = dst4.sin_addr;
- }
-
/* record route option */
- if (options & F_RROUTE) {
+ } else if (options & F_RROUTE) {
if (IN_MULTICAST(ntohl(dst4.sin_addr.s_addr)))
errx(1, "record route not valid to multicast"
" destinations");
@@ -773,7 +771,7 @@ main(int argc, char *argv[])
(void)signal(SIGINT, onsignal);
(void)signal(SIGINFO, onsignal);
- if ((options & F_FLOOD) == 0) {
+ if (!(options & F_FLOOD)) {
(void)signal(SIGALRM, onsignal);
itimer.it_interval = interval;
itimer.it_value = interval;
@@ -861,8 +859,8 @@ main(int argc, char *argv[])
* a path MTU notification.)
*/
if ((mtu = get_pathmtu(, )) > 0) {
- if ((options & F_VERBOSE) != 0) {
- printf("new path MTU (%d) is "
+ if (options & F_VERBOSE) {
+ (void)printf("new path MTU (%d) is "
"notified\n", mtu);
}
}
@@ -961,7 +959,7 @@ pr_addr(struct sockaddr *addr, socklen_t
static char buf[NI_MAXHOST];
int flag = 0;
- if ((options & F_HOSTNAME) == 0)
+ if (!(options & F_HOSTNAME))
flag |= NI_NUMERICHOST;
if (getnameinfo(addr, addrlen, buf, sizeof(buf), NULL, 0, flag) == 0)
@@ -1892,7 +1890,7 @@ get_pathmtu(struct msghdr *mhdr, struct
dst->sin6_scope_id &&
mtuctl->ip6m_addr.sin6_scope_id !=
dst->sin6_scope_id)) {
- if ((options & F_VERBOSE) != 0) {
+ if (options & F_VERBOSE) {
printf("path MTU for %s is notified. "
"(ignored)\n",
pr_addr((struct sockaddr *)
Re: brconfig: Unify/fix strtoul(3) handling
No need for temporary variables either, strtonum guarantees through
maxval that its return value won't overflow when casted.
Index: brconfig.c
===
RCS file: /cvs/src/sbin/ifconfig/brconfig.c,v
retrieving revision 1.15
diff -u -p -r1.15 brconfig.c
--- brconfig.c 7 Jun 2017 16:47:29 - 1.15
+++ brconfig.c 9 Jun 2017 17:33:10 -
@@ -220,7 +220,6 @@ bridge_ifsetflag(const char *ifsname, u_
err(1, "%s: ioctl SIOCBRDGGIFFLGS %s", name, ifsname);
req.ifbr_ifsflags |= flag & ~IFBIF_RO_MASK;
-
if (ioctl(s, SIOCBRDGSIFFLGS, (caddr_t)) < 0)
err(1, "%s: ioctl SIOCBRDGSIFFLGS %s", name, ifsname);
}
@@ -232,12 +231,10 @@ bridge_ifclrflag(const char *ifsname, u_
strlcpy(req.ifbr_name, name, sizeof(req.ifbr_name));
strlcpy(req.ifbr_ifsname, ifsname, sizeof(req.ifbr_ifsname));
-
if (ioctl(s, SIOCBRDGGIFFLGS, (caddr_t)) < 0)
err(1, "%s: ioctl SIOCBRDGGIFFLGS %s", name, ifsname);
req.ifbr_ifsflags &= ~(flag | IFBIF_RO_MASK);
-
if (ioctl(s, SIOCBRDGSIFFLGS, (caddr_t)) < 0)
err(1, "%s: ioctl SIOCBRDGSIFFLGS %s", name, ifsname);
}
@@ -400,18 +397,13 @@ void
bridge_timeout(const char *arg, int d)
{
struct ifbrparam bp;
- long newtime;
- char *endptr;
+ const char *errstr;
- errno = 0;
- newtime = strtol(arg, , 0);
- if (arg[0] == '\0' || endptr[0] != '\0' ||
- (newtime & ~INT_MAX) != 0L ||
- (errno == ERANGE && newtime == LONG_MAX))
- errx(1, "invalid arg for timeout: %s", arg);
+ bp.ifbrp_ctime = strtonum(arg, 0, INT_MAX, );
+ if (errstr != NULL)
+ errx(1, "%s arg for timeout: %s", errstr, arg);
strlcpy(bp.ifbrp_name, name, sizeof(bp.ifbrp_name));
- bp.ifbrp_ctime = newtime;
if (ioctl(s, SIOCBRDGSTO, (caddr_t)) < 0)
err(1, "%s", name);
}
@@ -420,17 +412,13 @@ void
bridge_maxage(const char *arg, int d)
{
struct ifbrparam bp;
- unsigned long v;
- char *endptr;
+ const char *errstr;
- errno = 0;
- v = strtoul(arg, , 0);
- if (arg[0] == '\0' || endptr[0] != '\0' || v > 0xffUL ||
- (errno == ERANGE && v == ULONG_MAX))
- errx(1, "invalid arg for maxage: %s", arg);
+ bp.ifbrp_maxage = strtonum(arg, 0, UINT8_MAX, );
+ if (errstr != NULL)
+ errx(1, "%s arg for maxage: %s", errstr, arg);
strlcpy(bp.ifbrp_name, name, sizeof(bp.ifbrp_name));
- bp.ifbrp_maxage = v;
if (ioctl(s, SIOCBRDGSMA, (caddr_t)) < 0)
err(1, "%s", name);
}
@@ -439,17 +427,13 @@ void
bridge_priority(const char *arg, int d)
{
struct ifbrparam bp;
- unsigned long v;
- char *endptr;
+ const char *errstr;
- errno = 0;
- v = strtoul(arg, , 0);
- if (arg[0] == '\0' || endptr[0] != '\0' || v > 0xUL ||
- (errno == ERANGE && v == ULONG_MAX))
- errx(1, "invalid arg for spanpriority: %s", arg);
+ bp.ifbrp_prio = strtonum(arg, 0, UINT16_MAX, );
+ if (errstr != NULL)
+ errx(1, "%s arg for spanpriority: %s", errstr, arg);
strlcpy(bp.ifbrp_name, name, sizeof(bp.ifbrp_name));
- bp.ifbrp_prio = v;
if (ioctl(s, SIOCBRDGSPRI, (caddr_t)) < 0)
err(1, "%s", name);
}
@@ -478,17 +462,13 @@ void
bridge_fwddelay(const char *arg, int d)
{
struct ifbrparam bp;
- unsigned long v;
- char *endptr;
+ const char *errstr;
- errno = 0;
- v = strtoul(arg, , 0);
- if (arg[0] == '\0' || endptr[0] != '\0' || v > 0xffUL ||
- (errno == ERANGE && v == ULONG_MAX))
- errx(1, "invalid arg for fwddelay: %s", arg);
+ bp.ifbrp_fwddelay = strtonum(arg, 0, UINT8_MAX, );
+ if (errstr != NULL)
+ errx(1, "%s arg for fwddelay: %s", errstr, arg);
strlcpy(bp.ifbrp_name, name, sizeof(bp.ifbrp_name));
- bp.ifbrp_fwddelay = v;
if (ioctl(s, SIOCBRDGSFD, (caddr_t)) < 0)
err(1, "%s", name);
}
@@ -497,17 +477,13 @@ void
bridge_hellotime(const char *arg, int d)
{
struct ifbrparam bp;
- unsigned long v;
- char *endptr;
+ const char *errstr;
- errno = 0;
- v = strtoul(arg, , 0);
- if (arg[0] == '\0' || endptr[0] != '\0' || v > 0xffUL ||
- (errno == ERANGE && v == ULONG_MAX))
- errx(1, "invalid arg for hellotime: %s", arg);
+ bp.ifbrp_hellotime = strtonum(arg, 0, UINT8_MAX, );
+ if (errstr != NULL)
+ errx(1, "%s arg for hellotime: %s", errstr, arg);
strlcpy(bp.ifbrp_name, name, sizeof(bp.ifbrp_name));
- bp.ifbrp_hellotime = v;
if (ioctl(s, SIOCBRDGSHT, (caddr_t)) < 0)
err(1, "%s", name);
}
@@ -516,17 +492,13 @@ void
bridge_maxaddr(const char *arg, int d)
{
ping: Style fixes/cleanups
Ignore return status of all printf calls consistently, merge
subsequent ones where appropiate, do not memset unneeded dst structure,
simplify/unify option checks, use err not perror, break lines at 80
chars, remove unnecessary parentheses, avoid unobvious integer divsion,
make usage format string and output look alike.
Feedback/OK?
Index: ping.c
===
RCS file: /cvs/src/sbin/ping/ping.c,v
retrieving revision 1.218
diff -u -p -r1.218 ping.c
--- ping.c 22 Feb 2017 13:43:35 - 1.218
+++ ping.c 13 Jun 2017 16:35:03 -
@@ -182,10 +182,10 @@ int ident;/* process id to
identify o
int v6flag = 0; /* are we ping6? */
/* counters */
-int64_t npackets; /* max packets to transmit */
+int64_t npackets; /* max # of packets to transmit */
int64_t nreceived; /* # of packets we got back */
-int64_t nrepeats; /* number of duplicates */
-int64_t ntransmitted; /* sequence # for outbound packets = #sent */
+int64_t nrepeats; /* # of duplicates */
+int64_t ntransmitted; /* sequence # for outbound packets = # sent */
int64_t nmissedmax = 1; /* max value of ntransmitted - nreceived - 1 */
struct timeval interval = {1, 0}; /* interval between packets */
@@ -208,34 +208,34 @@ volatile sig_atomic_t seenalrm;
volatile sig_atomic_t seenint;
volatile sig_atomic_t seeninfo;
-voidfill(char *, char *);
-voidsummary(void);
-voidonsignal(int);
-voidretransmit(int);
-int pinger(int);
-const char *pr_addr(struct sockaddr *, socklen_t);
-voidpr_pack(u_char *, int, struct msghdr *);
-__dead void usage(void);
+voidfill(char *, char *);
+voidsummary(void);
+voidonsignal(int);
+voidretransmit(int);
+int pinger(int);
+const char *pr_addr(struct sockaddr *, socklen_t);
+voidpr_pack(u_char *, int, struct msghdr *);
+__dead void usage(void);
/* IPv4 specific functions */
-voidpr_ipopt(int, u_char *);
-int in_cksum(u_short *, int);
-voidpr_icmph(struct icmp *);
-voidpr_retip(struct ip *);
-voidpr_iph(struct ip *);
+voidpr_ipopt(int, u_char *);
+int in_cksum(u_short *, int);
+voidpr_icmph(struct icmp *);
+voidpr_retip(struct ip *);
+voidpr_iph(struct ip *);
#ifndef SMALL
-int map_tos(char *, int *);
+int map_tos(char *, int *);
#endif /* SMALL */
/* IPv6 specific functions */
-int get_hoplim(struct msghdr *);
-int get_pathmtu(struct msghdr *, struct sockaddr_in6 *);
-voidpr_icmph6(struct icmp6_hdr *, u_char *);
-voidpr_iph6(struct ip6_hdr *);
-voidpr_exthdrs(struct msghdr *);
-voidpr_ip6opt(void *);
-voidpr_rthdr(void *);
-voidpr_retip6(struct ip6_hdr *, u_char *);
+int get_hoplim(struct msghdr *);
+int get_pathmtu(struct msghdr *, struct sockaddr_in6 *);
+voidpr_icmph6(struct icmp6_hdr *, u_char *);
+voidpr_iph6(struct ip6_hdr *);
+voidpr_exthdrs(struct msghdr *);
+voidpr_ip6opt(void *);
+voidpr_rthdr(void *);
+voidpr_retip6(struct ip6_hdr *, u_char *);
int
main(int argc, char *argv[])
@@ -434,8 +434,10 @@ main(int argc, char *argv[])
if (argc != 1)
usage();
- memset(, 0, sizeof(dst4));
- memset(, 0, sizeof(dst6));
+ if (v6flag)
+ memset(, 0, sizeof(dst6));
+ else
+ memset(, 0, sizeof(dst4));
target = *argv;
@@ -562,17 +564,17 @@ main(int argc, char *argv[])
(void)setsockopt(s, SOL_SOCKET, SO_DEBUG, ,
sizeof(optval));
- if ((options & F_FLOOD) && (options & F_INTERVAL))
+ if (options & (F_FLOOD | F_INTERVAL))
errx(1, "-f and -i options are incompatible");
- if ((options & F_FLOOD) && (options & (F_AUD_RECV | F_AUD_MISS)))
+ if (options & (F_FLOOD | F_AUD_RECV | F_AUD_MISS))
warnx("No audible output for flood pings");
if (datalen >= sizeof(struct payload)) /* can we time transfer */
timing = 1;
if (v6flag) {
- /* in F_VERBOSE case, we may get non-echoreply packets*/
+ /* in F_VERBOSE case, we may get non-echoreply packets */
if (options & F_VERBOSE && datalen < 2048) /* XXX 2048? */
packlen = 2048 + IP6LEN + ECHOLEN + EXTRA;
else
@@ -621,7 +623,7 @@ main(int argc, char
