Re: Improve error message in rcctl(8) again
> Hi Anthony.
>
> Thanks for the patch.
> Slightly modified version but it should do the same.
> Does this work for you?
>
> Index: rcctl.sh
> ===
> RCS file: /cvs/src/usr.sbin/rcctl/rcctl.sh,v
> retrieving revision 1.116
> diff -u -p -r1.116 rcctl.sh
> --- rcctl.sh 24 Apr 2023 14:31:15 - 1.116
> +++ rcctl.sh 13 Jul 2023 09:58:39 -
> @@ -535,13 +535,17 @@ case ${action} in
> shift 1
> svcs="$*"
> [ -z "${svcs}" ] && usage
> - # it's ok to disable a non-existing daemon
> - if [ "${action}" != "disable" ]; then
> - for svc in ${svcs}; do
> + for svc in ${svcs}; do
> + # it's ok to disable a non-existing daemon
> + if [ "${action}" != "disable" ]; then
> svc_is_avail ${svc} || \
> rcctl_err "service ${svc} does not
> exist" 2
>- done
>- fi
> + # but still check for bad input
> + else
> + _rc_check_name "${svc}" || \
> + rcctl_err "service ${svc} does not
> exist" 2
> + fi <<< TRAILING WHITESPACE >>>
> + done
> [...snip...]
That works, but note the trailing whitespace after the "fi" in the
second-last line of the code snippet above.
Thanks,
Anthony
Improve error message in rcctl(8) again
Seven years ago I tried to restart a configuration file and it didn't
work out: https://marc.info/?l=openbsd-tech=147318006722787=2
This morning I tried to disable a different configuration file and got
similar results. Maybe my hands get too used to typing ".conf" when I
am fiddling with config files and restarting services. Or maybe I have
the mind of a LISP programmer, to whom code and data are the same
thing. But if I have not stopped passing config files to rcctl after
seven years I will probably never stop, so we should fix the error
message again.
Test cases:
# rcctl disable foo.bar
# rcctl set foo.bar status off
Compare the error messages before and after the patch.
diff --git usr.sbin/rcctl/rcctl.sh usr.sbin/rcctl/rcctl.sh
index fb87943ba00..489c0217c45 100644
--- usr.sbin/rcctl/rcctl.sh
+++ usr.sbin/rcctl/rcctl.sh
@@ -541,6 +541,12 @@ case ${action} in
svc_is_avail ${svc} || \
rcctl_err "service ${svc} does not
exist" 2
done
+ else
+ # But you still have to catch invalid characters
+ for svc in ${svcs}; do
+ _rc_check_name "${svc}" || \
+ rcctl_err "service ${svc} does not
exist" 2
+ done
fi
;;
get|getdef)
@@ -572,6 +578,9 @@ case ${action} in
if [ "${action} ${var} ${args}" != "set status off" ]; then
svc_is_avail ${svc} || \
rcctl_err "service ${svc} does not exist" 2
+ else
+ _rc_check_name "${svc}" || \
+ rcctl_err "service ${svc} does not exist" 2
fi
[[ ${var} !=
@(class|execdir|flags|logger|rtable|status|timeout|user) ]] && usage
svc_is_meta ${svc} && [ "${var}" != "status" ] && \
Simplified strtonum(3)
pport only base <= 36, and the C
standard requires a word size of w >= 16 for integers, so we have
plenty of space between -INT_MIN and UINT_MAX:
base <= 36 <= 16384 = 2^(16 - 2) <= 2^(w-2)
That said, because this trick makes use of the maybe-overflow-maybe-not
values between -INT_MIN and UINT_MAX, it cannot be applied to the
unsigned variants strtoul and strtoull.
A complete implementation is provided below, along with a test harness
to catch issues around the critical boundaries of INT_MIN and INT_MAX.
I also added some assertions to the main loop to prove dynamically that
the computation of 10*acc + d doesn't overflow; obviously you would
remove these once you're comfortable that the approach is valid.
Regards,
Anthony Coulter
=== strtonum.c ===
#include
#include
#include
#include
#include
#include
#include
#include
long long strtonum2(const char *s, long long minval, long long maxval,
const char **errstrp)
{
enum { INVALID = 0, TOOSMALL = 1, TOOLARGE = 2 };
static const struct errval {
const char *errstr;
int err;
} ev[3] = {
{ "invalid",EINVAL },
{ "too small", ERANGE },
{ "too large", ERANGE },
};
int error;
if (minval > maxval) {
error = INVALID;
goto error;
}
while (isspace(*s))
s++;
bool neg;
if (*s == '-') {
neg = true;
s++;
} else {
neg = false;
if (*s == '+')
s++;
}
// The cast to (unsigned long long) avoids undefined behavior
// corresponding to the signed integer overflow that takes place
// when computing -LLONG_MIN. Strictly speaking, once we cast it
// there's no need to negate the result, but I include the minus
// sign for clarity and also because it's evaluated at compile
// time.
unsigned long long max = (-(unsigned long long)LLONG_MIN) / 10 + 1;
unsigned long long acc = 0;
do {
if (!isdigit(*s)) {
error = INVALID;
goto error;
} else if (acc > max) {
error = neg ? TOOSMALL : TOOLARGE;
goto error;
}
/*/
//
// Let's make sure we don't overflow the accumulator.
//
// An imperfect test:
assert(acc <= 10*acc + (*s - '0'));
// A perfect test:
unsigned long long product, dummy;
assert(!__builtin_umulll_overflow(10, acc, ));
assert(!__builtin_uaddll_overflow(product, *s, ));
/*/
acc = 10*acc + (*s - '0');
} while (*++s != '\0');
long long ll;
if (neg) {
ll = -acc;
if (ll > 0) {
error = TOOSMALL;
goto error;
}
} else {
ll = acc;
if (ll < 0) {
error = TOOLARGE;
goto error;
}
}
if (ll < minval) {
error = TOOSMALL;
goto error;
} else if (ll > maxval) {
error = TOOLARGE;
goto error;
}
if (errstrp != NULL)
*errstrp = NULL;
return ll;
error:
errno = ev[error].err;
if (errstrp != NULL)
*errstrp = ev[error].errstr;
return 0;
}
static bool test(const char *s, long long minval, long long maxval)
{
const char *err1, *err2;
int errno1, errno2;
errno = 1234;
long long a = strtonum(s, minval, maxval, );
errno1 = errno;
errno = 1234;
long long b = strtonum2(s, minval, maxval, );
errno2 = errno;
// The strings might be NULL
bool stringsmatch = (!err1 && !err2 || err1 && err2 && strcmp(err1,
err2) == 0);
if (a != b || errno1 != errno2 || !stringsmatch) {
printf("%d%d%d\n", a == b, errno1==errno2, stringsmatch);
printf("Them: %lld/%d/%s\n", a, errno1, err1 ? err1 : "(null)");
printf("Me : %lld/%d/%s\n", b, errno2, err2 ? err2 : "(null)");
}
return a == b && errno1 == errno2 && stringsmatch;
}
int main(int argc, char *argv[])
{
static const char *nums[] = {
"", // Oops, almost forgot to test the empty string
"0",
"1",
sysupgrade(8) and http_proxy
Hello @tech,
When I manually upgrade OpenBSD using bsd.rd, I have to set http_proxy
to fetch the file sets. When I reboot after installing, fw_update
succeeds because theinstall script was clever enough to export
http_proxy in /etc/rc.firsttime.
Unfortunately sysupgrade(8) does not remember that http_proxy was set
when it fetched the file sets, and so when I run sysupgrade I have to
either wait for fw_update to manually time out on first reboot, or kill
it manually with ^C.
Adding the line:
[ ${http_proxy} ] && echo "export http_proxy=${http_proxy}" >>/etc/rc.firsttime
to a spot near the bottom of /usr/sbin/sysupgrade fixes my fw_update
problem, at least until the upgrade restores all of my files to their
stock defaults. Is this something we could integrate into the official
repository?
Thanks and regards,
Anthony Coulter
Missing hardlink for /usr/bin/cc
$ ls -li /usr/bin/{cc,c++,clang,clang++,clang-cpp} /usr/libexec/cpp
156140 -r-xr-xr-x 5 root bin 46885664 May 4 11:12 /usr/bin/c++
155926 -r-xr-xr-x 1 root bin 46885664 May 4 11:12 /usr/bin/cc
156140 -r-xr-xr-x 5 root bin 46885664 May 4 11:12 /usr/bin/clang
156140 -r-xr-xr-x 5 root bin 46885664 May 4 11:12 /usr/bin/clang++
156140 -r-xr-xr-x 5 root bin 46885664 May 4 11:12 /usr/bin/clang-cpp
156140 -r-xr-xr-x 5 root bin 46885664 May 4 11:12 /usr/libexec/cpp
$ diff /usr/bin/{cc,clang}
$
I interpret this as /usr/bin/cc accidentally being a copy instead of a
hard link. Is this correct?
Regards,
Anthony Coulter
sysctl(3) or sysctl(2)?
According to /usr/src/sys/kern/syscalls.master, sysctl is system call number 202. But its manual page is in section 3, at /usr/src/lib/libc/gen/sysctl.3 Should it actually be in section 2? Regards, Anthony Coulter
vmd(8): Improve RFC 2132 compliance (DHCP)
The DHCP client available in the Alpine Linux installer (udhcpc, part
of BusyBox) does not accept responses that do not include the DHCP
message type option. Worse, it expects the message type to be
DHCPOFFER in some circumstances and DHCPREQUEST in others. The DHCP
server in vmd omits this option entirely, which makes it impossible
to install Alpine Linux in a virtual machine configured with "-L".
The simplest fix would be to use "resp.options[6] == DHCPOFFER" instead
of is_discover (see the patch below) because in practice the DHCP
message type will be the first option present after the magic cookie.
This was the first thing I tried, and it worked. But it's incorrect.
RFC 1534 says that requests with no message type can be treated as
BOOTP and not DHCP messages. It also says that we can send DHCP options
to BOOTP messages if we so desire, so it doesn't really matter whether
we initialize is_discover to zero or one.
Note that udhcpc also complains about two more options (server ID and
lease time) that are missing from the response message. I didn't do
anything about this because udhcpc uses sensible defaults.
I've tested this change with udhcpc (in a virtual Alpine Linux system)
and dhclient (in a virtual OpenBSD system) and it works for both. I
have not tried anything else.
Regards,
Anthony Coulter
Index: dhcp.c
===
RCS file: /cvs/src/usr.sbin/vmd/dhcp.c,v
retrieving revision 1.3
diff -u -p -u -p -r1.3 dhcp.c
--- dhcp.c 24 Apr 2017 07:14:27 - 1.3
+++ dhcp.c 8 Sep 2017 04:12:10 -
@@ -44,6 +44,7 @@ dhcp_request(struct vionet_dev *dev, cha
struct dhcp_packet req, resp;
struct in_addr in, mask;
size_t resplen, o;
+ int is_discover = 1;
if (buflen < (ssize_t)(BOOTP_MIN_LEN + sizeof(struct ether_header)))
return (-1);
@@ -76,6 +77,15 @@ dhcp_request(struct vionet_dev *dev, cha
if (req.ciaddr.s_addr != 0 || req.file[0] != '\0' || req.hops != 0)
return (-1);
+ for (o = DHCP_OPTIONS_COOKIE_LEN;
+ o + offsetof(struct dhcp_packet, options) < buflen &&
+ req.options[o] != DHO_END;
+ o += req.options[o+1] + 2)
+ if (req.options[o] == DHO_DHCP_MESSAGE_TYPE) {
+ is_discover = (req.options[o+2] == DHCPDISCOVER);
+ break;
+ }
+
memset(, 0, sizeof(resp));
resp.op = BOOTREPLY;
resp.htype = req.htype;
@@ -123,6 +133,10 @@ dhcp_request(struct vionet_dev *dev, cha
memcpy(,
DHCP_OPTIONS_COOKIE, DHCP_OPTIONS_COOKIE_LEN);
o+= DHCP_OPTIONS_COOKIE_LEN;
+
+ resp.options[o++] = DHO_DHCP_MESSAGE_TYPE;
+ resp.options[o++] = 1;
+ resp.options[o++] = is_discover ? DHCPOFFER : DHCPACK;
resp.options[o++] = DHO_SUBNET_MASK;
resp.options[o++] = sizeof(mask);
rc(8): Fix motd update
The man page for motd(5) says that all lines up to (but not including) the first line of /etc/motd are replaced by the kernel version at startup. But if the first line is blank the rc script ignores it and instead deletes everything between the first blank line and the second. This diff fixes that behavior. Index: etc/rc === RCS file: /cvs/src/etc/rc,v retrieving revision 1.493 diff -u -p -r1.493 rc --- etc/rc 26 Feb 2017 16:51:18 - 1.493 +++ etc/rc 11 Apr 2017 20:51:24 - @@ -505,7 +505,7 @@ fi if T=$(mktemp /tmp/_motd.XX); then sysctl -n kern.version | sed 1q >$T echo "" >>$T - sed '1,/^$/d' >$T + sed '0,/^$/d' >$T cmp -s $T /etc/motd || cp $T /etc/motd rm -f $T fi
Invalid references in ncurses(3) man page
The ncurses(3) and curs_util(3) man pages reference some manual pages that do not exist. $ man 3 curs_memleaks man: No entry for curs_memleak in the manual $ man 3 curs_trace man: No entry for curs_trace in the manual $ man 3 legacy_coding man: No entry for legacy_coding in the manual. This proposed diff simply excises the faulty references. This might not be the correct thing, particularly for the line about use_legacy_coding (which is at least described in curs_util(3) even if it is not listed at the top of that page). The other functions listed in curses.3tbl do not appear to be described in any extant man pages, however. Index: lib/libcurses/curs_util.3 === RCS file: /open/anoncvs/cvs/src/lib/libcurses/curs_util.3,v retrieving revision 1.6 diff -u -p -r1.6 curs_util.3 --- lib/libcurses/curs_util.3 12 Jan 2010 23:21:59 - 1.6 +++ lib/libcurses/curs_util.3 13 Sep 2016 15:18:22 - @@ -246,12 +246,10 @@ It was not supported on Version 7, BSD o It is recommended that any code depending on ncurses extensions be conditioned using NCURSES_VERSION. .SH SEE ALSO -\fBlegacy_coding\fR(3), \fBcurses\fR(3), \fBcurs_initscr\fR(3), \fBcurs_kernel\fR(3), \fBcurs_scr_dump\fR(3), -\fBlegacy_coding\fR(3). .\"# .\"# The following sets edit modes for GNU EMACS .\"# Local Variables: Index: lib/libcurses/curses.3tbl === RCS file: /open/anoncvs/cvs/src/lib/libcurses/curses.3tbl,v retrieving revision 1.25 diff -u -p -r1.25 curses.3tbl --- lib/libcurses/curses.3tbl 3 Dec 2015 11:29:55 - 1.25 +++ lib/libcurses/curses.3tbl 13 Sep 2016 15:32:11 - @@ -288,17 +288,6 @@ l l . = COLOR_PAIR/\fBcurs_color\fR(3) PAIR_NUMBER/\fBcurs_attr\fR(3) -_nc_free_and_exit/\fBcurs_memleaks\fR(3)* -_nc_freeall/\fBcurs_memleaks\fR(3)* -_nc_tracebits/\fBcurs_trace\fR(3)* -_traceattr/\fBcurs_trace\fR(3)* -_traceattr2/\fBcurs_trace\fR(3)* -_tracechar/\fBcurs_trace\fR(3)* -_tracechtype/\fBcurs_trace\fR(3)* -_tracechtype2/\fBcurs_trace\fR(3)* -_tracedump/\fBcurs_trace\fR(3)* -_tracef/\fBcurs_trace\fR(3)* -_tracemouse/\fBcurs_trace\fR(3)* add_wch/\fBcurs_add_wch\fR(3) add_wchnstr/\fBcurs_add_wchstr\fR(3) add_wchstr/\fBcurs_add_wchstr\fR(3) @@ -628,7 +617,6 @@ untouchwin/\fBcurs_touch\fR(3) use_default_colors/\fBdefault_colors\fR(3)* use_env/\fBcurs_util\fR(3) use_extended_names/\fBcurs_extend\fR(3)* -use_legacy_coding/\fBlegacy_coding\fR(3)* vid_attr/\fBterminfo\fR(3) vid_puts/\fBterminfo\fR(3) vidattr/\fBterminfo\fR(3)
ksh(1): manual says $PPID is read-only
The ksh(1) manual says that PPID should be read-only. But:
$ man ksh | grep PPID
PPID The process ID of the shell's parent (read-only).
$ echo ${PPID}
5967
$ PPID=123
$ echo ${PPID}
123
We can fix either the manual or ksh itself; this diff takes the latter
approach. It is tempting to do this with "typeset -ir PPID" but that
actually doesn't work:
$ FOO=123
$ typeset -ir FOO
ksh: FOO: is read only
So we fix this by putting "typeset -r PPID" on its own line. One could
imagine shortening the section below with something like:
"typeset", "-i", "PPID", "OPTIND=1", NULL,
"typeset", "-r", "KSH_VERSION", "PPID", NULL,
"typeset", "-x", "SHELL", "PATH", "HOME", NULL,
but that involves some reordering: the -i's MUST come before the -r's.
Index: bin/ksh/main.c
===
RCS file: /open/anoncvs/cvs/src/bin/ksh/main.c,v
retrieving revision 1.79
diff -u -p -r1.79 main.c
--- main.c 4 Mar 2016 15:11:06 - 1.79
+++ main.c 8 Sep 2016 13:17:29 -
@@ -85,6 +85,7 @@ static const char *initcoms [] = {
"typeset", "-r", "KSH_VERSION", NULL,
"typeset", "-x", "SHELL", "PATH", "HOME", NULL,
"typeset", "-i", "PPID", NULL,
+ "typeset", "-r", "PPID", NULL,
"typeset", "-i", "OPTIND=1", NULL,
"eval", "typeset -i RANDOM MAILCHECK=\"${MAILCHECK-600}\"
SECONDS=\"${SECONDS-0}\" TMOUT=\"${TMOUT-0}\"", NULL,
"alias",
Re: Improve error message in rcctl(8)
Regarding Jiri's suggestion: Here is a diff that makes
`rcctl ls all' only list executable files with valid service
names.
This diff also fixes two problems with my original submission:
1. The use of `[' instead of `[[' causes filename expansion to
take place on the right-hand side of the comparison; you get
different results depending on which directory you're sitting
in when you test. I have switched to `[[' to fix that problem.
2. There was a stray closing brace that somehow did not cause
problems in testing.
Index: usr.sbin/rcctl/rcctl.sh
===
RCS file: /open/anoncvs/cvs/src/usr.sbin/rcctl/rcctl.sh,v
retrieving revision 1.104
diff -u -p -r1.104 rcctl.sh
--- usr.sbin/rcctl/rcctl.sh 30 Jul 2016 06:25:21 - 1.104
+++ usr.sbin/rcctl/rcctl.sh 6 Sep 2016 20:03:33 -
@@ -53,8 +53,8 @@ ls_rcscripts()
cd /etc/rc.d && set -- *
for _s; do
- [[ ${_s} = *.* ]] && continue
- [ ! -d "${_s}" ] && echo "${_s}"
+ [[ "${_s}" != +([_0-9A-Za-z]) ]] && continue
+ [ -x "${_s}" ] && echo "${_s}"
done
}
@@ -182,7 +182,7 @@ svc_is_meta()
svc_is_special()
{
local _svc=$1
- [ -n "${_svc}" ] || return
+ [[ "${_svc}" = +([_0-9A-Za-z]) ]] || return
local _cached _ret
Re: Improve error message in rcctl(8)
Antoine writes:
> What about this?
> + [[ "${_svc}" == +([_/+[:alnum:]]) ]] || return
That doesn't fix the problem. You cannot use plus signs or slashes in a
service name because the options to a service are set in rc.conf.local
with the line
foo_flags=""
where `foo' is replaced by the service name. The ksh man page states
that only letters, numbers, and underscores can be used in variable
names.
Ludovic writes:
> If people are using daemon named like fastcgi.exemple.com, this will
> break there config.
They cannot use that name anyway, for the same reason listed above.
Service names are already restricted to letters, digits, and
underscores. The only thing we can do here is validate user input to
the rcctl command.
Anthony
Improve error message in rcctl(8)
Sometimes when I restart a service after changing its configuration file
I accidentally type:
# rcctl restart smtpd.conf
/usr/sbin/rcctl: ${cached_svc_is_special_smtpd.conf}: bad substitution
/usr/sbin/rcctl[556]: set: cached_svc_is_special_smtpd.conf: is not an
identifier
rcctl: service smtpd.conf does not exist
The message about a bad substitution is not helpful to the user, who
only needs to know that smtpd.conf is not a service.
The problem is the period in "smtpd.conf". Line 189 of rcctl fails:
_cached=$(eval print \${cached_svc_is_special_${_svc}})
Special service names are thus limited to underscores and alphanumerics
because they're concatenated into shell variable names. So instead of
checking for [ -n ${_svc} ] at the top of svc_is_special, we ought to
check that ${_svc} contains only legal characters.
I check only in svc_is_special and not in any of the other places that
test [ -n ${_svc} ] my only goal is to fix the error message people get
when they try to start or enable configuration files, and this is the
only place that needs the error. Adding a similar check to svc_is_avail
would block an error message when someone creates an executable file
called /etc/rc.d/foo.bar and then calls "rcctl enable foo.bar", but in
that case I think the message "${foo.bar_flags}: bad substitution" is
more helpful---the user is trying to create a service with an illegal
name and the system is telling him why it will never work.
Regards,
Anthony
Index: usr.sbin/rcctl/rcctl.sh
===
RCS file: /open/anoncvs/cvs/src/usr.sbin/rcctl/rcctl.sh,v
retrieving revision 1.104
diff -u -p -u -r1.104 rcctl.sh
--- usr.sbin/rcctl/rcctl.sh 30 Jul 2016 06:25:21 - 1.104
+++ usr.sbin/rcctl/rcctl.sh 6 Sep 2016 15:07:47 -
@@ -182,7 +182,7 @@ svc_is_meta()
svc_is_special()
{
local _svc=$1
- [ -n "${_svc}" ] || return
+ [ "${_svc}" = +([_0-9A-Za-z])} ] || return
local _cached _ret
Re: Make rc scripts use [[ instead of [
> You don't explain why `print' is more efficient than `echo'. Short answer: I'm an idiot who didn't realize that `echo' and `[' are shell builtins. The only parts of my change that still seem worthwhile to me now are the ones related to the awkward X-comparisons, but there are few enough of those and they're handled in an idiomatic way, so I'm not going to bother pushing it. Anybody who has real work to do can stop reading the email now. The advantage of continuing to read is that you get some insight into the psychology of a naive user. Long answer (written partly in a pointless attempt to save face, and partly to show appreciation to Jeremie's non-condescending response to what turned out to be a stupid suggestion): While I vaguely remember reading long ago that `echo' and `[' were shell builtins, I'm much more strongly aware that /bin/echo and /bin/[ are also available as separate executables. Their man pages don't mention that they have builtin ksh implementations, and I'm very aware that `[[' parses arguments differently from `[', e.g. $ foo="two words" $ [[ $foo = "two words" ]] && echo True || echo False True $ [ $foo = "two words" ] && echo True || echo False ksh: [: words: unexpected operator/operand False which I interpreted as further evidence that `[' and `[[' were fundamentally different in implementation. Of course, this is wrong, as "type [" and "type echo" prove. And I remember now that /bin/echo and /bin/[ are available as separate executables for POSIX reasons, and that their man pages don't cover the behavior of ksh because they *aren't* ksh. The point of all of this is that I made an assumption that seemed reasonable and while I checked some things in the manual, there were some things I didn't even think to check because they seemed so obvious. Oops. That happened in another way: > Nope, ''set -o sh'' sets strict mode on, ''set +o sh'' disables it. I checked to make sure that "sh" stood for strict mode and not unstrict mode (since I couldn't understand how that comment could be so flagrantly wrong), but it never occurred to me to check that `-' enables something and `+' disables it. Oops again. In any event, my main goal was to keep us from spawning needless processes, and my first draft of the change was pretty much just replacing `[' with `[[' and `echo' with `print.' But since I already had to change things like [ "$foo" ] to [[ -n "$foo" ]] it seemed wasteful not to take advantage of the improved argument-parsing of the `[[' operator and remove the quotation marks, to get [[ -n $foo ]]. But if I'm doing it in some places, I should do it in all places. So a lot of quotation marks went away to take advantage of the improved parsing characteristics of `[['. Some of my other "readability improvements" felt a little awkward and I couldn't find any reference to shell scripts in style(9) (which I *did* check before mailing in the proposed change), so I was careful to make them only in places where I was already breaking stuff. Replacing `` with $() was an example of that---I left backticks in place in yppasswdd when I didn't already have a "legitimate" reason to touch the line, and then I made a point to state explicitly what I had done to see if anyone would comment on it. Which you did. (I won't try to rationalize my change to numeric comparisons, since as you say it was a personal preference, and it was also fairly dubious.) There's one more specific point I want to make while I'm here about the awkward X-comparisons: if you search for the phrase "A common mistake" in the ksh manual, you'll find an example of a comparison that fails when a variable is unset. Oddly, it only works when the "if" keyword is used; the "[ foo ] && do_something" construct does not have any problems. So, this is the last you'll hear from me on this proposal---it was a bad idea from the start, and if I had typed "whence -v [" from the beginning I wouldn't have bothered. But overall I think my misunderstanding was reasonable, and my "needless churn" would have been better-received if the underlying reason for the change had been valid. And finally, I would again like to thank you (Jeremie) for a thorough and non-condescending response to a stupid idea. You assumed that I was sensible enough to read your reviewi and taught me something about the quote behavior of the eval operator---I hope this (otherwise pointless) email validates your faith that not all people with stupid ideas are themselves stupid. And sometimes we read the man pages and we still miss important things! Regards, Anthony
Make rc scripts use [[ instead of [
Some of the system scripts make inconsistent use of ksh-specific
features, specifically "print" and "[[" as efficient replacements for
"echo" and "[". This change makes the /etc/ksh.kshrc and all the rc
scripts use "print" and "[[" exclusively.
Switching from echo to print only brought up one interesting issue:
/etc/rc.d/mountd uses "print -n >foo" instead of "touch foo".
The latter is arguably more transparent but I didn't change to it.
Switching from [ to [[ makes a lot of things more readable; most of
the improvements result from removing unnecessary quotation marks.
We also don't have to test for unset variables as aggressively; a
number of awkward [ X"$foo" = XYes ] constructions were replaced
with [[ $foo = Yes ]] .
The test [[ ${_args} = "${_args# }" ]] appears in rcctl; I left the
quotation marks on the right-hand side because I'm afraid of breaking
something I don't understand.
The rcctl script also has a lot of lines like:
daemon_timeout="$(svc_getdef ${_svc} timeout)"
I think the quotation marks are unnecessary but the original author
went to a lot of trouble to escape internal quotes, e.g.
daemon_flags="$(eval print \"\${${_svc}_flags}\")"
so I'll give him the benefit of the doubt. I would encourage somone
with greater confidence to take a closer look.
I replaced `backticks` with $(subshell notation) in a few places, but
only in places where I was already changing [ to [[. I wanted to remove
them all but I can't really justify the change in terms of either
readability or process efficiency.
Regarding numeric changes: in ksh, [[ 0 -eq 00 ]] is true (numeric
comparison) but [[ 0 = 00 ]] is false (string comparison). The latter
is more readable, so I made the switch in places where the left-hand
side is a system output, e.g. [[ $(id -u ) = 0 ]] or [[ $# > 0 ]].
I would not expect id(1) to print the string "00" for root, nor would
I expect the shell to print "00" for $#. But I did *not* change
comparison operators in more complicated situations, like the bottom
of /etc/rc which has [[ ${daemon_rtable} -eq 0 ]]. It isn't immediately
obvious what sort of string is in ${daemon_rtable} so the numeric
comparison seems appropriate here.
I corrected a comment at the top of /etc/rc; it said that "set +o sh"
turned Strict Bourne shell mode off. I'm pretty sure it's actually
turning strict mode on.
My system still boots with these changes, and rcctl still appears to
work. I can't easily test all of the code paths in each of the rc
scripts. But the changes involved here are straightforward, and you
can validate this diff line-by-line.
Index: etc/ksh.kshrc
===
RCS file: /open/anoncvs/cvs/src/etc/ksh.kshrc,v
retrieving revision 1.20
diff -u -p -u -r1.20 ksh.kshrc
--- etc/ksh.kshrc 18 Feb 2015 08:39:32 - 1.20
+++ etc/ksh.kshrc 5 Sep 2016 04:03:43 -
@@ -62,7 +62,7 @@ case "$-" in
case "$TERM" in
sun*-s)
# sun console with status line
- if [ "$tty" != "$console" ]; then
+ if [[ $tty != $console ]]; then
# ilabel
ILS='\033]L'; ILE='\033\\'
# window title bar
@@ -81,7 +81,7 @@ case "$-" in
*) ;;
esac
# do we want window decorations?
- if [ "$ILS" ]; then
+ if [[ -n $ILS ]]; then
function ilabel { print -n "${ILS}$*${ILE}">/dev/tty; }
function label { print -n "${WLS}$*${WLE}">/dev/tty; }
@@ -136,14 +136,14 @@ function no_path {
}
# if $1 exists and is not in path, append it
function add_path {
- [ -d ${1:-.} ] && no_path $* && eval ${2:-PATH}="\$${2:-PATH}:$1"
+ [[ -d ${1:-.} ]] && no_path $* && eval ${2:-PATH}="\$${2:-PATH}:$1"
}
# if $1 exists and is not in path, prepend it
function pre_path {
- [ -d ${1:-.} ] && no_path $* && eval ${2:-PATH}="$1:\$${2:-PATH}"
+ [[ -d ${1:-.} ]] && no_path $* && eval ${2:-PATH}="$1:\$${2:-PATH}"
}
# if $1 is in path, remove it
function del_path {
- no_path $* || eval ${2:-PATH}=`eval echo :'$'${2:-PATH}: |
+ no_path $* || eval ${2:-PATH}=`eval print :'$'${2:-PATH}: |
sed -e "s;:$1:;:;g" -e "s;^:;;" -e "s;:\$;;"`
}
Index: etc/rc
===
RCS file: /open/anoncvs/cvs/src/etc/rc,v
retrieving revision 1.486
diff -u -p -u -r1.486 rc
--- etc/rc 10 Jul 2016 09:08:18 - 1.486
+++ etc/rc 4 Sep 2016 14:26:10 -
@@ -4,7 +4,7 @@
# Output and error are redirected to console by init, and the console is the
# controlling terminal.
-# Turn off Strict Bourne shell.
+# Turn on Strict Bourne shell.
set +o sh
# Subroutines (have to come first).
@@ -137,14 +137,14 @@ make_keys() {
local _iked_pub=/etc/iked/local.pub
if [[ ! -f $_isakmpd_key ]]; then
- echo -n "openssl: generating isakmpd/iked RSA keys... "
+ print -n "openssl:
ksh(1): Make PPID read-only
The ksh(1) man page says that PPID should be a read-only variable.
This diff makes that happen. It does not work to combine the two
lines into "typeset -ir PPID" as the following session demonstrates:
$ NUM=42
$ typeset -ir NUM
ksh: NUM: is read only
Curiously, I did this right the first time and tested it, but before
submitting it yesterday I changed it to the incorrect typeset -ir and
apparently failed to retest. I also submitted it from the command line
with the title "ksh ${PPID} should be read-only" and my shell helpfully
replaced ${PPID} with 641.
This change works correctly and hopefully has a correct subject line.
Index: main.c
===
RCS file: /cvs/src/bin/ksh/main.c,v
retrieving revision 1.79
diff -u -p -r1.79 main.c
--- main.c 4 Mar 2016 15:11:06 - 1.79
+++ main.c 28 May 2016 20:37:25 -
@@ -85,6 +85,7 @@ static const char *initcoms [] = {
"typeset", "-r", "KSH_VERSION", NULL,
"typeset", "-x", "SHELL", "PATH", "HOME", NULL,
"typeset", "-i", "PPID", NULL,
+ "typeset", "-r", "PPID", NULL,
"typeset", "-i", "OPTIND=1", NULL,
"eval", "typeset -i RANDOM MAILCHECK=\"${MAILCHECK-600}\"
SECONDS=\"${SECONDS-0}\" TMOUT=\"${TMOUT-0}\"", NULL,
"alias",
Pledge failure in nc(1)
When nc(1) tries to connect through an HTTP proxy that requires
authentication, nc calls readpassphrase(3) and aborts. Pledging "tty"
fixes this problem, but you'll notice that the diff has a lot of nasty
branches. My failure to check Pflag when connecting over unix sockets
is not an oversight; nc does not support that configuration.
To reproduce the failure without setting up a real HTTP proxy, open
two terminals and run nc as a coprocess in the first. The following
session causes a core dump:
(tty1)$ nc -lk 8080 |&
(tty2)$ nc -Xconnect -xlocalhost:8080 -Puser localhost 8081
(tty1)$ print -np "HTTP/1.0 407 Authentication Required\r\n\r\n"
(tty2) Abort trap (core dumped)
Index: netcat.c
===
RCS file: /cvs/src/usr.bin/nc/netcat.c,v
retrieving revision 1.150
diff -u -p -r1.150 netcat.c
--- netcat.c4 Jan 2016 02:18:31 - 1.150
+++ netcat.c28 May 2016 18:33:30 -
@@ -323,7 +323,13 @@ main(int argc, char *argv[])
if (pledge("stdio rpath wpath cpath tmppath unix", NULL) == -1)
err(1, "pledge");
} else if (Fflag) {
- if (pledge("stdio inet dns sendfd", NULL) == -1)
+ if (Pflag) {
+ if (pledge("stdio inet dns sendfd tty", NULL) == -1)
+ err(1, "pledge");
+ } else if (pledge("stdio inet dns sendfd", NULL) == -1)
+ err(1, "pledge");
+ } else if (Pflag) {
+ if (pledge("stdio inet dns tty", NULL) == -1)
err(1, "pledge");
} else if (usetls) {
if (pledge("stdio rpath inet dns", NULL) == -1)
@@ -434,7 +440,10 @@ main(int argc, char *argv[])
if (Kflag && (privkey = tls_load_file(Kflag, ,
NULL)) == NULL)
errx(1, "unable to load TLS key file %s", Kflag);
- if (pledge("stdio inet dns", NULL) == -1)
+ if (Pflag) {
+ if (pledge("stdio inet dns tty", NULL) == -1)
+ err(1, "pledge");
+ } else if (pledge("stdio inet dns", NULL) == -1)
err(1, "pledge");
if (tls_init() == -1)
Re: Pledge failure in disklabel(8)
Oops, I did not check the return value of fstat. Good catch.
But when I tested my change without yours, disklabel did not abort. Why
then does opendev need to occur before the pledge? Is there another
usage of disklabel that causes a different failure pattern?
On 05/28/2016 11:31 AM, Theo de Raadt wrote:
If you try to run disklabel(8) on a file that is not a device, it aborts
aborts for want of pledge("ioctl"). This diff prints an error message
and exits cleanly. I return exit code 1 but note that sometimes
disklabel returns 4; the man page doesn't explain the distinction
anywhere.
$ disklabel /
Abort trap (core dumped)
$ obj/disklabel /
disklabel: / is not a device
Indeed your diff is also needed on top of mine. Let's try this.
Pledge failure in disklabel(8)
If you try to run disklabel(8) on a file that is not a device, it aborts
aborts for want of pledge("ioctl"). This diff prints an error message
and exits cleanly. I return exit code 1 but note that sometimes
disklabel returns 4; the man page doesn't explain the distinction
anywhere.
$ disklabel /
Abort trap (core dumped)
$ obj/disklabel /
disklabel: / is not a device
Index: disklabel.c
===
RCS file: /cvs/src/sbin/disklabel/disklabel.c,v
retrieving revision 1.214
diff -u -p -r1.214 disklabel.c
--- disklabel.c 25 Nov 2015 17:17:38 - 1.214
+++ disklabel.c 27 May 2016 15:13:25 -
@@ -119,6 +119,7 @@ main(int argc, char *argv[])
int ch, f, error = 0;
FILE *t;
char *autotable = NULL;
+ struct stat sb;
getphysmem();
@@ -211,6 +212,9 @@ main(int argc, char *argv[])
);
if (f < 0)
err(4, "%s", specname);
+ fstat(f, );
+ if (!S_ISBLK(sb.st_mode) && !S_ISCHR(sb.st_mode))
+ errx(1, "%s is not a device", dkname);
if (autotable != NULL)
parse_autotable(autotable);
Re: ksh 641 should be read-only
> ksh won't start after this change. The problem is that it will try to
> assign the return value of getppid() to PPID *after* PPID was made read
> only.
Such is the price of not testing last-minute changes. When ksh
processes a typeset command, it sets the read-only flag before
attempting to convert the string to an integer. We can reproduce
the issue like so:
$ NUM=42
$ typeset -ir NUM
ksh: NUM: is read only
We can get around this problem by separating out the two lines.
Index: main.c
===
RCS file: /cvs/src/bin/ksh/main.c,v
retrieving revision 1.79
diff -u -p -r1.79 main.c
--- main.c 4 Mar 2016 15:11:06 - 1.79
+++ main.c 27 May 2016 13:18:01 -
@@ -85,6 +85,7 @@ static const char *initcoms [] = {
"typeset", "-r", "KSH_VERSION", NULL,
"typeset", "-x", "SHELL", "PATH", "HOME", NULL,
"typeset", "-i", "PPID", NULL,
+ "typeset", "-r", "PPID", NULL,
"typeset", "-i", "OPTIND=1", NULL,
"eval", "typeset -i RANDOM MAILCHECK=\"${MAILCHECK-600}\"
SECONDS=\"${SECONDS-0}\" TMOUT=\"${TMOUT-0}\"", NULL,
"alias",
Spelling errors
The long line in asr.c barely squeaks in at 79 characters. Index: lib/libtls/tls_init.3 === RCS file: /cvs/src/lib/libtls/tls_init.3,v retrieving revision 1.59 diff -u -p -r1.59 tls_init.3 --- lib/libtls/tls_init.3 28 Apr 2016 18:27:51 - 1.59 +++ lib/libtls/tls_init.3 27 May 2016 03:42:58 - @@ -421,7 +421,7 @@ can only succeed after the handshake is .Fn tls_peer_cert_contains_name checks if the peer of a TLS .Ar ctx -has povided a certificate that contains a +has provided a certificate that contains a SAN or CN that matches .Ar name . .Fn tls_peer_cert_contains_name Index: lib/libc/asr/asr.c === RCS file: /cvs/src/lib/libc/asr/asr.c,v retrieving revision 1.51 diff -u -p -r1.51 asr.c --- lib/libc/asr/asr.c 24 Feb 2016 20:52:53 - 1.51 +++ lib/libc/asr/asr.c 27 May 2016 03:42:58 - @@ -700,7 +700,7 @@ asr_ctx_parse(struct asr_ctx *ac, const /* * Check for environment variables altering the configuration as described - * in resolv.conf(5). Altough not documented there, this feature is disabled + * in resolv.conf(5). Although not documented there, this feature is disabled * for setuid/setgid programs. */ static void Index: usr.sbin/bind/lib/isc/include/isc/hash.h === RCS file: /cvs/src/usr.sbin/bind/lib/isc/include/isc/hash.h,v retrieving revision 1.3 diff -u -p -r1.3 hash.h --- usr.sbin/bind/lib/isc/include/isc/hash.h9 Dec 2007 13:39:44 - 1.3 +++ usr.sbin/bind/lib/isc/include/isc/hash.h27 May 2016 03:42:58 - @@ -36,7 +36,7 @@ * in the random vector are unpredictable, the probability of hash * collision between arbitrary two different values is at most 1/2^16. * - * Altough the API is generic about the hash keys, it mainly expects + * Although the API is generic about the hash keys, it mainly expects * DNS names (and sometimes IPv4/v6 addresses) as inputs. It has an * upper limit of the input length, and may run slow to calculate the * hash values for large inputs.
ksh 641 should be read-only
The man page for ksh says that ${PPID} should be read-only but
apparently it is not. There is also a line nearby that is more
than 80 characters long but I'm not comfortable enough with KNF
to select the best way to fix that.
Index: main.c
===
RCS file: /cvs/src/bin/ksh/main.c,v
retrieving revision 1.79
diff -u -p -r1.79 main.c
--- main.c 4 Mar 2016 15:11:06 - 1.79
+++ main.c 27 May 2016 02:37:41 -
@@ -84,7 +84,7 @@ static const char initsubs[] = "${PS2=>
static const char *initcoms [] = {
"typeset", "-r", "KSH_VERSION", NULL,
"typeset", "-x", "SHELL", "PATH", "HOME", NULL,
- "typeset", "-i", "PPID", NULL,
+ "typeset", "-ir", "PPID", NULL,
"typeset", "-i", "OPTIND=1", NULL,
"eval", "typeset -i RANDOM MAILCHECK=\"${MAILCHECK-600}\"
SECONDS=\"${SECONDS-0}\" TMOUT=\"${TMOUT-0}\"", NULL,
"alias",
