Re: grep: add --null flag

2021-01-24 Thread Ted Unangst
On 2021-01-25, Sebastian Benoit wrote:
> Sebastian Benoit(be...@openbsd.org) on 2021.01.25 00:27:05 +0100:
> > Theo de Raadt(dera...@openbsd.org) on 2021.01.24 16:01:32 -0700:
> > > Stuart Henderson  wrote:
> > > 
> > > > On 2021/01/24 12:10, Theo de Raadt wrote:
> > > > > I completely despise that the option is called "--null".
> > > > > 
> > > > > Someone was a complete idiot.
> > > > 
> > > > gnu grep has both --null and -z for this (why do they do that?!).
> > > > If it's added as --null it should be added as -z too.
> > > > 
> > > > Looking at Debian codesearch most things using it as --null use other
> > > > long options that we don't have. Maybe just adding as -z would be
> > > > enough. It does seem a useful and fairly widely supported feature.
> > > 
> > > Yes, maybe just add -z.
> > 
> > Actually it's "-Z, --null". The lowercase -z in gnu grep is
> > 
> >-z, --null-data
> >   Treat input and output data as sequences of lines, each
> >   terminated by a zero byte (the ASCII NUL character) instead of
> >   a newline.  Like the -Z or --null option, this option can be
> >   used with commands like sort-z to process arbitrary file
> >   names.
> 
> And we already have -z for "force grep to behave as zgrep".
> 
> Diff below with tedu@ suggestion and changed manpage text.

What happened to -Z?



> 
> > 
> > Note that we have the -z in sort(1), which at least is consistent.
> > That also is a non-posix extension. 
> >  
> > > > Should have been -0 to match xargs and be similar to find's -print0
> > > > but it's too late for that now.
> > > 
> > > Yes it should have been -0.
> > > 
> > > But unfortunately some an uneducated idiot got involved.  None of this
> > > is standardized.  Unix script portability is being ruined by idiots, not
> > > just the people proposing it or writing it originally, but also the people
> > > who don't say "wrong" quickly enough.  And much of this is because of
> > > intentional development silos.
> 
> diff --git usr.bin/grep/grep.1 usr.bin/grep/grep.1
> index 5cc228df222..e1edae7e432 100644
> --- usr.bin/grep/grep.1
> +++ usr.bin/grep/grep.1
> @@ -49,6 +49,7 @@
>  .Op Fl -context Ns Op = Ns Ar num
>  .Op Fl -label Ns = Ns Ar name
>  .Op Fl -line-buffered
> +.Op Fl -null
>  .Op Ar pattern
>  .Op Ar
>  .Ek
> @@ -297,6 +298,16 @@ instead of the filename before lines.
>  Force output to be line buffered.
>  By default, output is line buffered when standard output is a terminal
>  and block buffered otherwise.
> +.It Fl -null
> +Output a zero byte instead of the character that normally follows a
> +file name.
> +This option makes the output unambiguous, even in the presence of file
> +names containing unusual characters like newlines, making the output
> +suitable for use with the
> +.Fl 0
> +option to
> +.Xr xargs 1 .
> +This option is a non-POSIX extension and may not be portable.
>  .El
>  .Sh EXIT STATUS
>  The
> diff --git usr.bin/grep/grep.c usr.bin/grep/grep.c
> index f41b5e20ca6..279d949fae7 100644
> --- usr.bin/grep/grep.c
> +++ usr.bin/grep/grep.c
> @@ -80,6 +80,7 @@ int  vflag; /* -v: only show non-matching lines */
>  int   wflag; /* -w: pattern must start and end on word boundaries */
>  int   xflag; /* -x: pattern must match entire line */
>  int   lbflag;/* --line-buffered */
> +int   nullflag;  /* --null */
>  const char *labelname;   /* --label=name */
>  
>  int binbehave = BIN_FILE_BIN;
> @@ -89,6 +90,7 @@ enum {
>   HELP_OPT,
>   MMAP_OPT,
>   LINEBUF_OPT,
> + NULL_OPT,
>   LABEL_OPT,
>  };
>  
> @@ -134,6 +136,7 @@ static const struct option long_options[] =
>   {"mmap",no_argument,NULL, MMAP_OPT},
>   {"label",   required_argument,  NULL, LABEL_OPT},
>   {"line-buffered",   no_argument,NULL, LINEBUF_OPT},
> + {"null",no_argument,NULL, NULL_OPT},
>   {"after-context",   required_argument,  NULL, 'A'},
>   {"before-context",  required_argument,  NULL, 'B'},
>   {"context", optional_argument,  NULL, 'C'},
> @@ -436,6 +439,9 @@ main(int argc, char *argv[])
>   case LINEBUF_OPT:
>   lbflag = 1;
>   break;
> + case NULL_OPT:
> + nullflag = 1;
> + break;
>   case HELP_OPT:
>   default:
>   usage();
> diff --git usr.bin/grep/grep.h usr.bin/grep/grep.h
> index b3d24ae662b..37e295d4d40 100644
> --- usr.bin/grep/grep.h
> +++ usr.bin/grep/grep.h
> @@ -68,7 +68,7 @@ extern int   cflags, eflags;
>  extern intAflag, Bflag, Eflag, Fflag, Hflag, Lflag,
>Rflag, Zflag,
>bflag, cflag, hflag, iflag, lflag, mflag, nflag, oflag, qflag,
> -  sflag, vflag, wflag, xflag;
> +  sflag, vflag, wflag, xflag, nullflag;
>  extern 

Re: grep: add --null flag

2021-01-24 Thread Ted Unangst
On 2021-01-22, Omar Polo wrote:
> 
> quasi three-weekly ping.
> 
> Is this such a bad idea?

This seems reasonable. I think there's no need to change print line calls 
though, just patch the implementation once.

> 
> (TBH: I have still to look at how to write a regression test for this)
> 
> Omar Polo  writes:
> 
> > Hello,
> >
> > There are various program, especially the one written with GNU grep in
> > mind, that expects various flags that grep in base doesn't have.  While
> > some of the flags (like --color) can be easily worked out (i.e. by
> > patching/customising these programs) one thing that it isn't easily
> > workable is --null, because it alters the way grep outputs its data.
> >
> > --null makes grep output an ASCII NUL byte after the file name, so a
> > program can parse the output of grep unambiguously even when the file
> > names contains "funny" characters, such as a newline.
> >
> > GNU grep isn't the only one with a --null flag, also FreeBSD and NetBSD
> > grep do (at least by looking at their manpages), so it's somewhat
> > widespread.
> >
> > I searched the archives on marc.info but I haven't seen a previous
> > discussion about this.
> >
> > The following patch was tried against GNU grep (installed from packages)
> > and seem to behave consistently.
> >
> > I used the same text of the FreeBSD/NetBSD manpage for the description
> > of the --null option, but I really dislike it: it feels way to verbose
> > for what it's trying to say, but I wasn't able to come up with something
> > better.
> >
> > I'm not familiar at all with the grep codebase, so I hope I'm not
> > missing something.  If this has some chances of being accepted, I guess
> > I should also add some regress test; I'll try to work on them soon, but
> > in the meantime I'm sending this.
> >
> > Thanks,
> >
> > Omar Polo
> 
> 
> diff e992327fc31d0277a6f8518613a7db1b9face78b /home/op/w/openbsd-src
> blob - 5cc228df222c54a0553f289b5da8bbbe6afd171e
> file + usr.bin/grep/grep.1
> --- usr.bin/grep/grep.1
> +++ usr.bin/grep/grep.1
> @@ -49,6 +49,7 @@
>  .Op Fl -context Ns Op = Ns Ar num
>  .Op Fl -label Ns = Ns Ar name
>  .Op Fl -line-buffered
> +.Op Fl -null
>  .Op Ar pattern
>  .Op Ar
>  .Ek
> @@ -297,6 +298,25 @@ instead of the filename before lines.
>  Force output to be line buffered.
>  By default, output is line buffered when standard output is a terminal
>  and block buffered otherwise.
> +.It Fl -null
> +Output a zero byte (the ASCII NUL character) instead of the character
> +that normally follows a file name.
> +For example,
> +.Nm Fl l Fl -null
> +outputs a zero byte after each file name instead of the usual newline.
> +This option makes the output unambiguous, even in the presence of file
> +names containing unusual characters like newlines.
> +This option can be used with commands like
> +.Xr find 1
> +.Fl print0 Ns ,
> +.Xr perl 1
> +.Fl 0 Ns ,
> +.Xr sort 1
> +.Fl z Ns , and
> +.Xr args 1
> +.Fl 0
> +to process arbitrary file names, even those that contain newline
> +characters.
>  .El
>  .Sh EXIT STATUS
>  The
> blob - f41b5e20ca68c9e9a36d2f7dd3c44329c621f29b
> file + usr.bin/grep/grep.c
> --- usr.bin/grep/grep.c
> +++ usr.bin/grep/grep.c
> @@ -80,6 +80,7 @@ int  vflag; /* -v: only show non-matching lines */
>  int   wflag; /* -w: pattern must start and end on word boundaries */
>  int   xflag; /* -x: pattern must match entire line */
>  int   lbflag;/* --line-buffered */
> +int   nullflag;  /* --null */
>  const char *labelname;   /* --label=name */
>  
>  int binbehave = BIN_FILE_BIN;
> @@ -89,6 +90,7 @@ enum {
>   HELP_OPT,
>   MMAP_OPT,
>   LINEBUF_OPT,
> + NULL_OPT,
>   LABEL_OPT,
>  };
>  
> @@ -134,6 +136,7 @@ static const struct option long_options[] =
>   {"mmap",no_argument,NULL, MMAP_OPT},
>   {"label",   required_argument,  NULL, LABEL_OPT},
>   {"line-buffered",   no_argument,NULL, LINEBUF_OPT},
> + {"null",no_argument,NULL, NULL_OPT},
>   {"after-context",   required_argument,  NULL, 'A'},
>   {"before-context",  required_argument,  NULL, 'B'},
>   {"context", optional_argument,  NULL, 'C'},
> @@ -436,6 +439,9 @@ main(int argc, char *argv[])
>   case LINEBUF_OPT:
>   lbflag = 1;
>   break;
> + case NULL_OPT:
> + nullflag = 1;
> + break;
>   case HELP_OPT:
>   default:
>   usage();
> blob - b3d24ae662beb72c5632190c5c819bcc92f0389a
> file + usr.bin/grep/grep.h
> --- usr.bin/grep/grep.h
> +++ usr.bin/grep/grep.h
> @@ -68,7 +68,7 @@ extern int   cflags, eflags;
>  extern intAflag, Bflag, Eflag, Fflag, Hflag, Lflag,
>Rflag, Zflag,
>bflag, cflag, hflag, iflag, lflag, mflag, nflag, oflag, qflag,
> -  sflag, 

Re: doas sprinkle some more CFLAGS

2020-12-19 Thread Ted Unangst
On 2020-12-18, Martijn van Duren wrote:
> So I ended up in doas again, this time with the CFLAGS I use for most of
> my other projects. This popped up a few new not very exciting warnings.
> Diff below compiles clean with both clang and gcc on amd64.
 
>  static int
>  match(uid_t uid, gid_t *groups, int ngroups, uid_t target, const char *cmd,
> -const char **cmdargs, struct rule *r)
> +const char * const*cmdargs, struct rule *r)

That looks ugly, and I don't see the point. Const on the char prevents bugs,
but after that we're just piling on crap.



Re: doas: improve error message

2020-10-08 Thread Ted Unangst
On 2020-10-09, Klemens Nanni wrote:
> In case `cmd' and `args' in doas.conf(5) do not match, the generated
> log message is unclear and might be read as if the command executed but
> failed, i.e. returned non-zero:
> 
>   # cat /etc/doas.conf
>   permit nopass kn cmd echo args foo
>   $ doas echo foo
>   foo
>   $ doas echo bar
>   doas: Operation not permitted
> 
> The corresponding syslog(3) messages from /var/log/secure:
> 
>   Oct  9 01:05:14 eru doas: kn ran command echo foo as root from /home/kn
>   Oct  9 01:05:20 eru doas: failed command for kn: echo bar
> 
> The following reads unambiguous and better matches the EPERM wording:
> 
>   Oct  9 01:05:20 eru doas: command not permitted for kn: echo bar

ok, i think that wording was just copy/paste.



Re: apmd(8) and hw.perfpolicy quirks

2020-09-27 Thread Ted Unangst
On 2020-09-27, Jeremie Courreges-Anglas wrote:
> The diff below teaches apmd(8) -H to set hw.perfpolicy="manual" and
> hw.setperf=100, instead of setting hw.perfpolicy="high".

sure. if you would like to own this, by all means. :)



Re: apmd(8) and hw.perfpolicy quirks

2020-09-23 Thread Ted Unangst
On 2020-09-23, Jeremie Courreges-Anglas wrote:

> ok?

Seems fine.


> Note: I inlined the apmd(8)->apm(8) perfpolicy conversion for now, which
> brings a question.  I find it weird that there is a special "high"
> perfpolicy (effectively similar to perfpolicy=manual + setperf=100) but
> no "low" perfpolicy.  Is "high" useful to anyone?

If you're benchmarking or something, it's convenient to toggle between
high and auto. There's less use for low, since that's what auto defaults to.



Re: PATCH: better error return for exFAT filesystem

2020-08-09 Thread Ted Unangst
On 2020-08-09, Jonathan Gray wrote:
> mount_msdos(8) knows about EINVAL and will print "not an MSDOS filesystem"

I think mount_msdos could also inspect the filesytem for the common case of
exfat confusion.

Index: mount_msdos.c
===
RCS file: /home/cvs/src/sbin/mount_msdos/mount_msdos.c,v
retrieving revision 1.34
diff -u -p -r1.34 mount_msdos.c
--- mount_msdos.c   28 Jun 2019 13:32:45 -  1.34
+++ mount_msdos.c   9 Aug 2020 15:10:08 -
@@ -37,6 +37,7 @@
 #include 
 #include 
 #include 
+#include 
 #include 
 #include 
 #include 
@@ -58,6 +59,7 @@ gid_t a_gid(char *);
 uid_t  a_uid(char *);
 mode_t a_mask(char *);
 void   usage(void);
+intcheckexfat(const char *);
 
 int
 main(int argc, char **argv)
@@ -140,6 +142,9 @@ main(int argc, char **argv)
case EINVAL:
errcause =
"not an MSDOS filesystem";
+   if (checkexfat(dev)) {
+   errcause = "exFAT is not supported";
+   }
break;
default:
errcause = strerror(errno);
@@ -204,4 +209,20 @@ usage(void)
fprintf(stderr,
"usage: mount_msdos [-9ls] [-g gid] [-m mask] [-o options] [-u uid] 
special node\n");
exit(1);
+}
+
+int
+checkexfat(const char *dev)
+{
+   char buf[4096];
+   int fd;
+
+   fd = open(dev, O_RDONLY);
+   if (fd != -1) {
+   ssize_t amt = read(fd, buf, sizeof(buf));
+   close(fd);
+   if (amt >= 11 && memcmp(buf + 3, "EXFAT   ", 8) == 0)
+   return 1;
+   }
+   return 0;
 }



Re: disable libc sys wrappers?

2020-07-13 Thread Ted Unangst
On 2020-07-09, Theo de Raadt wrote:
> > Added a -T option to ktrace for transparency. I got ambitious here and made
> > it take suboptions, anticipating that other transparency modifications may
> > be desired.
> 
> Please don't do that.

Here is a simpler version.


Index: lib/libc/dlfcn/init.c
===
RCS file: /home/cvs/src/lib/libc/dlfcn/init.c,v
retrieving revision 1.8
diff -u -p -r1.8 init.c
--- lib/libc/dlfcn/init.c   6 Jul 2020 13:33:05 -   1.8
+++ lib/libc/dlfcn/init.c   13 Jul 2020 17:36:04 -
@@ -114,6 +114,8 @@ _libc_preinit(int argc, char **argv, cha
_timekeep->tk_version != TK_VERSION)
_timekeep = NULL;
}
+   if (issetugid() == 0 && getenv("LIBC_NOUSERTC"))
+   _timekeep = NULL;
break;
}
}
Index: usr.bin/ktrace/ktrace.1
===
RCS file: /home/cvs/src/usr.bin/ktrace/ktrace.1,v
retrieving revision 1.30
diff -u -p -r1.30 ktrace.1
--- usr.bin/ktrace/ktrace.1 15 May 2019 15:36:59 -  1.30
+++ usr.bin/ktrace/ktrace.1 13 Jul 2020 17:38:22 -
@@ -37,13 +37,13 @@
 .Nd enable kernel process tracing
 .Sh SYNOPSIS
 .Nm ktrace
-.Op Fl aBCcdi
+.Op Fl aCcdi
 .Op Fl f Ar trfile
 .Op Fl g Ar pgid
 .Op Fl p Ar pid
 .Op Fl t Ar trstr
 .Nm ktrace
-.Op Fl adi
+.Op Fl aBdiT
 .Op Fl f Ar trfile
 .Op Fl t Ar trstr
 .Ar command
@@ -109,6 +109,8 @@ processes.
 Enable (disable) tracing on the indicated process ID (only one
 .Fl p
 flag is permitted).
+.It Fl T
+Disable userland timekeeping, making time related system calls more prevalent.
 .It Fl t Ar trstr
 Select which information to put into the dump file.
 The argument can contain one or more of the following letters.
Index: usr.bin/ktrace/ktrace.c
===
RCS file: /home/cvs/src/usr.bin/ktrace/ktrace.c,v
retrieving revision 1.36
diff -u -p -r1.36 ktrace.c
--- usr.bin/ktrace/ktrace.c 28 Jun 2019 13:35:01 -  1.36
+++ usr.bin/ktrace/ktrace.c 13 Jul 2020 17:37:06 -
@@ -100,7 +100,7 @@ main(int argc, char *argv[])
usage();
}
} else {
-   while ((ch = getopt(argc, argv, "aBCcdf:g:ip:t:")) != -1)
+   while ((ch = getopt(argc, argv, "aBCcdf:g:ip:t:T")) != -1)
switch ((char)ch) {
case 'a':
append = 1;
@@ -140,6 +140,9 @@ main(int argc, char *argv[])
usage();
}
break;
+   case 'T':
+   putenv("LIBC_NOUSERTC=");
+   break;
default:
usage();
}
@@ -240,9 +243,9 @@ usage(void)
" [-u trspec] command\n",
__progname);
else
-   fprintf(stderr, "usage: %s [-aBCcdi] [-f trfile] [-g pgid]"
+   fprintf(stderr, "usage: %s [-aCcdi] [-f trfile] [-g pgid]"
" [-p pid] [-t trstr]\n"
-   "   %s [-adi] [-f trfile] [-t trstr] command\n",
+   "   %s [-aBdiT] [-f trfile] [-t trstr] command\n",
__progname, __progname);
exit(1);
 }



Re: disable libc sys wrappers?

2020-07-09 Thread Ted Unangst
On 2020-07-09, Theo de Raadt wrote:
> Hang on.  Do people ever want to force time calls, outside of ktrace?
> I doubt it.  Should ktrace maybe have a flag, similar to -B with LD_BIND_NOW,
> which sets the new environment variable?  Maybe -T?  The problem smells very
> similar to the root cause for LD_BIND_NOW setting..

So taking into account most feedback, another round.

env variable is now _LIBC_NOUSERTC. It's not libc's concern that ktrace may
want this, so I didn't go with that. The feature in question is usertc,
so that seems the most direct name.

Call getenv before issetugid.

Added a -T option to ktrace for transparency. I got ambitious here and made
it take suboptions, anticipating that other transparency modifications may
be desired. Fold in -B perhaps? I think typing -Tt is not so burdensome,
and saves us some alphabet down the line.

As a bonus, fix ktrace usage. -B only works with second synopsis, with command.


Index: lib/libc/dlfcn/init.c
===
RCS file: /home/cvs/src/lib/libc/dlfcn/init.c,v
retrieving revision 1.8
diff -u -p -r1.8 init.c
--- lib/libc/dlfcn/init.c   6 Jul 2020 13:33:05 -   1.8
+++ lib/libc/dlfcn/init.c   10 Jul 2020 02:12:41 -
@@ -114,6 +114,8 @@ _libc_preinit(int argc, char **argv, cha
_timekeep->tk_version != TK_VERSION)
_timekeep = NULL;
}
+   if (getenv("_LIBC_NOUSERTC") && issetugid() == 0)
+   _timekeep = NULL;
break;
}
}
Index: usr.bin/ktrace/extern.h
===
RCS file: /home/cvs/src/usr.bin/ktrace/extern.h,v
retrieving revision 1.4
diff -u -p -r1.4 extern.h
--- usr.bin/ktrace/extern.h 26 Apr 2018 18:30:36 -  1.4
+++ usr.bin/ktrace/extern.h 10 Jul 2020 02:21:37 -
@@ -26,3 +26,4 @@
  */
 
 int getpoints(const char *, int);
+int gettrans(const char *, int);
Index: usr.bin/ktrace/ktrace.1
===
RCS file: /home/cvs/src/usr.bin/ktrace/ktrace.1,v
retrieving revision 1.30
diff -u -p -r1.30 ktrace.1
--- usr.bin/ktrace/ktrace.1 15 May 2019 15:36:59 -  1.30
+++ usr.bin/ktrace/ktrace.1 10 Jul 2020 02:24:04 -
@@ -37,14 +37,15 @@
 .Nd enable kernel process tracing
 .Sh SYNOPSIS
 .Nm ktrace
-.Op Fl aBCcdi
+.Op Fl aCcdi
 .Op Fl f Ar trfile
 .Op Fl g Ar pgid
 .Op Fl p Ar pid
 .Op Fl t Ar trstr
 .Nm ktrace
-.Op Fl adi
+.Op Fl aBdi
 .Op Fl f Ar trfile
+.Op Fl T Ar transopts
 .Op Fl t Ar trstr
 .Ar command
 .Sh DESCRIPTION
@@ -109,6 +110,15 @@ processes.
 Enable (disable) tracing on the indicated process ID (only one
 .Fl p
 flag is permitted).
+.It Fl T Ar transopts
+Request additional transparency from libc.
+By default, no options are enabled.
+The argument can contain one or more of the following letters.
+.Pp
+.Bl -tag -width flag -offset indent -compact
+.It Cm t
+Disable userland timecounters so that time related system calls are visible.
+.El
 .It Fl t Ar trstr
 Select which information to put into the dump file.
 The argument can contain one or more of the following letters.
Index: usr.bin/ktrace/ktrace.c
===
RCS file: /home/cvs/src/usr.bin/ktrace/ktrace.c,v
retrieving revision 1.36
diff -u -p -r1.36 ktrace.c
--- usr.bin/ktrace/ktrace.c 28 Jun 2019 13:35:01 -  1.36
+++ usr.bin/ktrace/ktrace.c 10 Jul 2020 02:23:33 -
@@ -60,7 +60,7 @@ int
 main(int argc, char *argv[])
 {
enum { NOTSET, CLEAR, CLEARALL } clear;
-   int append, ch, fd, inherit, ops, pidset, trpoints;
+   int append, ch, fd, inherit, ops, pidset, trpoints, transparency;
pid_t pid;
char *tracefile, *tracespec;
mode_t omask;
@@ -71,6 +71,7 @@ main(int argc, char *argv[])
clear = NOTSET;
append = ops = pidset = inherit = pid = 0;
trpoints = is_ltrace ? KTRFAC_USER : DEF_POINTS;
+   transparency = 0;
tracefile = DEF_TRACEFILE;
tracespec = NULL;
 
@@ -100,7 +101,7 @@ main(int argc, char *argv[])
usage();
}
} else {
-   while ((ch = getopt(argc, argv, "aBCcdf:g:ip:t:")) != -1)
+   while ((ch = getopt(argc, argv, "aBCcdf:g:ip:t:T:")) != -1)
switch ((char)ch) {
case 'a':
append = 1;
@@ -140,6 +141,13 @@ main(int argc, char *argv[])
usage();
}
break;
+   case 'T':
+   transparency = gettrans(optarg, 0);
+   if (transparency < 0) {
+   warnx("unknown 

Re: pshared semaphores

2020-07-09 Thread Ted Unangst
On 2020-07-08, Mark Kettenis wrote:
> > From: "Ted Unangst" 
> > Date: Wed, 08 Jul 2020 05:20:23 -0400
> > 
> > On 2020-07-08, Ted Unangst wrote:
> > > I think this makes sem_init(pshared) work.
> > 
> > Whoops, need more context to include the header file changes.
> 
> It is a bit of a pity that we have to expose the internals here, but I
> don't see an easy way to avoid that, especially since hppa requires
> 16-byte alignment.  At least  doesn't do any
> namespace pollution as I believe a single underscore is enough here as
> everything is in file scope?
> 
> This will require a libpthread major bump, and those are really
> painful!  So I'm not sure we should do this just for pshared
> semaphores which hardly anybody uses.
> 
> I wonder if we do this, should we include some additional padding in
> the struct for future expansion?

We can also expose only a padded struct sem { int _pad[4]; } or so.
That's a bit more cumbersome, and we need to be careful. But certainly
possible.

If this is useful, I think we should also consider removing the
indirection from pthread and mutex, etc. This is more work, but I
don't mind doing it all at once. Semaphore is just first priority.

(I think the mail issues are better now, thanks everyone.)



Re: disable libc sys wrappers?

2020-07-09 Thread Ted Unangst
On 2020-07-08, Theo de Raadt wrote:
> I think we need something like this.
> 
> Documenting it will be a challenge.
> 
> I really don't like the name as is too generic, when the control is only
> for a narrow set of "current time" system calls.

I thought I'd start with something, but lots of questions.

Should it be per wrapper? I know in the past we've had some similar
conversations about eliminating syscalls (open/openat) and I imagine
there will be future instances. Initial thought is it's easier to make
one button, and then document it in ktrace perhaps? But we can also add
per function options, and document them in the appropriate pages.



disable libc sys wrappers?

2020-07-08 Thread Ted Unangst
Not sure how useful this will be, but I think it could be helpful to still
see section (2) functions in ktrace, even if there's magic to avoid that.

As proof of concept, if env LIBC_NOSYSWRAPPERS is set, the libc timecounters
are turned off. Now I see lots of gettimeofday syscalls in ktrace again.

Is this better than switching to ltrace? Combined ktrace and ltrace output
is fairly messy, but it seems to work. Setting it up to trace just a few
functions and all the system calls is a bit more involved.


Index: init.c
===
RCS file: /home/cvs/src/lib/libc/dlfcn/init.c,v
retrieving revision 1.8
diff -u -p -r1.8 init.c
--- init.c  6 Jul 2020 13:33:05 -   1.8
+++ init.c  8 Jul 2020 08:13:07 -
@@ -114,6 +114,8 @@ _libc_preinit(int argc, char **argv, cha
_timekeep->tk_version != TK_VERSION)
_timekeep = NULL;
}
+   if (issetugid() == 0 && getenv("LIBC_NOSYSWRAPPERS"))
+   _timekeep = NULL;
break;
}
}



pshared semaphores

2020-07-08 Thread Ted Unangst
I think this makes sem_init(pshared) work.
I have a test program from Lauri Tirkkonen and if I've understood it
correctly, it works now.

The essence of the diff is that we must eliminate the indirection so that
the application can properly allocate (mmap) the semaphore into shared memory.

There's two variations of the semaphore code (futex and compat). I tested the
compat on amd64, and think it works. The futex version also seems to work,
although I'm not sure how...


Index: rthread.h
===
RCS file: /home/cvs/src/lib/librthread/rthread.h,v
retrieving revision 1.64
diff -u -p -r1.64 rthread.h
--- rthread.h   13 Feb 2019 13:22:14 -  1.64
+++ rthread.h   8 Jul 2020 08:51:08 -
@@ -79,8 +79,8 @@ struct pthread_spinlock {
(((size) + (_thread_pagesize - 1)) & ~(_thread_pagesize - 1))
 
 __BEGIN_HIDDEN_DECLS
-int_sem_wait(sem_t, int, const struct timespec *, int *);
-int_sem_post(sem_t);
+int_sem_wait(sem_t *, int, const struct timespec *, int *);
+int_sem_post(sem_t *);
 
 void   _rthread_init(void);
 struct stack *_rthread_alloc_stack(pthread_t);
Index: rthread_sem.c
===
RCS file: /home/cvs/src/lib/librthread/rthread_sem.c,v
retrieving revision 1.32
diff -u -p -r1.32 rthread_sem.c
--- rthread_sem.c   6 Apr 2020 00:01:08 -   1.32
+++ rthread_sem.c   8 Jul 2020 08:58:07 -
@@ -55,7 +55,7 @@
  * Internal implementation of semaphores
  */
 int
-_sem_wait(sem_t sem, int can_eintr, const struct timespec *abstime,
+_sem_wait(sem_t *sem, int can_eintr, const struct timespec *abstime,
 int *delayed_cancel)
 {
unsigned int val;
@@ -86,7 +86,7 @@ _sem_wait(sem_t sem, int can_eintr, cons
 
 /* always increment count */
 int
-_sem_post(sem_t sem)
+_sem_post(sem_t *sem)
 {
membar_exit_before_atomic();
atomic_inc_int(>value);
@@ -98,70 +98,29 @@ _sem_post(sem_t sem)
  * exported semaphores
  */
 int
-sem_init(sem_t *semp, int pshared, unsigned int value)
+sem_init(sem_t *sem, int pshared, unsigned int value)
 {
-   sem_t sem;
 
if (value > SEM_VALUE_MAX) {
errno = EINVAL;
return (-1);
}
 
-   if (pshared) {
-   errno = EPERM;
-   return (-1);
-#ifdef notyet
-   char name[SEM_RANDOM_NAME_LEN];
-   sem_t *sempshared;
-   int i;
-
-   for (;;) {
-   for (i = 0; i < SEM_RANDOM_NAME_LEN - 1; i++)
-   name[i] = arc4random_uniform(255) + 1;
-   name[SEM_RANDOM_NAME_LEN - 1] = '\0';
-   sempshared = sem_open(name, O_CREAT | O_EXCL, 0, value);
-   if (sempshared != SEM_FAILED)
-   break;
-   if (errno == EEXIST)
-   continue;
-   if (errno != EPERM)
-   errno = ENOSPC;
-   return (-1);
-   }
-
-   /* unnamed semaphore should not be opened twice */
-   if (sem_unlink(name) == -1) {
-   sem_close(sempshared);
-   errno = ENOSPC;
-   return (-1);
-   }
-
-   *semp = *sempshared;
-   free(sempshared);
-   return (0);
-#endif
-   }
-
-   sem = calloc(1, sizeof(*sem));
-   if (!sem) {
-   errno = ENOSPC;
-   return (-1);
-   }
+   memset(sem, 0, sizeof(*sem));
sem->value = value;
-   *semp = sem;
+   sem->shared = pshared;
 
return (0);
 }
 
 int
-sem_destroy(sem_t *semp)
+sem_destroy(sem_t *sem)
 {
-   sem_t sem;
 
if (!_threads_ready) /* for SEM_MMAP_SIZE */
_rthread_init();
 
-   if (!semp || !(sem = *semp)) {
+   if (!sem) {
errno = EINVAL;
return (-1);
}
@@ -174,21 +133,19 @@ sem_destroy(sem_t *semp)
return (-1);
}
 
-   *semp = NULL;
if (sem->shared)
munmap(sem, SEM_MMAP_SIZE);
else
-   free(sem);
+   memset(sem, 0, sizeof(*sem));
 
return (0);
 }
 
 int
-sem_getvalue(sem_t *semp, int *sval)
+sem_getvalue(sem_t *sem, int *sval)
 {
-   sem_t sem;
 
-   if (!semp || !(sem = *semp)) {
+   if (!sem) {
errno = EINVAL;
return (-1);
}
@@ -199,11 +156,10 @@ sem_getvalue(sem_t *semp, int *sval)
 }
 
 int
-sem_post(sem_t *semp)
+sem_post(sem_t *sem)
 {
-   sem_t sem;
 
-   if (!semp || !(sem = *semp)) {
+   if (!sem) {
errno = EINVAL;
return (-1);
}
@@ -214,11 +170,10 @@ sem_post(sem_t *semp)
 }
 
 int
-sem_wait(sem_t *semp)
+sem_wait(sem_t *sem)
 {
struct tib *tib = 

Re: pshared semaphores

2020-07-08 Thread Ted Unangst
On 2020-07-08, Ted Unangst wrote:
> I think this makes sem_init(pshared) work.

Whoops, need more context to include the header file changes.

Index: include/semaphore.h
===
RCS file: /home/cvs/src/include/semaphore.h,v
retrieving revision 1.1
diff -u -p -r1.1 semaphore.h
--- include/semaphore.h 15 Oct 2017 23:40:33 -  1.1
+++ include/semaphore.h 8 Jul 2020 08:47:06 -
@@ -38,10 +38,16 @@
 #define _SEMAPHORE_H_
 
 #include 
+#include 
 
 /* Opaque type definition. */
-struct __sem;
-typedef struct __sem *sem_t;
+struct __sem {
+   _atomic_lock_t lock;
+   volatile int waitcount;
+   volatile int value;
+   int shared;
+};
+typedef struct __sem sem_t;
 struct timespec;
 
 #define SEM_FAILED  ((sem_t *)0)
Index: lib/libc/include/thread_private.h
===
RCS file: /home/cvs/src/lib/libc/include/thread_private.h,v
retrieving revision 1.35
diff -u -p -r1.35 thread_private.h
--- lib/libc/include/thread_private.h   13 Feb 2019 13:22:14 -  1.35
+++ lib/libc/include/thread_private.h   8 Jul 2020 08:44:45 -
@@ -273,13 +273,6 @@ __END_HIDDEN_DECLS
 
 #define_SPINLOCK_UNLOCKED _ATOMIC_LOCK_UNLOCKED
 
-struct __sem {
-   _atomic_lock_t lock;
-   volatile int waitcount;
-   volatile int value;
-   int shared;
-};
-
 TAILQ_HEAD(pthread_queue, pthread);
 
 #ifdef FUTEX
Index: lib/librthread/rthread.h
===
RCS file: /home/cvs/src/lib/librthread/rthread.h,v
retrieving revision 1.64
diff -u -p -r1.64 rthread.h
--- lib/librthread/rthread.h13 Feb 2019 13:22:14 -  1.64
+++ lib/librthread/rthread.h8 Jul 2020 09:18:34 -
@@ -79,8 +79,8 @@ struct pthread_spinlock {
(((size) + (_thread_pagesize - 1)) & ~(_thread_pagesize - 1))
 
 __BEGIN_HIDDEN_DECLS
-int_sem_wait(sem_t, int, const struct timespec *, int *);
-int_sem_post(sem_t);
+int_sem_wait(sem_t *, int, const struct timespec *, int *);
+int_sem_post(sem_t *);
 
 void   _rthread_init(void);
 struct stack *_rthread_alloc_stack(pthread_t);
Index: lib/librthread/rthread_sem.c
===
RCS file: /home/cvs/src/lib/librthread/rthread_sem.c,v
retrieving revision 1.32
diff -u -p -r1.32 rthread_sem.c
--- lib/librthread/rthread_sem.c6 Apr 2020 00:01:08 -   1.32
+++ lib/librthread/rthread_sem.c8 Jul 2020 09:18:34 -
@@ -55,7 +55,7 @@
  * Internal implementation of semaphores
  */
 int
-_sem_wait(sem_t sem, int can_eintr, const struct timespec *abstime,
+_sem_wait(sem_t *sem, int can_eintr, const struct timespec *abstime,
 int *delayed_cancel)
 {
unsigned int val;
@@ -86,7 +86,7 @@ _sem_wait(sem_t sem, int can_eintr, cons
 
 /* always increment count */
 int
-_sem_post(sem_t sem)
+_sem_post(sem_t *sem)
 {
membar_exit_before_atomic();
atomic_inc_int(>value);
@@ -98,70 +98,29 @@ _sem_post(sem_t sem)
  * exported semaphores
  */
 int
-sem_init(sem_t *semp, int pshared, unsigned int value)
+sem_init(sem_t *sem, int pshared, unsigned int value)
 {
-   sem_t sem;
 
if (value > SEM_VALUE_MAX) {
errno = EINVAL;
return (-1);
}
 
-   if (pshared) {
-   errno = EPERM;
-   return (-1);
-#ifdef notyet
-   char name[SEM_RANDOM_NAME_LEN];
-   sem_t *sempshared;
-   int i;
-
-   for (;;) {
-   for (i = 0; i < SEM_RANDOM_NAME_LEN - 1; i++)
-   name[i] = arc4random_uniform(255) + 1;
-   name[SEM_RANDOM_NAME_LEN - 1] = '\0';
-   sempshared = sem_open(name, O_CREAT | O_EXCL, 0, value);
-   if (sempshared != SEM_FAILED)
-   break;
-   if (errno == EEXIST)
-   continue;
-   if (errno != EPERM)
-   errno = ENOSPC;
-   return (-1);
-   }
-
-   /* unnamed semaphore should not be opened twice */
-   if (sem_unlink(name) == -1) {
-   sem_close(sempshared);
-   errno = ENOSPC;
-   return (-1);
-   }
-
-   *semp = *sempshared;
-   free(sempshared);
-   return (0);
-#endif
-   }
-
-   sem = calloc(1, sizeof(*sem));
-   if (!sem) {
-   errno = ENOSPC;
-   return (-1);
-   }
+   memset(sem, 0, sizeof(*sem));
sem->value = value;
-   *semp = sem;
+   sem->shared = pshared;
 
return (0);
 }
 
 int
-sem_destroy(sem_t *semp)
+sem_destroy(sem_t *sem)
 {
-   sem_t sem;
 
if (!_threads_ready)   

Re: Avoid offline cpu in automatic frequency scheduling

2020-05-28 Thread Ted Unangst
On 2020-05-28, Solene Rapenne wrote:
> > > In the current case, if you have offline cpu (because of hw.smt=0),
> > > the total will still sum all the idle cpu and it's unlikely that
> > > the total threshold is ever reached before the per cpu one.
> > > 
> > > so, this diff skip offline cpus in the loop (robert@ gave me the big
> > > clue to use cpu_is_online in the loop)  

This looks good.

> 
> It's a lot of magic numbers without explanation here. I'm curious if
> this could be improved. I guess that changing them would tip the
> balance between responsiveness and battery saving.

Did I do that? You have it about right, although there was never much science
behind the algorithm or numbers. It was something that seemed to work okay
without much complexity.



Re: Untangle proc * in VOP_*

2020-03-23 Thread Ted Unangst
On 2020-03-23, Martin Pieuchot wrote:
> Most of the VOP_* methods include an argument of type "struct proc *"
> called `a_p'.  This pointer is always set to `curproc' as confirmed by
> the diff below.  The diff has been though base build with NFS on amd64
> and sparc64 as well as a full port bulk on amd64 and is in the current
> octeon port bulk.
> 
> I'd like to commit it in order to start removing the requirement of
> passing a "struct proc *" pointer which is always set to curproc.  This
> is unnecessary and has spread in many other places in the kernel.
> 
> That makes codes unnecessarily complicated:
>  - a "struct proc *" is carried over just to satisfy already complex
>interfaces
>  - it isn't obvious which "struct proc *" is not the curproc and 
>asserts like KASSERT(p == curproc) are being made 
>  - in many places where curproc is used people put XXX as if it was
>wrong
> 
> So this is just a starting diff, cleanups can start afterward.

If this is a temporary measure, I think it should include some
annotation to that effect, so that it doesn't continue spreading.



Re: librthread sem_t opaqueness, storage & unnamed semaphore sharing

2020-03-02 Thread Ted Unangst
On 2020-03-02, Lauri Tirkkonen wrote:

> Thanks for the input, and ping - is there still something about this
> diff that I should fix?

I'm kinda busy, but I should be able to look into it eventually.



Re: librthread sem_t opaqueness, storage & unnamed semaphore sharing

2020-02-24 Thread Ted Unangst
Martin Pieuchot wrote:
> On 24/02/20(Mon) 11:29, Lauri Tirkkonen wrote:
> > On Mon, Feb 24 2020 10:24:53 +0100, Martin Pieuchot wrote:
> > > On 23/02/20(Sun) 14:48, Lauri Tirkkonen wrote:
> > > > I was working on a make jobserver implementation that uses POSIX
> > > > semaphores as job tokens instead of a complicated socket-based approach.
> > > > Initially I used named semaphores, which work fine, except if child
> > > > processes with less privileges need to also open the named semaphore
> > > > (eg. 'make build' as root executing 'su build -c make'). For that reason
> > > > I wanted to use an unnamed semaphore (sem_init()) which is stored in shm
> > > > -- that way I could leave the shm fd open and pass it to children.
> > > > 
> > > > But unfortunately, sem_t is currently just a pointer to the opaque
> > > > struct __sem, and sem_int() always calloc()s the storage for the struct.
> > > 
> > > That's by design.
> > 
> > Ok - could you elaborate what the design is?
> 
> If the size of a descriptor change, because some fields are added and/or
> removed, it doesn't matter for the application because it only manipulates
> pointers.  That means we can change the data types without creating an ABI
> break.

I think we are approaching the point where we can settle on fixed sized types
now. If we want to be cautious, we can add a reserved padding field, too. But
there are some edge cases which I think can be removed by eliminating the
dynamic allocation paths.

> > See the followup patch -- sharing the semaphore between processes does
> > work with it.
> 
> Well ignoring the `pshared' argument is questionable.  Why don't you
> remove the "#if notyet" and start playing with the existing code and
> try to figure out if something is missing for your use case?

I'm not sure the code in notyet will work. It was based on a misunderstanding
I had of the requirements. Returning control of the sem_t placement to the
application is the right approach.



refer to pointers as non-null

2020-01-31 Thread Ted Unangst
Noticed this in wait.2, though I imagine other occurences abound.

I believe non-null is clearer when refering to a pointer than non-zero, which
is a bit 80s, and less likely to be mistaken for the value pointed to. This
same page also refers to non-zero values, fwiw.


Index: wait.2
===
RCS file: /home/cvs/src/lib/libc/sys/wait.2,v
retrieving revision 1.30
diff -u -p -r1.30 wait.2
--- wait.2  1 May 2017 00:08:31 -   1.30
+++ wait.2  31 Jan 2020 10:28:42 -
@@ -70,7 +70,7 @@ On return from a successful
 .Fn wait
 call, the
 .Fa status
-area, if non-zero, is filled in with termination information about the
+area, if non-null, is filled in with termination information about the
 process that exited (see below).
 .Pp
 The
@@ -137,7 +137,7 @@ signal also have their status reported.
 .Pp
 If
 .Fa rusage
-is non-zero, a summary of the resources used by the terminated
+is non-null, a summary of the resources used by the terminated
 process and all its
 children is returned (this information is currently not available
 for stopped processes).



Re: Teach du(1) the -m flag, disk usage in megabytes

2020-01-26 Thread Ted Unangst
Jonathan Gray wrote:
> On Sun, Jan 26, 2020 at 11:59:33AM -0500, David Goerger wrote:
> > This diff teaches du(1) the -m flag, report disk usage in megabytes. 
> > This brings us in line with implementations in the other BSDs, Linux, 
> > and Illumos.
> 
> Why is it needed?  -k is required by POSIX, adding arguments for
> megabytes, gigabytes, terabytes, petabytes etc seems silly when
> there is already 512 byte blocks, kilobytes and -h output.

there is also env BLOCKSIZE=1m although it's unwieldy to use environment.

freebsd and linux both have -B blocksize which would be more flexible.



Re: apmd: fix autoaction timeout

2020-01-25 Thread Ted Unangst
Jeremie Courreges-Anglas wrote:
> 
> The diff below improves the way apmd -z/-Z may trigger.
> 
> I think the current behavior is bogus, incrementing and checking
> apmtimeout like this doesn't make much sense.

this all seems reasonable to me.

> - I think we want some throttling mechanism, like wait for 1mn after we
>   resume before autosuspending again.  But I want to fix obvious
>   problems first.

would agree with that as well.



apmd poll every minute

2020-01-24 Thread Ted Unangst
Sometimes (particuarly if you are using apmd -z) the default polling interval
of 10 minutes is a bit lazy and we fail to respond to low battery situations
before they become no battery situations.

Perhaps something smarter could be done, but the simplest change is to reduce
the default polling interval to one minute. This is still rather slow, so as
to not introduce unnecessary load on the system, but should make it more
responsive to changing conditions.


Index: apmd.8
===
RCS file: /home/cvs/src/usr.sbin/apmd/apmd.8,v
retrieving revision 1.50
diff -u -p -r1.50 apmd.8
--- apmd.8  4 Dec 2018 18:00:57 -   1.50
+++ apmd.8  25 Jan 2020 04:50:09 -
@@ -111,7 +111,7 @@ periodically polls the APM driver for th
 If the battery charge level changes substantially or the external power
 status changes, the new status is logged.
 The polling rate defaults to
-once per 10 minutes, but may be specified using the
+once per 1 minute, but may be specified using the
 .Fl t
 command-line flag.
 .It Fl Z Ar percent
Index: apmd.c
===
RCS file: /home/cvs/src/usr.sbin/apmd/apmd.c,v
retrieving revision 1.91
diff -u -p -r1.91 apmd.c
--- apmd.c  2 Nov 2019 00:41:36 -   1.91
+++ apmd.c  25 Jan 2020 04:49:54 -
@@ -366,7 +366,7 @@ resumed(int ctl_fd)
power_status(ctl_fd, 1, NULL);
 }
 
-#define TIMO (10*60)   /* 10 minutes */
+#define TIMO (1*60)/* 1 minute */
 
 int
 main(int argc, char *argv[])



Re: acpivout(4): directly call ws_[gs]et_param

2020-01-23 Thread Ted Unangst
Patrick Wildt wrote:
> acpivout_[gs]et_param don't know if they are being called by itself
> or by someone else.  I don't need to grab it again, I just need to
> have it before calling an ACPI method.  But when acpivout(4) calls
> itself, it already has it, so taking it again would break it, as it's
> non recursive.
> 
> Since it's a global function pointer that hides the implementation, the
> caller cannot just take the ACPI lock before calling ws_[gs]et_param.
> The caller doesn't know that the implementation needs ACPI.  On my X395
> the function set in ws_[gs]et_param have nothing to do with ACPI at all.

Can you release the lock before calling ws_set_param?



Re: acme-client calloc fix

2020-01-22 Thread Ted Unangst
Matthew Martin wrote:
> On Wed, Jan 22, 2020 at 12:44:18AM -0500, Ted Unangst wrote:
> > should not size the size until the allocation succeeds, or the free path 
> > will
> > try to deref the null array.
> > 
> > 
> > Index: json.c
> > ===
> > RCS file: /home/cvs/src/usr.sbin/acme-client/json.c,v
> > retrieving revision 1.14
> > diff -u -p -r1.14 json.c
> > --- json.c  18 Jun 2019 18:50:07 -  1.14
> > +++ json.c  22 Jan 2020 05:37:59 -
> > @@ -459,12 +459,13 @@ json_parse_order(struct jsmnn *n, struct
> > if ((array = json_getarray(n, "authorizations")) == NULL)
> > goto err;
> >  
> > -   if ((order->authsz = array->fields) > 0) {
> > +   if (array->fields > 0) {
> > order->auths = calloc(sizeof(*order->auths), order->authsz);
> 
> Shouldn't the second argument be switched to array->fields to maintain
> the same behavior?

thanks!



semicolon reduction

2020-01-21 Thread Ted Unangst
don't need semicolon after } in statements.


Index: ifconfig/brconfig.c
===
RCS file: /home/cvs/src/sbin/ifconfig/brconfig.c,v
retrieving revision 1.24
diff -u -p -r1.24 brconfig.c
--- ifconfig/brconfig.c 24 Oct 2019 18:54:10 -  1.24
+++ ifconfig/brconfig.c 22 Jan 2020 06:02:20 -
@@ -353,7 +353,7 @@ bridge_list(char *delim)
if ((1 << (v - 1)) & reqp->ifbr_protected)
printf(",%u", v);
}
-   };
+   }
if (reqp->ifbr_ifsflags & IFBIF_STP)
printf(" %s role %s",
stpstates[reqp->ifbr_state],
Index: kbd/kbd_wscons.c
===
RCS file: /home/cvs/src/sbin/kbd/kbd_wscons.c,v
retrieving revision 1.33
diff -u -p -r1.33 kbd_wscons.c
--- kbd/kbd_wscons.c28 Jun 2019 13:32:44 -  1.33
+++ kbd/kbd_wscons.c22 Jan 2020 06:01:26 -
@@ -198,7 +198,7 @@ kbd_list(void)
default:
t = SA_MAX;
break;
-   };
+   }
 
if (t != SA_MAX) {
kbds[t]++;
Index: mount_nfs/mount_nfs.c
===
RCS file: /home/cvs/src/sbin/mount_nfs/mount_nfs.c,v
retrieving revision 1.54
diff -u -p -r1.54 mount_nfs.c
--- mount_nfs/mount_nfs.c   5 Jan 2018 08:13:31 -   1.54
+++ mount_nfs/mount_nfs.c   22 Jan 2020 06:02:49 -
@@ -563,7 +563,7 @@ xdr_fh(XDR *xdrsp, struct nfhret *np)
if (!authfnd && (authcnt > 0 || np->auth != RPCAUTH_UNIX))
np->stat = EAUTH;
return (1);
-   };
+   }
return (0);
 }
 
Index: nfsd/nfsd.c
===
RCS file: /home/cvs/src/sbin/nfsd/nfsd.c,v
retrieving revision 1.39
diff -u -p -r1.39 nfsd.c
--- nfsd/nfsd.c 28 Jun 2019 13:32:45 -  1.39
+++ nfsd/nfsd.c 22 Jan 2020 05:59:42 -
@@ -135,7 +135,7 @@ main(int argc, char *argv[])
break;
default:
usage();
-   };
+   }
argv += optind;
argc -= optind;
 



acme-client calloc fix

2020-01-21 Thread Ted Unangst
should not size the size until the allocation succeeds, or the free path will
try to deref the null array.


Index: json.c
===
RCS file: /home/cvs/src/usr.sbin/acme-client/json.c,v
retrieving revision 1.14
diff -u -p -r1.14 json.c
--- json.c  18 Jun 2019 18:50:07 -  1.14
+++ json.c  22 Jan 2020 05:37:59 -
@@ -459,12 +459,13 @@ json_parse_order(struct jsmnn *n, struct
if ((array = json_getarray(n, "authorizations")) == NULL)
goto err;
 
-   if ((order->authsz = array->fields) > 0) {
+   if (array->fields > 0) {
order->auths = calloc(sizeof(*order->auths), order->authsz);
if (order->auths == NULL) {
warn("malloc");
goto err;
}
+   order->authsz = array->fields;
}
 
for (i = 0; i < array->fields; i++) {



Re: piixpm(4) on ATI SBx00

2020-01-21 Thread Ted Unangst
Karel Gardas wrote:
> 
> 
> On 1/21/20 2:33 AM, Claudio Jeker wrote:
> > Please test this since I can't test this properly at the moment. 
> 
> Would like to, but all hunks fail on today current:

it's been committed.



Re: check hhmm for leave

2020-01-21 Thread Ted Unangst
Scott Cheloha wrote:
> > 1) it isn't documented that + can take a smaller number
> > 2) it will be hard to train people to use +0001
> 
> These are my concerns as well.
> 
> An idea I had a while back was to drop support for +hhmm and just
> support +minutes, like we do with shutdown(8).  The invocation would
> then be:

this adds the check to just the clock time case. leave +mm isn't bad, but it's
a little awkward past 60. leave +180 for three hours? unfortunately that
clashes with existing usage. but my original concern is primarily with hhmm so
let's just address that.


Index: leave.c
===
RCS file: /home/cvs/src/usr.bin/leave/leave.c,v
retrieving revision 1.19
diff -u -p -r1.19 leave.c
--- leave.c 10 Feb 2018 00:00:47 -  1.19
+++ leave.c 22 Jan 2020 04:07:41 -
@@ -35,6 +35,7 @@
 #include 
 #include 
 #include 
+#include 
 #include 
 
 static __dead void usage(void);
@@ -83,6 +84,9 @@ main(int argc, char *argv[])
plusnow = 1;
++cp;
}
+
+   if (!plusnow && strlen(cp) != 4)
+   usage();
 
for (hours = 0; (c = *cp) && c != '\n'; ++cp) {
if (!isdigit((unsigned char)c))



check hhmm for leave

2020-01-21 Thread Ted Unangst
In testing leave, I found it accepts "12" and will alarm at 00:12, which is
probably not desirable. this check thats the specified time has 4 digits so
that the hour/minute math works properly.


Index: leave.c
===
RCS file: /home/cvs/src/usr.bin/leave/leave.c,v
retrieving revision 1.19
diff -u -p -r1.19 leave.c
--- leave.c 10 Feb 2018 00:00:47 -  1.19
+++ leave.c 22 Jan 2020 02:13:03 -
@@ -35,6 +35,7 @@
 #include 
 #include 
 #include 
+#include 
 #include 
 
 static __dead void usage(void);
@@ -83,6 +84,9 @@ main(int argc, char *argv[])
plusnow = 1;
++cp;
}
+
+   if (strlen(cp) != 4)
+   usage();
 
for (hours = 0; (c = *cp) && c != '\n'; ++cp) {
if (!isdigit((unsigned char)c))



leave.1

2020-01-21 Thread Ted Unangst
Rewrite some of the page to avoid second person.


Index: leave.1
===
RCS file: /home/cvs/src/usr.bin/leave/leave.1,v
retrieving revision 1.16
diff -u -p -r1.16 leave.1
--- leave.1 13 Jul 2018 16:59:46 -  1.16
+++ leave.1 22 Jan 2020 02:08:00 -
@@ -35,17 +35,17 @@
 .Os
 .Sh NAME
 .Nm leave
-.Nd remind you when you have to leave
+.Nd print reminders when it is time to leave
 .Sh SYNOPSIS
 .Nm leave
 .Op Oo Cm + Oc Ns Ar hhmm
 .Sh DESCRIPTION
 .Nm leave
-waits until the specified time, then reminds you that you
-have to leave.
-You are reminded 5 minutes and 1 minute before the actual
+waits until the specified time, then prints a reminder
+that it is time to leave.
+Reminders are printed 5 minutes and 1 minute before the actual
 time, at the time, and every minute thereafter.
-When you log off,
+If the login session ends,
 .Nm leave
 exits just before it would have
 printed the next message.
@@ -72,8 +72,7 @@ from the current time.
 .Pp
 If no argument is given,
 .Nm leave
-prompts with "When do you
-have to leave?".
+prompts with "When do you have to leave?".
 A reply of newline causes
 .Nm leave
 to exit,



Re: [patch] signify's file name parsing broken

2020-01-21 Thread Ted Unangst
Ted Unangst wrote:
> MarcusMüller wrote:
> > I've just stumbled across a malfunction in signify: It cannot handle
> > file names that contain a `)` character, when checking a list of hashes
> > generated by `sha256` command line utilities (`sha256sum --tags` on
> > Linux). 
> 
> This fix is unfortunately rather complicated for the problem. Files with ) are
> not used within openbsd, for instance. It may possible to simplify it a bit? 

Not much simpler, but maybe this is easier to follow. Keeps the hair in a
separate function.


Index: signify.c
===
RCS file: /home/cvs/src/usr.bin/signify/signify.c,v
retrieving revision 1.134
diff -u -p -r1.134 signify.c
--- signify.c   22 Dec 2019 06:37:25 -  1.134
+++ signify.c   21 Jan 2020 22:20:55 -
@@ -651,6 +651,37 @@ verifychecksum(struct checksum *c, int q
 }
 
 static void
+scanchecksum(const char *input, struct checksum *c)
+{
+   char *p, *algo, *file, *hash;
+   char line[2 * PATH_MAX];
+
+   if (strlcpy(line, input, sizeof(line)) >= sizeof(line))
+   goto fail;
+
+   /* algo (filename) = hash */
+   algo = line;
+   p = strchr(algo, ' ');
+   if (p == NULL || strncmp(p, " (", 2) != 0)
+   goto fail;
+   *p = 0;
+   file = p + 2;
+   p = strrchr(file, ')');
+   if (p == NULL || strncmp(p, ") = ", 4) != 0)
+   goto fail;
+   *p = 0;
+   hash = p + 4;
+
+   if (strlcpy(c->algo, algo, sizeof(c->algo)) >= sizeof(c->algo) ||
+   strlcpy(c->file, file, sizeof(c->file)) >= sizeof(c->file) ||
+   strlcpy(c->hash, hash, sizeof(c->hash)) >= sizeof(c->hash))
+   goto fail;
+   return;
+fail:
+   errx(1, "unable to parse checksum line %s", input);
+}
+
+static void
 verifychecksums(char *msg, int argc, char **argv, int quiet)
 {
struct ohash_info info = { 0, NULL, ecalloc, efree, NULL };
@@ -658,7 +689,7 @@ verifychecksums(char *msg, int argc, cha
struct checksum c;
char *e, *line, *endline;
int hasfailed = 0;
-   int i, rv;
+   int i;
unsigned int slot;
 
ohash_init(, 6, );
@@ -675,13 +706,8 @@ verifychecksums(char *msg, int argc, cha
while (line && *line) {
if ((endline = strchr(line, '\n')))
*endline++ = '\0';
-#if PATH_MAX < 1024 || HASHBUFSIZE < 224
-#error sizes are wrong
-#endif
-   rv = sscanf(line, "%31s (%1023[^)]) = %223s",
-   c.algo, c.file, c.hash);
-   if (rv != 3)
-   errx(1, "unable to parse checksum line %s", line);
+
+   scanchecksum(line, );
line = endline;
if (argc) {
slot = ohash_qlookup(, c.file);



Re: key guessing for signify -C

2020-01-20 Thread Ted Unangst
Theo Buehler wrote:
> Two small things for signify -C:
> 
> Contrary to what usage suggests, the -p pubkey argument for signify -C
> is optional in that signify will use the key specified in the untrusted
> comment. In -V mode, the key can be tied down a little by specifying -t.
> 
> Right now, the -t keytype argument is silently ignored in -C mode:
> 
> $ ftp 
> https://cdn.openbsd.org/pub/OpenBSD/6.6/amd64/{INSTALL.amd64,SHA256.sig} 
> >/dev/null
> $ signify -C -t pkg -x SHA256.sig INSTALL.amd64
> Signature Verified
> INSTALL.amd64: OK
> 
> Diff below fixes usage and makes the above command error out with:

yes, that's an oversight. this looks good.



Re: [patch] signify's file name parsing broken

2020-01-20 Thread Ted Unangst
MarcusMüller wrote:
> I've just stumbled across a malfunction in signify: It cannot handle
> file names that contain a `)` character, when checking a list of hashes
> generated by `sha256` command line utilities (`sha256sum --tags` on
> Linux). 

This fix is unfortunately rather complicated for the problem. Files with ) are
not used within openbsd, for instance. It may possible to simplify it a bit? 



Re: nfs mp vnode list remove safe

2020-01-14 Thread Ted Unangst
Alexander Bluhm wrote:
> Hi,
> 
> There is no remove and no sleep in this loop.  So _SAFE is unnecessary.
> 
> ok?

sure, with one bonus note.

> 
> bluhm
> 
> Index: nfs/nfs_subs.c
> ===
> RCS file: /data/mirror/openbsd/cvs/src/sys/nfs/nfs_subs.c,v
> retrieving revision 1.141
> diff -u -p -r1.141 nfs_subs.c
> --- nfs/nfs_subs.c10 Jan 2020 10:33:35 -  1.141
> +++ nfs/nfs_subs.c13 Jan 2020 18:02:54 -
> @@ -1509,16 +1509,16 @@ netaddr_match(int family, union nethosta
>  void
>  nfs_clearcommit(struct mount *mp)
>  {
> - struct vnode *vp, *nvp;
> - struct buf *bp, *nbp;
> + struct vnode *vp;
> + struct buf *bp;
>   int s;
> 
>   s = splbio();
>  loop:
> - TAILQ_FOREACH_SAFE(vp, >mnt_vnodelist, v_mntvnodes, nvp) {
> + TAILQ_FOREACH(vp, >mnt_vnodelist, v_mntvnodes) {
>   if (vp->v_mount != mp)  /* Paranoia */
>   goto loop;

that looks like an infinite loop? if there's a bad vnode on the list, it'll
still be there next time around.



Re: dangling vnode panic

2020-01-09 Thread Ted Unangst
Martin Pieuchot wrote:
> On 09/01/20(Thu) 10:46, Alexander Bluhm wrote:
> > Without this diff my regress machines became so unstable, that they
> > often do not finish the test run.
> > 
> > With this diff, they run fine.  No regressions.
> 
> Sadly this is a workaround.  What is the issue?  We're missing a barrier
> during umount?  How does other BSDs solved that?
> 
> If your diff work because you changed a TAIL vs a HEAD insert and now
> how to reproduce it you could capture stack traces from the function
> doing the insert.  Is it insmntque()?  Could we prevent adding the vnode
> to the list if a dying flag is on the mount point?

I don't think that's possible. syncing the filesystem can cause more work to
be queued. If you prevent adding the vnode to the list, how will that work be
done?

The alternative would be to add another loop around the list processing. I
think that's worse because we don't know how many times to keep looping.

bluhm's diff is the best approach imo. ok me.



Re: Scrolling in top(1)

2020-01-05 Thread Ted Unangst
Vadim Zhukov wrote:
> Today I get really upset and angry due limitation of top(1): it shows
> only hrrm, top processes (thank you, Chromium). Now here is a diff that
> allows you to scroll process list by line or by half a screen.
> 
> I've used the '0' and '9' keys to scroll down and up, respectively.
> Unfortunately, 'k' is already taken, so vi-like binding to 'j'/'k' keys
> is not possible. And emacs-style 'v'-for-all looks like too complex.
> Anyone, who wants to use up/down and page up/page down keys, be my guest
> for converting command_chars in top.c to using multi-byte sequences,
> or whatever is needed for proper handling of those keys.
> 
> Ideas, comments and (may I hope?) okays are welcome. :)

One of each? It would be nice to extend this with some indication of where we
are in the display (a first line that says skipping 19). I would have used
,.<> as navigation keys, but no matter. And it seems to work great, so ok.



Re: Add -R alias to -r for scp(1)

2020-01-01 Thread Ted Unangst
Kurt Mosiejczuk wrote:
> cp(1) uses -R for recursive copy. scp(1) uses -r. This diff adds -R as an
> alias for -r to scp(1) for those assuming consistency with cp(1).

But it doesn't implement cp -R semantics. It does the copy the way cp -r does.
(For symlinks, etc.)



Re: uvm_mapanon() & trylock

2019-12-01 Thread Ted Unangst
Martin Pieuchot wrote:
> On 08/11/19(Fri) 17:54, Martin Pieuchot wrote:
> > uvm_mapanon() is called in two occasions: mmap(2) and sigaltstack(2).
> > Both code paths are obviously in process context an can sleep.  That
> > explains why none of them set the UVM_FLAG_TRYLOCK when calling such
> > function.
> > 
> > The diff below removes support for this flag.  This introduces a
> > difference with uvm_map(9) but simplifies the overall code.  Removing
> > support for an unneeded "try" variant also means the lock can be grabbed
> > earlier.  This is a requirement to not lose atomicity between
> > uvm_mapanon() and uvm_map_pageable() in mmap(2).
> 
> Anyone?

yeah, this is partly why mapanon was split into another function, so we could
remove the cruft that makes it complicated. go ahead.



Re: [PATCH] make: implement jobserver and use it to avoid exponential behavior

2019-11-27 Thread Ted Unangst
Marc Espie wrote:
> reorganizing a large part of usr.bin or usr.sbin to just be one
> single variation of bsd.prog.mk with multiple progs and multiple object
> files... works just fine for, say 95% of the binaries in those directories
> 
> (considering there are lots of directories with one single C file or two
> C files, this sounds like a gain for make -jN, right ?)
> 
> End result: no measurable gain.   That on 4 cpu boxes at the time. 
> There are SMP issues to solve before this actually yields any 
> useful result...

I don't know. I wrote a lua script that unrolled all the subdir and ran a
single process make in each directory, up to 4 at a time, and got pretty good
results. It takes 30s on my newer laptop just to run unbound configure.
Nothing else is compiling during that time, and it can be. Just a few of those
can shave minutes off a build.

(This was before clang. The savings is somewhat diminished by the
total time consumed by the gnu directory.)



Re: hexdump in boot loader

2019-11-26 Thread Ted Unangst
Alexander Bluhm wrote:
> +
> + for (n = 1; n < cmd.argc; n++) {
> + p = cmd.argv[n];
> + if (*p == '0') {
> + p++;
> + if (*p == 'x' || *p == 'X') {
> + p++;
> + b = 16;
> + } else
> + b = 8;
> + } else
> + b = 10;
> + val[n-1] = 0;
> + for (; *p != '\0'; p++) {
> + if (*p >= '0' && *p <= '9')
> + d = *p - '0';
> + else if (*p >= 'a' && *p <= 'z')
> + d = *p - 'a' + 10;
> + else if (*p >= 'A' && *p <= 'Z')
> + d = *p - 'A' + 10;
> + else
> + goto err;
> + if (d >= b)
> + goto err;
> + val[n-1] = val[n-1] * b + d;

why not use strtol here?



Re: Go vs uvm_map_inentry()

2019-11-03 Thread Ted Unangst
Martin Pieuchot wrote:
> The last, now reverted change, to uvm_map_inentry() exposes a race that
> is reproducible while building lang/go on amd64 which makes uvm_fault()
> fail, resulting a in a SIGSEV of at least one of the processes.

> Interestingly the machine cannot reproduce the race if the KERNEL_LOCK()
> is used as a barrier, like in the diff below. 
> 
> I couldn't reproduce the race with the go programs in src/regress.
> Building go is huge and difficult to instrument.
> 
> By looking at sigpanic() in the traces it seems that stack manipulation
> is generally the trigger.  I'm guessing the issue needs a multi-threaded
> program to be exposed but at that stage I'd appreciate any pointer or
> any simpler way to trigger the race. 

Can you explain the race some more? The lock changes below would all seem to
come too late to resolve any races.

>   if (uvm_map_inentry_recheck(serial, addr, ie)) {
>   KERNEL_LOCK();
> + KERNEL_UNLOCK();

This in particular looks pretty weird.



Re: _pbuild user to have priority=5

2019-11-01 Thread Ted Unangst
Theo de Raadt wrote:
> What about all the other users who aren't in staff?
> 
> I think the approach is right.  Push non-interactive down.

The same then for src build user?



Re: uvm_map_inentry() & KERNEL_LOCK()

2019-10-31 Thread Ted Unangst
Theo de Raadt wrote:
> In uvm_map_inentry_fix(), two variables in the map are now being read
> without a per-map read lock, this was previously protected by the
> kernel lock
> 
> if (addr < map->min_offset || addr >= map->max_offset)
> return (FALSE);
> 
> When I wrote this I was told to either use KERNEL_LOCK() or
> vm_map_lock_read(), which is vm_map_lock_read_ln() .. if
> KERNEL_LOCK() is no longer good should vm_map_lock_read be moved
> upwards, or are you confident those offsets are safe to read
> independently without any locking??

indeed, another thread can expand the map, so that should have the read lock.



random safety for pbkdf

2019-10-15 Thread Ted Unangst
In the event that a program uses invalid parameters, I think we should
overwrite the key with random data. Otherwise, there's a chance the program
will continue with a zero key. It may even appear to work, encrypting and
decrypting data, but with a weak key. Random data means it fails closed, and
should also make it easier to detect such errors since it no longer
interoperates.


Index: bcrypt_pbkdf.c
===
RCS file: /home/cvs/src/lib/libutil/bcrypt_pbkdf.c,v
retrieving revision 1.13
diff -u -p -r1.13 bcrypt_pbkdf.c
--- bcrypt_pbkdf.c  12 Jan 2015 03:20:04 -  1.13
+++ bcrypt_pbkdf.c  15 Oct 2019 19:14:12 -
@@ -110,10 +110,10 @@ bcrypt_pbkdf(const char *pass, size_t pa
 
/* nothing crazy */
if (rounds < 1)
-   return -1;
+   goto bad;
if (passlen == 0 || saltlen == 0 || keylen == 0 ||
keylen > sizeof(out) * sizeof(out))
-   return -1;
+   goto bad;
stride = (keylen + sizeof(out) - 1) / sizeof(out);
amt = (keylen + stride - 1) / stride;
 
@@ -166,4 +166,8 @@ bcrypt_pbkdf(const char *pass, size_t pa
explicit_bzero(out, sizeof(out));
 
return 0;
+
+bad:
+   arc4random_buf(key, keylen);
+   return -1;
 }
Index: pkcs5_pbkdf2.c
===
RCS file: /home/cvs/src/lib/libutil/pkcs5_pbkdf2.c,v
retrieving revision 1.10
diff -u -p -r1.10 pkcs5_pbkdf2.c
--- pkcs5_pbkdf2.c  18 Apr 2017 04:06:21 -  1.10
+++ pkcs5_pbkdf2.c  15 Oct 2019 19:17:08 -
@@ -84,11 +84,11 @@ pkcs5_pbkdf2(const char *pass, size_t pa
size_t r;
 
if (rounds < 1 || key_len == 0)
-   return -1;
+   goto bad;
if (salt_len == 0 || salt_len > SIZE_MAX - 4)
-   return -1;
+   goto bad;
if ((asalt = malloc(salt_len + 4)) == NULL)
-   return -1;
+   goto bad;
 
memcpy(asalt, salt, salt_len);
 
@@ -118,4 +118,8 @@ pkcs5_pbkdf2(const char *pass, size_t pa
explicit_bzero(obuf, sizeof(obuf));
 
return 0;
+
+bad:
+   arc4random_buf(key, key_len);
+   return -1;
 }



Re: top: align CPU lines horizontally

2019-10-12 Thread Ted Unangst
Klemens Nanni wrote:
> On Sat, Oct 12, 2019 at 05:38:13PM -0500, Scott Cheloha wrote:
> > Also, just count how many spaces we need to print ncpuonline,
> > then use that when printing the individual CPU lines.
> Yup, here's a minimal diff that does that without additional buffers and
> globals but a single local static padding;  it's definition looks lengthy
> but that's what makes it nicely obvious (imho).
> 
> Only downside compared to your diff is that we still assume machines to
> have less than 1000 cores, that is keep combined stats at fixed "%-3d",
> but that seems fairly acceptable to me.

looks nicer and manages to remove more lines than it adds. :)



Re: top: align CPU lines horizontally

2019-10-12 Thread Ted Unangst
Klemens Nanni wrote:
> On Sat, Oct 12, 2019 at 09:53:44AM -0600, Theo de Raadt wrote:
> > I am suggesting you put the spaces after the cpu#.
> Is this better?
> 
> 4   CPUs:  0.0% user,  0.0% nice,  0.0% sys,  0.0% spin,  0.0% intr,  100% 
> idle
> 
> CPU  0  :  0.0% user,  0.0% nice,  0.0% sys,  0.0% spin,  0.0% intr,  100% 
> idle
> CPU  1  :  0.0% user,  0.0% nice,  0.0% sys,  0.0% spin,  0.0% intr,  100% 
> idle


CPU 0:0.0% user,  0.0% nice,  0.0% sys,  0.0% spin,  0.0% intr,  100% idle
CPU 10:   0.0% user,  0.0% nice,  0.0% sys,  0.0% spin,  0.0% intr,  100% idle
CPU 100:  0.0% user,  0.0% nice,  0.0% sys,  0.0% spin,  0.0% intr,  100% idle



Re: more grep options (for zstdgrep)

2019-10-07 Thread Ted Unangst
While I'm looking at the man page, a minor clarification.

"All long options are provided" can be read to mean that every gnu option is
provided. This is not intended, and a simple reordering makes things clearer
imo.


Index: grep.1
===
RCS file: /cvs/src/usr.bin/grep/grep.1,v
retrieving revision 1.49
diff -u -p -r1.49 grep.1
--- grep.1  7 Oct 2019 20:51:34 -   1.49
+++ grep.1  7 Oct 2019 20:53:26 -
@@ -364,7 +364,7 @@ are extensions to that specification, an
 .Fl f
 flag when used with an empty pattern file is left undefined.
 .Pp
-All long options are provided for compatibility with
+All provided long options are for compatibility with
 GNU versions of this utility.
 .Pp
 Historic versions of the



more grep options (for zstdgrep)

2019-10-06 Thread Ted Unangst
The zstd package includes a zstdgrep script, which should behave like zgrep,
however it assumes a few gnu grep behaviors we don't support.

1. --label=name prints a custom label, so you can get file.txt, not
file.txt.zst in the output.

2. It uses - for stdin instead of a missing argument.

Both are easy to address, as below. With this, I can grep my old log files
with having to do the zstdcat | grep dance myself.


Index: grep.1
===
RCS file: /home/cvs/src/usr.bin/grep/grep.1,v
retrieving revision 1.47
diff -u -p -r1.47 grep.1
--- grep.1  18 Jul 2019 15:32:50 -  1.47
+++ grep.1  6 Oct 2019 18:21:05 -
@@ -47,6 +47,7 @@
 .Op Fl m Ar num
 .Op Fl -binary-files Ns = Ns Ar value
 .Op Fl -context Ns Op = Ns Ar num
+.Op Fl -label Ar name
 .Op Fl -line-buffered
 .Op Ar pattern
 .Op Ar
@@ -283,6 +284,10 @@ do not search binary files;
 and
 .Ar text :
 treat all files as text.
+.It Fl -label Ar name
+Print
+.Ar name
+instead of the filename before lines.
 .It Fl -line-buffered
 Force output to be line buffered.
 By default, output is line buffered when standard output is a terminal
Index: grep.c
===
RCS file: /home/cvs/src/usr.bin/grep/grep.c,v
retrieving revision 1.60
diff -u -p -r1.60 grep.c
--- grep.c  18 Jul 2019 15:32:50 -  1.60
+++ grep.c  6 Oct 2019 18:13:02 -
@@ -80,6 +80,7 @@ intvflag; /* -v: only show non-matchi
 int wflag; /* -w: pattern must start and end on word boundaries */
 int xflag; /* -x: pattern must match entire line */
 int lbflag;/* --line-buffered */
+const char *labelname; /* --label=name */
 
 int binbehave = BIN_FILE_BIN;
 
@@ -87,7 +88,8 @@ enum {
BIN_OPT = CHAR_MAX + 1,
HELP_OPT,
MMAP_OPT,
-   LINEBUF_OPT
+   LINEBUF_OPT,
+   LABEL_OPT,
 };
 
 /* Housekeeping */
@@ -130,6 +132,7 @@ static const struct option long_options[
{"binary-files",required_argument,  NULL, BIN_OPT},
{"help",no_argument,NULL, HELP_OPT},
{"mmap",no_argument,NULL, MMAP_OPT},
+   {"label",   required_argument,  NULL, LABEL_OPT},
{"line-buffered",   no_argument,NULL, LINEBUF_OPT},
{"after-context",   required_argument,  NULL, 'A'},
{"before-context",  required_argument,  NULL, 'B'},
@@ -427,6 +430,9 @@ main(int argc, char *argv[])
case MMAP_OPT:
/* default, compatibility */
break;
+   case LABEL_OPT:
+   labelname = optarg;
+   break;
case LINEBUF_OPT:
lbflag = 1;
break;
@@ -458,6 +464,11 @@ main(int argc, char *argv[])
 
if (argc != 0 && needpattern) {
add_patterns(*argv);
+   --argc;
+   ++argv;
+   }
+   if (argc == 1 && strcmp(*argv, "-") == 0) {
+   /* stdin */
--argc;
++argv;
}
Index: grep.h
===
RCS file: /home/cvs/src/usr.bin/grep/grep.h,v
retrieving revision 1.26
diff -u -p -r1.26 grep.h
--- grep.h  23 Jan 2019 23:00:54 -  1.26
+++ grep.h  6 Oct 2019 18:09:32 -
@@ -70,6 +70,7 @@ extern int Aflag, Bflag, Eflag, Fflag, 
 bflag, cflag, hflag, iflag, lflag, mflag, nflag, oflag, qflag,
 sflag, vflag, wflag, xflag;
 extern int  binbehave;
+extern const char *labelname;
 
 extern int  first, matchall, patterns, tail, file_err;
 extern char**pattern;
Index: util.c
===
RCS file: /home/cvs/src/usr.bin/grep/util.c,v
retrieving revision 1.60
diff -u -p -r1.60 util.c
--- util.c  17 Jul 2019 04:24:20 -  1.60
+++ util.c  6 Oct 2019 18:10:07 -
@@ -122,6 +122,8 @@ procfile(char *fn)
}
 
ln.file = fn;
+   if (labelname)
+   ln.file = (char *)labelname;
ln.line_no = 0;
ln.len = 0;
linesqueued = 0;



Re: pretty borders for slitherins

2019-09-26 Thread Ted Unangst
Nicholas Marriott wrote:
> OK seems like it is always using ACS which is correct.
> 
> I just remembered jmc asked me about this before and it turned out that
> ACS doesn't work with the DRM console - I don't think the font have the
> right symbol for either ACS or UTF-8 line drawing, but there may be
> other problems in there as well.
> 
> If you are using DRM you can set TERM to pccon0 which is a variant with
> ASCII line drawing and ncurses will use ASCII for line drawing.

ah, yes, env TERM=pccon0 causes correct fallback to --|| style lines.



Re: pretty borders for slitherins

2019-09-25 Thread Ted Unangst
Scott Cheloha wrote:
> On Mon, Sep 23, 2019 at 06:23:32PM -0400, Ted Unangst wrote:
> > snake and worm draw boxes, but they can be prettier by using the default
> > style, which will use line drawing instead of ugly -*| characters.
> > 
> > should do the right thing on a vt100, but only tested in xterm.
> 
> It looks much prettier in an xterm but in the system console I'm
> getting question marks.  Is that a limitation of the terminal or
> am I missing the glyphs?  Dunno if this is the expected behavior
> but it is uglier than what we had before.

ah, that's disappointing. my understanding is it should revert to ascii, but
there seems to be a flaw, either in curses or my understanding. curses!



pretty borders for slitherins

2019-09-23 Thread Ted Unangst
snake and worm draw boxes, but they can be prettier by using the default
style, which will use line drawing instead of ugly -*| characters.

should do the right thing on a vt100, but only tested in xterm.

Index: snake/snake.c
===
RCS file: /home/cvs/src/games/snake/snake.c,v
retrieving revision 1.34
diff -u -p -r1.34 snake.c
--- snake/snake.c   28 Jun 2019 13:32:52 -  1.34
+++ snake/snake.c   13 Sep 2019 17:05:19 -
@@ -450,23 +450,8 @@ setup(void)
pchar([i], SNAKETAIL);
}
pchar([0], SNAKEHEAD);
-   drawbox();
+   border(0, 0, 0, 0, 0, 0, 0, 0);
refresh();
-}
-
-void
-drawbox(void)
-{
-   int i;
-
-   for (i = 1; i <= ccnt; i++) {
-   mvaddch(0, i, '-');
-   mvaddch(lcnt + 1, i, '-');
-   }
-   for (i = 0; i <= lcnt + 1; i++) {
-   mvaddch(i, 0, '|');
-   mvaddch(i, ccnt + 1, '|');
-   }
 }
 
 void
Index: worm/worm.c
===
RCS file: /home/cvs/src/games/worm/worm.c,v
retrieving revision 1.39
diff -u -p -r1.39 worm.c
--- worm/worm.c 24 Aug 2018 11:14:49 -  1.39
+++ worm/worm.c 13 Sep 2019 16:51:39 -
@@ -122,7 +122,7 @@ main(int argc, char **argv)
}
stw = newwin(1, COLS-1, 0, 0);
tv = newwin(LINES-1, COLS-1, 1, 0);
-   box(tv, '*', '*');
+   box(tv, 0, 0);
scrollok(tv, FALSE);
scrollok(stw, FALSE);
wmove(stw, 0, 0);



Re: msdosfs: remove timezone support

2019-08-30 Thread Ted Unangst
Scott Cheloha wrote:
> The FAT file system hails from Redmond and so it tracks a timezone.
> OpenBSD implicitly uses the kernel timezone when selecting which
> timezone to use when mounting a FAT file system.
> 
> This support is undocumented.
> 
> This patch removes that support.
> 
> The upshot is that your timestamps will be off if (a) you were making
> use of the kernel timezone in OpenBSD and (b) the file system in
> question is shared between machines that vary in their support for FAT
> timezones.

oh, and it seems bugged if your uptime crosses a DST transition? good riddance
i guess.



Re: Removing the kernel timezone: date(1): drop -d dst and -t minutes_west

2019-08-07 Thread Ted Unangst
Scott Cheloha wrote:
> - while ((ch = getopt(argc, argv, "ad:f:jr:ut:z:")) != -1)
> + while ((ch = getopt(argc, argv, "af:jr:uz:")) != -1)
>   switch(ch) {
>   case 'a':
>   slidetime = 1;
>   break;
> - case 'd':   /* daylight saving time */
> - tz.tz_dsttime = atoi(optarg) ? 1 : 0;
> - break;
>   case 'f':   /* parse with strptime */
>   pformat = optarg;
>   break;

as for the diff, this one looks good.



Re: Removing the kernel timezone: date(1): drop -d dst and -t minutes_west

2019-08-07 Thread Ted Unangst
Scott Cheloha wrote:
> It doesn't mean anything.  I guess I'm still gunshy about removing
> options and breaking things after the lock(1) thing.

If the default behavior changes, and the option is now meaningless, but still
results in the *same* behavior, keep the option. The user still obtains the
desired result.

If the code that makes the option work has been deleted, and the behavior will
change, delete the option. The user is not obtaining the desired result.



Re: csh - remove empty and duplicate unnecessary lines

2019-07-29 Thread Ted Unangst
Kalwe Caramalac wrote:
> Hi, this is my first diff submission, forgive me if have any error,
> if anyone has any tips on how to do this i appreciate it.

> @@ -588,7 +585,6 @@ dopopd(Char **v, struct command *t)
>  void
>  dfree(struct directory *dp)
>  {
> -
>  if (dp->di_count != 0) {

It's very common style to leave a blank line in functions without local
variables.

Also, your mailer has mangled the diff.



Re: apmd: zap useless globals

2019-07-21 Thread Ted Unangst
Klemens Nanni wrote:
> Initialize stack variables directly instead of using global state in
> between.
> 
> OK?

sure



Re: grep -ob: behave like ggrep, adapt documentation

2019-07-16 Thread Ted Unangst
Christopher Zimmermann wrote:
> Hi,
> 
> I just tried to find the byte position of a string within a binary file
> using grep. Our base grep -bo let me down because it will only display
> the position of the '\n' delimited line, not the position of the
> pattern.
> 
> That's what our grep(1) says:
> -bThe offset in bytes of a matched pattern is displayed in
>   front of the respective matched line.
> 
> what it actually does is displaying the offset of the line, not the
> pattern. I think it should read as following:
> 
> -bEach output line is preceded by its position (in bytes) in the
>   file.  If option -o is also specified, the offset in bytes of
> the actual matched pattern is displayed.

This seems like the right behavior. But the diff can be simpler?

> Index: util.c
> ===
> RCS file: /cvs/src/usr.bin/grep/util.c,v
> retrieving revision 1.59
> diff -u -p -r1.59 util.c
> --- util.c23 Jan 2019 23:00:54 -  1.59
> +++ util.c16 Jul 2019 21:16:18 -
> @@ -250,9 +250,9 @@ print:
>   if (Bflag > 0)
>   printqueue();
>   linesqueued = 0;
> - printline(l, ':', oflag ?  : NULL);
> + printline(l, ':', );
>   } else {
> - printline(l, '-', oflag ?  : NULL);
> + printline(l, '-', );
>   tail--;
>   }
>   }
> @@ -651,12 +651,14 @@ printline(str_t *line, int sep, regmatch
>   if (bflag) {
>   if (n)
>   putchar(sep);
> - printf("%lld", (long long)line->off);
> + printf("%lld",
> + (long long)line->off
> + + (pmatch && oflag ? pmatch->rm_so : 0));
>   ++n;
>   }
>   if (n)
>   putchar(sep);
> - if (pmatch)
> + if (oflag && pmatch)
>   fwrite(line->dat + pmatch->rm_so,
>   pmatch->rm_eo - pmatch->rm_so, 1, stdout);
>   else

I don't see why you needed to change this much.

-   printf("%lld", (long long)line->off);
+   printf("%lld",
+   (long long)line->off
+   + (pmatch ? pmatch->rm_so : 0));

That should be enough, no?



more doas.1 tweaks

2019-06-21 Thread Ted Unangst
I think this wording clarifies what's happening.

1. Start by talking about creating a new environment. That's what we always
do. Everything afterwards is an operation performed on this new environment.

2. Move the list of magic variables out of doas.conf. I think it's better to
document this in one place. Note that setenv comes after everything else.

3. Add DOAS_USER to the list of variables set.


Index: doas.1
===
RCS file: /cvs/src/usr.bin/doas/doas.1,v
retrieving revision 1.21
diff -u -p -r1.21 doas.1
--- doas.1  19 Jun 2019 09:50:13 -  1.21
+++ doas.1  21 Jun 2019 16:46:28 -
@@ -40,7 +40,7 @@ or
 .Fl s
 is specified.
 .Pp
-By default, the environment is reset.
+By default, a new environment is created.
 The variables
 .Ev HOME ,
 .Ev LOGNAME ,
@@ -51,6 +51,9 @@ and
 and the
 .Xr umask 2
 are set to values appropriate for the target user.
+.Ev DOAS_USER
+is set to the name of the user executing
+.Nm .
 The variables
 .Ev DISPLAY
 and
Index: doas.conf.5
===
RCS file: /cvs/src/usr.bin/doas/doas.conf.5,v
retrieving revision 1.38
diff -u -p -r1.38 doas.conf.5
--- doas.conf.5 19 Jun 2019 09:55:55 -  1.38
+++ doas.conf.5 21 Jun 2019 16:46:28 -
@@ -49,22 +49,11 @@ The user is not required to enter a pass
 After the user successfully authenticates, do not ask for a password
 again for some time.
 .It Ic keepenv
-The user's environment is maintained.
-The default is to retain the variables
-.Ev DISPLAY
-and
-.Ev TERM
-from the invoking process, reset
-.Ev HOME ,
-.Ev LOGNAME ,
-.Ev PATH ,
-.Ev SHELL ,
-and
-.Ev USER
-as appropriate for the target user, and discard the rest of the environment.
+Environment variables other than those listed in
+.Xr doas 1
+are retained when creating the environment for the new process.
 .It Ic setenv { Oo Ar variable ... Oc Oo Ar variable=value ... Oc Ic }
-In addition to the variables mentioned above, keep the space-separated
-specified variables.
+Keep or set the space-separated specified variables.
 Variables may also be removed with a leading
 .Sq -
 or set using the latter syntax.
@@ -74,6 +63,7 @@ is a
 .Ql $
 then the value to be set is taken from the existing environment
 variable of the indicated name.
+This option is processed after the default environment has been created.
 .El
 .It Ar identity
 The username to match.



doas environmental security

2019-06-20 Thread Ted Unangst
I've made some changes to how doas handles environment variables recently. The
diffs were in previous emails, and have been committed. Thanks to Sander Bos
for pointing out some particular edge cases with the old handling.

There are two (or more) ways to run doas. In the first, you use it to run some
commands as root. I use doas to run ifconfig to adjust the network on my
laptop. I also know the root password, but doas is a bit quicker and easier.
Either way, I'm fully trusted.

Another way to use doas is to setup a restricted root setting for an untrusted
user. This is treading on thin ice, in my opinion, since there may be ways for
the untrusted user to escape. The unix security model isn't very good at
letting somebody be root, but only kinda sorta.

Which brings us to environment variables. doas (previously) allowed the new
process to inherit HOME and PATH (among other variables) from the invoking
user. This reflected my attitude that, yeah, I'm running this program as
another user, but I'm still me. This however, causes trouble where if you try
to set up a restricted root setting, an evil doer can specify their own PATH
and possibly get a shell script to exec something. I think people knew this,
but maybe didn't pull on the string to see how far it goes?

So Sander looked into this, and discovered even a simple command like less is
potentially unsafe to run as restricted root. less will execute commands if
specified in the LESSOPEN environemnt variable, however even if that's not
set, it will open $HOME/.less and allow setting it from there. If you study
the manual sufficiently, it's all in there. (Setting LESSSECURE would have
been the safe thing if you wanted to restrict a user to just less, but I think
the general point remains that programs read all sorts of files, and
everything needs to exec everything else.)

After some reflection, I've been convinced that it's unlikely everybody reads
the manuals, or that the manuals are even correct or complete. So the new doas
behavior moving forward is to reset most everything to the target user's
environment.

Your action items, as we like to say in the biz, are:

1. Check existing configs for "restricted root" rules and verify that they are
run with the correct environment.

2. When updating, check for rules that intentionally use inherited environment
variables. They may need to be explicitly passing using setenv in doas.conf.




doas fix PATH inheritance

2019-06-17 Thread Ted Unangst
espie found a bug. we reset PATH in the parent too soon, and then it's not
possible to change it back via setenv. 

instead of trying to move code around, save a copy of the path before we mess
with it and make it available later. this lets setenv { PATH=$PATH } work.


Index: doas.c
===
RCS file: /cvs/src/usr.bin/doas/doas.c,v
retrieving revision 1.77
diff -u -p -r1.77 doas.c
--- doas.c  16 Jun 2019 18:16:34 -  1.77
+++ doas.c  17 Jun 2019 19:06:14 -
@@ -286,6 +286,7 @@ main(int argc, char **argv)
const char *confpath = NULL;
char *shargv[] = { NULL, NULL };
char *sh;
+   const char *p;
const char *cmd;
char cmdline[LINE_MAX];
char mypwbuf[_PW_BUF_LEN], targpwbuf[_PW_BUF_LEN];
@@ -401,6 +402,11 @@ main(int argc, char **argv)
 
authuser(mypw->pw_name, login_style, rule->options & PERSIST);
}
+
+   if ((p = getenv("PATH")) != NULL)
+   formerpath = strdup(p);
+   if (formerpath == NULL)
+   formerpath = "";
 
if (unveil(_PATH_LOGIN_CONF, "r") == -1)
err(1, "unveil");
Index: doas.h
===
RCS file: /cvs/src/usr.bin/doas/doas.h,v
retrieving revision 1.14
diff -u -p -r1.14 doas.h
--- doas.h  16 Jun 2019 18:16:34 -  1.14
+++ doas.h  17 Jun 2019 19:06:14 -
@@ -29,6 +29,8 @@ extern struct rule **rules;
 extern int nrules;
 extern int parse_errors;
 
+extern const char *formerpath;
+
 struct passwd;
 
 char **prepenv(const struct rule *, const struct passwd *,
Index: env.c
===
RCS file: /cvs/src/usr.bin/doas/env.c,v
retrieving revision 1.8
diff -u -p -r1.8 env.c
--- env.c   17 Jun 2019 16:01:26 -  1.8
+++ env.c   17 Jun 2019 19:06:14 -
@@ -28,6 +28,8 @@
 
 #include "doas.h"
 
+const char *formerpath;
+
 struct envnode {
RB_ENTRY(envnode) node;
const char *key;
@@ -198,8 +200,12 @@ fillenv(struct env *env, const char **en
/* assign value or inherit from environ */
if (eq) {
val = eq + 1;
-   if (*val == '$')
-   val = getenv(val + 1);
+   if (*val == '$') {
+   if (strcmp(val + 1, "PATH") == 0)
+   val = formerpath;
+   else
+   val = getenv(val + 1);
+   }
} else {
val = getenv(name);
}



Re: new env defaults for doas

2019-06-17 Thread Ted Unangst
Todd C. Miller wrote:
> On Mon, 17 Jun 2019 12:58:00 -0400, "Ted Unangst" wrote:
> 
> > Committed this. I'm not entirely happy with the wording. I think hiding them
> > under an option in the config man page is the wrong place. The default
> > behavior should be documented in a more default place.
> 
> I would just place that bit either before the options are described
> or after the options in an "Execution environment" sub-section.

Not sure what you mean. This diff does put it before the options.

same place as su, although su has other problems. fortunately, we don't have
the same combination of possibilities as su.



Re: new env defaults for doas

2019-06-17 Thread Ted Unangst
Ted Unangst wrote:
> Yes, I think it's better to always reset these things. Here's a diff.
> 
> 1. Always set HOME, PATH, SHELL etc to the target.

Committed this. I'm not entirely happy with the wording. I think hiding them
under an option in the config man page is the wrong place. The default
behavior should be documented in a more default place.

Also mention working directory is not changed.


Index: doas.1
===
RCS file: /cvs/src/usr.bin/doas/doas.1,v
retrieving revision 1.19
diff -u -p -r1.19 doas.1
--- doas.1  4 Sep 2016 15:20:37 -   1.19
+++ doas.1  17 Jun 2019 16:57:26 -
@@ -40,6 +40,23 @@ or
 .Fl s
 is specified.
 .Pp
+By default, the environment is reset.
+The variables
+.Ev HOME ,
+.Ev LOGNAME ,
+.Ev PATH ,
+.Ev SHELL ,
+and
+.Ev USER
+are set to values appropriate for the target user.
+The variables
+.Ev DISPLAY
+and
+.Ev TERM
+are inherited from the current environment.
+This behavior may be modified by the config file.
+The working directory is not changed.
+.Pp
 The options are as follows:
 .Bl -tag -width tenletters
 .It Fl a Ar style



Re: new env defaults for doas

2019-06-16 Thread Ted Unangst
Martijn van Duren wrote:
> > 2. When doing keepenv, nothing changes, except addition of above.
> 
> It feels inconsistent to make keepenv behave different here.
> - It may allow certain applications to behave unexpected compared to
>   without keepenv (e.g. scripts that use $HOME/.cache).
> - The values are hard to retrofit into doas.conf by the end-user, while
>   it's easy to keep the original with setenv { HOME ... }.

Yes, I think it's better to always reset these things. Here's a diff.

1. Always set HOME, PATH, SHELL etc to the target.

2. Without keepenv, other environment variables are discarded.

3. With keepenv, other variables are retained, but the above are still reset.

4. Possible to override this with setenv.

This is much more consistent and predictable.

Index: doas.conf.5
===
RCS file: /cvs/src/usr.bin/doas/doas.conf.5,v
retrieving revision 1.36
diff -u -p -r1.36 doas.conf.5
--- doas.conf.5 16 Jun 2019 18:16:34 -  1.36
+++ doas.conf.5 16 Jun 2019 18:20:20 -
@@ -54,6 +54,14 @@ The default is to reset the environment,
 .Ev DISPLAY
 and
 .Ev TERM .
+The variables
+.Ev HOME ,
+.Ev LOGNAME ,
+.Ev PATH ,
+.Ev SHELL ,
+and
+.Ev USER
+are always reset.
 .It Ic setenv { Oo Ar variable ... Oc Oo Ar variable=value ... Oc Ic }
 In addition to the variables mentioned above, keep the space-separated
 specified variables.
Index: env.c
===
RCS file: /cvs/src/usr.bin/doas/env.c,v
retrieving revision 1.7
diff -u -p -r1.7 env.c
--- env.c   16 Jun 2019 18:16:34 -  1.7
+++ env.c   16 Jun 2019 18:20:20 -
@@ -85,6 +85,10 @@ static struct env *
 createenv(const struct rule *rule, const struct passwd *mypw,
 const struct passwd *targpw)
 {
+   static const char *copyset[] = {
+   "DISPLAY", "TERM",
+   NULL
+   };
struct env *env;
u_int i;
 
@@ -95,6 +99,13 @@ createenv(const struct rule *rule, const
env->count = 0;
 
addnode(env, "DOAS_USER", mypw->pw_name);
+   addnode(env, "HOME", targpw->pw_dir);
+   addnode(env, "LOGNAME", targpw->pw_name);
+   addnode(env, "PATH", getenv("PATH"));
+   addnode(env, "SHELL", targpw->pw_shell);
+   addnode(env, "USER", targpw->pw_name);
+
+   fillenv(env, copyset);
 
if (rule->options & KEEPENV) {
extern const char **environ;
@@ -124,19 +135,6 @@ createenv(const struct rule *rule, const
env->count++;
}
}
-   } else {
-   static const char *copyset[] = {
-   "DISPLAY", "TERM",
-   NULL
-   };
-
-   addnode(env, "HOME", targpw->pw_dir);
-   addnode(env, "LOGNAME", targpw->pw_name);
-   addnode(env, "PATH", getenv("PATH"));
-   addnode(env, "SHELL", targpw->pw_shell);
-   addnode(env, "USER", targpw->pw_name);
-
-   fillenv(env, copyset);
}
 
return env;



Re: new env defaults for doas

2019-06-15 Thread Ted Unangst
Martijn van Duren wrote:
> I'm not convinced that LOGIN_SETPATH is a good idea here. From what I
> gathered that sets PATH from login.conf(5), while most environments I
> know will use .profile to set it and could cause unexpected behaviour
> if the my and targ PATH are reset to unexpected values.
> 
> I would vote to safe PATH from the caller instead of getting a likely
> unexpected or incomplete PATH environment based on login.conf.

Short answer is this is what su - does. We can set a default safe path, but
there's no reason that's better than what's in .profile either. With
login.conf, we get a safe default and a way for the admin to fix it if it's
not what they want.



new env defaults for doas

2019-06-12 Thread Ted Unangst
This has come up a few times before. For background, the default rule for doas
is to copy a few environment settings from the user and omit the rest. This is
to prevent confusion, and also supposedly for security. However, some of the
alleged safe variables like PATH probably aren't that safe. And things like
USER are confusing? And why even bother with MAIL? The list is kinda adhoc and
mostly copied from what I understood sudo to do at the time, but I believe
sudo has changed defaults as well.

So here's a new model which I think is safer and more consistent.

1. Always add a DOAS_USER with the invoking user's name.

2. When doing keepenv, nothing changes, except addition of above.

3. When starting with a new environment, fill in USER and HOME and such with
the values of the target user, instead of copying from the invoking user. You
run a command as a user, you should have that user's environment.

4. DISPLAY and TERM are still copied, since they represent a description of
the actual environment in which we are running. (Not sure how useful display
is without xauth cookies, but not my problem.)

5. As before, anything can be overridden with setenv in the config.

This is a kinda breaking change, but probably in a good way.
For simple configurations, I expect nothing changes. For more complicated
setups, this new handling is probably closer to what the admin expects or
desires.


Index: doas.c
===
RCS file: /home/cvs/src/usr.bin/doas/doas.c,v
retrieving revision 1.76
diff -u -p -r1.76 doas.c
--- doas.c  12 Jun 2019 02:50:29 -  1.76
+++ doas.c  13 Jun 2019 02:11:07 -
@@ -421,6 +421,7 @@ main(int argc, char **argv)
errx(1, "no passwd entry for target");
 
if (setusercontext(NULL, targpw, target, LOGIN_SETGROUP |
+   LOGIN_SETPATH |
LOGIN_SETPRIORITY | LOGIN_SETRESOURCES | LOGIN_SETUMASK |
LOGIN_SETUSER) != 0)
errx(1, "failed to set user context for target");
@@ -439,8 +440,13 @@ main(int argc, char **argv)
syslog(LOG_AUTHPRIV | LOG_INFO, "%s ran command %s as %s from %s",
mypw->pw_name, cmdline, targpw->pw_name, cwd);
 
-   envp = prepenv(rule);
+   envp = prepenv(rule, mypw, targpw);
 
+   if (rule->cmd) {
+   /* do this again after setusercontext reset it */
+   if (setenv("PATH", safepath, 1) == -1)
+   err(1, "failed to set PATH '%s'", safepath);
+   }
execvpe(cmd, argv, envp);
 fail:
if (errno == ENOENT)
Index: doas.conf.5
===
RCS file: /home/cvs/src/usr.bin/doas/doas.conf.5,v
retrieving revision 1.35
diff -u -p -r1.35 doas.conf.5
--- doas.conf.5 7 Feb 2018 05:13:57 -   1.35
+++ doas.conf.5 13 Jun 2019 01:41:18 -
@@ -51,15 +51,9 @@ again for some time.
 .It Ic keepenv
 The user's environment is maintained.
 The default is to reset the environment, except for the variables
-.Ev DISPLAY ,
-.Ev HOME ,
-.Ev LOGNAME ,
-.Ev MAIL ,
-.Ev PATH ,
-.Ev TERM ,
-.Ev USER
+.Ev DISPLAY
 and
-.Ev USERNAME .
+.Ev TERM .
 .It Ic setenv { Oo Ar variable ... Oc Oo Ar variable=value ... Oc Ic }
 In addition to the variables mentioned above, keep the space-separated
 specified variables.
Index: doas.h
===
RCS file: /home/cvs/src/usr.bin/doas/doas.h,v
retrieving revision 1.13
diff -u -p -r1.13 doas.h
--- doas.h  6 Apr 2017 21:12:06 -   1.13
+++ doas.h  13 Jun 2019 01:10:29 -
@@ -29,7 +29,10 @@ extern struct rule **rules;
 extern int nrules;
 extern int parse_errors;
 
-char **prepenv(const struct rule *);
+struct passwd;
+
+char **prepenv(const struct rule *, const struct passwd *,
+const struct passwd *);
 
 #define PERMIT 1
 #define DENY   2
Index: env.c
===
RCS file: /home/cvs/src/usr.bin/doas/env.c,v
retrieving revision 1.6
diff -u -p -r1.6 env.c
--- env.c   6 Apr 2017 21:12:06 -   1.6
+++ env.c   13 Jun 2019 02:12:57 -
@@ -24,6 +24,7 @@
 #include 
 #include 
 #include 
+#include 
 
 #include "doas.h"
 
@@ -38,6 +39,8 @@ struct env {
u_int count;
 };
 
+static void fillenv(struct env *env, const char **envlist);
+
 static int
 envcmp(struct envnode *a, struct envnode *b)
 {
@@ -68,8 +71,19 @@ freenode(struct envnode *node)
free(node);
 }
 
+static void
+addnode(struct env *env, const char *key, const char *value)
+{
+   struct envnode *node;
+
+   node = createnode(key, value);
+   RB_INSERT(envtree, >root, node);
+   env->count++;
+}
+
 static struct env *
-createenv(const struct rule *rule)
+createenv(const struct rule *rule, const struct passwd *mypw,
+const struct passwd *targpw)
 {
struct env *env;
u_int i;
@@ -80,6 +94,8 @@ createenv(const struct rule *rule)

Re: fsync(2) and I/O errors

2019-06-11 Thread Ted Unangst
Maximilian Lorlacks wrote:
> This looks okay to me.
> 
> (plus two months ping)

oh, good news, committed two months ago. sorry, forgot to reply.

> 
> ‐‐‐ Original Message ‐‐‐
> On Tuesday, April 16, 2019 8:19 PM, Ted Unangst  wrote:
> 
> > Oh, right, I reworded it slightly, but I think this is something we should
> > note.
> >
> > Index: fsync.2
> >
> > =
> >
> > RCS file: /home/cvs/src/lib/libc/sys/fsync.2,v
> > retrieving revision 1.14
> > diff -u -p -r1.14 fsync.2
> > --- fsync.2 10 Sep 2015 17:55:21 - 1.14
> > +++ fsync.2 16 Apr 2019 20:18:03 -
> > @@ -66,6 +66,16 @@ and
> > .Fn fdatasync
> > should be used by programs that require a file to be in a known state,
> > for example, in building a simple transaction facility.
> > +.Pp
> > +If
> > +.Fn fsync
> > +or
> > +.Fn fdatasync
> > +fails with
> > +.Er EIO ,
> > +the state of the on-disk data may have been only partially written.
> > +To guard against potential inconsistency, future calls will continue 
> > failing
> > +until all references to the file are closed.
> > .Sh RETURN VALUES
> > .Rv -std fsync fdatasync
> > .Sh ERRORS
> 
> 



Re: use getpwuid_r in doas

2019-06-10 Thread Ted Unangst
Ted Unangst wrote:
> I have a coming change which will need to access both the calling user and
> target users' passwd entries. In order to accomplish this, we need to switch
> to the reentrant flavor of getpwuid. No behaviorial change, but I think this
> is clearer and less error prone as well, versus reusing a pointer to static
> storage.

Improve a few more things noticed by martijn. We don't need to copy strings
that have long lifetimes. Also, fix some error checks to be more precise.

Index: doas.c
===
RCS file: /cvs/src/usr.bin/doas/doas.c,v
retrieving revision 1.75
diff -u -p -r1.75 doas.c
--- doas.c  10 Jun 2019 18:11:27 -  1.75
+++ doas.c  10 Jun 2019 18:15:34 -
@@ -288,7 +288,6 @@ main(int argc, char **argv)
char *sh;
const char *cmd;
char cmdline[LINE_MAX];
-   char myname[_PW_NAME_LEN + 1];
char mypwbuf[_PW_BUF_LEN], targpwbuf[_PW_BUF_LEN];
struct passwd mypwstore, targpwstore;
struct passwd *mypw, *targpw;
@@ -349,10 +348,10 @@ main(int argc, char **argv)
usage();
 
rv = getpwuid_r(uid, , mypwbuf, sizeof(mypwbuf), );
-   if (rv != 0 || mypw == NULL)
+   if (rv != 0)
err(1, "getpwuid_r failed");
-   if (strlcpy(myname, mypw->pw_name, sizeof(myname)) >= sizeof(myname))
-   errx(1, "pw_name too long");
+   if (mypw == NULL)
+   errx(1, "no passwd entry for self");
ngroups = getgroups(NGROUPS_MAX, groups);
if (ngroups == -1)
err(1, "can't get groups");
@@ -361,9 +360,7 @@ main(int argc, char **argv)
if (sflag) {
sh = getenv("SHELL");
if (sh == NULL || *sh == '\0') {
-   shargv[0] = strdup(mypw->pw_shell);
-   if (shargv[0] == NULL)
-   err(1, NULL);
+   shargv[0] = mypw->pw_shell;
} else
shargv[0] = sh;
argv = shargv;
@@ -394,7 +391,7 @@ main(int argc, char **argv)
if (!permit(uid, groups, ngroups, , target, cmd,
(const char **)argv + 1)) {
syslog(LOG_AUTHPRIV | LOG_NOTICE,
-   "failed command for %s: %s", myname, cmdline);
+   "failed command for %s: %s", mypw->pw_name, cmdline);
errc(1, EPERM, NULL);
}
 
@@ -402,7 +399,7 @@ main(int argc, char **argv)
if (nflag)
errx(1, "Authorization required");
 
-   authuser(myname, login_style, rule->options & PERSIST);
+   authuser(mypw->pw_name, login_style, rule->options & PERSIST);
}
 
if (unveil(_PATH_LOGIN_CONF, "r") == -1)
@@ -418,7 +415,9 @@ main(int argc, char **argv)
err(1, "pledge");
 
rv = getpwuid_r(target, , targpwbuf, sizeof(targpwbuf), 
);
-   if (rv != 0 || targpw == NULL)
+   if (rv != 0)
+   err(1, "getpwuid_r failed");
+   if (targpw == NULL)
errx(1, "no passwd entry for target");
 
if (setusercontext(NULL, targpw, target, LOGIN_SETGROUP |
@@ -438,7 +437,7 @@ main(int argc, char **argv)
err(1, "pledge");
 
syslog(LOG_AUTHPRIV | LOG_INFO, "%s ran command %s as %s from %s",
-   myname, cmdline, targpw->pw_name, cwd);
+   mypw->pw_name, cmdline, targpw->pw_name, cwd);
 
envp = prepenv(rule);
 



use getpwuid_r in doas

2019-05-21 Thread Ted Unangst
I have a coming change which will need to access both the calling user and
target users' passwd entries. In order to accomplish this, we need to switch
to the reentrant flavor of getpwuid. No behaviorial change, but I think this
is clearer and less error prone as well, versus reusing a pointer to static
storage.

Index: doas.c
===
RCS file: /home/cvs/src/usr.bin/doas/doas.c,v
retrieving revision 1.74
diff -u -p -r1.74 doas.c
--- doas.c  17 Jan 2019 05:35:35 -  1.74
+++ doas.c  21 May 2019 17:04:04 -
@@ -289,13 +289,15 @@ main(int argc, char **argv)
const char *cmd;
char cmdline[LINE_MAX];
char myname[_PW_NAME_LEN + 1];
-   struct passwd *pw;
+   char mypwbuf[_PW_BUF_LEN], targpwbuf[_PW_BUF_LEN];
+   struct passwd mypwstore, targpwstore;
+   struct passwd *mypw, *targpw;
const struct rule *rule;
uid_t uid;
uid_t target = 0;
gid_t groups[NGROUPS_MAX + 1];
int ngroups;
-   int i, ch;
+   int i, ch, rv;
int sflag = 0;
int nflag = 0;
char cwdpath[PATH_MAX];
@@ -346,10 +348,10 @@ main(int argc, char **argv)
} else if ((!sflag && !argc) || (sflag && argc))
usage();
 
-   pw = getpwuid(uid);
-   if (!pw)
-   err(1, "getpwuid failed");
-   if (strlcpy(myname, pw->pw_name, sizeof(myname)) >= sizeof(myname))
+   rv = getpwuid_r(uid, , mypwbuf, sizeof(mypwbuf), );
+   if (rv != 0 || mypw == NULL)
+   err(1, "getpwuid_r failed");
+   if (strlcpy(myname, mypw->pw_name, sizeof(myname)) >= sizeof(myname))
errx(1, "pw_name too long");
ngroups = getgroups(NGROUPS_MAX, groups);
if (ngroups == -1)
@@ -359,7 +361,7 @@ main(int argc, char **argv)
if (sflag) {
sh = getenv("SHELL");
if (sh == NULL || *sh == '\0') {
-   shargv[0] = strdup(pw->pw_shell);
+   shargv[0] = strdup(mypw->pw_shell);
if (shargv[0] == NULL)
err(1, NULL);
} else
@@ -415,11 +417,11 @@ main(int argc, char **argv)
if (pledge("stdio rpath getpw exec id", NULL) == -1)
err(1, "pledge");
 
-   pw = getpwuid(target);
-   if (!pw)
+   rv = getpwuid_r(target, , targpwbuf, sizeof(targpwbuf), 
);
+   if (rv != 0 || targpw == NULL)
errx(1, "no passwd entry for target");
 
-   if (setusercontext(NULL, pw, target, LOGIN_SETGROUP |
+   if (setusercontext(NULL, targpw, target, LOGIN_SETGROUP |
LOGIN_SETPRIORITY | LOGIN_SETRESOURCES | LOGIN_SETUMASK |
LOGIN_SETUSER) != 0)
errx(1, "failed to set user context for target");
@@ -436,7 +438,7 @@ main(int argc, char **argv)
err(1, "pledge");
 
syslog(LOG_AUTHPRIV | LOG_INFO, "%s ran command %s as %s from %s",
-   myname, cmdline, pw->pw_name, cwd);
+   myname, cmdline, targpw->pw_name, cwd);
 
envp = prepenv(rule);
 



Re: snake: unveil + pledge earlier

2019-05-20 Thread Ted Unangst
Jake Champlin wrote:
> - readscores(1);
>   penalty = loot = 0;
>   initscr();
> + if (unveil(scorepath, "rwc") == -1)
> + err(1, "unveil");
> +#ifdef LOGGING
> + if (unveil(logpath, "rwc") == -1)
> + err(1, "unveil");
> + logfile = fopen(logpath, "a");
> +#endif
> + readscores(1);
> 
>   if (pledge("stdio tty", NULL) == -1)
>   err(1, "pledge");

We're unveiling the same file we're going to immediately open. And then we
never open it later, as proved by pledge without rpath. There doesn't seem to
be any benefit in this case.

If we needed to reopen these files at some later point, this would be the
right thing.



Re: caesar(6) to accept negative argument

2019-05-15 Thread Ted Unangst
Otto Moerbeek wrote:
> We're computing modulo 26 here. Negative numbers have a positive
> equivalent. So you diff adds code for no benefit.

I think the amount of code added is an acceptable cost for improved user
experience. We could use this argument to remove subtraction from bc, but that
would be silly.



Re: free() sizes for ahc(4)

2019-05-14 Thread Ted Unangst
Miod Vallat wrote:
> Note ahc_set_name() gets invoked with the dv_xname field of a struct
> device, so it's not a good idea to free anything, should it be invoked
> more than once.

nice catch.



Re: softraid(4): fix wrong malloc size and zero sized free calls

2019-05-13 Thread Ted Unangst
Jan Klemkow wrote:
> Hi,
> 
> The diff mainly add sizes to free(9) calls.  But, while here fix a
> malloc(9) call with the wrong size in sr_ioctl_installboot().
> omi->omi_som is allocated with size of struct sr_meta_crypto, but used
> as struct sr_meta_boot later.
> 
> One free(9) with size zero left over in sr_discipline_free().  As, the
> allocated size of sd->sd_vol.sv_chunks is not ascertainable here without
> larger changes.
> 
> Build and run the kernel for testing.

The change in installboot for the size is suspicious. I would leave that out.
Especially if you haven't tested installboot to a crypto softraid. :)



Re: add newline for tpm0: dmesg line

2019-05-13 Thread Ted Unangst
f. holop wrote:
> dmesg:
> 
> ...
> acpibtn2 at acpi0: PWRB
> tpm0 at acpi0: TPM_ addr 0xfed4/0x5000acpivideo0 at acpi0: GFX0
> acpivout0 at acpivideo0: DD1F
> ...

thanks



Re: sv(4): fix free with zero size

2019-05-13 Thread Ted Unangst
Jan Klemkow wrote:
> Hi,
> 
> The following diff fixes "free with zero size" in sv(4).  Builds and
> stats the kernel with sv at pci and audio at sv enabled.

ok



Re: chroot vs su vs doas

2019-05-13 Thread Ted Unangst
Martijn van Duren wrote:
> >> Would
> >> doas -c /rootdir somecmd
> >> be of any use ?
> > 
> > Not particularly opposed, but the extend of this option should be
> > examined. E.g. do we want to extend it to the config to be something
> > similar to -u and limit it's use for certain commands?
> >
> Working this out I'm not particularly a fan of the extra code this would
> take, but the diff below seems to do the trick.

This seems like feature creep. We already have a command to chroot and switch
user.



Re: chroot vs su vs doas

2019-05-13 Thread Ted Unangst
Martijn van Duren wrote:
> > But what would it hurt to allow root usage ?
> > Specifically,
> > 
> > doas -u ${BUILDUSER} some unquoted command
> > 
> > as run by root.  This would not open any security hole, would it ?
> 
> I don't see any and I've been bitten by having a rootshell open and
> typing doas out of habit.

The reason there's no builtin config is to prevent confusion, even if it
sometimes causes mild annoyance. When there are invisible rules, it becomes
harder to know which rule is actually being taken.

For example, your rule below doesn't include keepenv. So next week we're going
to get bug reports that things work when run as a user, but not as root. And
for exactly the same reason, people only half set things up. The fact that the
default appears to work sometimes makes things even more annoying. And I
don't think the default should just be changed to include keepenv, because
maybe that's not what people want and then we'd need to explain how to undo
that.


> + static struct rule allowroot = {
> + .action = PERMIT,
> + .options = NOPASS,
> + .ident = NULL,
> + .target = NULL,
> + .cmd = NULL,
> + .cmdargs = NULL,
> + .envlist = NULL
> + };



Re: cmdfile for config -ef

2019-05-11 Thread Ted Unangst
Benjamin Baier wrote:
> On Sat, 11 May 2019 11:18:02 -0400
> "Ted Unangst"  wrote:
> 
> > This adds a new option to config, -c cmdfile, that allows reading commands
> > from a file instead of stdin. This makes it easier to script.
> 
> Interesting. Can the cmdfile be /bsd ?
> So something like config -u but without the need of /dev/mem ?

No, the intended use case is not to copy options between kernels, but to apply
a consistent set of changes after linking.



cmdfile for config -ef

2019-05-11 Thread Ted Unangst
This adds a new option to config, -c cmdfile, that allows reading commands
from a file instead of stdin. This makes it easier to script.


Index: config.8
===
RCS file: /home/cvs/src/usr.sbin/config/config.8,v
retrieving revision 1.66
diff -u -p -r1.66 config.8
--- config.825 Apr 2018 12:01:11 -  1.66
+++ config.811 May 2019 15:16:45 -
@@ -43,6 +43,7 @@
 .Op Fl s Ar srcdir
 .Op Ar config-file
 .Nm config
+.Op Fl c Ar cmdfile
 .Op Fl u
 .Op Fl f | o Ar outfile
 .Fl e
@@ -103,6 +104,9 @@ directories above the build directory).
 .Pp
 For kernel modification, the options are as follows:
 .Bl -tag -width Ds
+.It Fl c Ar cmdfile
+Read commands from the specified file instead of the standard input.
+Save and quit automatically when the end of file is reached.
 .It Fl e
 Allows the modification of kernel device configuration (see
 .Xr boot_config 8 ) .
Index: main.c
===
RCS file: /home/cvs/src/usr.sbin/config/main.c,v
retrieving revision 1.59
diff -u -p -r1.59 main.c
--- main.c  22 Jun 2017 15:57:16 -  1.59
+++ main.c  11 May 2019 15:00:34 -
@@ -82,7 +82,7 @@ usage(void)
 
fprintf(stderr,
"usage: %s [-p] [-b builddir] [-s srcdir] [config-file]\n"
-   "   %s [-u] [-f | -o outfile] -e infile\n",
+   "   %s [-c cmdfile] [-u] [-f | -o outfile] -e infile\n",
__progname, __progname);
 
exit(1);
@@ -92,6 +92,7 @@ int pflag = 0;
 char *sflag = NULL;
 char *bflag = NULL;
 char *startdir;
+char *cmdfile;
 
 int
 main(int argc, char *argv[])
@@ -105,9 +106,11 @@ main(int argc, char *argv[])
err(1, "pledge");
 
pflag = eflag = uflag = fflag = 0;
-   while ((ch = getopt(argc, argv, "epfb:s:o:u")) != -1) {
+   while ((ch = getopt(argc, argv, "c:epfb:s:o:u")) != -1) {
switch (ch) {
-
+   case 'c':
+   cmdfile = optarg;
+   break;
case 'o':
outfile = optarg;
break;
Index: misc.c
===
RCS file: /home/cvs/src/usr.sbin/config/misc.c,v
retrieving revision 1.9
diff -u -p -r1.9 misc.c
--- misc.c  2 Oct 2011 22:20:49 -   1.9
+++ misc.c  11 May 2019 15:08:56 -
@@ -38,13 +38,10 @@
 extern int verbose;
 
 int
-ask_cmd(cmd_t *cmd)
+parse_cmd(cmd_t *cmd, char *lbuf)
 {
-   char lbuf[100], *cp, *buf;
+   char *cp, *buf;
 
-   /* Get input */
-   if (fgets(lbuf, sizeof lbuf, stdin) == NULL)
-   errx(1, "eof");
lbuf[strcspn(lbuf, "\n")] = '\0';
if (verbose)
printf("%s\n", lbuf);
@@ -59,6 +56,17 @@ ask_cmd(cmd_t *cmd)
strlcpy(cmd->args, buf, sizeof cmd->args);
 
return (0);
+}
+
+int
+ask_cmd(cmd_t *cmd)
+{
+   char lbuf[100];
+
+   /* Get input */
+   if (fgets(lbuf, sizeof lbuf, stdin) == NULL)
+   errx(1, "eof");
+   return parse_cmd(cmd, lbuf);
 }
 
 int
Index: misc.h
===
RCS file: /home/cvs/src/usr.sbin/config/misc.h,v
retrieving revision 1.4
diff -u -p -r1.4 misc.h
--- misc.h  3 Jun 2003 00:52:35 -   1.4
+++ misc.h  11 May 2019 15:10:04 -
@@ -33,6 +33,7 @@
 
 /* Prototypes */
 int ask_cmd(cmd_t *);
+int parse_cmd(cmd_t *, char *);
 int ask_yn(const char *);
 
 #endif /* _MISC_H */
Index: ukcutil.c
===
RCS file: /home/cvs/src/usr.sbin/config/ukcutil.c,v
retrieving revision 1.23
diff -u -p -r1.23 ukcutil.c
--- ukcutil.c   27 Sep 2017 15:14:52 -  1.23
+++ ukcutil.c   11 May 2019 15:14:25 -
@@ -29,6 +29,7 @@
 #include 
 
 #include 
+#include 
 #include 
 #include 
 #include 
@@ -1295,13 +1296,61 @@ add_history(int devno, short unit, short
 }
 
 int
+config_fromfile(const char *cmdfile) {
+   FILE *fp;
+   cmd_t cmd;
+   int i;
+
+   fp = fopen(cmdfile, "r");
+   if (fp == NULL)
+   err(1, "open %s", cmdfile);
+
+   /* Set up command table pointer */
+   cmd.table = cmd_table;
+
+   /* Edit cycle */
+   do {
+   char lbuf[100];
+
+   /* Get input */
+   if (fgets(lbuf, sizeof lbuf, fp) == NULL)
+   break;
+   parse_cmd(, lbuf);
+
+   if (cmd.cmd[0] == '\0')
+   continue;
+   for (i = 0; cmd_table[i].cmd != NULL; i++)
+   if (strstr(cmd_table[i].cmd, cmd.cmd) ==
+   cmd_table[i].cmd)
+   break;
+
+   /* Check for valid command */
+   if (cmd_table[i].cmd == NULL) {
+   printf("Invalid command '%s'\n", cmd.cmd);
+  

Re: [patch] improve strptime(3) %z timezone parsing

2019-05-10 Thread Ted Unangst
Ingo Schwarze wrote:
> Ouch.  No, it does not.  Thanks for spotting the regression.
> 
> The following patch preserves the parsing behaviour
> and correctly stores the number of seconds into tm_gmtoff.

oops, missed that case too. this looks correct.



Re: [patch] improve strptime(3) %z timezone parsing

2019-05-10 Thread Ted Unangst
Ingo Schwarze wrote:
> Now let's get to the more serious part.
> Hiltjo observed that %Z and %z produce wrong results.
> 
> First of all, neither POSIX nor XPG define tm_gmtoff nor %Z nor %z:
> 
>   https://pubs.opengroup.org/onlinepubs/9699919799/basedefs/time.h.html
>   https://pubs.opengroup.org/onlinepubs/9699919799/functions/strptime.html
> 
> So i think the best way to find out what tm_gmtoff should be is to
> understand how programs in our own tree use it.
> 
> Here is a (hopefully) complete list of the users in OpenBSD base.
> The following files expect it to contain seconds:
>  - lib/libc/time/strftime.c
>  - lib/libc/time/wcsftime.c
>  - usr.sbin/smtpd/to.c
>  - usr.sbin/snmpd/mib.c
>  - usr.sbin/cron/cron.c
>  - usr.bin/ftp/util.c
>  - usr.bin/cvs/entries.c
>  - usr.bin/rcs/rcstime.c
>  - gnu/usr.bin/perl/time64.c
> 
> I failed to find any users that do *not* expect seconds.
> So my conclusion is that the documentation is right and
> what the code in strptime.c does is wrong.
> 
> Here is a patch to fix the code.

this looks good to me. ok.



Re: ssl(8), fix text about web browsers and SAN

2019-05-10 Thread Ted Unangst
Stuart Henderson wrote:
> it's standard behaviour for web browsers to not use hostnames in
> Subject at all but require SAN. current ssl(8) text suggests "some new"
> and "deprecated" rather than "stopped supporting".
> 
> comments/ok?

I was trying to avoid argument "but my browser still works" :) but I agree
this wording is closer to reality. ok.



Re: [patch] improve strptime(3) %z timezone parsing

2019-05-09 Thread Ted Unangst
Ingo Schwarze wrote:
> I'm not mixing anything else into this diff.  The other bugs should
> be handled separately.
> 
> OK?

Works for me. (with additional comment removal)



Re: stack trace / free(0) in isascan()

2019-05-09 Thread Ted Unangst
Sebastien Marie wrote:
> 
> So calling free() with cf->cf_attach->ca_devsize should be fine.
> Diff below.

ok. didn't get to this one yet.



less discriminatory battlestar

2019-05-08 Thread Ted Unangst
there are lists of annointed usernames in battlestar. this creates an unfair
playing field! worse, there is a list of "bad" people! and i'm almost one of
them!

-static const char *const badguys[] = {
-   "wnj",
-   "root",
-   "ted",
-   0
-};


Index: com1.c
===
RCS file: /home/cvs/src/games/battlestar/com1.c,v
retrieving revision 1.15
diff -u -p -r1.15 com1.c
--- com1.c  31 Dec 2015 17:51:19 -  1.15
+++ com1.c  9 May 2019 03:31:36 -
@@ -129,7 +129,7 @@ news(void)
}
rythmn = ourtime - ourtime % CYCLE;
}
-   if (!wiz && !tempwiz)
+   if (!tempwiz)
if ((TestBit(inven, TALISMAN) || TestBit(wear, TALISMAN)) && 
(TestBit(inven, MEDALION) || TestBit(wear, MEDALION)) && (TestBit(inven, 
AMULET) || TestBit(wear, AMULET))) {
tempwiz = 1;
puts("The three amulets glow and reenforce each other 
in power.\nYou are now a wizard.");
Index: com4.c
===
RCS file: /home/cvs/src/games/battlestar/com4.c,v
retrieving revision 1.15
diff -u -p -r1.15 com4.c
--- com4.c  31 Dec 2015 17:51:19 -  1.15
+++ com4.c  9 May 2019 03:32:49 -
@@ -53,7 +53,7 @@ take(unsigned int from[])
printf("%s:\n", objsht[value]);
heavy = (carrying + objwt[value]) <= WEIGHT;
bulky = (encumber + objcumber[value]) <= CUMBER;
-   if ((TestBit(from, value) || wiz || tempwiz) && heavy 
&& bulky && !TestBit(inven, value)) {
+   if ((TestBit(from, value) || tempwiz) && heavy && bulky 
&& !TestBit(inven, value)) {
SetBit(inven, value);
carrying += objwt[value];
encumber += objcumber[value];
Index: com6.c
===
RCS file: /home/cvs/src/games/battlestar/com6.c,v
retrieving revision 1.24
diff -u -p -r1.24 com6.c
--- com6.c  7 Feb 2018 20:22:23 -   1.24
+++ com6.c  9 May 2019 03:33:19 -
@@ -130,13 +130,10 @@ post(char ch)
 
if (score_fp != NULL) {
fprintf(score_fp, "%s  %31s  %c%20s", date, username, ch, 
rate());
-   if (wiz)
-   fprintf(score_fp, "   wizard\n");
+   if (tempwiz)
+   fprintf(score_fp, "   WIZARD!\n");
else
-   if (tempwiz)
-   fprintf(score_fp, "   WIZARD!\n");
-   else
-   fprintf(score_fp, "\n");
+   fprintf(score_fp, "\n");
}
sigprocmask(SIG_SETMASK, , (sigset_t *)0);
 }
Index: cypher.c
===
RCS file: /home/cvs/src/games/battlestar/cypher.c,v
retrieving revision 1.19
diff -u -p -r1.19 cypher.c
--- cypher.c31 Dec 2015 17:51:19 -  1.19
+++ cypher.c9 May 2019 03:34:13 -
@@ -105,7 +105,7 @@ cypher(void)
break;
 
case UP:
-   if (location[position].access || wiz || tempwiz) {
+   if (location[position].access || tempwiz) {
if (!location[position].access)
puts("Zap!  A gust of wind lifts you 
up.");
if (!moveplayer(location[position].up, AHEAD))
@@ -318,7 +318,7 @@ cypher(void)
break;
 
case SU:
-   if (wiz || tempwiz) {
+   if (tempwiz) {
getnum(, "\nRoom (was %d) = ", 
position);
getnum(, "Time (was %d) = ", ourtime);
getnum(, "Fuel (was %d) = ", fuel);
@@ -326,8 +326,8 @@ cypher(void)
getnum(, "CUMBER (was %d) = ", CUMBER);
getnum(, "WEIGHT (was %d) = ", WEIGHT);
getnum(, "Clock (was %d) = ", 
ourclock);
-   if (getnum(, "Wizard (was %d, %d) = ", 
wiz, tempwiz) != -1 && !junk)
-   tempwiz = wiz = 0;
+   if (getnum(, "Wizard (was %d) = ", 
tempwiz) != -1 && !junk)
+   tempwiz = 0;
printf("\nDONE.\n");
return (0); /* No commands after a SU */
} else
Index: extern.h
===
RCS file: /home/cvs/src/games/battlestar/extern.h,v
retrieving revision 1.20
diff -u -p -r1.20 extern.h
--- extern.h31 Dec 2015 17:51:19 -  1.20
+++ extern.h  

Re: Avoid system(3) in ikectl

2019-05-08 Thread Ted Unangst
Reyk Floeter wrote:
> I meant: could you use /* */ instead of //?

oh, sure. done.

> Yes, it looks slightly better.
> 
> grumble OK reyk

thanks.

Matthew, thanks.



Re: ftpd(8): rm dead code and simplifies popen clone

2019-05-08 Thread Ted Unangst
Jan Klemkow wrote:
> - * Special version of popen which avoids call to shell.  This ensures noone
> + * Special version of popen which avoids call to shell.  This ensures none

If we don't like noone, the correct spelling is no one.

Rest of the diff seems like a good improvement.



Re: Avoid system(3) in ikectl

2019-05-08 Thread Ted Unangst
Reyk Floeter wrote:
> On Wed, May 08, 2019 at 06:44:32PM -0400, Ted Unangst wrote:
> > Ted Unangst wrote:
> > > Matthew Martin wrote:
> > > > I did that originally [1], but Reyk preferred the varargs approach [2],
> > > > so I changed the patch to match.
> > > 
> > > Sorry, only wading into the thread at this point. Seems not everybody has 
> > > the
> > > same taste in code... Well, we have the original. Let me bring that back.
> > 
> > OK, here's diff one, but with run() renamed to ca_exec(). I think picking a
> > better name was at least one improvement. :)
> > 
> > And the file: comment added.
> > 
> 
> His first diff included other things that got cleaned up, please don't
> revert to this one (eg. the // comment).

I looked at history to pick that up. Anything else?

I think returning an error code is the right thing, even if not checked. A
void function means can't fail, not fails silently.

ca_exec or ca_system I could go either way, but there's no longer sh involved,
so that's why I went back to exec.

> If you want to switch to an array instead of varargs, fine.  But the
> definition of char *cmd[] somewhere in the function, especially in an
> extra {} block, looks very ugly to me.  That's why I suggested the
> varargs in the first place to avoid such a thing.

I do think it's less ugly without the extra {}. Here's a version with less of
that.


Index: ikeca.c
===
RCS file: /home/cvs/src/usr.sbin/ikectl/ikeca.c,v
retrieving revision 1.48
diff -u -p -r1.48 ikeca.c
--- ikeca.c 26 Feb 2019 14:21:30 -  1.48
+++ ikeca.c 8 May 2019 22:59:09 -
@@ -18,6 +18,7 @@
 
 #include 
 #include 
+#include 
 #include 
 #include 
 #include 
@@ -63,12 +64,12 @@
 
 struct ca {
char sslpath[PATH_MAX];
-   char passfile[PATH_MAX];
+   char passfile[PATH_MAX + 5]; // Includes the "file:" prefix
char index[PATH_MAX];
char serial[PATH_MAX];
char sslcnf[PATH_MAX];
char extcnf[PATH_MAX];
-   char batch[PATH_MAX];
+   char*batch;
char*caname;
 };
 
@@ -116,6 +117,7 @@ void ca_setenv(const char *, const cha
 voidca_clrenv(void);
 voidca_setcnf(struct ca *, const char *);
 voidca_create_index(struct ca *);
+int static  ca_exec(char *const []);
 
 /* util.c */
 int expand_string(char *, size_t, const char *, const char *);
@@ -130,7 +132,6 @@ int
 ca_key_create(struct ca *ca, char *keyname)
 {
struct stat  st;
-   char cmd[PATH_MAX * 2];
char path[PATH_MAX];
 
snprintf(path, sizeof(path), "%s/private/%s.key", ca->sslpath, keyname);
@@ -140,10 +141,8 @@ ca_key_create(struct ca *ca, char *keyna
return (0);
}
 
-   snprintf(cmd, sizeof(cmd),
-   "%s genrsa -out %s 2048",
-   PATH_OPENSSL, path);
-   system(cmd);
+   char *cmd[] = { PATH_OPENSSL, "genrsa", "-out", path, "2048", NULL };
+   ca_exec(cmd);
chmod(path, 0600);
 
return (0);
@@ -200,9 +199,9 @@ ca_delkey(struct ca *ca, char *keyname)
 int
 ca_request(struct ca *ca, char *keyname, int type)
 {
-   charcmd[PATH_MAX * 2];
charhostname[HOST_NAME_MAX+1];
charname[128];
+   charkey[PATH_MAX];
charpath[PATH_MAX];
 
ca_setenv("$ENV::CERT_CN", keyname);
@@ -226,13 +225,12 @@ ca_request(struct ca *ca, char *keyname,
 
ca_setcnf(ca, keyname);
 
+   snprintf(key, sizeof(key), "%s/private/%s.key", ca->sslpath, keyname);
snprintf(path, sizeof(path), "%s/private/%s.csr", ca->sslpath, keyname);
-   snprintf(cmd, sizeof(cmd), "%s req %s-new"
-   " -key %s/private/%s.key -out %s -config %s",
-   PATH_OPENSSL, ca->batch, ca->sslpath, keyname,
-   path, ca->sslcnf);
 
-   system(cmd);
+   char *cmd[] = { PATH_OPENSSL, "req", "-new", "-key", key, "-out", path,
+   "-config", ca->sslcnf, ca->batch, NULL };
+   ca_exec(cmd);
chmod(path, 0600);
 
return (0);
@@ -241,8 +239,11 @@ ca_request(struct ca *ca, char *keyname,
 int
 ca_sign(struct ca *ca, char *keyname, int type)
 {
-   charcmd[PATH_MAX * 2];
-   const char  *extensions = NULL;
+   charcakey[PATH_MAX];
+   charcacrt[PATH_MAX];
+   charout[PATH_MAX];
+   charin[PATH_MAX];
+  

Re: Avoid system(3) in ikectl

2019-05-08 Thread Ted Unangst
Ted Unangst wrote:
> Matthew Martin wrote:
> > I did that originally [1], but Reyk preferred the varargs approach [2],
> > so I changed the patch to match.
> 
> Sorry, only wading into the thread at this point. Seems not everybody has the
> same taste in code... Well, we have the original. Let me bring that back.

OK, here's diff one, but with run() renamed to ca_exec(). I think picking a
better name was at least one improvement. :)

And the file: comment added.

Index: ikeca.c
===
RCS file: /home/cvs/src/usr.sbin/ikectl/ikeca.c,v
retrieving revision 1.48
diff -u -p -r1.48 ikeca.c
--- ikeca.c 26 Feb 2019 14:21:30 -  1.48
+++ ikeca.c 8 May 2019 22:41:08 -
@@ -18,6 +18,7 @@
 
 #include 
 #include 
+#include 
 #include 
 #include 
 #include 
@@ -63,12 +64,12 @@
 
 struct ca {
char sslpath[PATH_MAX];
-   char passfile[PATH_MAX];
+   char passfile[PATH_MAX + 5]; // Includes the "file:" prefix
char index[PATH_MAX];
char serial[PATH_MAX];
char sslcnf[PATH_MAX];
char extcnf[PATH_MAX];
-   char batch[PATH_MAX];
+   char*batch;
char*caname;
 };
 
@@ -116,6 +117,7 @@ void ca_setenv(const char *, const cha
 voidca_clrenv(void);
 voidca_setcnf(struct ca *, const char *);
 voidca_create_index(struct ca *);
+int static  ca_exec(char *const []);
 
 /* util.c */
 int expand_string(char *, size_t, const char *, const char *);
@@ -130,7 +132,6 @@ int
 ca_key_create(struct ca *ca, char *keyname)
 {
struct stat  st;
-   char cmd[PATH_MAX * 2];
char path[PATH_MAX];
 
snprintf(path, sizeof(path), "%s/private/%s.key", ca->sslpath, keyname);
@@ -140,10 +141,8 @@ ca_key_create(struct ca *ca, char *keyna
return (0);
}
 
-   snprintf(cmd, sizeof(cmd),
-   "%s genrsa -out %s 2048",
-   PATH_OPENSSL, path);
-   system(cmd);
+   char *cmd[] = { PATH_OPENSSL, "genrsa", "-out", path, "2048", NULL };
+   ca_exec(cmd);
chmod(path, 0600);
 
return (0);
@@ -200,9 +199,9 @@ ca_delkey(struct ca *ca, char *keyname)
 int
 ca_request(struct ca *ca, char *keyname, int type)
 {
-   charcmd[PATH_MAX * 2];
charhostname[HOST_NAME_MAX+1];
charname[128];
+   charkey[PATH_MAX];
charpath[PATH_MAX];
 
ca_setenv("$ENV::CERT_CN", keyname);
@@ -226,13 +225,12 @@ ca_request(struct ca *ca, char *keyname,
 
ca_setcnf(ca, keyname);
 
+   snprintf(key, sizeof(key), "%s/private/%s.key", ca->sslpath, keyname);
snprintf(path, sizeof(path), "%s/private/%s.csr", ca->sslpath, keyname);
-   snprintf(cmd, sizeof(cmd), "%s req %s-new"
-   " -key %s/private/%s.key -out %s -config %s",
-   PATH_OPENSSL, ca->batch, ca->sslpath, keyname,
-   path, ca->sslcnf);
 
-   system(cmd);
+   char *cmd[] = { PATH_OPENSSL, "req", "-new", "-key", key, "-out", path,
+   "-config", ca->sslcnf, ca->batch, NULL };
+   ca_exec(cmd);
chmod(path, 0600);
 
return (0);
@@ -241,8 +239,11 @@ ca_request(struct ca *ca, char *keyname,
 int
 ca_sign(struct ca *ca, char *keyname, int type)
 {
-   charcmd[PATH_MAX * 2];
-   const char  *extensions = NULL;
+   charcakey[PATH_MAX];
+   charcacrt[PATH_MAX];
+   charout[PATH_MAX];
+   charin[PATH_MAX];
+   char*extensions = NULL;
 
if (type == HOST_IPADDR) {
extensions = "x509v3_IPAddr";
@@ -258,19 +259,16 @@ ca_sign(struct ca *ca, char *keyname, in
ca_setenv("$ENV::CASERIAL", ca->serial);
ca_setcnf(ca, keyname);
 
-   snprintf(cmd, sizeof(cmd),
-   "%s ca -config %s -keyfile %s/private/ca.key"
-   " -cert %s/ca.crt"
-   " -extfile %s -extensions %s -out %s/%s.crt"
-   " -in %s/private/%s.csr"
-   " -passin file:%s -outdir %s -batch",
-   PATH_OPENSSL, ca->sslcnf, ca->sslpath,
-   ca->sslpath,
-   ca->extcnf, extensions, ca->sslpath, keyname,
-   ca->sslpath, keyname,
-   ca->passfile, ca->sslpath);
-
-   system(cmd);
+   snprintf(cakey, sizeof(cakey), "%s/private/ca.key", ca->sslpath);
+   snprintf(cacrt, sizeof(cacrt), "%s/ca.crt"

Re: Avoid system(3) in ikectl

2019-05-08 Thread Ted Unangst
Matthew Martin wrote:
> I did that originally [1], but Reyk preferred the varargs approach [2],
> so I changed the patch to match.

Sorry, only wading into the thread at this point. Seems not everybody has the
same taste in code... Well, we have the original. Let me bring that back.



Re: Avoid system(3) in ikectl

2019-05-08 Thread Ted Unangst
Theo de Raadt wrote:
> Isn't something like better -- to avoid marshalling code to convert
> arguments -> array?

this requires mixing declarations and code, but all our compilers are c99
compliant now, and this does make ca_system simpler.

> 
>   char *pkcs_args[] =
>   PATH_OPENSSL,
>   "pkcs12",
>   "-export",
>   "-caname",
>   ca->caname,
>   "-name",
>   ca->caname,
>   "-cacerts",
>   "-nokeys",
>   "-in",
>   cacrt,
>   "-out",
>   capfx,
>   "-passout",
>   "env:EXPASS",
>   "-passin",
>   ca->passfile,
>   NULL
>   };
> 
>   ca_system(pkcs_args);
> 



Re: Avoid system(3) in ikectl

2019-05-08 Thread Ted Unangst
Matthew Martin wrote:
> ping
> 
> On Thu, Apr 25, 2019 at 11:21:00PM -0500, Matthew Martin wrote:
> > On Thu, Apr 25, 2019 at 08:59:56PM -0600, Theo de Raadt wrote:
> > > > +   argv = alloca((n + 1) * sizeof(*argv));
> > > 
> > > Our source tree is exceedingly sparing in the use of alloca().
> > > This will not do.
> > 
> > Was staying as close as possible to exec.c, but avoiding alloca is
> > preferable; replaced with reallocarray. err message is "reallocarray" to
> > match style with the calloc call in the same file.
> > 
> > 
> - if (keyname != NULL) {
> - snprintf(cmd, sizeof(cmd), "env EXPASS=%s %s pkcs12 -export"
> - " -name %s -CAfile %s/ca.crt -inkey %s/private/%s.key"
> - " -in %s/%s.crt -out %s/private/%s.pfx -passout env:EXPASS"
> - " -passin file:%s", pass, PATH_OPENSSL, keyname,
> - ca->sslpath, ca->sslpath, keyname, ca->sslpath, keyname,
> - ca->sslpath, oname, ca->passfile);
> - system(cmd);
> - }
> -
> - snprintf(cmd, sizeof(cmd), "env EXPASS=%s %s pkcs12 -export"
> - " -caname '%s' -name '%s' -cacerts -nokeys"
> - " -in %s/ca.crt -out %s/ca.pfx -passout env:EXPASS -passin file:%s",
> - pass, PATH_OPENSSL, ca->caname, ca->caname, ca->sslpath,
> - ca->sslpath, ca->passfile);
> - system(cmd);
> + snprintf(cacrt, sizeof(cacrt), "%s/ca.crt", ca->sslpath);
> + snprintf(capfx, sizeof(capfx), "%s/ca.pfx", ca->sslpath);
> + snprintf(key, sizeof(key), "%s/private/%s.key", ca->sslpath, keyname);
> + snprintf(crt, sizeof(crt), "%s/%s.crt", ca->sslpath, keyname);
> + snprintf(pfx, sizeof(pfx), "%s/private/%s.pfx", ca->sslpath, oname);
> +
> + snprintf(passenv, sizeof(passenv), "EXPASS=%s", pass);
> + putenv(passenv);
> +
> + if (keyname != NULL)
> + ca_system(PATH_OPENSSL, "pkcs12", "-export", "-name", keyname,
> + "-CAfile", cacrt, "-inkey", key, "-in", crt, "-out", pfx,
> + "-passout", "env:EXPASS", "-passin", ca->passfile, NULL);
> +
> + ca_system(PATH_OPENSSL, "pkcs12", "-export", "-caname", ca->caname,
> + "-name", ca->caname, "-cacerts", "-nokeys", "-in", cacrt,
> + "-out", capfx, "-passout", "env:EXPASS", "-passin", ca->passfile,
> + NULL);
> +
> + unsetenv("EXPASS");
> + explicit_bzero(passenv, sizeof(passenv));

This looks weird, but it's still probably an improvement over the original
code, which is trying to avoid leaking password via argv but only mostly
succeeds.

There's probably a better way to do environment handling, but I think this
diff is better than using system() now. ok with me.





Re: libevent: prevent integer overflow in kqueue

2019-05-07 Thread Ted Unangst
Tobias Stoeckmann wrote:
> This patch avoids a possible integer overflow on excessively large
> amount of events added to an event base in kqueue mode (default).
> 
> Just as with previous changes, this is very unlikely to trigger and
> is a just a defensive measure.
> 
> Changes in this diff:
> 
> * KNF (sorted imports and added limits.h for INT_MAX)
> * recallocarray instead of reallocarray, in sync with minheap change
> * adjusted error messages from malloc to recallocarray
> 
> Okay?

ok



Re: libevent: endless loop on excessively large buffers

2019-05-02 Thread Ted Unangst
Tobias Stöckmann wrote:
> Generally this is a rather theoretical case. Normal users are not
> allowed to allocate so much memory. But better be safe than sorry,
> especially if login.conf values were adjusted (or the process runs
> as root).
> 
> This patch completely removes "unsigned int" from buffer.c.

yes please.



Re: libevent: remove non-monotonic compat code

2019-04-30 Thread Ted Unangst
Jeremie Courreges-Anglas wrote:
> So the diff below removes the fallback path and all associated code and
> variables.  I have left out some minor cleanups for now to ease reviews.

makes sense to me.



signify xr sysupgrade.8

2019-04-26 Thread Ted Unangst
Simplify examples section. The magic recipe is contained in sysupgrade, so we
can omit it, and instead add a .xr to sysupgrade.8.


Index: signify.1
===
RCS file: /home/cvs/src/usr.bin/signify/signify.1,v
retrieving revision 1.46
diff -u -p -r1.46 signify.1
--- signify.1   23 Mar 2019 07:10:06 -  1.46
+++ signify.1   26 Apr 2019 20:32:24 -
@@ -166,18 +166,6 @@ Sign a file, specifying a signature name
 Verify a signature, using the default signature name:
 .Dl $ signify -V -p key.pub -m generalsorders.txt
 .Pp
-Verify a release directory containing
-.Pa SHA256.sig
-and a full set of release files:
-.Bd -literal -offset indent -compact
-$ signify -C -p /etc/signify/openbsd-66-base.pub -x SHA256.sig
-.Ed
-.Pp
-Verify a bsd.rd before an upgrade:
-.Bd -literal -offset indent -compact
-$ signify -C -p /etc/signify/openbsd-66-base.pub -x SHA256.sig bsd.rd
-.Ed
-.Pp
 Sign a gzip archive:
 .Bd -literal -offset indent -compact
 $ signify -Sz -s key-arc.sec -m in.tgz -x out.tgz
@@ -191,7 +179,8 @@ $ ftp url | signify -Vz -t arc | tar ztf
 .Xr fw_update 1 ,
 .Xr gzip 1 ,
 .Xr pkg_add 1 ,
-.Xr sha256 1
+.Xr sha256 1 ,
+.Xr sysupgrade 8
 .Sh HISTORY
 The
 .Nm



Re: tmpfile and pledge

2019-04-25 Thread Ted Unangst
Martijn van Duren wrote:
> Is there a sound reason to keep this code here that I'm overlooking
> or can we please remove it?

I'll add that the umask calls make this function unsafe in a threaded program,
which I think is unexpected.



Re: Booting Threadripper 2950x with -current

2019-04-23 Thread Ted Unangst
Bryan Steele wrote:
> > I just got through building a new desktop machine and thought I'd
> > install OpenBSD -current on it.  The install kernel booted quite fast,
> > but now that I have the real kernel there, it takes approximately 5
> > minutes to boot.  The vast majority of the time is spent at the very
> > beginning where the "spinning text graphic" sits there until it kicks
> > out a number and then does the next one (not sure what to call that).
> 
> That sounds like the part where it's loading the kernel from the disk,
> which is using BIOS/UEFI functions. Not sure if there's much all that
> can be done about that. :)

Gets slower with every new generation, as more and more "classic" code is
pushed into emulation layers emulating other emulation layers... Or something,
who knows what computers do these days.

Gzipping the kernel may make things faster, but breaks things like karl. As
does compiling a smaller kernel. etc.

The installer kernel boots faster because:
1. it's smaller
2. it used a different loader? bios vs uefi? you may have some control over
this if you can choose the opposite of whatever you did choose.



Re: libevent: Protect integer multiplications (min_heap)

2019-04-23 Thread Ted Unangst
Tobias Stoeckmann wrote:
> On Mon, Apr 22, 2019 at 01:24:13PM -0400, Ted Unangst wrote:
> > ah, good catch. I don't think it's wrong (until it overflows) but needs to 
> > be
> > fixed. Needs some more review then. Thanks.
> 
> I have added following changes:
> 
> - converted 0u to 0 as bluhm pointed out
> - converted -1 to SIZE_MAX whereever it was used as "illegal index"
>   as bluhm mentioned as well
> - converted timemap's size usages in event.c from (u)int to size_t
> 
> Passes regress tests.
> 
> I'll discuss this with libevent maintainers to get these changes into
> upstream as well.

looks pretty good to me.



Re: libevent: Protect integer multiplications (min_heap)

2019-04-22 Thread Ted Unangst
Tobias Stoeckmann wrote:
> Changing n means that at least timeout_correct in event.c must be
> adjusted as well, because it is used as an unsigned:

ah, good catch. I don't think it's wrong (until it overflows) but needs to be
fixed. Needs some more review then. Thanks.



Re: libevent: Protect integer multiplications (min_heap)

2019-04-20 Thread Ted Unangst
Tobias Stoeckmann wrote:
> I would like to protect min_heap_push against integer overflows,
> which could either be triggered on a 64 bit system with massive
> amounts of RAM (to overflow s->n) or on a 32 bit system with tight
> memory layout (overflowing a * sizeof *p).
> 
> Both cases are basically not possible to be triggered, but I prefer
> to have reallocarray in place to be sure about it.

ok, so back to this. here's a diff which solves this by using appropriate
size_t types.

Note the first chunk changes the size of struct event, so this is a library
ABI change. (Maybe it's padded? I don't feel like digging that deep.)

Index: event.h
===
RCS file: /cvs/src/lib/libevent/event.h,v
retrieving revision 1.30
diff -u -p -r1.30 event.h
--- event.h 5 Jan 2015 23:14:36 -   1.30
+++ event.h 20 Apr 2019 23:28:09 -
@@ -196,7 +196,7 @@ struct event {
TAILQ_ENTRY (event) ev_next;
TAILQ_ENTRY (event) ev_active_next;
TAILQ_ENTRY (event) ev_signal_next;
-   unsigned int min_heap_idx;  /* for managing timeouts */
+   size_t min_heap_idx;/* for managing timeouts */
 
struct event_base *ev_base;
 
Index: min_heap.h
===
RCS file: /cvs/src/lib/libevent/min_heap.h,v
retrieving revision 1.5
diff -u -p -r1.5 min_heap.h
--- min_heap.h  20 Apr 2019 23:22:28 -  1.5
+++ min_heap.h  20 Apr 2019 23:28:09 -
@@ -33,7 +33,7 @@
 
 typedef struct min_heap {
struct event **p;
-   unsigned n, a;
+   size_t n, a;
 } min_heap_t;
 
 static inline void min_heap_ctor(min_heap_t * s);
@@ -41,14 +41,14 @@ static inline void min_heap_dtor(min_hea
 static inline void min_heap_elem_init(struct event * e);
 static inline int min_heap_elem_greater(struct event * a, struct event * b);
 static inline int min_heap_empty(min_heap_t * s);
-static inline unsigned min_heap_size(min_heap_t * s);
+static inline size_t min_heap_size(min_heap_t * s);
 static inline struct event *min_heap_top(min_heap_t * s);
-static inline int min_heap_reserve(min_heap_t * s, unsigned n);
+static inline int min_heap_reserve(min_heap_t * s, size_t n);
 static inline int min_heap_push(min_heap_t * s, struct event * e);
 static inline struct event *min_heap_pop(min_heap_t * s);
 static inline int min_heap_erase(min_heap_t * s, struct event * e);
-static inline void min_heap_shift_up_(min_heap_t * s, unsigned hole_index, 
struct event * e);
-static inline void min_heap_shift_down_(min_heap_t * s, unsigned hole_index, 
struct event * e);
+static inline void min_heap_shift_up_(min_heap_t * s, size_t hole_index, 
struct event * e);
+static inline void min_heap_shift_down_(min_heap_t * s, size_t hole_index, 
struct event * e);
 
 int 
 min_heap_elem_greater(struct event * a, struct event * b)
@@ -77,7 +77,7 @@ min_heap_empty(min_heap_t * s)
 {
return 0u == s->n;
 }
-unsigned 
+size_t 
 min_heap_size(min_heap_t * s)
 {
return s->n;
@@ -112,9 +112,9 @@ min_heap_pop(min_heap_t * s)
 int 
 min_heap_erase(min_heap_t * s, struct event * e)
 {
-   if (((unsigned int)-1) != e->min_heap_idx) {
+   if (e->min_heap_idx != -1) {
struct event *last = s->p[--s->n];
-   unsigned parent = (e->min_heap_idx - 1) / 2;
+   size_t parent = (e->min_heap_idx - 1) / 2;
/*
 * we replace e with the last element in the heap.  We might
 * need to shift it upward if it is less than its parent, or
@@ -133,14 +133,14 @@ min_heap_erase(min_heap_t * s, struct ev
 }
 
 int 
-min_heap_reserve(min_heap_t * s, unsigned n)
+min_heap_reserve(min_heap_t * s, size_t n)
 {
if (s->a < n) {
struct event **p;
-   unsigned a = s->a ? s->a * 2 : 8;
+   size_t a = s->a ? s->a * 2 : 8;
if (a < n)
a = n;
-   if (!(p = realloc(s->p, a * sizeof *p)))
+   if (!(p = reallocarray(s->p, a, sizeof *p)))
return -1;
s->p = p;
s->a = a;
@@ -149,9 +149,9 @@ min_heap_reserve(min_heap_t * s, unsigne
 }
 
 void 
-min_heap_shift_up_(min_heap_t * s, unsigned hole_index, struct event * e)
+min_heap_shift_up_(min_heap_t * s, size_t hole_index, struct event * e)
 {
-   unsigned parent = (hole_index - 1) / 2;
+   size_t parent = (hole_index - 1) / 2;
while (hole_index && min_heap_elem_greater(s->p[parent], e)) {
s->p[hole_index] = s->p[parent];
s->p[hole_index]->min_heap_idx = hole_index;
@@ -163,9 +163,9 @@ min_heap_shift_up_(min_heap_t * s, unsig
 }
 
 void 
-min_heap_shift_down_(min_heap_t * s, unsigned hole_index, struct event * e)
+min_heap_shift_down_(min_heap_t * s, size_t hole_index, struct event * e)
 {
-   unsigned min_child = 2 * (hole_index + 1);
+   size_t min_child = 2 * (hole_index + 

  1   2   3   4   5   6   7   8   9   10   >