Re: Ignore /tmp/.Xauth* in daily

[Vadim Zhukov]

сб, 6 мар. 2021 г. в 23:14, Matthieu Herrb :
>
> On Sat, Mar 06, 2021 at 09:52:58PM +0300, Vadim Zhukov wrote:
> > сб, 6 мар. 2021 г. в 21:30, Theo de Raadt :
> > >
> > > Matthieu Herrb  wrote:
> > >
> > > > Linux, systemd and XDG have inventend this /run/user/$uid tmpfs that
> > > > is created automagically and they use that in place of /tmp for
> > > > volatile things that don't beloing to $HOME, but this is not a can of
> > > > worms I want to open now.
> > >
> > > Awesome, another directory to drop stuff and run a filesystem out of space
> > > with unclear consequences...
> > >
> > > This does not fit with our direction either.
> >
> > So this code appeared in X11R4. There was no VCS repo, I suppose, so no 
> > history.
> >
> > There are basically four cases why xdm may fail to create ~/.Xauthority:
> >
> > a) home directory doesn't exist
> > b) home directory is non-writeable due to permissions
> > c) /home is full
> > d) /home is on NFS and there are locking/network issues.
> >
> > I'm not sure if (a) is a valid case. (b) is a variant of my case, as I
> > said, I can live without this feature. In the case of (c) users
> > (non-admins) won't be able to do something anyway. Can't speak for NFS
> > (I've quit the job where /home on NFS has been set up a few years ago)
> > so no opinion on (d).
> >
>
> I think 4 his not an issue anymore.the locking mecanism used by xauth
> is working with all current NFS implementations (including
> OpenBSD's).
>
> Here is a patch to remve the backup authorization file. Unfortunatly
> there is no simple way to display an explicit error message. One will
> need to check the xenodm.log file.
>
> Xsession can be patched too to remove the fallback to /tmp/xes- log
> file if ~/.xsession-errors cannot be writen. This will be a separate
> diff.
>
> Index: include/dm.h
> ===
> RCS file: /cvs/OpenBSD/xenocara/app/xenodm/include/dm.h,v
> retrieving revision 1.15
> diff -u -p -u -r1.15 dm.h
> --- include/dm.h10 Jan 2021 09:18:30 -  1.15
> +++ include/dm.h6 Mar 2021 17:53:44 -
> @@ -122,7 +122,6 @@ struct display {
> char**authNames;/* authorization protocol names */
> unsigned short  *authNameLens;  /* authorization protocol name lens */
> char*clientAuthFile;/* client specified auth file */
> -   char*userAuthDir;   /* backup directory for tickets */
> int authComplain;   /* complain when no auth for XDMCP */
>
> /* information potentially derived from resources */
> Index: man/xenodm.man
> ===
> RCS file: /cvs/OpenBSD/xenocara/app/xenodm/man/xenodm.man,v
> retrieving revision 1.11
> diff -u -p -u -r1.11 xenodm.man
> --- man/xenodm.man  15 Aug 2019 16:23:33 -  1.11
> +++ man/xenodm.man  6 Mar 2021 17:53:44 -
> @@ -582,18 +582,6 @@ to occur, during which time the new auth
>  The default is
>  .Cm false ,
>  which will work for all MIT servers.
> -.It Ic DisplayManager. Ns Ar DISPLAY Ns Ic .userAuthDir
> -When
> -.Nm
> -is unable to write to the usual user authorization file
> -.Pq Pa $HOME/.Xauthority ,
> -it creates a unique file name in this directory and points the environment
> -variable
> -.Ev XAUTHORITY
> -at the created file.
> -It uses
> -.Pa /tmp
> -by default.
>  .El
>  .Sh CONFIGURATION FILE
>  First, the
> Index: xenodm/auth.c
> ===
> RCS file: /cvs/OpenBSD/xenocara/app/xenodm/xenodm/auth.c,v
> retrieving revision 1.15
> diff -u -p -u -r1.15 auth.c
> --- xenodm/auth.c   1 Jan 2021 18:09:07 -   1.15
> +++ xenodm/auth.c   6 Mar 2021 17:53:44 -
> @@ -752,7 +752,7 @@ void
>  SetUserAuthorization (struct display *d, struct verify_info *verify)
>  {
>  FILE   *old = NULL, *new;
> -char   home_name[1024], backup_name[1024], new_name[1024];
> +char   home_name[1024], new_name[1024];
>  char   *name = NULL;
>  char   *home;
>  char   *envname = NULL;
> @@ -762,7 +762,6 @@ SetUserAuthorization (struct display *d,
>  struct statstatb;
>  inti;
>  intmagicCookie;
> -intfd;
>
>  Debug ("SetUserAuthorization\n");
>  auths = d->authorizations;
> @@ -793,45 +792,10 @@ SetUserAuthorization (struct display *d,
> }
>   

Re: Ignore /tmp/.Xauth* in daily

[Vadim Zhukov]

сб, 6 мар. 2021 г. в 21:30, Theo de Raadt :
>
> Matthieu Herrb  wrote:
>
> > Linux, systemd and XDG have inventend this /run/user/$uid tmpfs that
> > is created automagically and they use that in place of /tmp for
> > volatile things that don't beloing to $HOME, but this is not a can of
> > worms I want to open now.
>
> Awesome, another directory to drop stuff and run a filesystem out of space
> with unclear consequences...
>
> This does not fit with our direction either.

So this code appeared in X11R4. There was no VCS repo, I suppose, so no history.

There are basically four cases why xdm may fail to create ~/.Xauthority:

a) home directory doesn't exist
b) home directory is non-writeable due to permissions
c) /home is full
d) /home is on NFS and there are locking/network issues.

I'm not sure if (a) is a valid case. (b) is a variant of my case, as I
said, I can live without this feature. In the case of (c) users
(non-admins) won't be able to do something anyway. Can't speak for NFS
(I've quit the job where /home on NFS has been set up a few years ago)
so no opinion on (d).

-- 
  WBR,
  Vadim Zhukov

Re: Ignore /tmp/.Xauth* in daily

[Vadim Zhukov]

сб, 6 мар. 2021 г. в 20:53, Theo de Raadt :
>
> Vadim Zhukov  wrote:
>
> > > The backup dir can be configured to something else, but it needs to be
> > > writeable by the user whois login in. It could be a subdir of /tmp (if
> > > the rc.d script takes care of creating it) or I can remove that
> > > feature from xenodm and fail the login if /home is not writeable.
> >
> > I've sent a diff for subdir of /tmp already. ;-)
>
> Sure, but the creation of a directory introduces new concerns.
>
> Why must non-readable $HOME work, and is the trade-off for placing
> "keys" in /tmp worthwhile.
>
> It made sense to someone 30 years ago.  Does it make sense now?

Please correct me if I wrong: you said "non-readable $HOME" a few
times during discussion, did you mean "read-only $HOME" instead?

-- 
  WBR,
  Vadim Zhukov

Re: Ignore /tmp/.Xauth* in daily

[Vadim Zhukov]

сб, 6 мар. 2021 г. в 20:50, Theo de Raadt :
>
> Matthieu Herrb  wrote:
>
> > On Sat, Mar 06, 2021 at 09:56:34AM -0700, Theo de Raadt wrote:
> > > Matthieu Herrb  wrote:
> > >
> > > > On Fri, Mar 05, 2021 at 09:10:32PM +0300, Vadim Zhukov wrote:
> > > > > чт, 4 мар. 2021 г. в 02:02, Vadim Zhukov :
> > > > > >
> > > > > > Hello all.
> > > > > >
> > > > > > Since xenodm has DEF_USER_AUTH_DIR set to "/tmp", we need to ignore
> > > > > > /tmp/.Xauth* in daily cleanup, don't we?
> > > > > >
> > > > > > Found the hard way a few minutes ago on my X240.
> > > > >
> > > > > Thanks sthen@, I've realized this happens only when xenodm could not
> > > > > create ~/.Xauthority. In my case this happens because my laptop starts
> > > > > with /home mounted read-only, but there may be others. Mattieu, the
> > > > > xenodm logic itself is correct, right? If yes, anyone brave enough to
> > > > > okay the diff below then? :-)
> > > >
> > > > Hi,
> > > >
> > > > Yes I think the xenodm logic (inherithed from xdm) is correct.
> > > >
> > > > Althoug in my experience, when an X session cnnot write to $HOME it
> > > > generally doesn't get very far (iirc not beeing able to write to
> > > > .xsession-errors used to be fatal)...
> >
> > I tried to log in with a read-only /home and it failed as I remembered
> > it. Vadim are your doing something special in you X session to make
> > that work   ?
> >
> >
> > > >
> > > > Anyways ok to skip that directory if it exists in daily.
> > >
> > > It is not a directory -- it is a file.
> > >
> > > I don't understand how this file is created.  Well-known names in /tmp
> > > are raceable -- therefore we and others increasingly use directories 
> > > containing
> > > files as a safer pattern.  Where is the code that creates this file?  Is 
> > > it
> > > safe?  I am suspicious.
> >
> > It's created by mkstemp(3) in
> > /usr/xenocara/app/xenodm/xenodm/auth.c:798
> >
> > >
> > > I strongly disagree with the pattern ".Xauth*".  It should be EXACT.  If
> > > someone else creates a file called .Xauthsadflkjdsaf, it should not be
> > > deleted.
> > >
> > > As a final point, is this strategy of considering /tmp a safe place 
> > > acceptable
> > > at all?  If $HOME doesn't work, why not just have X fail to work correctly
> > > and consider this "fail over to /tmp" a junk idea from the past?
> >
> > The backup dir can be configured to something else, but it needs to be
> > writeable by the user whois login in. It could be a subdir of /tmp (if
> > the rc.d script takes care of creating it) or I can remove that
> > feature from xenodm and fail the login if /home is not writeable.
>
> My question is more basic:  Why must there be a backup dir.
>
> Why not just fail hard.
>
> Should we change ssh so that if the home directory is non-readable, it
> store ssh keys and other ephmeral information in /tmp?   No surely not.

But you can still log in to read-only home via SSH.

> So why does X need to follow this pattern?  Is there a strong justification,
> or is it simply a decision made 30 years ago?

I feel myself having too little experience to judge. But: previously I
used "startx" and I was happy to type "mrw /home; startx" after
logging in (mrw is small script doing mount -uorw). Then startx was
broken and now I'm forced to use xenodm. So I have it enabled, and I
log in graphically from the start. If we'll drop /tmp failover, I'll
have to switch consoles and log in to text console in addition. Not a
big deal, of course.

-- 
  WBR,
  Vadim Zhukov

Re: Ignore /tmp/.Xauth* in daily

[Vadim Zhukov]

сб, 6 мар. 2021 г. в 20:41, Matthieu Herrb :
>
> On Sat, Mar 06, 2021 at 09:56:34AM -0700, Theo de Raadt wrote:
> > Matthieu Herrb  wrote:
> >
> > > On Fri, Mar 05, 2021 at 09:10:32PM +0300, Vadim Zhukov wrote:
> > > > чт, 4 мар. 2021 г. в 02:02, Vadim Zhukov :
> > > > >
> > > > > Hello all.
> > > > >
> > > > > Since xenodm has DEF_USER_AUTH_DIR set to "/tmp", we need to ignore
> > > > > /tmp/.Xauth* in daily cleanup, don't we?
> > > > >
> > > > > Found the hard way a few minutes ago on my X240.
> > > >
> > > > Thanks sthen@, I've realized this happens only when xenodm could not
> > > > create ~/.Xauthority. In my case this happens because my laptop starts
> > > > with /home mounted read-only, but there may be others. Mattieu, the
> > > > xenodm logic itself is correct, right? If yes, anyone brave enough to
> > > > okay the diff below then? :-)
> > >
> > > Hi,
> > >
> > > Yes I think the xenodm logic (inherithed from xdm) is correct.
> > >
> > > Althoug in my experience, when an X session cnnot write to $HOME it
> > > generally doesn't get very far (iirc not beeing able to write to
> > > .xsession-errors used to be fatal)...
>
> I tried to log in with a read-only /home and it failed as I remembered
> it. Vadim are your doing something special in you X session to make
> that work   ?

Well, I do... nothing? I can show my .xsession, there's nothing
special. I run cwm, FWIW, and do not start anything heavy from the
beginning, to have a working desktop ASAP. KDE and GNOME won't work
this way for sure. :-)

For .xsession-errors, there is /tmp-based handling in
/etc/X11/xenodm/Xsession already. Oh, now I know where the disk space
on /tmp gets lost from time to time...

> > > Anyways ok to skip that directory if it exists in daily.
> >
> > It is not a directory -- it is a file.
> >
> > I don't understand how this file is created.  Well-known names in /tmp
> > are raceable -- therefore we and others increasingly use directories 
> > containing
> > files as a safer pattern.  Where is the code that creates this file?  Is it
> > safe?  I am suspicious.
>
> It's created by mkstemp(3) in
> /usr/xenocara/app/xenodm/xenodm/auth.c:798
>
> >
> > I strongly disagree with the pattern ".Xauth*".  It should be EXACT.  If
> > someone else creates a file called .Xauthsadflkjdsaf, it should not be
> > deleted.
> >
> > As a final point, is this strategy of considering /tmp a safe place 
> > acceptable
> > at all?  If $HOME doesn't work, why not just have X fail to work correctly
> > and consider this "fail over to /tmp" a junk idea from the past?
>
> The backup dir can be configured to something else, but it needs to be
> writeable by the user whois login in. It could be a subdir of /tmp (if
> the rc.d script takes care of creating it) or I can remove that
> feature from xenodm and fail the login if /home is not writeable.

I've sent a diff for subdir of /tmp already. ;-)

--
  WBR,
  Vadim Zhukov

Re: Ignore /tmp/.Xauth* in daily

[Vadim Zhukov]

чт, 4 мар. 2021 г. в 02:02, Vadim Zhukov :
>
> Hello all.
>
> Since xenodm has DEF_USER_AUTH_DIR set to "/tmp", we need to ignore
> /tmp/.Xauth* in daily cleanup, don't we?
>
> Found the hard way a few minutes ago on my X240.

Thanks sthen@, I've realized this happens only when xenodm could not
create ~/.Xauthority. In my case this happens because my laptop starts
with /home mounted read-only, but there may be others. Mattieu, the
xenodm logic itself is correct, right? If yes, anyone brave enough to
okay the diff below then? :-)

> Index: daily
> ===
> RCS file: /cvs/src/etc/daily,v
> retrieving revision 1.95
> diff -u -p -r1.95 daily
> --- daily   20 Oct 2020 22:42:29 -  1.95
> +++ daily   3 Mar 2021 22:58:28 -
> @@ -49,7 +49,7 @@ if [ -d /tmp -a ! -L /tmp ]; then
> cd /tmp && {
> find -x . \
> \( -path './ssh-*' -o -path ./.X11-unix -o -path ./.ICE-unix \
> -   -o -path './tmux-*' \) \
> +   -o -path './tmux-*' -o -path './.Xauth*' \) \
> -prune -o -type f -atime +7 -delete 2>/dev/null
> find -x . -type d -mtime +1 ! -path ./vi.recover ! -path ./.X11-unix \
> ! -path ./.ICE-unix ! -name . \

-- 
  WBR,
  Vadim Zhukov

Re: Fix assigning array variables in ksh

[Vadim Zhukov]

пт, 5 мар. 2021 г. в 18:14, Theo Buehler :
>
> On Sun, Feb 21, 2021 at 10:04:07PM +0300, Vadim Zhukov wrote:
> > Hello all.
> >
> > This continues the 'Another potential ksh bug?' thread on misc@:
> > https://marc.info/?l=openbsd-misc=160736700220621=2
> > This present is a bit too late for Christmas, but at least the Day of
> > Red Army is coming soon. I'm sure noone is against this patch then.
> >
> > The code in typeset() function, which is responsible for almost all
> > shell variable tweaking, contains a bug: in case of array it passes
> > "foo=var" instead of only variable name ("foo"), if the value was
> > ever given. This results in, e.g., a bug reported by Jordan Geoghegan.
> > Generally speaking, we had creating a (temporary) variable via global()
> > in vpbase, and of course it didn't get the read-only marker.
> >
> > This fix is to use 'tvar' variable, which always contains shell
> > variable name, without value.
>
> ok tb
>
> > As a bonus, this fixes the ifs.t:IFS-null-1 test (previously failing).
> > I'm too lazy to check why. :-)
>
> I'm confused. I have 11 failing tests before and after your patch.
> Also IFS-null-1 has been passing for a long time.

Hm-m-m. Probably something in my environment... I'll experiment separately then.

Thanks!

Ignore /tmp/.Xauth* in daily

[Vadim Zhukov]

Hello all.

Since xenodm has DEF_USER_AUTH_DIR set to "/tmp", we need to ignore
/tmp/.Xauth* in daily cleanup, don't we?

Found the hard way a few minutes ago on my X240.

Okay?

--
WBR,
  Vadim Zhukov


Index: daily
===
RCS file: /cvs/src/etc/daily,v
retrieving revision 1.95
diff -u -p -r1.95 daily
--- daily   20 Oct 2020 22:42:29 -  1.95
+++ daily   3 Mar 2021 22:58:28 -
@@ -49,7 +49,7 @@ if [ -d /tmp -a ! -L /tmp ]; then
cd /tmp && {
find -x . \
\( -path './ssh-*' -o -path ./.X11-unix -o -path ./.ICE-unix \
-   -o -path './tmux-*' \) \
+   -o -path './tmux-*' -o -path './.Xauth*' \) \
-prune -o -type f -atime +7 -delete 2>/dev/null
find -x . -type d -mtime +1 ! -path ./vi.recover ! -path ./.X11-unix \
! -path ./.ICE-unix ! -name . \

Fix assigning array variables in ksh

[Vadim Zhukov]

Hello all.

This continues the 'Another potential ksh bug?' thread on misc@:
https://marc.info/?l=openbsd-misc=160736700220621=2
This present is a bit too late for Christmas, but at least the Day of
Red Army is coming soon. I'm sure noone is against this patch then.

The code in typeset() function, which is responsible for almost all
shell variable tweaking, contains a bug: in case of array it passes
"foo=var" instead of only variable name ("foo"), if the value was
ever given. This results in, e.g., a bug reported by Jordan Geoghegan.
Generally speaking, we had creating a (temporary) variable via global()
in vpbase, and of course it didn't get the read-only marker.

This fix is to use 'tvar' variable, which always contains shell
variable name, without value.

As a bonus, this fixes the ifs.t:IFS-null-1 test (previously failing).
I'm too lazy to check why. :-)

Okay anyone?

--
WBR,
  Vadim Zhukov


Index: regress/bin/ksh/obsd-regress.t
===
RCS file: /cvs/src/regress/bin/ksh/obsd-regress.t,v
retrieving revision 1.10
diff -u -p -r1.10 obsd-regress.t
--- regress/bin/ksh/obsd-regress.t  8 Dec 2018 21:03:51 -   1.10
+++ regress/bin/ksh/obsd-regress.t  21 Feb 2021 18:51:54 -
@@ -503,3 +503,16 @@ description:
 stdin:
kill -s SIGINFO $$
 ---
+
+name: overwrite-ro-array
+description:
+   do not allow to override first element of a read-only array
+   via the non-array access.
+stdin:
+   arr[0]=foo
+   readonly arr
+   arr=bar
+expected-exit: e == 1
+expected-stderr-pattern:
+   /: arr: is read only$/
+---
Index: bin/ksh/var.c
===
RCS file: /cvs/src/bin/ksh/var.c,v
retrieving revision 1.71
diff -u -p -r1.71 var.c
--- bin/ksh/var.c   21 Feb 2020 18:21:23 -  1.71
+++ bin/ksh/var.c   21 Feb 2021 18:51:54 -
@@ -644,7 +644,7 @@ typeset(const char *var, int set, int cl
global(tvar);
set &= ~(LOCAL|LOCAL_COPY);
 
-   vpbase = (vp->flag & ARRAY) ? global(arrayname(var)) : vp;
+   vpbase = (vp->flag & ARRAY) ? global(arrayname(tvar)) : vp;
 
/* only allow export flag to be set.  at ksh allows any attribute to
 * be changed, which means it can be truncated or modified

Re: fortune: allow to use symlinks

[Vadim Zhukov]

> I very rarely use fortune and I'm not familiar with the codebase, but
> from some quick testing and code-scanning I found the following:
> is_fortfile appends .dat to the input file and sees if it's accessible.
> Adding a symlink to the corresponding .dat file make the thing work
> perfectly well for me. Your diff seems to work, because you resolve
> the path, which in turn gives back the correct location for the .dat,
> not because symlinks are not suported.

Thank you very much for review. Indeed, adding second symlink fixes
the initial issue, so the patch now seems useless...

> Personally I don't see good reason to add the additional logic which
> can be solved with a second simple symlink, but if others think it's
> worth it, comments inline.

... but following your comments I've discovered that copy() function
in fortune.c can return NULL, and that value isn't checked.

So I'm posting the whole diff for the sake of completeness, and what
I really think worths committing is the last chunk, replacing "return
NULL" with "err(1, NULL)" in copy(). Okay for it?

> martijn@
> 
> On Tue, 2020-12-15 at 01:09 +0300, Vadim Zhukov wrote:
> > Hello, all.
> > 
> > This allows fortune(6) to open fortune files via symlinks.
> > Maybe this is a stupid idea, I dunno, but it seems inconsistent
> > that I can't put a symlink to my fortunes collection in
> > /usr/share/games/fortune and then simply call "fortune foo"
> > instead of "fortune /usr/local/share/games/fortune/ru/foo".
> > 
> > I decided to call readlink(2) explicitly rather than adding
> > check of file type being open, since that resulted in less code.
> > 
> > Ah, and yes, I have a port of Russian fortune files pending,
> > but that's for another list and another day.
> > 
> > So... okay/notokay anyone? :)

--
WBR,
  Vadim Zhukov


Index: fortune.c
===
RCS file: /cvs/src/games/fortune/fortune/fortune.c,v
retrieving revision 1.60
diff -u -p -r1.60 fortune.c
--- fortune.c   10 Aug 2017 17:00:08 -  1.60
+++ fortune.c   15 Dec 2020 11:46:00 -
@@ -39,6 +39,7 @@
 #include 
 #include 
 #include 
+#include 
 #include 
 #include 
 #include 
@@ -388,8 +389,9 @@ add_file(int percent, char *file, char *
 FILEDESC *parent)
 {
FILEDESC*fp;
+   ssize_t  lnklen;
int fd;
-   char*path, *offensive;
+   char*path, *offensive, lnkpath[PATH_MAX];
boolwas_malloc;
boolisdir;
 
@@ -420,8 +422,22 @@ add_file(int percent, char *file, char *
}
 
DPRINTF(1, (stderr, "adding file \"%s\"\n", path));
+
 over:
+   lnklen = readlink(path, lnkpath, sizeof(lnkpath));
+   if (lnklen == -1) {
+   if (errno != EINVAL)
+   goto openfailed;
+   } else {
+   lnkpath[lnklen] = '\0';
+   if (was_malloc)
+   free(path);
+   path = copy(lnkpath, NULL);
+   was_malloc = 1;
+   }
+
if ((fd = open(path, O_RDONLY)) < 0) {
+openfailed:
/*
 * This is a sneak.  If the user said -a, and if the
 * file we're given isn't a file, we check to see if
@@ -718,7 +734,7 @@ copy(char *str, char *suf)
char*new;
 
if (asprintf(, "%s%s", str, suf ? suf : "") == -1)
-   return NULL;
+   err(1, NULL);
return new;
 }
 

fortune: allow to use symlinks

[Vadim Zhukov]

Hello, all.

This allows fortune(6) to open fortune files via symlinks.
Maybe this is a stupid idea, I dunno, but it seems inconsistent
that I can't put a symlink to my fortunes collection in
/usr/share/games/fortune and then simply call "fortune foo"
instead of "fortune /usr/local/share/games/fortune/ru/foo".

I decided to call readlink(2) explicitly rather than adding
check of file type being open, since that resulted in less code.

Ah, and yes, I have a port of Russian fortune files pending,
but that's for another list and another day.

So... okay/notokay anyone? :)

--
WBR,
  Vadim Zhukov


Index: fortune.c
===
RCS file: /cvs/src/games/fortune/fortune/fortune.c,v
retrieving revision 1.60
diff -u -p -r1.60 fortune.c
--- fortune.c   10 Aug 2017 17:00:08 -  1.60
+++ fortune.c   14 Dec 2020 22:05:33 -
@@ -39,6 +39,7 @@
 #include 
 #include 
 #include 
+#include 
 #include 
 #include 
 #include 
@@ -388,8 +389,9 @@ add_file(int percent, char *file, char *
 FILEDESC *parent)
 {
FILEDESC*fp;
+   ssize_t  lnklen;
int fd;
-   char*path, *offensive;
+   char*path, *offensive, lnkpath[PATH_MAX];
boolwas_malloc;
boolisdir;
 
@@ -420,8 +422,22 @@ add_file(int percent, char *file, char *
}
 
DPRINTF(1, (stderr, "adding file \"%s\"\n", path));
+
 over:
+   lnklen = readlink(path, lnkpath, PATH_MAX);
+   if (lnklen == -1) {
+   if (errno != EINVAL)
+   goto openfailed;
+   } else {
+   lnkpath[lnklen] = '\0';
+   if (was_malloc)
+   free(path);
+   path = strdup(lnkpath);
+   was_malloc = 1;
+   }
+
if ((fd = open(path, O_RDONLY)) < 0) {
+openfailed:
/*
 * This is a sneak.  If the user said -a, and if the
 * file we're given isn't a file, we check to see if

Re: Unbreak X:Y user/group spec in pf.conf

[Vadim Zhukov]

16 января 2020 г. 15:58:09 GMT+03:00, Klemens Nanni  пишет:
>On Thu, Jan 16, 2020 at 01:16:27PM +0100, Alexandr Nedvedicky wrote:
>> sentence 'The syntax is similar to the one for ports' sets my
>expectations
>> I can define a range of users in the same way I define a range of
>ports.
>> Looks useful to me, though a bug in parse.y might be just a tip
>of iceberg
>> here.
>I *assume* Vadim tripped over this implication, but that's what I
>wanted
>to know.  That said, probably being biased here, "similar to the one
>for
>ports" does not read like "the same as ports" to me.

(2Theo: yes, I'm lazy, sorry :) )

I agree, that "X:Y" syntax for "user" could be confusing, and "X>After convincing Sasha in the hackroom that the range syntax for
>user/group is rather misleading and not worth the effort, he in turn
>made a convincing point about how mapping user ranges with existing
>syntax might go wrong:
>
>   $  echo 'pass on lo proto tcp user { >= 1000 , <= 2000 }' | pfctl
>-vnf-
>   pass on lo proto tcp all user >= 1000 flags S/SA
>   pass on lo proto tcp all user <= 2000 flags S/SA
>
>Note how --depending on other keywords-- the provided inclusive range
>might evaluate to rules that pass more than desired;  above example
>will
>pass all users since the [1000, 2000] is eventually used as [1000, inf]
>and [0, 2000] which together make for [0, inf], that is all users.
>
>With proper ranges as for ports the ruleset would evaluate to what
>users
>actually wanted.  So ranges *can* already be covered but not in a sane
>and actually safe way.

The "expansion" feature could be used wrong for other config clauses as well, 
especially when negation comes to play. I'm not sure if this should be changed 
at all... We may force that only one of two syntaxes may be used:

user { foo, bar }
user { 1000 till 1999, >=1 }

-- 
With best regards,
Vadim Zhukov

Re: Unbreak X:Y user/group spec in pf.conf

[Vadim Zhukov]

16 января 2020 г. 9:14:43 GMT+03:00, Theo de Raadt  пишет:
>I'll bite, using text from your regress.
>
>> +pass out proto tcp all user 1234:12345 flags S/SA
>> +pass out proto tcp all user 0:12345 flags S/SA
>> +pass out proto tcp all group 1234:12345 flags S/SA
>> +pass out proto tcp all group 0:12345 flags S/SA
>
>What does 1234:12345 mean.  It must be uid 1234 _and_ gid 12345?

The manpage for "user"and "group" clauses say they have similar syntax with 
"port". I was surprised when tried to use "X:Y" notation in "user"clause, 
though.

>I don't quite understand, what is the purpose to this precise check?
>How often does this kind of precise request happen?
>
>i would expect the normal pattern is to pass "for this uid" or "for
>this gid", but I'm surprised to see "for this uid AND this gid, both
>must match", and I doubt you are representing "either this uid or this
>gid".
>
>Can you explain the use case?

I have a server with shell accounts for students. Their uids are in, say, 2000 
to 5999 range. I want to allow all of them to connect e.g. to Github or 
anoncvs, but not to whole Internet.

I can use "1999><6000" as alternative, of course, but then manual must be 
tweaked, IMO.

>Vadim Zhukov  wrote:
>
>> I've just found that pfctl doesn't like 'X:Y' syntax for user and
>group
>> clauses, despite of the words in manpage. The problem is caused by
>> parser eating the colon character in STRING version of "uid" and
>"gid"
>> rules. The solution is similar to the way ports parsing is done. Now
>we
>> have "uidrange" and "gidrange" rules, similar to "portrange". I
>didn't
>> try to unify those two (or even three) to avoid increasing
>parentheses
>> madness, but if somebody would come with better diff/idea, I'm open.
>:)
>> 
>> This diff also adds a regression test, for the sake of completeness.
>> Existing regression tests pass on amd64.
>> 
>> OK?
>> 
>> --
>> WBR,
>>   Vadim Zhukov
>> 
>> 
>> Index: sbin/pfctl/parse.y
>> ===
>> RCS file: /cvs/src/sbin/pfctl/parse.y,v
>> retrieving revision 1.699
>> diff -u -p -r1.699 parse.y
>> --- sbin/pfctl/parse.y   17 Oct 2019 21:54:28 -  1.699
>> +++ sbin/pfctl/parse.y   16 Jan 2020 00:44:14 -
>> @@ -468,6 +468,7 @@ typedef struct {
>>  #define PPORT_RANGE 1
>>  #define PPORT_STAR  2
>>  int parseport(char *, struct range *r, int);
>> +int parse_ugid(char *, struct range *r, int);
>>  
>>  #define DYNIF_MULTIADDR(addr) ((addr).type == PF_ADDR_DYNIFTL && \
>>  (!((addr).iflags & PFI_AFLAG_NOALIAS) || \
>> @@ -496,7 +497,7 @@ int  parseport(char *, struct range *r, i
>>  %tokenNUMBER
>>  %token PORTBINARY
>>  %type  interface if_list if_item_not if_item
>> -%type number icmptype icmp6type uid gid
>> +%type number icmptype icmp6type
>>  %type tos not yesno optnodf
>>  %typeprobability
>>  %type optweight
>> @@ -504,7 +505,7 @@ int  parseport(char *, struct range *r, i
>>  %type  sourcetrack flush unaryop statelock
>>  %type  action
>>  %type  flags flag blockspec prio
>> -%type  portplain portstar portrange
>> +%type  portplain portstar portrange uidrange 
>> gidrange
>>  %typehashkey
>>  %type  proto proto_list proto_item
>>  %type protoval
>> @@ -2957,69 +2958,67 @@ uid_list : uid_item optnl{ $$ = 
>> $1; }
>>  }
>>  ;
>>  
>> -uid_item: uid   {
>> +uid_item: uidrange  {
>>  $$ = calloc(1, sizeof(struct node_uid));
>>  if ($$ == NULL)
>>  err(1, "uid_item: calloc");
>> -$$->uid[0] = $1;
>> -$$->uid[1] = $1;
>> -$$->op = PF_OP_EQ;
>> +$$->uid[0] = $1.a;
>> +$$->uid[1] = $1.b;
>> +if ($1.t)
>> +$$->op = PF_OP_RRG;
>> +els

Unbreak X:Y user/group spec in pf.conf

[Vadim Zhukov]

Hi all.

I've just found that pfctl doesn't like 'X:Y' syntax for user and group
clauses, despite of the words in manpage. The problem is caused by
parser eating the colon character in STRING version of "uid" and "gid"
rules. The solution is similar to the way ports parsing is done. Now we
have "uidrange" and "gidrange" rules, similar to "portrange". I didn't
try to unify those two (or even three) to avoid increasing parentheses
madness, but if somebody would come with better diff/idea, I'm open. :)

This diff also adds a regression test, for the sake of completeness.
Existing regression tests pass on amd64.

OK?

--
WBR,
  Vadim Zhukov


Index: sbin/pfctl/parse.y
===
RCS file: /cvs/src/sbin/pfctl/parse.y,v
retrieving revision 1.699
diff -u -p -r1.699 parse.y
--- sbin/pfctl/parse.y  17 Oct 2019 21:54:28 -  1.699
+++ sbin/pfctl/parse.y  16 Jan 2020 00:44:14 -
@@ -468,6 +468,7 @@ typedef struct {
 #define PPORT_RANGE1
 #define PPORT_STAR 2
 intparseport(char *, struct range *r, int);
+intparse_ugid(char *, struct range *r, int);
 
 #define DYNIF_MULTIADDR(addr) ((addr).type == PF_ADDR_DYNIFTL && \
(!((addr).iflags & PFI_AFLAG_NOALIAS) || \
@@ -496,7 +497,7 @@ int parseport(char *, struct range *r, i
 %token   NUMBER
 %tokenPORTBINARY
 %type interface if_list if_item_not if_item
-%typenumber icmptype icmp6type uid gid
+%typenumber icmptype icmp6type
 %typetos not yesno optnodf
 %type   probability
 %typeoptweight
@@ -504,7 +505,7 @@ int parseport(char *, struct range *r, i
 %type sourcetrack flush unaryop statelock
 %type action
 %type flags flag blockspec prio
-%type portplain portstar portrange
+%type portplain portstar portrange uidrange gidrange
 %type   hashkey
 %type proto proto_list proto_item
 %typeprotoval
@@ -2957,69 +2958,67 @@ uid_list: uid_item optnl{ $$ = 
$1; }
}
;
 
-uid_item   : uid   {
+uid_item   : uidrange  {
$$ = calloc(1, sizeof(struct node_uid));
if ($$ == NULL)
err(1, "uid_item: calloc");
-   $$->uid[0] = $1;
-   $$->uid[1] = $1;
-   $$->op = PF_OP_EQ;
+   $$->uid[0] = $1.a;
+   $$->uid[1] = $1.b;
+   if ($1.t)
+   $$->op = PF_OP_RRG;
+   else
+   $$->op = PF_OP_EQ;
$$->next = NULL;
$$->tail = $$;
}
-   | unaryop uid   {
-   if ($2 == -1 && $1 != PF_OP_EQ && $1 != PF_OP_NE) {
+   | unaryop uidrange  {
+   if ($2.a == -1 && $1 != PF_OP_EQ && $1 != PF_OP_NE) {
yyerror("user unknown requires operator = or "
"!=");
YYERROR;
}
+   if ($2.t) {
+   yyerror("':' cannot be used with an other "
+   "user operator");
+   YYERROR;
+   }
$$ = calloc(1, sizeof(struct node_uid));
if ($$ == NULL)
err(1, "uid_item: calloc");
-   $$->uid[0] = $2;
-   $$->uid[1] = $2;
+   $$->uid[0] = $2.a;
+   $$->uid[1] = $2.b;
$$->op = $1;
$$->next = NULL;
$$->tail = $$;
}
-   | uid PORTBINARY uid{
-   if ($1 == -1 || $3 == -1) {
+   | uidrange PORTBINARY uidrange  {
+   if ($1.a == -1 || $3.a == -1) {
yyerror("user unknown requires operator = or "
"!=");
YYERROR;
}
+   if ($1.t || $3.t) {
+   yyerror("':' cannot be used with an other "
+   "user operator");
+   YYERROR;
+  

avoid uvideo lockup when nothing is transferred

[Vadim Zhukov]

Hi all.

This fixes an issue I'm seeing with a uvideo(4), that doesn't like
commands we're sending to it. The camera simply sends nothing,
and since we're sleeping forever (xfer timeout is 0, which is
USBD_NO_TIMEOUT), we never get out from 'while (bulk_running)' loop,
visible in the scond chunk of the diff. This not only makes video(1)
and other V4L2 users lockup, but also results in process freeze upon
detach: it calls uvideo_vs_close(), which in turn calls
usbd_ref_wait(), which doesn't return because we still have something
in queue.

Also, in case of error we keep bulk_running set. This seems just
a leftover, as well as the first chunk: if we failed to create kthread,
there won't be anything running under bulk_running==1 for sure.

I must thank kettenis@ for help during diagnosis and mpi@ for a patch
for related issue.

OK?

--
WBR,
  Vadim Zhukov


Index: uvideo.c
===
RCS file: /cvs/src/sys/dev/usb/uvideo.c,v
retrieving revision 1.205
diff -u -p -r1.205 uvideo.c
--- uvideo.c14 Oct 2019 09:20:48 -  1.205
+++ uvideo.c15 Jan 2020 21:54:43 -
@@ -2026,6 +2027,7 @@ uvideo_vs_start_bulk(struct uvideo_softc
error = kthread_create(uvideo_vs_start_bulk_thread, sc, NULL,
DEVNAME(sc));
if (error) {
+   sc->sc_vs_cur->bulk_running = 0;
printf("%s: can't create kernel thread!", DEVNAME(sc));
return (error);
}
@@ -2044,6 +2046,12 @@ uvideo_vs_start_bulk_thread(void *arg)
while (sc->sc_vs_cur->bulk_running) {
size = UGETDW(sc->sc_desc_probe.dwMaxPayloadTransferSize);
 
+   /*
+* We can't wait infinitely, since otherwise we'll
+* block forever if camera stops (or don't even starts)
+* sending frames. Use '2*' multiplier to compensate
+* possible lags.
+*/
usbd_setup_xfer(
sc->sc_vs_cur->bxfer.xfer,
sc->sc_vs_cur->pipeh,
@@ -2051,10 +2059,11 @@ uvideo_vs_start_bulk_thread(void *arg)
sc->sc_vs_cur->bxfer.buf,
size,
USBD_NO_COPY | USBD_SHORT_XFER_OK | USBD_SYNCHRONOUS,
-   0,
+   2 * 1000 / (sc->sc_frame_rate || 1),
NULL);
error = usbd_transfer(sc->sc_vs_cur->bxfer.xfer);
if (error != USBD_NORMAL_COMPLETION) {
+   sc->sc_vs_cur->bulk_running = 0;
DPRINTF(1, "%s: error in bulk xfer: %s!\n",
DEVNAME(sc), usbd_errstr(error));
break;

Re: Scrolling in top(1)

[Vadim Zhukov]

5 января 2020 г. 22:37:05 GMT+02:00, 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.

Yes, indication would be good and I thought about it, too. But implementing it 
in current top(1) code is not that straightforward, unfortunately, so I planned 
to do this separately, avoiding complication of the current diff. Thanks!
-- 
With best regards,
Vadim Zhukov

Scrolling in top(1)

[Vadim Zhukov]

Hi all.

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. :)

--
WBR,
  Vadim Zhukov


Index: machine.c
===
RCS file: /cvs/src/usr.bin/top/machine.c,v
retrieving revision 1.101
diff -u -p -r1.101 machine.c
--- machine.c   16 Dec 2019 19:21:17 -  1.101
+++ machine.c   5 Jan 2020 14:10:44 -
@@ -546,6 +546,14 @@ format_comm(struct kinfo_proc *kp)
return (buf);
 }
 
+void
+skip_next_process(struct handle *hndl)
+{
+   /* find and remember the next proc structure */
+   hndl->next_proc++;
+   hndl->remaining--;
+}
+
 char *
 format_next_process(struct handle *hndl, const char *(*get_userid)(uid_t, int),
 pid_t *pid)
Index: machine.h
===
RCS file: /cvs/src/usr.bin/top/machine.h,v
retrieving revision 1.27
diff -u -p -r1.27 machine.h
--- machine.h   8 Oct 2019 20:51:03 -   1.27
+++ machine.h   5 Jan 2020 14:10:44 -
@@ -90,6 +90,7 @@ extern void get_system_info(struct s
 extern struct handle
 *get_process_info(struct system_info *, struct process_select *,
 int (*) (const void *, const void *));
+extern void skip_next_process(struct handle *);
 extern char*format_next_process(struct handle *,
 const char *(*)(uid_t, int), pid_t *);
 extern uid_tproc_owner(pid_t);
Index: top.1
===
RCS file: /cvs/src/usr.bin/top/top.1,v
retrieving revision 1.71
diff -u -p -r1.71 top.1
--- top.1   28 Nov 2018 22:00:30 -  1.71
+++ top.1   5 Jan 2020 14:10:44 -
@@ -291,6 +291,10 @@ or any process highlighting put in place
 interactive command.
 .It 1
 Toggle the display of per CPU or combined CPU statistics.
+.It 9 | 0 
+Scroll up/down the process list by one line.
+.It \&( | \&) 
+Scroll up/down the process list by screen half.
 .It C
 Toggle the display of process command line arguments.
 .It d Ar count
Index: top.c
===
RCS file: /cvs/src/usr.bin/top/top.c,v
retrieving revision 1.101
diff -u -p -r1.101 top.c
--- top.c   8 Oct 2019 20:51:03 -   1.101
+++ top.c   5 Jan 2020 14:10:44 -
@@ -68,6 +68,7 @@ static void   reset_display(void);
 intrundisplay(void);
 
 static int max_topn;   /* maximum displayable processes */
+static int skip;   /* how many processes to skip (scroll) */
 
 extern int ncpu;
 extern int ncpuonline;
@@ -126,6 +127,10 @@ struct statics  statics;
 #define CMD_add21
 #define CMD_hl 22
 #define CMD_cpus   23
+#define CMD_down   24
+#define CMD_up 25
+#define CMD_pagedown   26
+#define CMD_pageup 27
 
 static void
 usage(void)
@@ -557,6 +562,15 @@ restart:
active_procs = topn;
if (active_procs > max_topn)
active_procs = max_topn;
+   /* determine how many process to skip, if asked to */
+   /*
+* this number is tweaked by user, but gets shrinked
+* when number of active processes lowers too much
+*/
+   if (skip + active_procs > system_info.p_active)
+   skip = system_info.p_active - active_procs;
+   for (i = skip; i > 0; i--)
+   skip_next_process(processes);
/* now show the top "n" processes. */
for (i = 0; i < active_procs; i++) {
pid_t pid;
@@ -618,7 +632,7 @@ rundisplay(void)
char ch, *iptr;
int change, i;
struct pollfd pfd[1];
-   static char command_chars[] = "\f qh?en#sdkriIuSopCHg+P1";
+   static char command_chars[] = "\f qh?en#sdkriIuSopCHg+P109)(";
 
/*
 * assume valid command unless told
@@ -966,6 +980,21 @@ rundisplay(void)
combine_cpus = !combine_cpus;
max_topn = display_resize();
reset_display();
+   break;
+   case CMD_down:
+   skip++;
+ 

Re: syspatch -c as non-root

[Vadim Zhukov]

сб, 14 дек. 2019 г. в 14:35, Antoine Jacoutot :
>
> On Sat, Dec 14, 2019 at 10:12:36AM +0300, Vadim Zhukov wrote:
> > Hello all (long time no see!)
> >
> > TL;DR: Allow syspatch -c run under non-priviledged user.
> >
> > Reasoning: instead of putting syspatch -c in crontab, I've implemented
> > a Zabbix trigger. Since the Zabbix agent runs as unpriviledged user,
> > I had to add _zabbix line to doas.conf, allowing it to run syspatch -c.
> > This way it works, but in the end I want this check either in stock
> > Zabbix code, or in our packages. And requirement to modify doas.conf
> > ruins all the fun.
> >
> > There is another way: writing some SUID script as simple as:
> >
> >   #!/bin/sh
> >   case $1 in
> >   missing-syspatches) syspatch -c;;
> >   failed-services) rcctl ls failed;;
> >   *) echo "usage: ..." >&2; exit 1;;
> >   esac
> >
> > This way we could handle all possible future root-is-required cases
> > at once. But adding SUID script... is it a Good Thing(TM)?
> >
> > But anyway, root-only requirement for listing available syspatches
> > seems a bit silly.
> >
> > The patch was tested on 6.6. Well, it doesn't apply cleanly to 6.6,
> > since "set +e" magic you see arrived in -CURRENT, but tweaked version
> > runs smoothly.
> >
> > BTW, why do we ever call eval in unpriv()? Without eval, set +e isn't
> > needed, FWIW.
>
> I don't really like this.
> It defeats the purpose of unpriv which guarantees you that a specific
> unprivileged user will be used to run unsafe commands.

Just to make sure I understand you correctly: are the "unsafe
commands" you mentioned the ftp(1) calls? The default case ("apply new
syspatches") and -R/-r cases do require running as root still, so no
changes in behavior here: the root check happens before any unpriv()
call. The -l case, which is already allowed for non-root, accesses
local files in /var/syspatch, and this already looks like more fragile
(accessing local data), than calling pledged ftp(1).

Or you mean that unpriv() function itself should be run as root only,
to clearly indicate intent?

> > Index: syspatch.sh
> > ===
> > RCS file: /cvs/src/usr.sbin/syspatch/syspatch.sh,v
> > retrieving revision 1.159
> > diff -u -p -r1.159 syspatch.sh
> > --- syspatch.sh   10 Dec 2019 17:11:06 -  1.159
> > +++ syspatch.sh   14 Dec 2019 06:50:20 -
> > @@ -179,7 +179,7 @@ ls_missing()
> >   ${_MIRROR}/syspatch${_OSrev}-${_p}.tgz"
> >   { unpriv ${_cmd} | tar tzf -; } 2>/dev/null | while read _f; 
> > do
> >   [[ -f /${_f} ]] || continue && echo ${_p} && pkill -u 
> > \
> > - _syspatch -xf "${_cmd}" || true && break
> > + $_USER -xf "${_cmd}" || true && break
> >   done
> >   done | sort -V
> >  }
> > @@ -245,22 +245,26 @@ must be run manually to install the new
> >
> >  unpriv()
> >  {
> > - local _file=$2 _rc=0 _user=_syspatch
> > + local _file=$2 _rc=0
> >
> >   if [[ $1 == -f && -n ${_file} ]]; then
> >   >${_file}
> > - chown "${_user}" "${_file}"
> > + chown "${_USER}" "${_file}"
> >   chmod 0711 ${_TMP}
> >   shift 2
> >   fi
> >   (($# >= 1))
> >
> > - # XXX ksh(1) bug; send error code to the caller instead of failing 
> > hard
> > - set +e
> > - eval su -s /bin/sh ${_user} -c "'$@'" || _rc=$?
> > - set -e
> > -
> > - [[ -n ${_file} ]] && chown root "${_file}"
> > + if (($(id -u) == 0)); then
> > + # XXX ksh(1) bug; send error code to the caller instead of 
> > failing hard
> > + set +e
> > + eval su -s /bin/sh ${_USER} -c "'$@'" || _rc=$?
> > + set -e
> > +
> > + [[ -n ${_file} ]] && chown root "${_file}"
> > + else
> > + "$@" || _rc=$?
> > + fi
> >
> >   return ${_rc}
> >  }
> > @@ -270,7 +274,7 @@ set -A _KERNV -- $(sysctl -n kern.versio
> >   sed 's/^OpenBSD \([1-9][0-9]*\.[0-9]\)\([^ ]*\).*/\1 \2/;q')
> >  ((${#_KERNV[*]} > 1)) && sp_err "Unsupported release: 
> > ${_KERNV[0]}${_KERNV[1]}"
> >
> > -[[ $@ == @(|-[[:alpha:]]) ]] || usage; [[ $@ == @(|-(c|R|r)) ]] &&
> > +[[ $@ == @(|-[[:alpha:]]) ]] || usage; [[ $@ == @(|-(R|r)) ]] &&
> >   (($(id -u) != 0)) && sp_err "need root privileges"
> >  [[ $@ == @(|-(R|r)) ]] && pgrep -qxf '/bin/ksh .*reorder_kernel' &&
> >   sp_err "cannot apply patches while reorder_kernel is running"
> > @@ -290,7 +294,13 @@ _PDIR="/var/syspatch"
> >  _TMP=$(mktemp -d -p ${TMPDIR:-/tmp} syspatch.XX)
> >  _KARL=false
> >
> > -readonly _BSDMP _KERNV _MIRROR _OSrev _PDIR _TMP
> > +if (($(id -u) != 0)); then
> > + _USER=$(id -nu)
> > +else
> > + _USER=_syspatch
> > +fi
> > +
> > +readonly _BSDMP _KERNV _MIRROR _OSrev _PDIR _TMP _USER
> >
> >  trap 'trap_handler' EXIT
> >  trap exit HUP INT TERM

-- 
  WBR,
  Vadim Zhukov

syspatch -c as non-root

[Vadim Zhukov]

Hello all (long time no see!)

TL;DR: Allow syspatch -c run under non-priviledged user.

Reasoning: instead of putting syspatch -c in crontab, I've implemented
a Zabbix trigger. Since the Zabbix agent runs as unpriviledged user,
I had to add _zabbix line to doas.conf, allowing it to run syspatch -c.
This way it works, but in the end I want this check either in stock
Zabbix code, or in our packages. And requirement to modify doas.conf
ruins all the fun.

There is another way: writing some SUID script as simple as:

  #!/bin/sh
  case $1 in
  missing-syspatches) syspatch -c;;
  failed-services) rcctl ls failed;;
  *) echo "usage: ..." >&2; exit 1;;
  esac

This way we could handle all possible future root-is-required cases
at once. But adding SUID script... is it a Good Thing(TM)?

But anyway, root-only requirement for listing available syspatches
seems a bit silly.

The patch was tested on 6.6. Well, it doesn't apply cleanly to 6.6,
since "set +e" magic you see arrived in -CURRENT, but tweaked version
runs smoothly.

BTW, why do we ever call eval in unpriv()? Without eval, set +e isn't
needed, FWIW.

--
WBR,
  Vadim Zhukov


Index: syspatch.sh
===
RCS file: /cvs/src/usr.sbin/syspatch/syspatch.sh,v
retrieving revision 1.159
diff -u -p -r1.159 syspatch.sh
--- syspatch.sh 10 Dec 2019 17:11:06 -  1.159
+++ syspatch.sh 14 Dec 2019 06:50:20 -
@@ -179,7 +179,7 @@ ls_missing()
${_MIRROR}/syspatch${_OSrev}-${_p}.tgz"
{ unpriv ${_cmd} | tar tzf -; } 2>/dev/null | while read _f; do
[[ -f /${_f} ]] || continue && echo ${_p} && pkill -u \
-   _syspatch -xf "${_cmd}" || true && break
+   $_USER -xf "${_cmd}" || true && break
done
done | sort -V
 }
@@ -245,22 +245,26 @@ must be run manually to install the new 
 
 unpriv()
 {
-   local _file=$2 _rc=0 _user=_syspatch
+   local _file=$2 _rc=0
 
if [[ $1 == -f && -n ${_file} ]]; then
>${_file}
-   chown "${_user}" "${_file}"
+   chown "${_USER}" "${_file}"
chmod 0711 ${_TMP}
shift 2
fi
(($# >= 1))
 
-   # XXX ksh(1) bug; send error code to the caller instead of failing hard
-   set +e
-   eval su -s /bin/sh ${_user} -c "'$@'" || _rc=$?
-   set -e
-
-   [[ -n ${_file} ]] && chown root "${_file}"
+   if (($(id -u) == 0)); then
+   # XXX ksh(1) bug; send error code to the caller instead of 
failing hard
+   set +e
+   eval su -s /bin/sh ${_USER} -c "'$@'" || _rc=$?
+   set -e
+
+   [[ -n ${_file} ]] && chown root "${_file}"
+   else
+   "$@" || _rc=$?
+   fi
 
return ${_rc}
 }
@@ -270,7 +274,7 @@ set -A _KERNV -- $(sysctl -n kern.versio
sed 's/^OpenBSD \([1-9][0-9]*\.[0-9]\)\([^ ]*\).*/\1 \2/;q')
 ((${#_KERNV[*]} > 1)) && sp_err "Unsupported release: ${_KERNV[0]}${_KERNV[1]}"
 
-[[ $@ == @(|-[[:alpha:]]) ]] || usage; [[ $@ == @(|-(c|R|r)) ]] &&
+[[ $@ == @(|-[[:alpha:]]) ]] || usage; [[ $@ == @(|-(R|r)) ]] &&
(($(id -u) != 0)) && sp_err "need root privileges"
 [[ $@ == @(|-(R|r)) ]] && pgrep -qxf '/bin/ksh .*reorder_kernel' &&
sp_err "cannot apply patches while reorder_kernel is running"
@@ -290,7 +294,13 @@ _PDIR="/var/syspatch"
 _TMP=$(mktemp -d -p ${TMPDIR:-/tmp} syspatch.XX)
 _KARL=false
 
-readonly _BSDMP _KERNV _MIRROR _OSrev _PDIR _TMP
+if (($(id -u) != 0)); then
+   _USER=$(id -nu)
+else
+   _USER=_syspatch
+fi
+
+readonly _BSDMP _KERNV _MIRROR _OSrev _PDIR _TMP _USER
 
 trap 'trap_handler' EXIT
 trap exit HUP INT TERM

Re: About differences with GNU patch(1)

[Vadim Zhukov]

So after a few discussions I propose to add --dry-run as synonim for -C
in our patch(1). Quick summary:

  * GNU got --dry-run earlier than us;
  * --dry-run way more popular name than --check for such functionality;
  * FreeBSD and NetBSD has the same for a long time already;
  * We don't care about long option names generally, anyway.

On the second option, --binary, since we have no functionality and
I see no demand for it, lets just keep things as is. Failing with
"unkonwn option" message is clear indicator that requested functionality
isn't supported.

Builds and runs on (almost) -CURRENT. Okay or not?

--
  WBR,
Vadim Zhukov


Index: patch.c
===
RCS file: /cvs/src/usr.bin/patch/patch.c,v
retrieving revision 1.65
diff -u -p -r1.65 patch.c
--- patch.c 7 Apr 2018 14:55:13 -   1.65
+++ patch.c 22 Jun 2018 09:12:53 -
@@ -454,6 +454,7 @@ get_some_switches(void)
{"context", no_argument,0,  'c'},
{"debug",   required_argument,  0,  'x'},
{"directory",   required_argument,  0,  'd'},
+   {"dry-run", no_argument,0,  'C'},
{"ed",  no_argument,0,  'e'},
{"force",   no_argument,0,  'f'},
{"forward", no_argument,0,  'N'},

Re: About differences with GNU patch(1)

[Vadim Zhukov]

ср, 20 июн. 2018 г. в 22:09, Stuart Henderson :
>
> On 2018/06/20 21:17, Vadim Zhukov wrote:
> > Hi,
> >
> > The Ansible "patch" module fails to work on OpenBSD because it tries
> > to use "--dry-run" command-line option on patch(1), while ours
> > supports -C/--check instead. For now, I have an obvious, hm, patch :)
> > for patch.py that replaces --dry-run with --check. But what way do
> > people prefer: to move OpenBSD's patch(1) closer to GNU one, or to add
> > some logic to patch.py that'll detect correct command-line option to
> > use?
>
> I think it would be right to add some logic. By the time you're running
> a command like this Ansible already knows what OS it's connecting to,
> it should use that knowledge, and that way it will work on old OpenBSD
> versions too.
>
> Since patch already accepts long options, it might also be worth
> accepting --dry-run as a synonym for --check, as long as it does the
> same thing.

It's absolutely the same, yes.

> > There's also an issue with --binary: we don't support this option on
> > OpenBSD, and  GNU patch manual page says it's a no-op on POSIX anyway.
> > Would it be okay to add --binary as a no-op option to our patch?
>
> It says "This option is needed on POSIX systems when applying patches
> generated on non-POSIX systems to non-POSIX files", so I think it does
> do something there? However our patch doesn't have heuristics for
> handling line-endings like GNU patch does..

Ah, I've misunderstood "binary mode". Then, yes, this is not a no-op.
This option isn't a real problem though, since if you're feeding our
patch() with invalid line endings, you're screwed already. And you can
just do not set "binary" flag in Ansible to avoid --binary there. :)

--
  WBR,
  Vadim Zhukov

Re: About differences with GNU patch(1)

[Vadim Zhukov]

ср, 20 июн. 2018 г. в 21:59, Chris Cappuccio :
>
> Vadim Zhukov [persg...@gmail.com] wrote:
> > , 20 ??. 2018 ??. ?? 21:17, Vadim Zhukov :
> > >
> > > Hi,
> > >
> > > The Ansible "patch" module fails to work on OpenBSD because it tries
> > > to use "--dry-run" command-line option on patch(1), while ours
> > > supports -C/--check instead. For now, I have an obvious, hm, patch :)
> > > for patch.py that replaces --dry-run with --check. But what way do
> > > people prefer: to move OpenBSD's patch(1) closer to GNU one, or to add
> > > some logic to patch.py that'll detect correct command-line option to
> > > use?
> > >
> > > There's also an issue with --binary: we don't support this option on
> > > OpenBSD, and  GNU patch manual page says it's a no-op on POSIX anyway.
> > > Would it be okay to add --binary as a no-op option to our patch?
> >
> > BTW, the FreeBSD patch got --dry-run 4 years ago:
> > https://svnweb.freebsd.org/base/head/usr.bin/patch/patch.c?r1=267490=267512
> >
> > NetBSD got it in 2005:
> > http://cvsweb.netbsd.org/bsdweb.cgi/src/usr.bin/patch/patch.c.diff?r1=1.22=1.23_with_tag=MAIN
> >
> > Neither has --binary.
> >
>
> Marc Espie did -C in 1998. See:
>
> http://cvsweb.openbsd.org/cgi-bin/cvsweb/src/usr.bin/patch/patch.c.diff?r1=1.10=1.11
>
> I think OpenBSD generally shies away from --gnu-style-options because they're
> rather ugly compared to the originals. Unfortunately GNU is apparently setting
> the standard for FreeBSD and NetBSD now.

But to be honest, GNU implemented --dry-run in 1997, so they were earlier.

--
  WBR,
  Vadim Zhukov

Re: About differences with GNU patch(1)

[Vadim Zhukov]

ср, 20 июн. 2018 г. в 21:17, Vadim Zhukov :
>
> Hi,
>
> The Ansible "patch" module fails to work on OpenBSD because it tries
> to use "--dry-run" command-line option on patch(1), while ours
> supports -C/--check instead. For now, I have an obvious, hm, patch :)
> for patch.py that replaces --dry-run with --check. But what way do
> people prefer: to move OpenBSD's patch(1) closer to GNU one, or to add
> some logic to patch.py that'll detect correct command-line option to
> use?
>
> There's also an issue with --binary: we don't support this option on
> OpenBSD, and  GNU patch manual page says it's a no-op on POSIX anyway.
> Would it be okay to add --binary as a no-op option to our patch?

BTW, the FreeBSD patch got --dry-run 4 years ago:
https://svnweb.freebsd.org/base/head/usr.bin/patch/patch.c?r1=267490=267512

NetBSD got it in 2005:
http://cvsweb.netbsd.org/bsdweb.cgi/src/usr.bin/patch/patch.c.diff?r1=1.22=1.23_with_tag=MAIN

Neither has --binary.

--
  WBR,
  Vadim Zhukov

About differences with GNU patch(1)

[Vadim Zhukov]

Hi,

The Ansible "patch" module fails to work on OpenBSD because it tries
to use "--dry-run" command-line option on patch(1), while ours
supports -C/--check instead. For now, I have an obvious, hm, patch :)
for patch.py that replaces --dry-run with --check. But what way do
people prefer: to move OpenBSD's patch(1) closer to GNU one, or to add
some logic to patch.py that'll detect correct command-line option to
use?

There's also an issue with --binary: we don't support this option on
OpenBSD, and  GNU patch manual page says it's a no-op on POSIX anyway.
Would it be okay to add --binary as a no-op option to our patch?

--
  WBR,
  Vadim Zhukov

Re: unbuffered fread does not set error/EOF flags

[Vadim Zhukov]

#if 0
2018-05-05 22:17 GMT+03:00 Vadim Zhukov <persg...@gmail.com>:
> 2018-05-05 22:12 GMT+03:00 Vadim Zhukov <persg...@gmail.com>:
>> Hi all!
>>
>> Recently I was working on a program that uses stdio functions heavily.
>> While hunting for a bug, I've temporarily disabled buffering (via
>> setvbuf(f, NULL, _IONBF, 0)) and realized that program behavior was
>> changed heavily. The cause was that fread() stopped setting error flag
>> after hitting EAGAIN. It looks like its code is missing setting
>> EOF/error flags totally in case of unbuffered read, and first patch
>> (below) fixes this.
>>
>> I'm not sure if "r" variable value should be propagated to fp->_r, or
>> not. I've assumed that since things work without it now, this doesn't
>> need a change. And headache stops me from further investigation for now,
>> sorry.
>>
>> I understand that use stdio functions without buffering is somewhat
>> exotic, and that's for sure is the reason noone found this bug yet. But
>> this is still a bad thing, IMHO.
>>
>> While looking through the libc code I've also found that __sread() drops
>> __SOFF flag unconditionally for all read errors. IMHO, the EAGAIN case
>> shouldn't be affected here, since, obviously current offset doesn't
>> change and is still well-known.
>>
>> So... any comments, or ever okays? :)
>
> Doing "cvs diff" before mailing the patch works better than doing after.
> Sorry.
>
> --
> WBR,
>  Vadim Zhukov
>
>
> Index: stdio/fread.c
> ===
> RCS file: /cvs/src/lib/libc/stdio/fread.c,v
> retrieving revision 1.15
> diff -u -p -r1.15 fread.c
> --- stdio/fread.c21 Sep 2016 04:38:56 -   1.15
> +++ stdio/fread.c5 May 2018 19:14:27 -
> @@ -79,6 +79,10 @@ fread(void *buf, size_t size, size_t cou
> p += r;
> resid -= r;
> }
> +if (r == 0)
> +fp->_flags |= __SEOF;
> +else if (r < 0)
> +fp->_flags |= __SERR;
> FUNLOCKFILE(fp);
> return ((total - resid) / size);
> }
> Index: stdio/stdio.c
> ===
> RCS file: /cvs/src/lib/libc/stdio/stdio.c,v
> retrieving revision 1.10
> diff -u -p -r1.10 stdio.c
> --- stdio/stdio.c21 Sep 2016 04:38:56 -   1.10
> +++ stdio/stdio.c5 May 2018 19:14:27 -
> @@ -32,6 +32,7 @@
>  */
>
> #include 
> +#include 
> #include 
> #include "local.h"
>
> @@ -49,7 +50,7 @@ __sread(void *cookie, char *buf, int n)
> /* if the read succeeded, update the current offset */
> if (ret >= 0)
>     fp->_offset += ret;
> -else
> +else if (errno != EAGAIN)
> fp->_flags &= ~__SOFF; /* paranoia */
> return (ret);
> }

So, is anybody interested? Here is a program that demonstrates the bug.
The buffered and unbuffered input should produce same values here,
but it doesn't. The patch above fixes the situation.

--
WBR,
  Vadim Zhukov
#endif


#include 
#include 
#include 
#include 
#include 
#include 
#include 
#include 

int
main(int argc, char **argv) {
FILE*f1, *f2;
ssize_t  nwf1, nwf2, nrf1, nrf2;
int  pair1[2], pair2[2], rv = 0;
char msg[4096];

arc4random_buf(msg, sizeof(msg));

if (pipe2(pair1, O_NONBLOCK) == -1)
err(1, "pipe");
if (pipe2(pair2, O_NONBLOCK) == -1)
err(1, "pipe");

if ((f1 = fdopen(pair1[0], "r")) == NULL)
err(1, "fdopen");
if ((f2 = fdopen(pair2[0], "r")) == NULL)
err(1, "fdopen");

setvbuf(f2, NULL, _IONBF, 0);

nwf1 = write(pair1[1], msg, sizeof(msg));
printf("sent %zd bytes in buffered pipe\n", nwf1);
nwf2 = write(pair2[1], msg, sizeof(msg));
printf("sent %zd bytes in unbuffered pipe\n", nwf2);

printf("first read:\n");

nrf1 = fread(msg, 1, nwf1, f1);
printf("  buffered: fread()=>%zd, feof()=>%d, ferror()=>%d\n",
nrf1, feof(f1),   ferror(f1));
if (feof(f1))
rv++;
if (ferror(f1))
rv++;

nrf2 = fread(msg, 1, nwf2, f2);
printf("unbuffered: fread()=>%zd, feof()=>%d, ferror()=>%d\n",
nrf2, feof(f2),   ferror(f2));
if (feof(f2))
rv++;
if (ferror(f2))
rv++;

printf("second read:\n");

nrf1 = fread(msg

tcp(4) does not mention netinet/tcp.h

[Vadim Zhukov]

Hi all!

The tcp(4) page talks about TCP options which are available through
, but doesn't menthion this header. DragonFly BSD,
FreeBSD & Linux do; Solaris mentions this header only; NetBSD is like
us currently.

Okay? Or should we remove netinet/in.h from here as well (at least
getsockopt(TCP_MAXSEG) call compiles still)?

--
WBR,
  Vadim Zhukov


Index: tcp.4
===
RCS file: /cvs/src/share/man/man4/tcp.4,v
retrieving revision 1.23
diff -u -p -r1.23 tcp.4
--- tcp.4   10 Sep 2015 17:55:21 -  1.23
+++ tcp.4   10 May 2018 12:20:08 -
@@ -39,6 +39,7 @@
 .Sh SYNOPSIS
 .In sys/socket.h
 .In netinet/in.h
+.In netinet/tcp.h
 .Ft int
 .Fn socket AF_INET SOCK_STREAM 0
 .Ft int

Re: unbuffered fread does not set error/EOF flags

[Vadim Zhukov]

2018-05-05 22:12 GMT+03:00 Vadim Zhukov <persg...@gmail.com>:
> Hi all!
>
> Recently I was working on a program that uses stdio functions heavily.
> While hunting for a bug, I've temporarily disabled buffering (via
> setvbuf(f, NULL, _IONBF, 0)) and realized that program behavior was
> changed heavily. The cause was that fread() stopped setting error flag
> after hitting EAGAIN. It looks like its code is missing setting
> EOF/error flags totally in case of unbuffered read, and first patch
> (below) fixes this.
>
> I'm not sure if "r" variable value should be propagated to fp->_r, or
> not. I've assumed that since things work without it now, this doesn't
> need a change. And headache stops me from further investigation for now,
> sorry.
>
> I understand that use stdio functions without buffering is somewhat
> exotic, and that's for sure is the reason noone found this bug yet. But
> this is still a bad thing, IMHO.
>
> While looking through the libc code I've also found that __sread() drops
> __SOFF flag unconditionally for all read errors. IMHO, the EAGAIN case
> shouldn't be affected here, since, obviously current offset doesn't
> change and is still well-known.
>
> So... any comments, or ever okays? :)

Doing "cvs diff" before mailing the patch works better than doing after.
Sorry.

--
WBR,
  Vadim Zhukov


Index: stdio/fread.c
===
RCS file: /cvs/src/lib/libc/stdio/fread.c,v
retrieving revision 1.15
diff -u -p -r1.15 fread.c
--- stdio/fread.c   21 Sep 2016 04:38:56 -  1.15
+++ stdio/fread.c   5 May 2018 19:14:27 -
@@ -79,6 +79,10 @@ fread(void *buf, size_t size, size_t cou
p += r;
resid -= r;
}
+if (r == 0)
+fp->_flags |= __SEOF;
+else if (r < 0)
+fp->_flags |= __SERR;
FUNLOCKFILE(fp);
return ((total - resid) / size);
}
Index: stdio/stdio.c
===
RCS file: /cvs/src/lib/libc/stdio/stdio.c,v
retrieving revision 1.10
diff -u -p -r1.10 stdio.c
--- stdio/stdio.c   21 Sep 2016 04:38:56 -  1.10
+++ stdio/stdio.c   5 May 2018 19:14:27 -
@@ -32,6 +32,7 @@
  */
 
 #include 
+#include 
 #include 
 #include "local.h"
 
@@ -49,7 +50,7 @@ __sread(void *cookie, char *buf, int n)
/* if the read succeeded, update the current offset */
if (ret >= 0)
fp->_offset += ret;
-   else
+   else if (errno != EAGAIN)
fp->_flags &= ~__SOFF;  /* paranoia */
return (ret);
 }

unbuffered fread does not set error/EOF flags

[Vadim Zhukov]

Hi all!

Recently I was working on a program that uses stdio functions heavily.
While hunting for a bug, I've temporarily disabled buffering (via
setvbuf(f, NULL, _IONBF, 0)) and realized that program behavior was
changed heavily. The cause was that fread() stopped setting error flag
after hitting EAGAIN. It looks like its code is missing setting
EOF/error flags totally in case of unbuffered read, and first patch
(below) fixes this.

I'm not sure if "r" variable value should be propagated to fp->_r, or
not. I've assumed that since things work without it now, this doesn't
need a change. And headache stops me from further investigation for now,
sorry.

I understand that use stdio functions without buffering is somewhat
exotic, and that's for sure is the reason noone found this bug yet. But
this is still a bad thing, IMHO.

While looking through the libc code I've also found that __sread() drops
__SOFF flag unconditionally for all read errors. IMHO, the EAGAIN case
shouldn't be affected here, since, obviously current offset doesn't
change and is still well-known.

So... any comments, or ever okays? :)

--
WBR,
  Vadim Zhukov


Index: stdio/fread.c
===
RCS file: /cvs/src/lib/libc/stdio/fread.c,v
retrieving revision 1.15
diff -u -p -r1.15 fread.c
--- stdio/fread.c   21 Sep 2016 04:38:56 -  1.15
+++ stdio/fread.c   5 May 2018 19:02:04 -
@@ -79,6 +79,10 @@ fread(void *buf, size_t size, size_t cou
p += r;
resid -= r;
}
+if (r == 0)
+fp->_flags |= __SEOF;
+else if (r < 0)
+fp->_flags |= __SERR;
FUNLOCKFILE(fp);
return ((total - resid) / size);
}
Index: stdio/stdio.c
===
RCS file: /cvs/src/lib/libc/stdio/stdio.c,v
retrieving revision 1.10
diff -u -p -r1.10 stdio.c
--- stdio/stdio.c   21 Sep 2016 04:38:56 -  1.10
+++ stdio/stdio.c   5 May 2018 19:02:04 -
@@ -49,7 +49,7 @@ __sread(void *cookie, char *buf, int n)
/* if the read succeeded, update the current offset */
if (ret >= 0)
fp->_offset += ret;
-   else
+   else if (errno != EAGAIN)
fp->_flags &= ~__SOFF;  /* paranoia */
return (ret);
 }

Re: libkvm requires kvm_getargv before kvm_getenv when both needed

[Vadim Zhukov]

2018-05-03 18:59 GMT+03:00 Otto Moerbeek <o...@drijf.net>:
> Yes, looks good from reading. But all te extra checks before calling
> free can go. That's idiom from a *long* time ago.

Like that? I've checked all free() calls in libkvm.

I've also added zeroing of vmst field in mips64 code, like it's done
for other archs.

--
WBR,
  Vadim Zhukov


Index: kvm.c
===
RCS file: /cvs/src/lib/libkvm/kvm.c,v
retrieving revision 1.64
diff -u -p -r1.64 kvm.c
--- kvm.c   3 May 2018 15:47:41 -   1.64
+++ kvm.c   3 May 2018 16:08:37 -
@@ -649,27 +649,18 @@ kvm_close(kvm_t *kd)
if (kd->vmst)
_kvm_freevtop(kd);
kd->cpu_dsize = 0;
-   if (kd->cpu_data != NULL)
-   free((void *)kd->cpu_data);
-   if (kd->kcore_hdr != NULL)
-   free((void *)kd->kcore_hdr);
+   free(kd->cpu_data);
+   free(kd->kcore_hdr);
free(kd->filebase);
free(kd->procbase);
-   if (kd->swapspc != 0)
-   free((void *)kd->swapspc);
-   if (kd->argspc != 0)
-   free((void *)kd->argspc);
-   if (kd->argbuf != 0)
-   free((void *)kd->argbuf);
-   if (kd->argv != 0)
-   free((void *)kd->argv);
-   if (kd->envspc != 0)
-   free((void *)kd->envspc);
-   if (kd->envbuf != 0)
-   free((void *)kd->envbuf);
-   if (kd->envp != 0)
-   free((void *)kd->envp);
-   free((void *)kd);
+   free(kd->swapspc);
+   free(kd->argspc);
+   free(kd->argbuf);
+   free(kd->argv);
+   free(kd->envspc);
+   free(kd->envbuf);
+   free(kd->envp);
+   free(kd);
 
return (error);
 }
Index: kvm_mips64.c
===
RCS file: /cvs/src/lib/libkvm/kvm_mips64.c,v
retrieving revision 1.15
diff -u -p -r1.15 kvm_mips64.c
--- kvm_mips64.c9 Feb 2015 22:23:47 -   1.15
+++ kvm_mips64.c3 May 2018 16:08:37 -
@@ -71,8 +71,8 @@ struct vmstate {
 void
 _kvm_freevtop(kvm_t *kd)
 {
-   if (kd->vmst != 0)
-   free(kd->vmst);
+   free(kd->vmst);
+   kd->vmst = NULL;
 }
 
 int

Re: libkvm requires kvm_getargv before kvm_getenv when both needed

[Vadim Zhukov]

2018-05-02 16:54 GMT+03:00 Todd C. Miller <todd.mil...@sudo.ws>:
> On Tue, 01 May 2018 13:35:54 -0600, "Theo de Raadt" wrote:
>> >   b) Their working space should be independent of each other. This
>> >  isn't hard, just splitting kd->argbuf into kd->argbuf and
>> >  kd->envbuf. Seems a bit saner.
>>
>> I think (b) would be the better solution, this seems very fragile.
>>
>> Todd and Guenther -- what do you think?
>
> I'd prefer separate buffer spaces as well, the current situation is
> fragile as we've seen.

Here is patch for libkvm that fixes a few memory handling problems.
Most changes are mechanical, with some exceptions:

  1. Most notable: this splits argv buffer into argv-specific one
 and environ-specific one. This makes ps -eww totally happy.

  2. realloc() usage in kvm_argv() is now ENOMEM-prone. This is
 actually the same change as in one of the patches I've sent
 earlier, since that other patch heavily conflicts with this one.

  3. The "int off" changed to "ptrdiff_t off", as it should be.
 If I understand things correctly, we're lucky that this didn't
 strike us on 64-bit archs yet. The  is needed only
 for the ptrdiff_t.

If required, I'll split (1), (2) and (3) in separate commits; it's
that reviewing them in a single mail and a single context looks like
easier for me; please correct me if I'm wrong... okay, okay, I'm just
a lazy guy.

--
WBR,
  Vadim Zhukov


Index: kvm.c
===
RCS file: /cvs/src/lib/libkvm/kvm.c,v
retrieving revision 1.63
diff -u -p -r1.63 kvm.c
--- kvm.c   14 Dec 2017 17:06:33 -  1.63
+++ kvm.c   3 May 2018 10:42:51 -
@@ -191,6 +191,9 @@ _kvm_open(kvm_t *kd, const char *uf, con
kd->argspc = 0;
kd->argbuf = 0;
kd->argv = 0;
+   kd->envspc = 0;
+   kd->envbuf = 0;
+   kd->envp = 0;
kd->vmst = NULL;
kd->vm_page_buckets = 0;
kd->kcore_hdr = 0;
@@ -660,6 +663,12 @@ kvm_close(kvm_t *kd)
free((void *)kd->argbuf);
if (kd->argv != 0)
free((void *)kd->argv);
+   if (kd->envspc != 0)
+   free((void *)kd->envspc);
+   if (kd->envbuf != 0)
+   free((void *)kd->envbuf);
+   if (kd->envp != 0)
+   free((void *)kd->envp);
free((void *)kd);
 
return (error);
Index: kvm_private.h
===
RCS file: /cvs/src/lib/libkvm/kvm_private.h,v
retrieving revision 1.25
diff -u -p -r1.25 kvm_private.h
--- kvm_private.h   14 Dec 2017 17:06:33 -  1.25
+++ kvm_private.h   3 May 2018 10:42:51 -
@@ -53,10 +53,16 @@ struct __kvm {
struct kinfo_file *filebase;
int nbpg;   /* page size */
char*swapspc;   /* (dynamic) storage for swapped pages */
+
char*argspc, *argbuf; /* (dynamic) storage for argv strings */
int arglen; /* length of the above */
char**argv; /* (dynamic) storage for argv pointers */
int argc;   /* length of above (not actual # present) */
+
+   char*envspc, *envbuf; /* (dynamic) storage for environ strings */
+   int envlen; /* length of the above */
+   char**envp; /* (dynamic) storage for environ pointers */
+   int envc;   /* length of above (not actual # present) */
 
/*
 * Header structures for kernel dumps. Only gets filled in for
Index: kvm_proc.c
===
RCS file: /cvs/src/lib/libkvm/kvm_proc.c,v
retrieving revision 1.58
diff -u -p -r1.58 kvm_proc.c
--- kvm_proc.c  7 Nov 2016 00:26:33 -   1.58
+++ kvm_proc.c  3 May 2018 10:42:51 -
@@ -76,6 +76,7 @@
 #include 
 #include 
 #include 
+#include 
 #include 
 #include 
 #include 
@@ -100,9 +101,9 @@
 static char*_kvm_ureadm(kvm_t *, const struct kinfo_proc *, u_long, u_long 
*);
 static ssize_t kvm_ureadm(kvm_t *, const struct kinfo_proc *, u_long, char *, 
size_t);
 
-static char**kvm_argv(kvm_t *, const struct kinfo_proc *, u_long, int, 
int);
+static char**kvm_argv(kvm_t *, const struct kinfo_proc *, u_long, int, 
int, int);
 
-static char**kvm_doargv(kvm_t *, const struct kinfo_proc *, int,
+static char**kvm_doargv(kvm_t *, const struct kinfo_proc *, int, int,
void (*)(struct ps_strings *, u_long *, int *));
 static int proc_verify(kvm_t *, const struct kinfo_proc *);
 static voidps_str_a(struct ps_strings *, u_long *, int *);
@@ -257,11 +258,12 @@ _kvm_reallocarray(kvm_t *kd, void *p, si
  */
 static char **
 kvm_argv(kvm_t *kd, const struct kinfo_proc *p, u_long addr, int narg,
-int maxcnt)
+int maxcnt, int isen

Re: libkvm requires kvm_getargv before kvm_getenv when both needed

[Vadim Zhukov]

2018-05-01 21:53 GMT+03:00 Theo de Raadt <dera...@openbsd.org>:
> ktrace makes the problem more clear:
> 
>  25908 ps   CALL  
> sysctl(1.55.75675.1,0xed0cc78,0x7f7cd3d8,0,0)
>  25908 ps   RET   sysctl -1 errno 14 Bad address

And that's it, thanks!

Now little ps(1) is happy. But now there's a question, about
kvm_getargv() and kvm_getenv(): what behaviour do we want?

  a) They use same space, overwriting each other results (this is what
 happens now, and noone complains).

  b) Their working space should be independent of each other. This
 isn't hard, just splitting kd->argbuf into kd->argbuf and
 kd->envbuf. Seems a bit saner.

I'm fine with any direction. The patch below implements (a), since
it's less patching. Is it okay, or should it be (b)?

--
WBR,
  Vadim Zhukov


Index: kvm_proc.c
===
RCS file: /cvs/src/lib/libkvm/kvm_proc.c,v
retrieving revision 1.58
diff -u -p -r1.58 kvm_proc.c
--- kvm_proc.c  7 Nov 2016 00:26:33 -   1.58
+++ kvm_proc.c  1 May 2018 19:23:01 -
@@ -458,12 +458,14 @@ kvm_arg_sysctl(kvm_t *kd, pid_t pid, int
 {
size_t len, orglen;
int mib[4], ret;
-   char *buf;
+   void *buf;
 
orglen = env ? kd->nbpg : 8 * kd->nbpg; /* XXX - should be ARG_MAX */
-   if (kd->argbuf == NULL &&
-   (kd->argbuf = _kvm_malloc(kd, orglen)) == NULL)
-   return (NULL);
+
+   buf = _kvm_realloc(kd, kd->argbuf, orglen);
+   if (buf == NULL)
+   return NULL;
+   kd->argbuf = buf;
 
 again:
mib[0] = CTL_KERN;

wrong realloc in libkvm

[Vadim Zhukov]

Hi all.

The "p = realloc(p, size)" idiom is obviously wrong. Since both
places to be fixed were similar, I went further and unified the code
as well.

Note that "argv" is later initialized from kd->argv, so there is
no problem in reusing it here.

The amd64 is happy. Okay?

--
WBR,
  Vadim Zhukov


Index: kvm_proc.c
===
RCS file: /cvs/src/lib/libkvm/kvm_proc.c,v
retrieving revision 1.58
diff -u -p -r1.58 kvm_proc.c
--- kvm_proc.c  7 Nov 2016 00:26:33 -   1.58
+++ kvm_proc.c  1 May 2018 18:41:59 -
@@ -262,6 +262,7 @@ kvm_argv(kvm_t *kd, const struct kinfo_p
char *np, *cp, *ep, *ap, **argv;
u_long oaddr = -1;
int len, cc;
+   size_t argc;
 
/*
 * Check that there aren't an unreasonable number of arguments,
@@ -270,22 +271,19 @@ kvm_argv(kvm_t *kd, const struct kinfo_p
if (narg > ARG_MAX || addr < VM_MIN_ADDRESS || addr >= 
VM_MAXUSER_ADDRESS)
return (0);
 
-   if (kd->argv == 0) {
-   /*
-* Try to avoid reallocs.
-*/
-   kd->argc = MAX(narg + 1, 32);
-   kd->argv = _kvm_reallocarray(kd, NULL, kd->argc,
-   sizeof(*kd->argv));
-   if (kd->argv == 0)
-   return (0);
-   } else if (narg + 1 > kd->argc) {
-   kd->argc = MAX(2 * kd->argc, narg + 1);
-   kd->argv = (char **)_kvm_reallocarray(kd, kd->argv, kd->argc,
-   sizeof(*kd->argv));
-   if (kd->argv == 0)
-   return (0);
-   }
+   if (kd->argv == 0)
+   argc = MAX(narg + 1, 32);
+   else if (narg + 1 > kd->argc)
+   argc = MAX(2 * kd->argc, narg + 1);
+   else
+   goto argv_allocated;
+   argv = _kvm_reallocarray(kd, kd->argv, argc, sizeof(*kd->argv));
+   if (argv == 0)
+   return (0);
+   kd->argv = argv;
+   kd->argc = argc;
+
+argv_allocated:
if (kd->argspc == 0) {
kd->argspc = _kvm_malloc(kd, kd->nbpg);
if (kd->argspc == 0)

libkvm requires kvm_getargv before kvm_getenv when both needed

[Vadim Zhukov]

Hi all.

So I finally got bored of ps not displaying command args when "-e" is
present. Yes, ps(1) is broken: compare end of lines in output of "ps
-ww" and "ps -eww". And IIRC it behaves this way long enough, but I
always thought that it's me not missing something in ps(1) manual. Bad
zhuk@.

This is not a ps(1) bug, though: the simple diff below "fixes" it.
Yep, calling kvm_getargv(3) before kvm_getenv(3) makes everyone happy
again.

I've tried to dive into libkvm but went out of oxygen. The only
problem I found was misuse of reallocarray(), to be addressed in
another letter.

So, does any libkvm hacker have any clues where to look for this
argv-envp bug? I'm not sure that I'll find the root issue myself fast
enough (in less than half a year).

--
  WBR,
  Vadim Zhukov


Index: print.c
===
RCS file: /cvs/src/bin/ps/print.c,v
retrieving revision 1.69
diff -u -p -r1.69 print.c
--- print.c 8 Sep 2016 15:11:29 -   1.69
+++ print.c 1 May 2018 18:29:52 -
@@ -118,6 +118,7 @@ command(const struct kinfo_proc *kp, VAR
left = INT_MAX;

if (needenv && kd != NULL) {
+   argv = kvm_getargv(kd, kp, termwidth);
argv = kvm_getenvv(kd, kp, termwidth);
if ((p = argv) != NULL) {
while (*p) {

Stop telling patch runs ed

[Vadim Zhukov]

Hi all.

Lets stop lying that patch(1) runs ed(1). Okay? Or should we tell
explicitly mention that we don't do bad things?

--
WBR,
  Vadim Zhukov


Index: patch.1
===
RCS file: /cvs/src/usr.bin/patch/patch.1,v
retrieving revision 1.30
diff -u -p -r1.30 patch.1
--- patch.1 26 Jul 2015 14:32:19 -  1.30
+++ patch.1 11 Apr 2018 09:05:37 -
@@ -64,12 +64,6 @@ will attempt to determine the type of th
 or
 .Fl u
 option.
-Context diffs (old-style, new-style, and unified) and
-normal diffs are applied directly by the
-.Nm
-program itself, whereas ed diffs are simply fed to the
-.Xr ed 1
-editor via a pipe.
 .Pp
 If the
 .Ar patchfile
Wed Apr 11 12:05:38 MSK 2018: Updating sources ended: diff patch.1

Stop ping telling world its pid

[Vadim Zhukov]

Hi all!

While working on home job for students, I've come across two
questionable thingies in ping.c:

1. It sends process PID (well, last 16 bits) to the network.
   Maybe I'm a bit paranoid, but this looks like bad idea for me.
   I understand that this worked good when PIDs were 16-bit limited,
   because in that case you'll get 100% guarantee two different
   ping processes won't overlap. But nowadays we have larger PID
   space, so clashes are possible anyway. I propose to go straight
   with arc4random().

2. There is no point in calling ntohs/htons on the ident value.
   Or we should do this in IPv4 too, then: I'm not opposed, but
   at least consistency should be honored, IMHO.

Okay? Comments?

--
WBR,
  Vadim Zhukov

P.S.: I'm not fully back yet, sorry.


Index: ping.c
===
RCS file: /cvs/src/sbin/ping/ping.c,v
retrieving revision 1.224
diff -u -p -r1.224 ping.c
--- ping.c  8 Nov 2017 17:27:39 -   1.224
+++ ping.c  10 Apr 2018 20:03:50 -
@@ -586,7 +586,7 @@ main(int argc, char *argv[])
for (i = ECHOTMLEN; i < datalen; ++i)
*datap++ = i;
 
-   ident = getpid() & 0x;
+   ident = arc4random() & 0x;
 
/*
 * When trying to send large packets, you must increase the
@@ -1033,7 +1033,7 @@ pinger(int s)
icp6->icmp6_cksum = 0;
icp6->icmp6_type = ICMP6_ECHO_REQUEST;
icp6->icmp6_code = 0;
-   icp6->icmp6_id = htons(ident);
+   icp6->icmp6_id = ident;
icp6->icmp6_seq = seq;
} else {
icp = (struct icmp *)outpack;
@@ -1151,7 +1151,7 @@ pr_pack(u_char *buf, int cc, struct msgh
}
 
if (icp6->icmp6_type == ICMP6_ECHO_REPLY) {
-   if (ntohs(icp6->icmp6_id) != ident)
+   if (icp6->icmp6_id != ident)
return; /* 'Twas not our ECHO */
seq = icp6->icmp6_seq;
echo_reply = 1;

Add D-Link DWA-125/B2 to run(4)

[Vadim Zhukov]

Hi all.

The D-Link DWA-125/B2 works out-of-the box here on amd64. Okay?

(I remember to commit usbdevs.h and usbdevs_data.h separately)

--
WBR,
  Vadim Zhukov


Index: sys/dev/usb/usbdevs
===
RCS file: /cvs/src/sys/dev/usb/usbdevs,v
retrieving revision 1.680
diff -u -p -r1.680 usbdevs
--- sys/dev/usb/usbdevs 11 Jan 2018 09:26:36 -  1.680
+++ sys/dev/usb/usbdevs 29 Jan 2018 22:21:06 -
@@ -1549,6 +1549,7 @@ product DLINK RT3072  0x3c0a  RT3072
 product DLINK DWA140B3 0x3c15  DWA-140 rev B3
 product DLINK DWA160B2 0x3c1a  DWA-160 rev B2
 product DLINK DWA127   0x3c1b  DWA-127
+product DLINK DWA125B2 0x3c1e  DWA-125 rev B2
 product DLINK DWA162   0x3c1f  DWA-162 Wireless Adapter
 product DLINK DWA140D1 0x3c20  DWA-140 rev D1
 product DLINK DWA130F1 0x3c25  DWA-130 rev F1
Index: sys/dev/usb/usbdevs.h
===
RCS file: /cvs/src/sys/dev/usb/usbdevs.h,v
retrieving revision 1.692
diff -u -p -r1.692 usbdevs.h
--- sys/dev/usb/usbdevs.h   11 Jan 2018 09:27:20 -  1.692
+++ sys/dev/usb/usbdevs.h   29 Jan 2018 22:21:07 -
@@ -1556,6 +1556,7 @@
 #defineUSB_PRODUCT_DLINK_DWA140B3  0x3c15  /* DWA-140 rev 
B3 */
 #defineUSB_PRODUCT_DLINK_DWA160B2  0x3c1a  /* DWA-160 rev 
B2 */
 #defineUSB_PRODUCT_DLINK_DWA1270x3c1b  /* DWA-127 */
+#defineUSB_PRODUCT_DLINK_DWA125B2  0x3c1e  /* DWA-125 rev 
B2 */
 #defineUSB_PRODUCT_DLINK_DWA1620x3c1f  /* DWA-162 
Wireless Adapter */
 #defineUSB_PRODUCT_DLINK_DWA140D1  0x3c20  /* DWA-140 rev 
D1 */
 #defineUSB_PRODUCT_DLINK_DWA130F1  0x3c25  /* DWA-130 rev 
F1 */
Index: sys/dev/usb/usbdevs_data.h
===
RCS file: /cvs/src/sys/dev/usb/usbdevs_data.h,v
retrieving revision 1.686
diff -u -p -r1.686 usbdevs_data.h
--- sys/dev/usb/usbdevs_data.h  11 Jan 2018 09:27:20 -  1.686
+++ sys/dev/usb/usbdevs_data.h  29 Jan 2018 22:21:07 -
@@ -2602,6 +2602,10 @@ const struct usb_known_product usb_known
"DWA-127",
},
{
+   USB_VENDOR_DLINK, USB_PRODUCT_DLINK_DWA125B2,
+   "DWA-125 rev B2",
+   },
+   {
USB_VENDOR_DLINK, USB_PRODUCT_DLINK_DWA162,
"DWA-162 Wireless Adapter",
},
Index: sys/dev/usb/if_run.c
===
RCS file: /cvs/src/sys/dev/usb/if_run.c,v
retrieving revision 1.124
diff -u -p -r1.124 if_run.c
--- sys/dev/usb/if_run.c26 Oct 2017 15:00:28 -  1.124
+++ sys/dev/usb/if_run.c29 Jan 2018 22:21:08 -
@@ -152,6 +152,7 @@ static const struct usb_devno run_devs[]
USB_ID(COREGA,  RT2870_3),
USB_ID(COREGA,  RT3070),
USB_ID(CYBERTAN,RT2870),
+   USB_ID(DLINK,   DWA125B2),
USB_ID(DLINK,   DWA127),
USB_ID(DLINK,   DWA130F1),
USB_ID(DLINK,   DWA137A1),
Index: share/man/man4/run.4
===
RCS file: /cvs/src/share/man/man4/run.4,v
retrieving revision 1.50
diff -u -p -r1.50 run.4
--- share/man/man4/run.42 Aug 2017 18:28:02 -   1.50
+++ share/man/man4/run.429 Jan 2018 22:21:08 -
@@ -119,6 +119,7 @@ The following adapters should work:
 .It Corega CG-WLUSB2GNR
 .It Corega CG-WLUSB300AGN
 .It Corega CG-WLUSB300GNM
+.It D-Link DWA-125 rev B2
 .It D-Link DWA-127
 .It D-Link DWA-130 rev B1, F1
 .It D-Link DWA-140 rev B1, B2, B3, \

Re: ypldap patch 5: fix aldap_close

[Vadim Zhukov]

2017-12-17 6:35 GMT+03:00 Jonathan Matthew <jonat...@d14n.org>:
> On Sat, Dec 16, 2017 at 08:38:59PM +0300, Vadim Zhukov wrote:
>> 2017-12-06 19:12 GMT+03:00 Vadim Zhukov <persg...@gmail.com>:
>> >> The aldap_close() return value is never checked, and I do not see
>> >> any good reason to do that. Also, in case close(2) fails, it'll miss
>> >> freeing other data.
>> >
>> > I'm blind. :-\
>> >
>> > The problem I was looking for was right here: the aldap_close() closes
>> > the wrong file descriptor. So here is a better patch that solves
>> > actual leak. I ever treat this as a candidate for -STABLE, since
>> > when ypldap get stuck, you could be locked out of system.
>>
>> Sorry for noise, I'm just trying to make this patch go in. I think it
>> should because it fixes a real issue seen in the wild (if an isolated
>> AD-enabled LAN could be called "wild"). Well, actually it fixes two
>> issues, but I found zero code paths for making close() fail in current
>> code.
>>
>> The patched version happily runs for more than a week on (otherwise) 
>> 6.2-STABLE.
>
> Your diff is correct, but only for the non-tls case.  I missed cleaning
> up the tls context when I added tls support, and we need to keep the fd
> around so we can close it, since tls_close doesn't close file descriptors
> that libtls didn't open.
>
> ok?
>
> Index: aldap.c
> ===
> RCS file: /cvs/src/usr.sbin/ypldap/aldap.c,v
> retrieving revision 1.37
> diff -u -p -u -p -r1.37 aldap.c
> --- aldap.c 30 May 2017 09:33:31 -  1.37
> +++ aldap.c 17 Dec 2017 03:19:02 -
> @@ -70,10 +70,11 @@ aldap_application(struct ber_element *el
>  int
>  aldap_close(struct aldap *al)
>  {
> -   if (al->fd != -1)
> -   if (close(al->ber.fd) == -1)
> -   return (-1);
> -
> +   if (al->tls != NULL) {
> +   tls_close(al->tls);
> +   tls_free(al->tls);
> +   }
> +   close(al->fd);
> ber_free(>ber);
>     evbuffer_free(al->buf);
> free(al);
> @@ -120,7 +121,6 @@ aldap_tls(struct aldap *ldap, struct tls
> return (-1);
> }
>
> -   ldap->fd = -1;
> return (0);
>  }

Thank you for answering.

The diff reads correct to me, okay zhuk@.

--
  WBR,
  Vadim Zhukov

Re: ypldap patch 5: fix aldap_close

[Vadim Zhukov]

2017-12-06 19:12 GMT+03:00 Vadim Zhukov <persg...@gmail.com>:
>> The aldap_close() return value is never checked, and I do not see
>> any good reason to do that. Also, in case close(2) fails, it'll miss
>> freeing other data.
>
> I'm blind. :-\
>
> The problem I was looking for was right here: the aldap_close() closes
> the wrong file descriptor. So here is a better patch that solves
> actual leak. I ever treat this as a candidate for -STABLE, since
> when ypldap get stuck, you could be locked out of system.

Sorry for noise, I'm just trying to make this patch go in. I think it
should because it fixes a real issue seen in the wild (if an isolated
AD-enabled LAN could be called "wild"). Well, actually it fixes two
issues, but I found zero code paths for making close() fail in current
code.

The patched version happily runs for more than a week on (otherwise) 6.2-STABLE.

> Index: aldap.c
> ===
> RCS file: /cvs/src/usr.sbin/ypldap/aldap.c,v
> retrieving revision 1.37
> diff -u -p -r1.37 aldap.c
> --- aldap.c 30 May 2017 09:33:31 -  1.37
> +++ aldap.c 6 Dec 2017 13:11:59 -
> @@ -67,18 +67,14 @@ aldap_application(struct ber_element *el
> return BER_TYPE_OCTETSTRING;
>  }
>
> -int
> +void
>  aldap_close(struct aldap *al)
>  {
> if (al->fd != -1)
> -   if (close(al->ber.fd) == -1)
> -   return (-1);
> -
> +   close(al->fd);
> ber_free(>ber);
> evbuffer_free(al->buf);
> free(al);
> -
> -   return (0);
>  }
>
>  struct aldap *
> Index: aldap.h
> ===
> RCS file: /cvs/src/usr.sbin/ypldap/aldap.h,v
> retrieving revision 1.10
> diff -u -p -r1.10 aldap.h
> --- aldap.h 30 May 2017 09:33:31 -  1.10
> +++ aldap.h 6 Dec 2017 13:11:59 -
> @@ -206,7 +206,7 @@ enum subfilter {
>  struct aldap   *aldap_init(int);
>  int aldap_tls(struct aldap *, struct tls_config *,
> const char *);
> -int aldap_close(struct aldap *);
> +void    aldap_close(struct aldap *);
>  struct aldap_message   *aldap_parse(struct aldap *);
>  voidaldap_freemsg(struct aldap_message *);
>

--
  WBR,
  Vadim Zhukov

Re: ypldap patch 5: fix aldap_close

[Vadim Zhukov]

> The aldap_close() return value is never checked, and I do not see
> any good reason to do that. Also, in case close(2) fails, it'll miss
> freeing other data.

I'm blind. :-\

The problem I was looking for was right here: the aldap_close() closes
the wrong file descriptor. So here is a better patch that solves
actual leak. I ever treat this as a candidate for -STABLE, since
when ypldap get stuck, you could be locked out of system.

Okay?

--
WBR,
  Vadim Zhukov


Index: aldap.c
===
RCS file: /cvs/src/usr.sbin/ypldap/aldap.c,v
retrieving revision 1.37
diff -u -p -r1.37 aldap.c
--- aldap.c 30 May 2017 09:33:31 -  1.37
+++ aldap.c 6 Dec 2017 13:11:59 -
@@ -67,18 +67,14 @@ aldap_application(struct ber_element *el
return BER_TYPE_OCTETSTRING;
 }
 
-int
+void
 aldap_close(struct aldap *al)
 {
if (al->fd != -1)
-   if (close(al->ber.fd) == -1)
-   return (-1);
-
+   close(al->fd);
ber_free(>ber);
evbuffer_free(al->buf);
free(al);
-
-   return (0);
 }
 
 struct aldap *
Index: aldap.h
===
RCS file: /cvs/src/usr.sbin/ypldap/aldap.h,v
retrieving revision 1.10
diff -u -p -r1.10 aldap.h
--- aldap.h 30 May 2017 09:33:31 -  1.10
+++ aldap.h 6 Dec 2017 13:11:59 -
@@ -206,7 +206,7 @@ enum subfilter {
 struct aldap   *aldap_init(int);
 int aldap_tls(struct aldap *, struct tls_config *,
const char *);
-int aldap_close(struct aldap *);
+voidaldap_close(struct aldap *);
 struct aldap_message   *aldap_parse(struct aldap *);
 voidaldap_freemsg(struct aldap_message *);
 

ypldap patch 6: make client_addr_init() return void

[Vadim Zhukov]

The client_addr_init never returns anything but 0, and this value
isn't checked as well, so just make it void.

Okay?

--
WBR,
  Vadim Zhukov


Index: ldapclient.c
===
RCS file: /cvs/src/usr.sbin/ypldap/ldapclient.c,v
retrieving revision 1.39
diff -u -p -r1.39 ldapclient.c
--- ldapclient.c30 May 2017 09:33:31 -  1.39
+++ ldapclient.c6 Dec 2017 15:28:12 -
@@ -54,7 +54,7 @@ int   client_build_req(struct idm *, struc
 intclient_search_idm(struct env *, struct idm *, struct aldap *,
char **, char *, int, int, enum imsg_type);
 intclient_try_idm(struct env *, struct idm *);
-intclient_addr_init(struct idm *);
+void   client_addr_init(struct idm *);
 intclient_addr_free(struct idm *);
 
 struct aldap   *client_aldap_open(struct ypldap_addr_list *);
@@ -94,6 +94,6 @@ client_aldap_open(struct ypldap_addr_lis
 }
 
-int
+void
 client_addr_init(struct idm *idm)
 {
 struct sockaddr_in  *sa_in;
@@ -127,8 +127,6 @@ client_addr_init(struct idm *idm)
 /* not reached */
 }
 }
-
-return (0);
 }
 
 int

ypldap patch 5: fix aldap_close

[Vadim Zhukov]

The aldap_close() return value is never checked, and I do not see
any good reason to do that. Also, in case close(2) fails, it'll miss
freeing other data.

Okay?

--
WBR,
  Vadim Zhukov


Index: aldap.c
===
RCS file: /cvs/src/usr.sbin/ypldap/aldap.c,v
retrieving revision 1.37
diff -u -p -r1.37 aldap.c
--- aldap.c 30 May 2017 09:33:31 -  1.37
+++ aldap.c 6 Dec 2017 13:11:59 -
@@ -67,18 +67,14 @@ aldap_application(struct ber_element *el
return BER_TYPE_OCTETSTRING;
 }
 
-int
+void
 aldap_close(struct aldap *al)
 {
if (al->fd != -1)
-   if (close(al->ber.fd) == -1)
-   return (-1);
-
+   close(al->ber.fd);
ber_free(>ber);
evbuffer_free(al->buf);
free(al);
-
-   return (0);
 }
 
 struct aldap *
Index: aldap.h
===
RCS file: /cvs/src/usr.sbin/ypldap/aldap.h,v
retrieving revision 1.10
diff -u -p -r1.10 aldap.h
--- aldap.h 30 May 2017 09:33:31 -  1.10
+++ aldap.h 6 Dec 2017 13:11:59 -
@@ -206,7 +206,7 @@ enum subfilter {
 struct aldap   *aldap_init(int);
 int aldap_tls(struct aldap *, struct tls_config *,
const char *);
-int aldap_close(struct aldap *);
+voidaldap_close(struct aldap *);
 struct aldap_message   *aldap_parse(struct aldap *);
 voidaldap_freemsg(struct aldap_message *);
 

ypldap patch 4: fix fd leak when calling aldap_init

[Vadim Zhukov]

I've just found that under heavy load ypldap's LDAP engine leaks
file descriptors. I'll try to post issues I found one-by-one,
and here is the first leak I've found.

This fixes an fd leak happening if aldap_init() returns NULL. Okay?

--
WBR,
  Vadim Zhukov


Index: ldapclient.c
===
RCS file: /cvs/src/usr.sbin/ypldap/ldapclient.c,v
retrieving revision 1.39
diff -u -p -r1.39 ldapclient.c
--- ldapclient.c30 May 2017 09:33:31 -  1.39
+++ ldapclient.c6 Dec 2017 12:43:57 -
@@ -66,7 +66,8 @@ struct aldap *
 client_aldap_open(struct ypldap_addr_list *addr)
 {
int  fd = -1;
-   struct ypldap_addr   *p;
+   struct ypldap_addr  *p;
+   struct aldap*al;
 
TAILQ_FOREACH(p, addr, next) {
char hbuf[NI_MAXHOST], sbuf[NI_MAXSERV];
@@ -90,7 +91,10 @@ client_aldap_open(struct ypldap_addr_lis
if (fd == -1)
return NULL;
 
-   return aldap_init(fd);
+   al = aldap_init(fd);
+   if (al == NULL)
+   close(fd);
+   return al;
 }
 
 int

Re: ypldap patch 3: no reason for malloc?

[Vadim Zhukov]

2017-12-05 13:45 GMT+03:00 Vadim Zhukov <persg...@gmail.com>:
> The third nit in ypldap code, and the most questionable one.
>
> IMHO, there is no reason to dynamically allocate relatively small
> internal structures without buffers in it:

Please disregard this one, I'm an idiot that looks at and doesn't see
the RB_INSERT call.

--
  WBR,
  Vadim Zhukov

ypldap patch 3: no reason for malloc?

[Vadim Zhukov]

The third nit in ypldap code, and the most questionable one.

IMHO, there is no reason to dynamically allocate relatively small
internal structures without buffers in it:

struct userent {
RB_ENTRY(userent)ue_name_node;
RB_ENTRY(userent)ue_uid_node;
uid_tue_uid;
char*ue_line;
char*ue_netid_line;
gid_tue_gid;
};

struct groupent {
RB_ENTRY(groupent)   ge_name_node;
RB_ENTRY(groupent)   ge_gid_node;
gid_tge_gid;
char*ge_line;
};

Okay?

--
WBR,
  Vadim Zhukov


Index: ypldap.c
===
RCS file: /cvs/src/usr.sbin/ypldap/ypldap.c,v
retrieving revision 1.21
diff -u -p -r1.21 ypldap.c
--- ypldap.c20 Jan 2017 12:39:36 -  1.21
+++ ypldap.c5 Dec 2017 10:09:45 -
@@ -383,53 +383,49 @@ main_dispatch_client(int fd, short event
main_start_update(env);
break;
case IMSG_PW_ENTRY: {
-   struct userent  *ue;
+   struct userent   ue;
size_t   len;
 
if (env->update_trashed)
break;
 
(void)memcpy(, imsg.data, sizeof(ir));
-   if ((ue = calloc(1, sizeof(*ue))) == NULL ||
-   (ue->ue_line = strdup(ir.ir_line)) == NULL) {
+   if ((ue.ue_line = strdup(ir.ir_line)) == NULL) {
/*
 * should cancel tree update instead.
 */
fatal("out of memory");
}
-   ue->ue_uid = ir.ir_key.ik_uid;
-   len = strlen(ue->ue_line) + 1;
-   ue->ue_line[strcspn(ue->ue_line, ":")] = '\0';
+   ue.ue_uid = ir.ir_key.ik_uid;
+   len = strlen(ue.ue_line) + 1;
+   ue.ue_line[strcspn(ue.ue_line, ":")] = '\0';
if (RB_INSERT(user_name_tree, env->sc_user_names_t,
-   ue) != NULL) { /* dup */
-   free(ue->ue_line);
-   free(ue);
+   ) != NULL) { /* dup */
+   free(ue.ue_line);
} else
env->sc_user_line_len += len;
break;
}
case IMSG_GRP_ENTRY: {
-   struct groupent *ge;
+   struct groupent  ge;
size_t   len;
 
if (env->update_trashed)
break;
 
(void)memcpy(, imsg.data, sizeof(ir));
-   if ((ge = calloc(1, sizeof(*ge))) == NULL ||
-   (ge->ge_line = strdup(ir.ir_line)) == NULL) {
+   if ((ge.ge_line = strdup(ir.ir_line)) == NULL) {
/*
 * should cancel tree update instead.
 */
fatal("out of memory");
}
-   ge->ge_gid = ir.ir_key.ik_gid;
-   len = strlen(ge->ge_line) + 1;
-   ge->ge_line[strcspn(ge->ge_line, ":")] = '\0';
+   ge.ge_gid = ir.ir_key.ik_gid;
+   len = strlen(ge.ge_line) + 1;
+   ge.ge_line[strcspn(ge.ge_line, ":")] = '\0';
if (RB_INSERT(group_name_tree, env->sc_group_names_t,
-   ge) != NULL) { /* dup */
-   free(ge->ge_line);
-   free(ge);
+   ) != NULL) { /* dup */
+   free(ge.ge_line);
} else
env->sc_group_line_len += len;
break;

ypldap patch 2: small code dedup

[Vadim Zhukov]

The second nit in ypldap code.

This deduplicates code a bit. Okay?

--
WBR,
  Vadim Zhukov


Index: ypldap.c
===
RCS file: /cvs/src/usr.sbin/ypldap/ypldap.c,v
retrieving revision 1.21
diff -u -p -r1.21 ypldap.c
--- ypldap.c20 Jan 2017 12:39:36 -  1.21
+++ ypldap.c5 Dec 2017 10:09:45 -
@@ -239,17 +239,15 @@ main_create_user_groups(struct env *env)
if (!(cp = strsep(, ",")))
break;
ukey.ue_line = cp;
-   if ((ue = RB_FIND(user_name_tree, env->sc_user_names_t,
-   )) == NULL) {
+   ue = RB_FIND(user_name_tree, env->sc_user_names_t, 
);
+   if (bp != NULL)
+   *(bp-1) = ',';
+   if (ue == NULL) {
/* User not found */
log_warnx("main: unknown user %s in group %s\n",
ukey.ue_line, ge->ge_line);
-   if (bp != NULL)
-   *(bp-1) = ',';
continue;
}
-   if (bp != NULL)
-   *(bp-1) = ',';
 
/* Make sure the new group doesn't equal to the main 
gid */
if (ge->ge_gid == ue->ue_gid)

ypldap patch 1: RB_NFIND instead of RB_INSERT+RB_NEXT

[Vadim Zhukov]

Recently I had to start diffing deeper in ypldap. The main problem I'm
trying to solve is that _some_ LDAP attributes are case-insensetive,
including _some_ that could be used as user names. Until I get all
the picture myself, I won't post all the boring details I came with.

But while browsing the ypldap code, I've come across some nits, which
I want to share. Here is the first one.

Those are cases RB_NFIND is exactly for, aren't them? Okay?

--
WBR,
  Vadim Zhukov


Index: yp.c
===
RCS file: /cvs/src/usr.sbin/ypldap/yp.c,v
retrieving revision 1.18
diff -u -p -r1.18 yp.c
--- yp.c29 Nov 2016 17:15:27 -  1.18
+++ yp.c5 Dec 2017 10:09:45 -
@@ -490,25 +490,10 @@ ypproc_next_2_svc(ypreq_key *arg, struct
(void)strncpy(key, arg->key.keydat_val,
arg->key.keydat_len);
ukey.ue_line = key;
-   if ((ue = RB_FIND(user_name_tree, env->sc_user_names,
+   if ((ue = RB_NFIND(user_name_tree, env->sc_user_names,
)) == NULL) {
-   /*
-* canacar's trick:
-* the user might have been deleted in between calls
-* to next since the tree may be modified by a reload.
-* next should still return the next user in
-* lexicographical order, hence insert the search key
-* and look up the next field, then remove it again.
-*/
-   RB_INSERT(user_name_tree, env->sc_user_names, );
-   if ((ue = RB_NEXT(user_name_tree, >sc_user_names,
-   )) == NULL) {
-   RB_REMOVE(user_name_tree, env->sc_user_names,
-   );
-   res.stat = YP_NOKEY;
-   return ();
-   }
-   RB_REMOVE(user_name_tree, env->sc_user_names, );
+   res.stat = YP_NOKEY;
+   return ();
}
line = ue->ue_line + (strlen(ue->ue_line) + 1);
line = line + (strlen(line) + 1);
@@ -522,20 +507,10 @@ ypproc_next_2_svc(ypreq_key *arg, struct
arg->key.keydat_len);

gkey.ge_line = key;
-   if ((ge = RB_FIND(group_name_tree, env->sc_group_names,
+   if ((ge = RB_NFIND(group_name_tree, env->sc_group_names,
)) == NULL) {
-   /*
-* canacar's trick reloaded.
-*/
-   RB_INSERT(group_name_tree, env->sc_group_names, );
-   if ((ge = RB_NEXT(group_name_tree, >sc_group_names,
-   )) == NULL) {
-   RB_REMOVE(group_name_tree, env->sc_group_names,
-   );
-   res.stat = YP_NOKEY;
-   return ();
-   }
-   RB_REMOVE(group_name_tree, env->sc_group_names, );
+   res.stat = YP_NOKEY;
+   return ();
}
 
line = ge->ge_line + (strlen(ge->ge_line) + 1);

Re: [PATCH] Remove useless sys/sys/dkbad.h

[Vadim Zhukov]

2017-07-29 2:15 GMT+03:00 Андрей Болконский <andrey0bolkon...@gmail.com>:
> sorry. correct patch:
>
> 2017-07-28 23:52 GMT+03:00 Андрей Болконский <andrey0bolkon...@gmail.com>:
>
>> This header not used since retire sparc.
>> build amd64 is ok

Committed, thanks.

--
  WBR,
  Vadim Zhukov

Re: [PATCH] Remove useless sys/sys/dkbad.h

[Vadim Zhukov]

2017-07-29 2:15 GMT+03:00 Андрей Болконский <andrey0bolkon...@gmail.com>:
> sorry. correct patch:
>
> 2017-07-28 23:52 GMT+03:00 Андрей Болконский <andrey0bolkon...@gmail.com>:
>
>> This header not used since retire sparc.
>> build amd64 is ok

FWIW, I've tested this patch on amd64. It lacks removing dkbad.h from
src/distrib/sets/lists/comp/mi, but otherwise works fine here.

--
  WBR,
  Vadim Zhukov

Re: rc: Use here document for temporary pf rule set

[Vadim Zhukov]

2017-07-17 0:15 GMT+03:00 Klemens Nanni <k...@posteo.org>:
> On Sun, Jul 16, 2017 at 08:15:38PM +, Robert Peichaer wrote:
>> > +   ifconfig lo0 inet6 >/dev/null 2>&1 &&
>>
>> Please leave the if-then-fi construct intact. This short form is
>> mostly used for on-line commands (with only a few exceptions).
> OK.
>
>> > +   RULES="$RULES"'
>>
>> What is the reason to use double quotes and single quotes here?
>> Why not just use double quotes like this?
> Personal preference to make clear nothing inside the rules gets
> substituted. Using double quotes only will work just fine here.
>
>> This is not equivalent to the existing code.
>>
>> > +   pass in proto { tcp, udp } from any port { sunrpc, nfsd } to 
>> > any
>> > +   pass out proto { tcp, udp } from any to any port { sunrpc, 
>> > nfsd } !received-on any'
>> > +   print -- "$RULES" | pfctl -nf -
> Of course, fixed. Thanks!
>
>> Unless one of the pf people speaks up in favour of combining it,
>> I'd like to leave the two steps separated as you noted in your
>> original email too.
> Sure.
>
> This is hopefully the final version of my diff. After all it now only
> merges consecutive assignments of RULE into single ones.
>
> Feedback?
>
> Index: rc
> ===
> RCS file: /cvs/src/etc/rc,v
> retrieving revision 1.507
> diff -u -p -r1.507 rc
> --- rc  4 Jul 2017 19:02:11 -   1.507
> +++ rc  16 Jul 2017 21:10:48 -
> @@ -402,28 +399,35 @@ wsconsctl_conf
>
>  # Set initial temporary pf rule set.
>  if [[ $pf != NO ]]; then
> -   RULES="block all"
> -   RULES="$RULES\npass on lo0"
> -   RULES="$RULES\npass in proto tcp from any to any port ssh keep state"
> -   RULES="$RULES\npass out proto { tcp, udp } from any to any port 
> domain keep state"
> -   RULES="$RULES\npass out inet proto icmp all icmp-type echoreq keep 
> state"
> -   RULES="$RULES\npass out inet proto udp from any port bootpc to any 
> port bootps"
> -   RULES="$RULES\npass in inet proto udp from any port bootps to any 
> port bootpc"
> +   RULES='
> +   block all
> +   pass on lo0
> +   pass in proto tcp from any to any port ssh keep state
> +   pass out proto { tcp, udp } from any to any port domain keep state
> +   pass out inet proto icmp all icmp-type echoreq keep state
> +   pass out inet proto udp from any port bootpc to any port bootps
> +   pass in inet proto udp from any port bootps to any port bootpc'
> +
> if ifconfig lo0 inet6 >/dev/null 2>&1; then
> -   RULES="$RULES\npass out inet6 proto icmp6 all icmp6-type 
> neighbrsol"
> -   RULES="$RULES\npass in inet6 proto icmp6 all icmp6-type 
> neighbradv"
> -   RULES="$RULES\npass out inet6 proto icmp6 all icmp6-type 
> routersol"
> -   RULES="$RULES\npass in inet6 proto icmp6 all icmp6-type 
> routeradv"
> -   RULES="$RULES\npass out inet6 proto udp from any port 
> dhcpv6-client to any port dhcpv6-server"
> -   RULES="$RULES\npass in inet6 proto udp from any port 
> dhcpv6-server to any port dhcpv6-client"
> +   RULES="$RULES
> +   pass out inet6 proto icmp6 all icmp6-type neighbrsol
> +   pass in inet6 proto icmp6 all icmp6-type neighbradv
> +   pass out inet6 proto icmp6 all icmp6-type routersol
> +   pass in inet6 proto icmp6 all icmp6-type routeradv
> +   pass out inet6 proto udp from any port dhcpv6-client to any 
> port dhcpv6-server
> +   pass in inet6 proto udp from any port dhcpv6-server to any 
> port dhcpv6-client"
> fi
> -   RULES="$RULES\npass in proto carp keep state (no-sync)"
> -   RULES="$RULES\npass out proto carp !received-on any keep state 
> (no-sync)"
> +
> +   RULES="$RULES
> +   pass in proto carp keep state (no-sync)
> +   pass out proto carp !received-on any keep state (no-sync)"
> +
> +   # Don't kill NFS.
> if [[ $(sysctl vfs.mounts.nfs 2>/dev/null) == *[1-9]* ]]; then
> -   # Don't kill NFS.
> -   RULES="set reassemble yes no-df\n$RULES"
> -   RULES="$RULES\npass in proto { tcp, udp } from any port { 
> sunrpc, nfsd } to any"
> -   RULES="$RULES\npass out proto { tcp, udp } from any to any 
> port { sunrpc, nfsd } !received-on any"
> +   RULES="set reassemble yes no-df
> +   $RULES
> +   pass in proto { tcp, udp } from any port { sunrpc, nfsd } to 
> any
> +   pass out proto { tcp, udp } from any to any port { sunrpc, 
> nfsd } !received-on any"
> fi
> print -- "$RULES" | pfctl -f -
> pfctl -e

I like this, okay zhuk@.

--
  WBR,
  Vadim Zhukov

Re: rc: reorder_libs: [2/2] Pick archive versions more efficiently

[Vadim Zhukov]

2017-07-17 14:03 GMT+03:00 Klemens Nanni <k...@posteo.org>:
> On Mon, Jul 17, 2017 at 11:57:02AM +0300, Vadim Zhukov wrote:
>> > +   for _liba in /usr/lib/lib{c,crypto}; do
>> > +   _libas="$_libas $(ls $_liba.so.+([0-9.]).a | sort -V | 
>> > tail -1)"
>> > done
>> > +   _libas=${_libas# }
>> >
>> > # Remount read-write, if /usr/lib is on a read-only ffs filesystem.
>> > if [[ $_mp == *' type ffs '*'read-only'* ]]; then
>> >
>>
>> As a matter of microoptimization we could use "sort -rV | head -1"
>> instead of "sort -V | tail -1". I'm okay with current version of this
>> diff already, though.
> Sorting requires to read the entire input, otherwise you'd only sort a
> subset of the input. I don't see anything being optimized here.

head -1 returns earlier than tail -1. ;)

--
  WBR,
  Vadim Zhukov

Re: rc: reorder_libs: [2/2] Pick archive versions more efficiently

[Vadim Zhukov]

2017-07-16 14:55 GMT+03:00 Klemens Nanni :
> On Sun, Jul 16, 2017 at 10:26:25AM +, Robert Peichaer wrote:
>> But I'd like to stay strict matching the filenames.
>>
>> + for _liba in /usr/lib/lib{c,crypto}; do
>> + _libas="$_libas $(ls $_liba.so.+([0-9.]).a | sort -V | tail 
>> -1)"
>> + done
>>
>> > +   _libas=${_libas# }
> Agreed, I had a similiar approach first but then tried to reduce the
> differences to the essentials.
>
> Here's an updated diff taking this into while also dropping $_l together
> with this hunk instead of the other one.
>
> Feedback?
>
> Index: rc
> ===
> RCS file: /cvs/src/etc/rc,v
> retrieving revision 1.507
> diff -u -p -r1.507 rc
> --- rc  4 Jul 2017 19:02:11 -   1.507
> +++ rc  16 Jul 2017 11:54:36 -
> @@ -158,7 +158,7 @@ make_keys() {
>
>  # Re-link libraries, placing the objects in a random order.
>  reorder_libs() {
> -   local _dkdev _l _liba _libas _mp _tmpdir _remount=false _error=false
> +   local _dkdev _liba _libas _mp _tmpdir _remount=false _error=false
>
> [[ $library_aslr == NO ]] && return
>
> @@ -171,13 +171,10 @@ reorder_libs() {
> echo -n 'reordering libraries:'
>
> # Only choose the latest version of the libraries.
> -   for _liba in /usr/lib/libc.so.*.a /usr/lib/libcrypto.so.*.a; do
> -   _liba=$(ls ${_liba%%.[0-9]*}*.a | sort -V | tail -1)
> -   for _l in $_libas; do
> -   [[ $_l == $_liba ]] && continue 2
> -   done
> -   _libas="$_libas $_liba"
> +   for _liba in /usr/lib/lib{c,crypto}; do
> +   _libas="$_libas $(ls $_liba.so.+([0-9.]).a | sort -V | tail 
> -1)"
> done
> +   _libas=${_libas# }
>
> # Remount read-write, if /usr/lib is on a read-only ffs filesystem.
> if [[ $_mp == *' type ffs '*'read-only'* ]]; then
>

As a matter of microoptimization we could use "sort -rV | head -1"
instead of "sort -V | tail -1". I'm okay with current version of this
diff already, though.

Re: Make clang accept and use relative filenames for tools

[Vadim Zhukov]

2017-05-29 21:14 GMT+03:00 Mark Kettenis <mark.kette...@xs4all.nl>:
>> From: Vadim Zhukov <persg...@gmail.com>
>> Date: Mon, 29 May 2017 20:29:20 +0300
>>
>> 2017-05-29 20:26 GMT+03:00 Vadim Zhukov <persg...@gmail.com>:
>> > The clang and gcc behave differently regarding executing tools.
>> > While gcc simply runs what he said to, clang tries to be clever
>> > and always find absolute path for a tool, refusing start otherwise.
>> >
>> > The actual problem is starting a linker: ports infrastructure
>> > expects tools are called by name, not by path, and thus could be
>> > overriden via stuff in ${WRKDIR}/bin. This functionality is used,
>> > e.g., to implement USE_WXNEEDED port option.
>> >
>> > But clang calls "/usr/bin/ld", not "ld", and thus ${WRKDIR}/bin/ld
>> > misses a chance to do its magic, and binaries are built without
>> > OPENBSD_WXNEEDED, and some ports blow up (when compiled using clang).
>> >
>> > The patch below is enough to make clang accept and use non-absolute
>> > tool file names and make lang/mono and lang/libv8 happy (more likely
>> > to follow, there is much work to be done).
>> >
>> > So I'm asking compiler gurus and base maintainers, if a patch like
>> > that would be acceptable? And if yes, how do you want it look like
>> > to make supporting it easier?
>> >
>> > Thank you in advance.
>>
>> P.S.: The Program.inc file is not picked up as dependency, so you'll
>> need to touch gnu/llvm/lib/Support/Program.cpp manually before
>> building clang.
>
> I'm not in favour of this approach.  I'm pretty sure it'll break the
> cross-compilation toolchain.  And it makes us deviate from how clang
> behaves on other platforms quite a bit.

OK, understood, thank you.

--
  WBR,
  Vadim Zhukov

Re: Make clang accept and use relative filenames for tools

[Vadim Zhukov]

2017-05-29 20:54 GMT+03:00 Stuart Henderson <s...@spacehopper.org>:
> On 2017/05/29 20:26, Vadim Zhukov wrote:
>> The clang and gcc behave differently regarding executing tools.
>> While gcc simply runs what he said to, clang tries to be clever
>> and always find absolute path for a tool, refusing start otherwise.
>>
>> The actual problem is starting a linker: ports infrastructure
>> expects tools are called by name, not by path, and thus could be
>> overriden via stuff in ${WRKDIR}/bin. This functionality is used,
>> e.g., to implement USE_WXNEEDED port option.
>>
>> But clang calls "/usr/bin/ld", not "ld", and thus ${WRKDIR}/bin/ld
>> misses a chance to do its magic, and binaries are built without
>> OPENBSD_WXNEEDED, and some ports blow up (when compiled using clang).
>
> One thing we _could_ do is pass -fuse-ld=${WRKDIR}/bin/ld when linking ..

This requires: 1) linker not being called directly; 2) LDFLAGS being
passed. I'm not sure if all 50 (or so) USE_WXNEEDED ports behave like
this. The reason we have USE_WXNEEDED implemented the current way was
to catch such situations, IIUC

 Yes, we could tweak ${WRKDIR}/bin/{cc,cxx} to force always passing
-fuse-ld=${WRKDIR}/bin/ld. That'll require combining gcc4.port.mk and
clang.port.mk, though; otherwise they'll become unmaintaineable,
because they already have to respect USE_CCACHE. Maybe we'll have a
${PORTSDIR}/mk/compiler.mk that's called from modules.port.mk and
implements all the logic we want from different compilers... Until
that the dirty hacks in mono, libv8 and whatever else will stay.

--
  WBR,
  Vadim Zhukov

Re: Make clang accept and use relative filenames for tools

[Vadim Zhukov]

2017-05-29 20:26 GMT+03:00 Vadim Zhukov <persg...@gmail.com>:
> The clang and gcc behave differently regarding executing tools.
> While gcc simply runs what he said to, clang tries to be clever
> and always find absolute path for a tool, refusing start otherwise.
>
> The actual problem is starting a linker: ports infrastructure
> expects tools are called by name, not by path, and thus could be
> overriden via stuff in ${WRKDIR}/bin. This functionality is used,
> e.g., to implement USE_WXNEEDED port option.
>
> But clang calls "/usr/bin/ld", not "ld", and thus ${WRKDIR}/bin/ld
> misses a chance to do its magic, and binaries are built without
> OPENBSD_WXNEEDED, and some ports blow up (when compiled using clang).
>
> The patch below is enough to make clang accept and use non-absolute
> tool file names and make lang/mono and lang/libv8 happy (more likely
> to follow, there is much work to be done).
>
> So I'm asking compiler gurus and base maintainers, if a patch like
> that would be acceptable? And if yes, how do you want it look like
> to make supporting it easier?
>
> Thank you in advance.

P.S.: The Program.inc file is not picked up as dependency, so you'll
need to touch gnu/llvm/lib/Support/Program.cpp manually before
building clang.

--
  WBR,
  Vadim Zhukov

Make clang accept and use relative filenames for tools

[Vadim Zhukov]

The clang and gcc behave differently regarding executing tools.
While gcc simply runs what he said to, clang tries to be clever
and always find absolute path for a tool, refusing start otherwise.

The actual problem is starting a linker: ports infrastructure
expects tools are called by name, not by path, and thus could be
overriden via stuff in ${WRKDIR}/bin. This functionality is used,
e.g., to implement USE_WXNEEDED port option.

But clang calls "/usr/bin/ld", not "ld", and thus ${WRKDIR}/bin/ld
misses a chance to do its magic, and binaries are built without
OPENBSD_WXNEEDED, and some ports blow up (when compiled using clang).

The patch below is enough to make clang accept and use non-absolute
tool file names and make lang/mono and lang/libv8 happy (more likely
to follow, there is much work to be done).

So I'm asking compiler gurus and base maintainers, if a patch like
that would be acceptable? And if yes, how do you want it look like
to make supporting it easier?

Thank you in advance.

--
WBR,
  Vadim Zhukov


Index: tools/clang/lib/Driver/ToolChain.cpp
===
RCS file: /cvs/src/gnu/llvm/tools/clang/lib/Driver/ToolChain.cpp,v
retrieving revision 1.1.1.3
diff -u -p -r1.1.1.3 ToolChain.cpp
--- tools/clang/lib/Driver/ToolChain.cpp24 Jan 2017 08:33:12 -  
1.1.1.3
+++ tools/clang/lib/Driver/ToolChain.cpp29 May 2017 17:17:19 -
@@ -357,17 +357,17 @@ std::string ToolChain::GetLinkerPath() c
   if (llvm::sys::path::is_absolute(UseLinker)) {
 // If we're passed what looks like an absolute path, don't attempt to
 // second-guess that.
-if (llvm::sys::fs::exists(UseLinker))
+//FOOif (llvm::sys::fs::exists(UseLinker))
   return UseLinker;
   } else if (UseLinker.empty() || UseLinker == "ld") {
 // If we're passed -fuse-ld= with no argument, or with the argument ld,
 // then use whatever the default system linker is.
-return GetProgramPath(getDefaultLinker());
+return /*FOO GetProgramPath(*/ getDefaultLinker() /*)*/;
   } else {
 llvm::SmallString<8> LinkerName("ld.");
 LinkerName.append(UseLinker);
 
-std::string LinkerPath(GetProgramPath(LinkerName.c_str()));
+std::string LinkerPath(/*FOO GetProgramPath(*/ LinkerName.c_str() /*)*/ );
 if (llvm::sys::fs::exists(LinkerPath))
   return LinkerPath;
   }
@@ -375,7 +375,7 @@ std::string ToolChain::GetLinkerPath() c
   if (A)
 getDriver().Diag(diag::err_drv_invalid_linker_name) << 
A->getAsString(Args);
 
-  return GetProgramPath(getDefaultLinker());
+  return /*FOO GetProgramPath(*/ getDefaultLinker() /*)*/;
 }
 
 types::ID ToolChain::LookupTypeForExtension(StringRef Ext) const {
Index: lib/Support/Unix/Program.inc
===
RCS file: /cvs/src/gnu/llvm/lib/Support/Unix/Program.inc,v
retrieving revision 1.1.1.1
diff -u -p -r1.1.1.1 Program.inc
--- lib/Support/Unix/Program.inc3 Sep 2016 22:47:02 -   1.1.1.1
+++ lib/Support/Unix/Program.inc29 May 2017 17:17:19 -
@@ -181,12 +181,14 @@ static void SetMemoryLimits (unsigned si
 static bool Execute(ProcessInfo , StringRef Program, const char **args,
 const char **envp, const StringRef **redirects,
 unsigned memoryLimit, std::string *ErrMsg) {
+/* let the OS decides if the executable exists
   if (!llvm::sys::fs::exists(Program)) {
 if (ErrMsg)
   *ErrMsg = std::string("Executable \"") + Program.str() +
 std::string("\" doesn't exist!");
 return false;
   }
+*/
 
   // If this OS has posix_spawn and there is no memory limit being implied, use
   // posix_spawn.  It is more efficient than fork/exec.
@@ -240,7 +242,7 @@ static bool Execute(ProcessInfo , Str
 // Explicitly initialized to prevent what appears to be a valgrind false
 // positive.
 pid_t PID = 0;
-int Err = posix_spawn(, Program.str().c_str(), FileActions,
+int Err = posix_spawnp(, Program.str().c_str(), FileActions,
   /*attrp*/nullptr, const_cast(args),
   const_cast(envp));
 

KSH bug: case inside command substitution

[Vadim Zhukov]

It looks like I've just found a bug in (our) ksh. Not to be brave
enough to fix it right now, but I think it's still worths adding
regression test.

Bash and zsh pass this test without problem.

The idea is using the case...in...esac inside $(...) or `...`.
It starts failing when you add a single case match, i.e.:

a=foo
data=$(
case $a in
esac
)
echo $data

doesn't fail, while 

a=foo
data=$(
case $a in
*) echo OK;;
esac
)
echo $data

fails with error:

$ ksh tt.sh
./tt.sh[4]: syntax error: `;;' unexpected

What's worse, it fails even if I comment the line it's whining after.
So... okay to add a regression test?

--
WBR,
  Vadim Zhukov


Index: caseincmdsubst.t
===
RCS file: caseincmdsubst.t
diff -N caseincmdsubst.t
--- /dev/null   1 Jan 1970 00:00:00 -
+++ caseincmdsubst.t29 May 2017 02:03:02 -
@@ -0,0 +1,26 @@
+name: case-inside-cmd-subst-parentheses
+description:
+   See if case...in...esac works inside parentesed command substitution
+stdin:
+   a=foo
+   data=$(
+   case $a in
+   *) echo OK;;
+   esac
+   )
+expected-stdout:
+   OK
+---
+name: case-inside-cmd-subst-backticks
+description:
+   See if case...in...esac works inside backticks command substitution
+stdin:
+   a=foo
+   data=`
+   case $a in
+   *) echo OK;;
+   esac
+   `
+expected-stdout:
+   OK
+---

Re: switch decltype to variadic macros

[Vadim Zhukov]

2017-05-28 18:32 GMT+03:00 Mark Kettenis <mark.kette...@xs4all.nl>:
>> From: "Ted Unangst" <t...@tedunangst.com>
>> Date: Sun, 28 May 2017 11:16:21 -0400
>>
>> Vadim Zhukov wrote:
>> > While working on getting Boost & its friends work under Clang,
>> > I've stumbled upon the code that looks like the following:
>> >
>> >decltype(x, y) z = w;
>>
>> why not espie's fix from earlier?
>
> Well, I asked him to report this upstream and see what they say.
>
> I don't pretend to understand any of this, but I'd be seriously
> surprised if libc++ wouldn't work with something mainstream as boost.
> So my suspicion is that something else is wrong...

My suspicion is that almost nobody really cares on compiling boost
without c++11 support anymore. :-\
--
  WBR,
  Vadim Zhukov

Re: switch decltype to variadic macros

[Vadim Zhukov]

2017-05-28 18:35 GMT+03:00 Vadim Zhukov <persg...@gmail.com>:
> 2017-05-28 18:16 GMT+03:00 Ted Unangst <t...@tedunangst.com>:
>> Vadim Zhukov wrote:
>>> While working on getting Boost & its friends work under Clang,
>>> I've stumbled upon the code that looks like the following:
>>>
>>>decltype(x, y) z = w;
>>
>> why not espie's fix from earlier?
>
> Because I've missed it. :( I have no idea why espie@ did it this way, 
> though...
>
> And what's more interesting we shouldn't get into this code when
> compiling with Clang. This should happen only for old GCCs. See the
> program:
>
> #define _LIBCPP_HAS_NO_DECLTYPE
> #include 
> int main() {
> int x;
> double y;
> decltype(x, y) z = y;
> }
>
> If you comment out the first line, it compiles fine under Clang. So
> basically the problem only raises when the program being compiled
> plays dirty games with _GNU_VER or something like that...

Ah, now I've got it: it's compiling without -std=c++11 which triggers error.
I.e., when run:

clang++ x.cpp -o x

you'll get error with current __config, and won't with patched. And
this is how we compile boost - without c++11. And it's likely will be
a pain and ever likely break stuff, if we'll switch boost to c++11
mode, since we'll have to switch all software depending on boost,
either directly or indirectly (that's a really big number!), to eg++
on non-clang archs...

>> but clang++ has variadic macros, like C99... the following patch allows
>> macro decltype()  to work a lot more like builtin __decltype in pre-C++11
>>
>> (note that still works with -std=c++98 -pedantic, much to my surprise, but
>> which is cool)
>>
>> Index: __config
>> ===
>> RCS file: /cvs/src/lib/libcxx/include/__config,v
>> retrieving revision 1.3
>> diff -u -p -r1.3 __config
>> --- __config19 Sep 2016 22:17:22 -  1.3
>> +++ __config5 May 2017 15:21:58 -
>> @@ -670,7 +670,7 @@ template  struct __static_asse
>>  #ifdef _LIBCPP_HAS_NO_DECLTYPE
>>  // GCC 4.6 provides __decltype in all standard modes.
>>  #if !__is_identifier(__decltype) || _GNUC_VER >= 406
>> -#  define decltype(__x) __decltype(__x)
>> +#  define decltype(__x, ...) __decltype(__x, ##__VA_ARGS__)
>>  #else
>>  #  define decltype(__x) __typeof__(__x)
>>  #endif
>>
>
> --
>   WBR,
>   Vadim Zhukov

--
  WBR,
  Vadim Zhukov

Re: switch decltype to variadic macros

[Vadim Zhukov]

2017-05-28 18:16 GMT+03:00 Ted Unangst <t...@tedunangst.com>:
> Vadim Zhukov wrote:
>> While working on getting Boost & its friends work under Clang,
>> I've stumbled upon the code that looks like the following:
>>
>>decltype(x, y) z = w;
>
> why not espie's fix from earlier?

Because I've missed it. :( I have no idea why espie@ did it this way, though...

And what's more interesting we shouldn't get into this code when
compiling with Clang. This should happen only for old GCCs. See the
program:

#define _LIBCPP_HAS_NO_DECLTYPE
#include 
int main() {
int x;
double y;
decltype(x, y) z = y;
}

If you comment out the first line, it compiles fine under Clang. So
basically the problem only raises when the program being compiled
plays dirty games with _GNU_VER or something like that...

> but clang++ has variadic macros, like C99... the following patch allows
> macro decltype()  to work a lot more like builtin __decltype in pre-C++11
>
> (note that still works with -std=c++98 -pedantic, much to my surprise, but
> which is cool)
>
> Index: __config
> ===
> RCS file: /cvs/src/lib/libcxx/include/__config,v
> retrieving revision 1.3
> diff -u -p -r1.3 __config
> --- __config19 Sep 2016 22:17:22 -  1.3
> +++ __config5 May 2017 15:21:58 -
> @@ -670,7 +670,7 @@ template  struct __static_asse
>  #ifdef _LIBCPP_HAS_NO_DECLTYPE
>  // GCC 4.6 provides __decltype in all standard modes.
>  #if !__is_identifier(__decltype) || _GNUC_VER >= 406
> -#  define decltype(__x) __decltype(__x)
> +#  define decltype(__x, ...) __decltype(__x, ##__VA_ARGS__)
>  #else
>  #  define decltype(__x) __typeof__(__x)
>  #endif
>

--
  WBR,
  Vadim Zhukov

switch decltype to variadic macros

[Vadim Zhukov]

While working on getting Boost & its friends work under Clang,
I've stumbled upon the code that looks like the following:

   decltype(x, y) z = w;

The "x, y" is a perfectly valid expression, and should work.
But it's not, because decltype is actually defined as a macros
in libcxx:

#if !__is_identifier(__decltype) || _GNUC_VER >= 406
#  define decltype(__x) __decltype(__x)
#else
#  define decltype(__x) __typeof__(__x)
#endif

This is easily being fixed by variadic macros, which are supported
by GCC 3.0+ and all Clang versions. So I'm proposing the following
change, which likely worths getting upstream as well.

--
WBR,
  Vadim Zhukov


Index: __config
===
RCS file: /cvs/src/lib/libcxx/include/__config,v
retrieving revision 1.3
diff -u -p -r1.3 __config
--- __config19 Sep 2016 22:17:22 -  1.3
+++ __config28 May 2017 14:57:43 -
@@ -669,10 +669,11 @@ template  struct __static_asse
 
 #ifdef _LIBCPP_HAS_NO_DECLTYPE
 // GCC 4.6 provides __decltype in all standard modes.
+// variadic macros are required since decltype(x, y) is valid
 #if !__is_identifier(__decltype) || _GNUC_VER >= 406
-#  define decltype(__x) __decltype(__x)
+#  define decltype(...) __decltype(__VA_ARGS__)
 #else
-#  define decltype(__x) __typeof__(__x)
+#  define decltype(...) __typeof__(__VA_ARGS__)
 #endif
 #endif
 

Re: iwm scan abort

[Vadim Zhukov]

2017-05-28 13:14 GMT+03:00 Stefan Sperling <s...@stsp.name>:
> If net80211 is not in SCAN state anymore at the point in time where
> the device tells us it is done scanning, do nothing instead of
> proceeding to select an AP anyway and eventually sending some
> commands that will just confuse the firmware.
>
> This might silence some error messages people have seen with iwm(4).
>
> Briefly tested on an 8260 device.

Seems correct to me.

--
  WBR,
  Vadim Zhukov

Re: remove waf from port-modules(5)

[Vadim Zhukov]

2017-05-27 22:45 GMT+03:00 Joerg Jung :
> Hi,
>
> I think devel/waf is gone since two years and may not come back, so no
> need to mention in port-modules(5).
>
> OK?

Yes, and don't forget to bring some gas for nice burning!

Shrink and give example for sets question

[Vadim Zhukov]

Let's make it more informative and smaller size at the same time?

Index: install.sub
===
RCS file: /cvs/src/distrib/miniroot/install.sub,v
retrieving revision 1.958
diff -u -p -r1.958 install.sub
--- install.sub 24 Jan 2017 10:14:14 -  1.958
+++ install.sub 26 May 2017 15:49:08 -
@@ -1268,8 +1268,7 @@ select_sets() {
cat <<__EOT
 
 Select sets by entering a set name, a file name pattern or 'all'. De-select
-sets by prepending a '-' to the set name, file name pattern or 'all'. Selected
-sets are labelled '[X]'.
+sets by prepending a '-', e.g.: '-x*'. Selected sets are labelled '[X]'.
 __EOT
while :; do
for _f in $_avail; do

Re: ftp(1): anonymous login and .netrc

[Vadim Zhukov]

2017-01-04 11:51 GMT+03:00 Anton Lindqvist <anton.lindqv...@gmail.com>:
> I'm running a script as root which drops privileges while fetching files
> using ftp(1) with anonymous login enabled:
>
> # doas -u unpriv ftp -a URL
>
> ... and was surprised to see the following error:
>
> ftp: /root/.netrc: Permission denied
>
> I'm not sure if the .netrc file should be considered when anonymous
> login is enabled. If not, here's a patch:
>
> Index: util.c
> ===
> RCS file: /cvs/src/usr.bin/ftp/util.c,v
> retrieving revision 1.81
> diff -u -p -r1.81 util.c
> --- util.c  20 Aug 2016 20:18:42 -  1.81
> +++ util.c  3 Jan 2017 20:19:16 -
> @@ -221,7 +221,7 @@ ftp_login(const char *host, char *user,
> struct passwd *pw;
>
>  #ifndef SMALL
> -   if (user == NULL) {
> +   if (user == NULL && !anonftp) {
> if (ruserpass(host, , , ) < 0) {
> code = -1;
>     return (0);
>

The diff looks like correct to me, okay@ for anyone willing to commit.

--
  WBR,
  Vadim Zhukov

Re: PATCH: Tell what trailing asterisk in sa output means

[Vadim Zhukov]

2016-12-20 20:30 GMT+03:00 Theo de Raadt <dera...@openbsd.org>:
>> I had to dig into sa sources to get know what trailing '*' in
>> command names mean. IMHO, that's not how manuals should work. :)
>>
>> Okay?
>
> That seems a little awkward mentioning fork directly.  How about
>
> Children which have not yet called
> .Xr execve 2
> will have
> .Sq *
> appended to their command names.

Seems fine for me as well.

--
  WBR,
  Vadim Zhukov

PATCH: Tell what trailing asterisk in sa output means

[Vadim Zhukov]

Hi all.

I had to dig into sa sources to get know what trailing '*' in
command names mean. IMHO, that's not how manuals should work. :)

Okay?

--
WBR,
  Vadim Zhukov


Index: sa.8
===
RCS file: /cvs/src/usr.sbin/sa/sa.8,v
retrieving revision 1.19
diff -u -p -r1.19 sa.8
--- sa.816 Jul 2013 11:13:34 -  1.19
+++ sa.820 Dec 2016 12:26:51 -
@@ -205,6 +205,14 @@ flag is specified, only the
 and
 .Fl s
 flags are honored.
+.Pp
+Processes created with
+.Xr fork 2
+without
+.Xr exec 3
+will have
+.Sq *
+appended to their command names.
 .Sh FILES
 .Bl -tag -width /var/account/usracct -compact
 .It Pa /var/account/acct

Re: Better for mount(2)

[Vadim Zhukov]

Here is an improved version, after a lot of feedback from jmc@.

Quick list of changes:

  * the unmount description moved closer to the top, since we
mention it in flags description anyway;
  * list of mount options is now sorted;
  * the description of the MNT_RELOAD flag was added;
  * the descriptions of the MNT_FORCE and MNT_UPDATE flags were reworked
and moved to separate paragraphs.

Okay to commit?
--
WBR,
  Vadim Zhukov


Index: mount.2
===
RCS file: /cvs/src/lib/libc/sys/mount.2,v
retrieving revision 1.46
diff -u -p -r1.46 mount.2
--- mount.2 27 May 2016 19:45:04 -  1.46
+++ mount.2 17 Jul 2016 22:13:06 -
@@ -70,31 +70,41 @@ at the time
 of a successful mount are swept under the carpet, so to speak, and
 are unavailable until the filesystem is unmounted.
 .Pp
+The
+.Fn unmount
+function disassociates the filesystem from the specified
+mount point
+.Fa dir .
+.Pp
 The following
 .Fa flags
-may be specified to
-suppress default semantics which affect filesystem access.
+may be specified to change default behaviour:
 .Bl -tag -width MNT_SYNCHRONOUS
-.It Dv MNT_RDONLY
-The filesystem should be treated as read-only:
-even the superuser may not write to it.
+.It Dv MNT_ASYNC
+All I/O to the filesystem should be done asynchronously.
 .It Dv MNT_NOATIME
 Do not update the access time on files in the filesystem unless
 the modification or status change times are also being updated.
+.It Dv MNT_NODEV
+Do not interpret special files on the filesystem.
 .It Dv MNT_NOEXEC
 Do not allow files to be executed from the filesystem.
 .It Dv MNT_NOSUID
 Do not honor setuid or setgid bits on files when executing them.
-.It Dv MNT_NODEV
-Do not interpret special files on the filesystem.
-.It Dv MNT_SYNCHRONOUS
-All I/O to the filesystem should be done synchronously.
-.It Dv MNT_ASYNC
-All I/O to the filesystem should be done asynchronously.
+.It Dv MNT_RDONLY
+The filesystem should be treated as read-only:
+even the superuser may not write to it.
+.It Dv MNT_RELOAD
+Reload a MNT_RDONLY filesystem.
+Used, for example, by
+.Xr fsck 8
+after modification of on-disk structures.
 .It Dv MNT_SOFTDEP
 Use soft dependencies.
 Applies to FFS filesystems only (see 'softdep' in
 .Xr mount 8 ) .
+.It Dv MNT_SYNCHRONOUS
+All I/O to the filesystem should be done synchronously.
 .It MNT_WXALLOWED
 Processes that ask for memory to be made writeable plus executable
 using the
@@ -108,10 +118,13 @@ The option is typically used on the
 filesystem.
 .El
 .Pp
-The flag
+Two more flags affect the behaviour of system calls themselves,
+and not the behaviour of the corresponding filesystem:
+.Pp
+The
 .Dv MNT_UPDATE
-indicates that the mount command is being applied
-to an already mounted filesystem.
+flag indicates that the mount command is being applied to an already
+mounted filesystem.
 This allows the mount flags to be changed without requiring
 that the filesystem be unmounted and remounted.
 Some filesystems may not allow all flags to be changed.
@@ -119,6 +132,18 @@ For example,
 most filesystems will not allow a change from read-write to read-only.
 .Pp
 The
+.Dv MNT_FORCE
+flag, if used together with
+.Dv MNT_UPDATE ,
+allows transition from read-write to read-only,
+even if there are files open for writing.
+And when used on unmount, it specifies that the filesystem
+should be forcibly unmounted even if files are still active;
+active special devices continue to work,
+but any further access to any other active files result in errors,
+even if the filesystem is later remounted.
+.Pp
+The
 .Fa type
 argument defines the type of the filesystem.
 The types of filesystems known to the system are defined in
@@ -237,22 +262,6 @@ struct udf_args {
char*fspec; /* block special device to mount */
 };
 .Ed
-.Pp
-The
-.Fn unmount
-function call disassociates the filesystem from the specified
-mount point
-.Fa dir .
-.Pp
-The
-.Fa flags
-argument may specify
-.Dv MNT_FORCE
-to specify that the filesystem should be forcibly unmounted even if files are
-still active.
-Active special devices continue to work,
-but any further accesses to any other active files result in errors
-even if the filesystem is later remounted.
 .Sh RETURN VALUES
 .Rv -std
 .Sh ERRORS

Better for mount(2)

[Vadim Zhukov]

Hello all.

As found by some of my students, mount(2) lacks description of some
flags used by userland. After trying to sync things out, I realized
that this page is not well organized at all. For example, unmount
syscall is mentioned closer to end, after description of flags that
it could use.

I didn't document all of the flags involved in mount calls, since
some of them are purely kernel internal. But I could easily miss
something crucial...

So this is my first public draft of mount(2) patch. Looking for
comments & thanks in advance!

--
WBR,
  Vadim Zhukov


Index: mount.2
===
RCS file: /cvs/src/lib/libc/sys/mount.2,v
retrieving revision 1.46
diff -u -p -r1.46 mount.2
--- mount.2 27 May 2016 19:45:04 -  1.46
+++ mount.2 9 Jul 2016 15:16:29 -
@@ -70,31 +70,57 @@ at the time
 of a successful mount are swept under the carpet, so to speak, and
 are unavailable until the filesystem is unmounted.
 .Pp
+The
+.Fn unmount
+function call disassociates the filesystem from the specified
+mount point
+.Fa dir .
+.Pp
 The following
 .Fa flags
 may be specified to
 suppress default semantics which affect filesystem access.
 .Bl -tag -width MNT_SYNCHRONOUS
-.It Dv MNT_RDONLY
-The filesystem should be treated as read-only:
-even the superuser may not write to it.
+.It Dv MNT_ASYNC
+All I/O to the filesystem should be done asynchronously.
+.It Dv MNT_FORCE
+On unmount, specifies that the filesystem should be forcibly unmounted
+even if files are still active.
+Active special devices continue to work,
+but any further accesses to any other active files result in errors
+even if the filesystem is later remounted.
+On update, allows to forcibly change from read-write to read-only,
+even if there are files open for writing.
 .It Dv MNT_NOATIME
 Do not update the access time on files in the filesystem unless
 the modification or status change times are also being updated.
+.It Dv MNT_NODEV
+Do not interpret special files on the filesystem.
 .It Dv MNT_NOEXEC
 Do not allow files to be executed from the filesystem.
 .It Dv MNT_NOSUID
 Do not honor setuid or setgid bits on files when executing them.
-.It Dv MNT_NODEV
-Do not interpret special files on the filesystem.
-.It Dv MNT_SYNCHRONOUS
-All I/O to the filesystem should be done synchronously.
-.It Dv MNT_ASYNC
-All I/O to the filesystem should be done asynchronously.
+.It Dv MNT_RDONLY
+The filesystem should be treated as read-only:
+even the superuser may not write to it.
+.It Dv MNT_RELOAD
+Reload already mounted filesystem.
+Used, e.g., by
+.Xr fsck 8 .
 .It Dv MNT_SOFTDEP
 Use soft dependencies.
 Applies to FFS filesystems only (see 'softdep' in
 .Xr mount 8 ) .
+.It Dv MNT_SYNCHRONOUS
+All I/O to the filesystem should be done synchronously.
+.It Dv MNT_UPDATE
+Indicates that the mount command is being applied to an already mounted
+filesystem.
+This allows the mount flags to be changed without requiring
+that the filesystem be unmounted and remounted.
+Some filesystems may not allow all flags to be changed.
+For example,
+most filesystems will not allow a change from read-write to read-only.
 .It MNT_WXALLOWED
 Processes that ask for memory to be made writeable plus executable
 using the
@@ -108,16 +134,6 @@ The option is typically used on the
 filesystem.
 .El
 .Pp
-The flag
-.Dv MNT_UPDATE
-indicates that the mount command is being applied
-to an already mounted filesystem.
-This allows the mount flags to be changed without requiring
-that the filesystem be unmounted and remounted.
-Some filesystems may not allow all flags to be changed.
-For example,
-most filesystems will not allow a change from read-write to read-only.
-.Pp
 The
 .Fa type
 argument defines the type of the filesystem.
@@ -237,22 +253,6 @@ struct udf_args {
char*fspec; /* block special device to mount */
 };
 .Ed
-.Pp
-The
-.Fn unmount
-function call disassociates the filesystem from the specified
-mount point
-.Fa dir .
-.Pp
-The
-.Fa flags
-argument may specify
-.Dv MNT_FORCE
-to specify that the filesystem should be forcibly unmounted even if files are
-still active.
-Active special devices continue to work,
-but any further accesses to any other active files result in errors
-even if the filesystem is later remounted.
 .Sh RETURN VALUES
 .Rv -std
 .Sh ERRORS

Re: remove cluster_write() remains

[Vadim Zhukov]

2016-05-21 23:49 GMT+03:00 Martin Natano :
> VOP_ALLOCBLKS() and related code is unused since the removal of
> cluster_write(). This diff even shrinks /bsd by 8kb. Ok?

Unless someone really wants to reuse this code, it obviously serves no
purpose. Nice finding! :)

okay@ zhuk

Make fstat exit status like grep has

[Vadim Zhukov]

This makes fstat(1) behave more like grep(1), regarding exit status.
The idea itself came from bluhm@.

For manpage, I mostly copied the text from grep(1), but changed
phrases talking about selected lines to matching processes. Or
should those be selected processes?..

--
WBR,
  Vadim Zhukov


Index: fstat.1
===
RCS file: /cvs/src/usr.bin/fstat/fstat.1,v
retrieving revision 1.51
diff -u -p -r1.51 fstat.1
--- fstat.1 31 Jan 2015 19:33:45 -  1.51
+++ fstat.1 4 May 2016 01:43:05 -
@@ -278,6 +278,19 @@ Each
 .Xr systrace 4
 device is printed with only the kernel address of the
 device private data.
+.Sh EXIT STATUS
+The
+.Nm
+utility exits with one of the following values:
+.Pp
+.Bl -tag -width Ds -offset indent -compact
+.It Li 0
+One or more matching processes were found.
+.It Li 1
+No processes matching given criteria were found.
+.It Li \*(Gt1
+An error occurred.
+.El
 .Sh SEE ALSO
 .Xr netstat 1 ,
 .Xr nfsstat 1 ,
Index: fstat.c
===
RCS file: /cvs/src/usr.bin/fstat/fstat.c,v
retrieving revision 1.86
diff -u -p -r1.86 fstat.c
--- fstat.c 2 Jan 2016 13:22:52 -   1.86
+++ fstat.c 4 May 2016 01:43:05 -
@@ -273,7 +273,7 @@ main(int argc, char *argv[])
}
 
if ((kf = kvm_getfiles(kd, what, arg, sizeof(*kf), )) == NULL)
-   errx(1, "%s", kvm_geterr(kd));
+   errx((errno == ESRCH) ? 1 : 2, "%s", kvm_geterr(kd));
 
if (fuser) {
/*

Remove lib/libc/regex/WHATSNEW

[Vadim Zhukov]

Is there any point in keeping lib/libc/regex/WHATSNEW? Okay to rm?

--
WBR,
  Vadim Zhukov


Index: WHATSNEW
===
RCS file: WHATSNEW
diff -N WHATSNEW
--- WHATSNEW28 Apr 1997 20:44:56 -  1.3
+++ /dev/null   1 Jan 1970 00:00:00 -
@@ -1,95 +0,0 @@
-#  $OpenBSD: WHATSNEW,v 1.3 1997/04/28 20:44:56 millert Exp $
-# @(#)WHATSNEW 8.3 (Berkeley) 3/18/94
-
-New in alpha3.4:  The complex bug alluded to below has been fixed (in a
-slightly kludgey temporary way that may hurt efficiency a bit; this is
-another "get it out the door for 4.4" release).  The tests at the end of
-the tests file have accordingly been uncommented.  The primary sign of
-the bug was that something like a?b matching ab matched b rather than ab.
-(The bug was essentially specific to this exact situation, else it would
-have shown up earlier.)
-
-New in alpha3.3:  The definition of word boundaries has been altered
-slightly, to more closely match the usual programming notion that "_"
-is an alphabetic.  Stuff used for pre-ANSI systems is now in a subdir,
-and the makefile no longer alludes to it in mysterious ways.  The
-makefile has generally been cleaned up some.  Fixes have been made
-(again!) so that the regression test will run without -DREDEBUG, at
-the cost of weaker checking.  A workaround for a bug in some folks'
- has been added.  And some more things have been added to
-tests, including a couple right at the end which are commented out
-because the code currently flunks them (complex bug; fix coming).
-Plus the usual minor cleanup.
-
-New in alpha3.2:  Assorted bits of cleanup and portability improvement
-(the development base is now a BSDI system using GCC instead of an ancient
-Sun system, and the newer compiler exposed some glitches).  Fix for a
-serious bug that affected REs using many [] (including REG_ICASE REs
-because of the way they are implemented), *sometimes*, depending on
-memory-allocation patterns.  The header-file prototypes no longer name
-the parameters, avoiding possible name conflicts.  The possibility that
-some clot has defined CHAR_MIN as (say) `-128' instead of `(-128)' is
-now handled gracefully.  "uchar" is no longer used as an internal type
-name (too many people have the same idea).  Still the same old lousy
-performance, alas.
-
-New in alpha3.1:  Basically nothing, this release is just a bookkeeping
-convenience.  Stay tuned.
-
-New in alpha3.0:  Performance is no better, alas, but some fixes have been
-made and some functionality has been added.  (This is basically the "get
-it out the door in time for 4.4" release.)  One bug fix:  regfree() didn't
-free the main internal structure (how embarrassing).  It is now possible
-to put NULs in either the RE or the target string, using (resp.) a new
-REG_PEND flag and the old REG_STARTEND flag.  The REG_NOSPEC flag to
-regcomp() makes all characters ordinary, so you can match a literal
-string easily (this will become more useful when performance improves!).
-There are now primitives to match beginnings and ends of words, although
-the syntax is disgusting and so is the implementation.  The REG_ATOI
-debugging interface has changed a bit.  And there has been considerable
-internal cleanup of various kinds.
-
-New in alpha2.3:  Split change list out of README, and moved flags notes
-into Makefile.  Macro-ized the name of regex(7) in regex(3), since it has
-to change for 4.4BSD.  Cleanup work in engine.c, and some new regression
-tests to catch tricky cases thereof.
-
-New in alpha2.2:  Out-of-date manpages updated.  Regerror() acquires two
-small extensions -- REG_ITOA and REG_ATOI -- which avoid debugging kludges
-in my own test program and might be useful to others for similar purposes.
-The regression test will now compile (and run) without REDEBUG.  The
-BRE \$ bug is fixed.  Most uses of "uchar" are gone; it's all chars now.
-Char/uchar parameters are now written int/unsigned, to avoid possible
-portability problems with unpromoted parameters.  Some unsigned casts have
-been introduced to minimize portability problems with shifting into sign
-bits.
-
-New in alpha2.1:  Lots of little stuff, cleanup and fixes.  The one big
-thing is that regex.h is now generated, using mkh, rather than being
-supplied in the distribution; due to circularities in dependencies,
-you have to build regex.h explicitly by "make h".  The two known bugs
-have been fixed (and the regression test now checks for them), as has a
-problem with assertions not being suppressed in the absence of REDEBUG.
-No performance work yet.
-
-New in alpha2:  Backslash-anything is an ordinary character, not an
-error (except, of course, for the handful of backslashed metacharacters
-in BREs), which should reduce script breakage.  The regression test
-checks *where* null strings are supposed to match, and has generally
-been tightened up somewhat.  Small bug fixe

Re: Return ESRCH instead of zero result for KERN_FILE sysctl

[Vadim Zhukov]

So, after initial input, here is improved version. Changelist:

  - Simplified handling of "matched" variable in sysctl_file(),
from kettenis@.

  - ERRORS section for kvm_getfiles.2 (to be committed separately),
requested by bluhm@. I also moved beginning of RETURN VALUES
section a bit up, IMHO, that makes more sense; but if any
our documentation maintainer will object, I'll revert, of course.

Okay to commit this version?

--
WBR,
  Vadim Zhukov


Index: sys/kern/kern_sysctl.c
===
RCS file: /cvs/src/sys/kern/kern_sysctl.c,v
retrieving revision 1.300
diff -u -p -r1.300 kern_sysctl.c
--- sys/kern/kern_sysctl.c  29 Feb 2016 19:44:07 -  1.300
+++ sys/kern/kern_sysctl.c  3 May 2016 19:40:37 -
@@ -1225,7 +1225,7 @@ sysctl_file(int *name, u_int namelen, ch
struct process *pr;
size_t buflen, elem_size, elem_count, outsize;
char *dp = where;
-   int arg, i, error = 0, needed = 0;
+   int arg, i, error = 0, needed = 0, matched;
u_int op;
int show_pointers;
 
@@ -1325,6 +1325,7 @@ sysctl_file(int *name, u_int namelen, ch
error = EINVAL;
break;
}
+   matched = 0;
LIST_FOREACH(pr, , ps_list) {
/*
 * skip system, exiting, embryonic and undead
@@ -1336,6 +1337,7 @@ sysctl_file(int *name, u_int namelen, ch
/* not the pid we are looking for */
continue;
}
+   matched = 1;
fdp = pr->ps_fd;
if (pr->ps_textvp)
FILLIT(NULL, NULL, KERN_FILE_TEXT, 
pr->ps_textvp, pr);
@@ -1353,6 +1355,8 @@ sysctl_file(int *name, u_int namelen, ch
FILLIT(fp, fdp, i, NULL, pr);
}
}
+   if (!matched)
+   error = ESRCH;
break;
case KERN_FILE_BYUID:
LIST_FOREACH(pr, , ps_list) {
Index: lib/libkvm/kvm_file2.c
===
RCS file: /cvs/src/lib/libkvm/kvm_file2.c,v
retrieving revision 1.47
diff -u -p -r1.47 kvm_file2.c
--- lib/libkvm/kvm_file2.c  4 Sep 2015 02:58:14 -   1.47
+++ lib/libkvm/kvm_file2.c  3 May 2016 19:40:38 -
@@ -149,7 +149,7 @@ kvm_getfiles(kvm_t *kd, int op, int arg,
/* find size and alloc buffer */
rv = sysctl(mib, 6, NULL, , NULL, 0);
if (rv == -1) {
-   if (kd->vmfd != -1)
+   if (errno != ESRCH && kd->vmfd != -1)
goto deadway;
_kvm_syserr(kd, kd->program, "kvm_getfiles");
return (NULL);
@@ -266,7 +266,7 @@ kvm_deadfile_byid(kvm_t *kd, int op, int
 {
size_t buflen;
struct nlist nl[4], *np;
-   int n = 0;
+   int n = 0, matched = 0;
char *where;
struct kinfo_file kf;
struct file *fp, file;
@@ -312,6 +312,9 @@ kvm_deadfile_byid(kvm_t *kd, int op, int
kd->filebase = (void *)where;
buflen = (nfiles + 10) * esize;
 
+   if (op != KERN_FILE_BYPID || arg <= 0)
+   matched = 1;
+
for (pr = LIST_FIRST();
pr != NULL;
pr = LIST_NEXT(, ps_list)) {
@@ -333,10 +336,12 @@ kvm_deadfile_byid(kvm_t *kd, int op, int
goto cleanup;
}
 
-   if (op == KERN_FILE_BYPID && arg > 0 &&
-   proc.p_pid != (pid_t)arg) {
-   /* not the pid we are looking for */
+   if (op == KERN_FILE_BYPID) {
+   /* check if this is the pid we are looking for */
+   if (arg > 0 && proc.p_pid != (pid_t)arg)
continue;
+   else
+   matched = 1;
}
 
if (KREAD(kd, (u_long)process.ps_ucred, )) {
@@ -457,6 +462,10 @@ kvm_deadfile_byid(kvm_t *kd, int op, int
buflen -= esize;
n++;
}
+   }
+   if (!matched) {
+   errno = ESRCH;
+   goto cleanup;
}
 done:
*cnt = n;
Index: lib/libkvm/kvm_getfiles.3
===
RCS file: /cvs/src/lib/libkvm/kvm_getfiles.3,v
retrieving revision 1.17
diff -u -p -r1.17 kvm_getfiles.3
--- lib/libkvm/kvm_getfiles.3   11 Feb 2015 03:03:08 -  1.17
+++ lib/libkvm/kvm_getfiles.3   3 May 2016 19:40:38 -
@@ -107,7 +107,7 @@ the

Re: Return ESRCH instead of zero result for KERN_FILE sysctl

[Vadim Zhukov]

2016-05-03 1:19 GMT+03:00 Mark Kettenis <mark.kette...@xs4all.nl>:
>> Date: Tue, 3 May 2016 00:04:13 +0200
>> From: Alexander Bluhm <alexander.bl...@gmx.net>
>>
>> On Sun, May 01, 2016 at 01:24:19AM +0300, Vadim Zhukov wrote:
>> > When matching by PID, we'd better return ESRCH expicitly rather
>> > than zero entries. This makes, for example, fstat(1) behaviour
>> > more consistent and makes it easier to replace "if lsof -p ..."
>> > checks in third-party code with "if fstat -p ...".
>> >
>> > For libkvm, I explicitly test for ESRCH to avoid going inside
>> > kmem without real need.
>> >
>> > Tested on amd64.
>> >
>> > Comments? Okays?
>>
>> Your use case makes sense.  So I think this should go in.
>
> The code change is abit convoluted though though.  It's enough to
> initialize matched to 0 before the LIST_FOREACH and set it
> unconditionally to after the
>
>   if (arg > 0 && pr->ps_pid != (pid_t)arg)
>
> check.  So:
>
> matched = 0
> LIST_FOREACH(pr, , ps_list) {
> /*
>  * skip system, exiting, embryonic and undead
>  * processes
>  */
> if (pr->ps_flags & (PS_SYSTEM | PS_EMBRYO | 
> PS_EXITING))
> continue;
> if (arg > 0 && pr->ps_pid != (pid_t)arg) {
> /* not the pid we are looking for */
> continue;
> }
> matched = 1;

I think you're right. I've modelled the kernel code after libkvm, and
your suggestion makes it much easier to read, thanks!

I'll send updated diff a bit later, have to close lid right now. Thanks!

>> > Index: sys/kern/kern_sysctl.c
>> > ===
>> > RCS file: /cvs/src/sys/kern/kern_sysctl.c,v
>> > retrieving revision 1.300
>> > diff -u -p -r1.300 kern_sysctl.c
>> > --- sys/kern/kern_sysctl.c  29 Feb 2016 19:44:07 -  1.300
>> > +++ sys/kern/kern_sysctl.c  30 Apr 2016 22:26:06 -
>> > @@ -1225,7 +1225,7 @@ sysctl_file(int *name, u_int namelen, ch
>> > struct process *pr;
>> > size_t buflen, elem_size, elem_count, outsize;
>> > char *dp = where;
>> > -   int arg, i, error = 0, needed = 0;
>> > +   int arg, i, error = 0, needed = 0, matched;
>> > u_int op;
>> > int show_pointers;
>> >
>> > @@ -1325,6 +1325,7 @@ sysctl_file(int *name, u_int namelen, ch
>> > error = EINVAL;
>> > break;
>> > }
>> > +   matched = (arg <= 0);
>> > LIST_FOREACH(pr, , ps_list) {
>> > /*
>> >  * skip system, exiting, embryonic and undead
>> > @@ -1332,9 +1333,12 @@ sysctl_file(int *name, u_int namelen, ch
>> >  */
>> > if (pr->ps_flags & (PS_SYSTEM | PS_EMBRYO | 
>> > PS_EXITING))
>> > continue;
>> > -   if (arg > 0 && pr->ps_pid != (pid_t)arg) {
>> > -   /* not the pid we are looking for */
>> > -   continue;
>> > +   if (arg > 0) {
>> > +   /* is this the pid we are looking for? */
>> > +   if (pr->ps_pid == (pid_t)arg)
>> > +   matched = 1;
>> > +   else
>> > +   continue;
>> > }
>> > fdp = pr->ps_fd;
>> > if (pr->ps_textvp)
>> > @@ -1353,6 +1357,8 @@ sysctl_file(int *name, u_int namelen, ch
>> > FILLIT(fp, fdp, i, NULL, pr);
>> > }
>> > }
>> > +   if (!matched)
>> > +   error = ESRCH;
>> > break;
>> > case KERN_FILE_BYUID:
>> > LIST_FOREACH(pr, , ps_list) {
>> > Index: lib/libkvm/kvm_file2.c
>> > ===
>> > RCS file: /cvs/src/lib/libkvm/kvm_file2.c,v
>> > retrieving revision 1.47
>> > diff -u -p -r1.4

Re: Return ESRCH instead of zero result for KERN_FILE sysctl

[Vadim Zhukov]

2016-05-03 1:04 GMT+03:00 Alexander Bluhm <alexander.bl...@gmx.net>:
> On Sun, May 01, 2016 at 01:24:19AM +0300, Vadim Zhukov wrote:
>> When matching by PID, we'd better return ESRCH expicitly rather
>> than zero entries. This makes, for example, fstat(1) behaviour
>> more consistent and makes it easier to replace "if lsof -p ..."
>> checks in third-party code with "if fstat -p ...".
>>
>> For libkvm, I explicitly test for ESRCH to avoid going inside
>> kmem without real need.
>>
>> Tested on amd64.
>>
>> Comments? Okays?
>
> Your use case makes sense.  So I think this should go in.
>
> Can we have the same logic for KERN_FILE_BYPID and KERN_FILE_BYUID?

I don't have strong opinion here. On one side, while PID matching is a
single match by definition, UID matching is not. On the other side, if
you're looking for particular UID, you do know something about
already... If you want it so, then let it be so. I'll send adjusted
patch a bit later, having to close laptop ASAP.

> man kvm_getfiles says (-1 for all processes), but code does arg > 0.
> I think it should be arg => 0 like in KERN_FILE_BYUID.
>
> The (arg < -1) EINVAL check is missing in KERN_FILE_BYUID.  Does
> anyone know why?

I was stumbled by those differences as well, but preferred to keep the
current logic; it could be fixed separately anyway.

> errno ESRCH should be documented in kvm_getfiles(3).

At the moment, there is not errors section in kvm_getfiles(3) at all.
I'm not sure what else should go there: EINVAL?

> exit code 1 should be documented in fstat(1).  Should it behave
> like grep?
>0   One or more lines were selected.
>1   No lines were selected.
>>1  An error occurred.

I think that's good idea.

Thank you for review!

Re: midiplay: Fix out-of-bounds memory access

[Vadim Zhukov]

2016-04-30 7:38 GMT+03:00 Jonathan Gray <j...@jsg.id.au>:
> On Wed, Apr 27, 2016 at 07:49:50PM -0700, Geoff Hill wrote:
>> Fix possible reads past the end of the buffer.
>>
>> Found by random fuzz testing (zzuf). Without the fix the fuzzer crashes
>> in several seconds; with the patch, the fuzzer runs clean for hours.
>
> Any reason to not replace the somewhat arbitary earlier test
> for this?
>
> Index: midiplay.c
> ===
> RCS file: /cvs/src/usr.bin/midiplay/midiplay.c,v
> retrieving revision 1.17
> diff -u -p -U9 -r1.17 midiplay.c
> --- midiplay.c  8 Feb 2015 23:40:34 -   1.17
> +++ midiplay.c  30 Apr 2016 04:35:31 -
> @@ -306,19 +306,19 @@ playdata(u_char *buf, u_int tot, char *n
> tracks = calloc(ntrks, sizeof(struct track));
> if (tracks == NULL)
> err(1, "malloc() tracks failed");
> for (t = 0; t < ntrks; ) {
> if (p >= end - MARK_LEN - SIZE_LEN) {
> warnx("Cannot find track %d", t);
> goto ret;
> }
> len = GET32(p + MARK_LEN);
> -   if (len > 100) { /* a safe guard */
> +   if (p + MARK_LEN + SIZE_LEN + len > end) {

It's better to avoid "+" in checks to avoid overflow, no? Something like:

if (len >=  end - (p + MARK_LEN + SIZE_LEN)) {

> warnx("Crazy track length");
> goto ret;
> }
> if (memcmp(p, MARK_TRACK, MARK_LEN) == 0) {
> tracks[t].start = p + MARK_LEN + SIZE_LEN;
>     tracks[t].end = tracks[t].start + len;
> tracks[t].curtime = getvar([t]);
> t++;
> }
>

--
  WBR,
  Vadim Zhukov

Return ESRCH instead of zero result for KERN_FILE sysctl

[Vadim Zhukov]

When matching by PID, we'd better return ESRCH expicitly rather
than zero entries. This makes, for example, fstat(1) behaviour
more consistent and makes it easier to replace "if lsof -p ..."
checks in third-party code with "if fstat -p ...".

For libkvm, I explicitly test for ESRCH to avoid going inside
kmem without real need.

Tested on amd64.

Comments? Okays? 

--
WBR,
  Vadim Zhukov


Index: sys/kern/kern_sysctl.c
===
RCS file: /cvs/src/sys/kern/kern_sysctl.c,v
retrieving revision 1.300
diff -u -p -r1.300 kern_sysctl.c
--- sys/kern/kern_sysctl.c  29 Feb 2016 19:44:07 -  1.300
+++ sys/kern/kern_sysctl.c  30 Apr 2016 22:26:06 -
@@ -1225,7 +1225,7 @@ sysctl_file(int *name, u_int namelen, ch
struct process *pr;
size_t buflen, elem_size, elem_count, outsize;
char *dp = where;
-   int arg, i, error = 0, needed = 0;
+   int arg, i, error = 0, needed = 0, matched;
u_int op;
int show_pointers;
 
@@ -1325,6 +1325,7 @@ sysctl_file(int *name, u_int namelen, ch
error = EINVAL;
break;
}
+   matched = (arg <= 0);
LIST_FOREACH(pr, , ps_list) {
/*
 * skip system, exiting, embryonic and undead
@@ -1332,9 +1333,12 @@ sysctl_file(int *name, u_int namelen, ch
 */
if (pr->ps_flags & (PS_SYSTEM | PS_EMBRYO | PS_EXITING))
continue;
-   if (arg > 0 && pr->ps_pid != (pid_t)arg) {
-   /* not the pid we are looking for */
-   continue;
+   if (arg > 0) {
+   /* is this the pid we are looking for? */
+   if (pr->ps_pid == (pid_t)arg)
+   matched = 1;
+   else
+   continue;
}
fdp = pr->ps_fd;
if (pr->ps_textvp)
@@ -1353,6 +1357,8 @@ sysctl_file(int *name, u_int namelen, ch
FILLIT(fp, fdp, i, NULL, pr);
}
}
+   if (!matched)
+   error = ESRCH;
break;
case KERN_FILE_BYUID:
LIST_FOREACH(pr, , ps_list) {
Index: lib/libkvm/kvm_file2.c
===
RCS file: /cvs/src/lib/libkvm/kvm_file2.c,v
retrieving revision 1.47
diff -u -p -r1.47 kvm_file2.c
--- lib/libkvm/kvm_file2.c  4 Sep 2015 02:58:14 -   1.47
+++ lib/libkvm/kvm_file2.c  30 Apr 2016 22:26:06 -
@@ -149,7 +149,7 @@ kvm_getfiles(kvm_t *kd, int op, int arg,
/* find size and alloc buffer */
rv = sysctl(mib, 6, NULL, , NULL, 0);
if (rv == -1) {
-   if (kd->vmfd != -1)
+   if (errno != ESRCH && kd->vmfd != -1)
goto deadway;
_kvm_syserr(kd, kd->program, "kvm_getfiles");
return (NULL);
@@ -266,7 +266,7 @@ kvm_deadfile_byid(kvm_t *kd, int op, int
 {
size_t buflen;
struct nlist nl[4], *np;
-   int n = 0;
+   int n = 0, matched = 0;
char *where;
struct kinfo_file kf;
struct file *fp, file;
@@ -312,6 +312,9 @@ kvm_deadfile_byid(kvm_t *kd, int op, int
kd->filebase = (void *)where;
buflen = (nfiles + 10) * esize;
 
+   if (op != KERN_FILE_BYPID || arg <= 0)
+   matched = 1;
+
for (pr = LIST_FIRST();
pr != NULL;
pr = LIST_NEXT(, ps_list)) {
@@ -333,10 +336,12 @@ kvm_deadfile_byid(kvm_t *kd, int op, int
goto cleanup;
}
 
-   if (op == KERN_FILE_BYPID && arg > 0 &&
-   proc.p_pid != (pid_t)arg) {
-   /* not the pid we are looking for */
+   if (op == KERN_FILE_BYPID) {
+   /* check if this is the pid we are looking for */
+   if (arg > 0 && proc.p_pid != (pid_t)arg)
continue;
+   else
+   matched = 1;
}
 
if (KREAD(kd, (u_long)process.ps_ucred, )) {
@@ -457,6 +462,10 @@ kvm_deadfile_byid(kvm_t *kd, int op, int
buflen -= esize;
n++;
}
+   }
+   if (!matched) {
+   errno = ESRCH;
+   goto cleanup;
}
 done:
*cnt = n;

Re: anti-ROP mechanism in libc

[Vadim Zhukov]

26 Apr. 2016 19:58 "Theo de Raadt" <dera...@cvs.openbsd.org> wrote:
>
> Here is a new version that does a more comprehensive test of the new
> libc.so before installing it, and uses install -S
>
> Index: etc/rc
> ===
> RCS file: /cvs/src/etc/rc,v
> retrieving revision 1.474
> diff -u -p -u -r1.474 rc
> --- etc/rc  29 Dec 2015 19:41:24 -  1.474
> +++ etc/rc  26 Apr 2016 11:56:46 -
> @@ -158,6 +158,35 @@ make_keys() {
> ssh-keygen -A
>  }
>
> +rebuildlibs() {
> +   local _l _liba _libas _tmpdir
> +
> +   # Only choose newest
> +   for _liba in /usr/lib/libc.so.*.a; do
> +   _liba=$(ls ${_liba%%.[0-9]*}*.a | sort -n | tail -1)
> +   for _l in $_libas; do
> +   [[ $_l == $_liba ]] && continue 2
> +   done
> +   _libas="$_libas $_liba"
> +   done

I'm afraid sort -n would not behave the way you probably think:

$ (echo 10.2; echo 10.10; echo 10.50) | sort -n
10.10
10.2
10.50

Also, you code does something strange, because $_liba will be always the
same thing in the loop.

> +   for _liba in $_libas; do
> +   _tmpdir=$(mktemp -dq /tmp/_librebuild.) || return
> +   (
> +   set -o errexit
> +   _lib=${_liba#/usr/lib/}
> +   _lib=${_lib%.a}
> +   cd $_tmpdir
> +   ar x ${_liba}
> +   cc -shared -o $_lib $(ls *.so | sort -R) $(cat .ldadd)
> +   [[ -s $_lib ]] && file $_lib | fgrep -q 'shared 
> object'
> +   LD_BIND_NOW=1 LD_LIBRARY_PATH=$_tmpdir awk 'BEGIN 
> {exit 0}'
> +   install -S -o root -g bin -m 0444 $_lib /usr/lib/$_lib
> +   )
> +   rm -rf /tmp/_librebuild.${_tmpdir#*.}
> +   done
> +}

So I propose something like that instead:

find_newest() {
set -x
local _l _ls _bestmaj _bestmin _maj _min

for _l in /usr/lib/lib$1.so.+([0-9]).+([0-9]); do
_ls=${_l%.*}
_maj=${_ls##*.}
_min=${_l##*.}
if [ _maj -gt _bestmaj -o \
 _maj -eq _bestmaj -a _min -gt _bestmin ]; then
_bestmaj=$_maj
_bestmin=$_min
fi
done
if [ -n $_bestmaj ]; then
echo $_bestmaj.$_bestmin
else
return 1
fi
}

rebuildlibs() {
local _lib _tmpdir _v

_v=$(find_newest c) || return
_lib=libc.so.$_v
_tmpdir=$(mktemp -dq /tmp/_librebuild.) || return
(
set -o errexit
cd $_tmpdir
ar x ${_lib}.a
cc -shared -o $_lib $(ls *.so | sort -R) $(cat .ldadd)
[[ -s $_lib ]] && file $_lib | fgrep -q 'shared object'
LD_BIND_NOW=1 LD_LIBRARY_PATH=$_tmpdir awk 'BEGIN {exit 0}'
install -S -o root -g bin -m 0444 $_lib /usr/lib/$_lib
)
}

--
WBR,
  Vadim Zhukov

Re: Allow bioctl to go through all controllers at once

[Vadim Zhukov]

> 2 packets transmitted, 0 packets received, 100.0% packet loss :)

I see many people are doing something in persistent memory
subsystems now. Maybe this diff could get some attention now
as well. :) Resending the whole thing.

--
WBR,
  Vadim Zhukov


Index: bioctl.c
===
RCS file: /cvs/src/sbin/bioctl/bioctl.c,v
retrieving revision 1.129
diff -u -p -r1.129 bioctl.c
--- bioctl.c18 Jul 2015 23:23:20 -  1.129
+++ bioctl.c18 Apr 2016 11:17:06 -
@@ -69,7 +69,8 @@ void  bio_kdf_generate(struct sr_crypto
 void   derive_key_pkcs(int, u_int8_t *, size_t, u_int8_t *,
size_t, char *, int);
 
-void   bio_inq(char *);
+void   bio_listall();
+void   bio_inq(char *, int);
 void   bio_alarm(char *);
 intbio_getvolbyname(char *);
 void   bio_setstate(char *, int, char *);
@@ -110,12 +111,17 @@ main(int argc, char *argv[])
u_int16_t   cr_level = 0;
int biodev = 0;
 
-   if (argc < 2)
-   usage();
+   if (argc < 2) {
+   bio_listall();
+   return 0;
+   }
 
-   while ((ch = getopt(argc, argv, "a:b:C:c:dH:hik:l:O:Pp:qr:R:st:u:v")) !=
+   while ((ch = getopt(argc, argv, "Aa:b:C:c:dH:hik:l:O:Pp:qr:R:st:u:v")) 
!=
-1) {
switch (ch) {
+   case 'A':
+   bio_listall();
+   return 0;
case 'a': /* alarm */
func |= BIOC_ALARM;
al_arg = optarg;
@@ -243,7 +249,7 @@ main(int argc, char *argv[])
} else if (changepass && !biodev) {
bio_changepass(devicename);
} else if (func & BIOC_INQ) {
-   bio_inq(devicename);
+   bio_inq(devicename, 0);
} else if (func == BIOC_ALARM) {
bio_alarm(al_arg);
} else if (func == BIOC_BLINK) {
@@ -273,7 +279,7 @@ usage(void)
extern char *__progname;
 
fprintf(stderr,
-   "usage: %s [-hiqv] [-a alarm-function] "
+   "usage: %s [-Ahiqv] [-a alarm-function] "
"[-b channel:target[.lun]]\n"
"\t[-H channel:target[.lun]] "
"[-R device | channel:target[.lun]]\n"
@@ -355,6 +361,43 @@ str2patrol(const char *string, struct ti
 }
 
 void
+bio_listall(void)
+{
+   struct  bio_locate  bl;
+   struct  bioc_controllerlist cl;
+   int rv;
+
+   memset(, 0, sizeof(cl));
+   memset(, 0, sizeof(bl));
+
+   devh = open("/dev/bio", O_RDONLY);
+   if (devh == -1)
+   err(1, "Can't open %s", "/dev/bio");
+
+   rv = ioctl(devh, BIOCLISTCONTROLLERS, );
+   if (rv == -1)
+   err(1, "1 BIOCLISTCONTROLLERS");
+   if (cl.bcl_size == 0)
+   return;
+
+   cl.bcl_list = calloc(cl.bcl_size, sizeof(struct bioc_controller));
+   if (cl.bcl_list == NULL)
+   err(1, "calloc");
+   rv = ioctl(devh, BIOCLISTCONTROLLERS, );
+   if (rv == -1)
+   err(1, "2 BIOCLISTCONTROLLERS");
+   while (cl.bcl_size--) {
+   bl.bl_name = cl.bcl_list[cl.bcl_size].bc_xname;
+   if (ioctl(devh, BIOCLOCATE, ))
+   errx(1, "Can't locate %s device via %s",
+   bl.bl_name, "/dev/bio");
+   bio_cookie = bl.bl_bio.bio_cookie;
+   bio_inq(NULL, 1);
+   }
+   free(cl.bcl_list);
+}
+
+void
 bio_status(struct bio_status *bs)
 {
extern char *__progname;
@@ -378,7 +421,7 @@ bio_status(struct bio_status *bs)
 }
 
 void
-bio_inq(char *name)
+bio_inq(char *name, int ignorenotsupp)
 {
char*status, *cache;
charsize[64], scsiname[16], volname[32];
@@ -394,9 +437,10 @@ bio_inq(char *name)
bi.bi_bio.bio_cookie = bio_cookie;
 
if (ioctl(devh, BIOCINQ, )) {
-   if (errno == ENOTTY)
-   bio_diskinq(name);
-   else
+   if (errno == ENOTTY) {
+   if (!ignorenotsupp)
+   bio_diskinq(name);
+   } else
err(1, "BIOCINQ");
return;
}

Re: Scheduler hack for multi-threaded processes

[Vadim Zhukov]

27 марта 2016 г. 11:05 пользователь "Stefan Sperling" <s...@stsp.name>
написал:
>
> On Sat, Mar 26, 2016 at 05:47:38PM -0600, attila wrote:
> > Now it will be a challenge to see if I can cvs up, back out the patch
> > and build a kernel without ringing the bell (100DegC).  I freely admit
> > this is an old, P.O.S. laptop and that there might be some HW issue
>
> Did you check for dust in the fan vent?
>
> I've had to treat my x60 with a hoover more than once after it had fallen
> ill with a temperature...

That's dangerous: you at least need fan cables from motherboard, to avoid
it generating electricity when spinning. I know two cases of killed this
way motherboards (not ThinkPad, but still).

--
WBR,
  Vadim Zhukov

Re: The Case of Lost TM_ZONE

[Vadim Zhukov]

2016-03-06 1:23 GMT+03:00 Christian Weisgerber <na...@mips.inka.de>:
> > It looks like decided to use TM_ZONE as a wrapper for tm.tm_zone,
> > until the tm_zone is gone. So, until the later happens (I found
> > no usage of tm_zone in base, BTW), we should at least use TM_ZONE
> > consistently. Okay for the patch?
> 
> ok naddy@
> 
> Note that tm_gmtoff is wrapped the same way by TM_GMTOFF, and there
> is a similar inconsistent use of tm_gmtoff in wcsftime.c

You're absolutely right. Here is an updated patch.

Since %z relies on tm_gmtoff, all we can do is to expand %z to empty
string, as specified in strftime(3).

Okay for this version?

--
WBR,
  Vadim Zhukov


Index: wcsftime.c
===
RCS file: /cvs/src/lib/libc/time/wcsftime.c,v
retrieving revision 1.6
diff -u -p -r1.6 wcsftime.c
--- wcsftime.c  9 Feb 2015 14:52:28 -   1.6
+++ wcsftime.c  6 Mar 2016 12:12:16 -
@@ -438,9 +438,11 @@ label:
pt = _yconv(t->tm_year, TM_YEAR_BASE, 1, 1, pt, ptlim);
continue;
case 'Z':
-   if (t->tm_zone != NULL)
+#ifdef TM_ZONE
+   if (t->TM_ZONE != NULL)
pt = _sadd(t->TM_ZONE, pt, ptlim);
else
+#endif
if (t->tm_isdst >= 0)
pt = _sadd(tzname[t->tm_isdst != 0], 
pt, ptlim);
@@ -451,13 +453,14 @@ label:
*/
continue;
case 'z':
+#ifdef TM_GMTOFF
{
int diff;
wchar_t const * sign;
 
if (t->tm_isdst < 0)
continue;
-   diff = t->tm_gmtoff;
+   diff = t->TM_GMTOFF;
if (diff < 0) {
sign = L"-";
diff = -diff;
@@ -469,6 +472,7 @@ label:
(diff % MINSPERHOUR);
pt = _conv(diff, L"%04d", pt, ptlim);
}
+#endif
continue;
case '+':
pt = _fmt(Locale->date_fmt, t, pt, ptlim, warnp);

The Case of Lost TM_ZONE

[Vadim Zhukov]

It looks like decided to use TM_ZONE as a wrapper for tm.tm_zone,
until the tm_zone is gone. So, until the later happens (I found
no usage of tm_zone in base, BTW), we should at least use TM_ZONE
consistently. Okay for the patch? Or should I proceed to the TM_ZONE
removal together with next libc major bump?

Please note that some software doesn't zero out struct tm before
use, resulting in garbage being written to this field. If you'd
call the strftime() or wcsftime() on such struct, you'll get core
dumped (in the best case). I'm looking at you, Qt!

--
WBR,
  Vadim Zhukov


Index: wcsftime.c
===
RCS file: /cvs/src/lib/libc/time/wcsftime.c,v
retrieving revision 1.6
diff -u -p -r1.6 wcsftime.c
--- wcsftime.c  9 Feb 2015 14:52:28 -   1.6
+++ wcsftime.c  28 Feb 2016 11:01:48 -
@@ -438,9 +438,11 @@ label:
pt = _yconv(t->tm_year, TM_YEAR_BASE, 1, 1, pt, ptlim);
continue;
case 'Z':
-   if (t->tm_zone != NULL)
+#ifdef TM_ZONE
+   if (t->TM_ZONE != NULL)
pt = _sadd(t->TM_ZONE, pt, ptlim);
else
+#endif
if (t->tm_isdst >= 0)
pt = _sadd(tzname[t->tm_isdst != 0], 
pt, ptlim);

Re: [patch] cleanup vi

[Vadim Zhukov]

2016-01-23 14:08 GMT+03:00 Theo Buehler <t...@math.ethz.ch>:
> On Sat, Jan 23, 2016 at 11:03:39AM +0100, Martijn van Duren wrote:
>> Here's a small update:
>> On 01/22/16 21:18, Martijn van Duren wrote:
>> >3) 3_vi_remove_progname.diff: Don't keep a copy of the progname in
>> >memory, use getprogname instead.
>> Attached without the setprogname. The reason I included setprogname was
>> because getprogname in the manpage doesn't mention that it always returns a
>> stripped version and it's a strict requirement of vi.
>> >4) 4_vi_remove_tail.diff: Use basename instead of tail
>> The second basename is inside #define DEBUG, so I missed it in the compile
>> check.
>> Furthermore it's only used for msgq, which uses it it vsnprint and I can't
>> imagine it can hurt in there.
>> As for the input, it's used with a variable file and __FILE__, so it
>> shouldn't cause any more harm than tail.
>
> patches 1-4 and 6 are fine with me now, modulo s/baneame/basename in
> vi/vs_refresh.c:477 (I think that's what zhuk@ meant).
>
> I must say I liked the previous version of 5 better.
>
> I haven't checked all of 5, but the addition of these msgq(..) calls in
> common/{screen.c,seq.c} looks wrong to me.  The error paths goto mem*
> already contain such a call to msgq.

I absolutely agree that those calls look like wrong and could do bad
things in case of ENOMEM. My point is that we should make them visible
first, by doing a simple, reviewable change like unmacrosing the C
module files, and only then remove/rework stuff.

--
  WBR,
  Vadim Zhukov

Don't override all errors in opendev

[Vadim Zhukov]

Not all errors coming from diskmap should be overwritten.
In particular, if we get EBUSY, that doesn't mean "try again with
accessing /dev/foo, fail and return ENOENT instead".

Maybe we should add more errors to this list, or even invert the
filtering logic - I'm: 1) not an expert in this area; 2) open
for improvements.

How to reproduce: tunefs /

Before patch:
tunefs: /dev/r286cb876606dc1e0.l: No such file or directory

After patch:
tunefs: /dev/sd0l: Device busy

Okay?

--
WBR,
  Vadim Zhukov


Index: opendev.c
===
RCS file: /cvs/src/lib/libutil/opendev.c,v
retrieving revision 1.15
diff -u -p -r1.15 opendev.c
--- opendev.c   30 Jun 2011 15:04:58 -  1.15
+++ opendev.c   19 Jan 2016 22:05:12 -
@@ -79,7 +79,8 @@ opendev(const char *path, int oflags, in
if (ioctl(fd, DIOCMAP, ) == -1) {
close(fd);
fd = -1;
-   errno = ENOENT;
+   if (errno != EBUSY)
+   errno = ENOENT;
}
}
}

Re: Don't override all errors in opendev

[Vadim Zhukov]

2016-01-20 1:10 GMT+03:00 Vadim Zhukov <persg...@gmail.com>:
> Not all errors coming from diskmap should be overwritten.
> In particular, if we get EBUSY, that doesn't mean "try again with
> accessing /dev/foo, fail and return ENOENT instead".
>
> Maybe we should add more errors to this list, or even invert the
> filtering logic - I'm: 1) not an expert in this area; 2) open
> for improvements.
>
> How to reproduce: tunefs /
>
> Before patch:
> tunefs: /dev/r286cb876606dc1e0.l: No such file or directory

This one should be ".a", not ".l", of course: I've messed up with
output from different partition, sorry.

> After patch:
> tunefs: /dev/sd0l: Device busy

--
  WBR,
  Vadim Zhukov

Re: Putting more consistency on SYNOPSIS in section 5

[Vadim Zhukov]

2016-01-07 19:12 GMT+03:00 Ingo Schwarze <schwa...@usta.de>:
> Hi Vadim,
>
> Vadim Zhukov wrote on Wed, Jan 06, 2016 at 04:41:13PM +0300:
>
>> While mdoc(7) says that section 5 manuals do not need SYNOPSIS usually,
>> we have 34 pages that do:
> [...]
>
>> Two of those contain some complicated stuff in SYNOPSIS:
>>
>> lib/libkeynote/keynote.5
>> share/man/man5/bsd.port.arch.mk.5
>>
>> I think this is okay, since this is the info you likely want to see,
>> except when opening manual page the first time.
>
> Agreed.  Such cases could only be improved on a case-by-case basis,
> and only if they need it.
>
>> Then, many files do only mention .In or some equivalent in SYNOPSIS:
>>
>> sbin/disklabel/disklabel.5
>> share/man/man5/acct.5
>> share/man/man5/ar.5
>> share/man/man5/bsd.port.mk.5
>> share/man/man5/bsd.regress.mk.5
>> share/man/man5/core.5
>> share/man/man5/dir.5
>> share/man/man5/disktab.5
>> share/man/man5/elf.5
>> share/man/man5/fs.5
>> share/man/man5/fstab.5
>> share/man/man5/mk.conf.5
>> share/man/man5/ranlib.5
>> share/man/man5/utmp.5
>>
>> I'm not sure about those, but those are likely being useful.
>
> Agreed.  I don't think these need to be changed, at least i can't
> think of an improvement right now.
>
>> Then, we have following files showing their exact placement in SYNOPSIS:
>>
>> share/man/man5/bootparams.5   .Nm /etc/bootparams
>> share/man/man5/changelist.5   .Nm /etc/changelist
>> share/man/man5/defaultdomain.5.Nm /etc/defaultdomain
>> share/man/man5/login.conf.5   .Nm /etc/login.conf
>> share/man/man5/mygate.5   .Nm /etc/myname
>> share/man/man5/myname.5   .Nm /etc/myname
>> share/man/man5/netgroup.5 .Nm /etc/netgroup
>> share/man/man5/spamd.conf.5   .Nm /etc/mail/spamd.conf
>> usr.bin/ssh/ssh_config.5  .Nm /etc/ssh/ssh_config
>> usr.bin/ssh/ssh_config.5  .Nm ~/.ssh/config
>> usr.bin/ssh/sshd_config.5 .Nm /etc/ssh/sshd_config
>> usr.sbin/npppd/npppd/npppd-users.5.Nm /etc/npppd/npppd-users
>>
>> I'm not sure about these much more. I usually prefer looking at the
>> FILES section, but maybe this could be put in the first paragraph
>> of DESCRIPTION, or whatever. Any opinions?
>
> I think these SYNOPSIS sections should be removed, making sure
> the FILES sections are present, correct, and complete.  FILES
> should always be used when relevant.  A section containing no
> information whatsoever (like SYNOPSIS in these cases) is useless,
> so it can be removed, improving conciseness.
>
>> And some files contain totaly extraneous SYNOPSIS lines, either
>> just ".Nm", or ".Nm foo". Those are totally useless, and the patch
>> for those is below. Okay?
>
> OK schwarze@.
>
> But please add FILES to gettytab(5) and printcap(5) when comitting.

Ingo,

thank you for responding! I've just comitted the "easy" part, and
going to convert more SYNOPSIS to FILES, which you agreed upon, a bit
later. I'll show the diff separately.

--
  WBR,
  Vadim Zhukov

Putting more consistency on SYNOPSIS in section 5 (was: mention /etc/doas.conf instead of plain doas.conf)

[Vadim Zhukov]

2016-01-01 2:39 GMT+03:00 Amit Kulkarni <amitk...@gmail.com>:
> I just switched from sudo to doas and was stumped by this.
>
> The doas code expects doas.conf in /etc yet the manpage does not explicitly
> make that clear. I added a SYNOPSIS like in "man login.conf".

While mdoc(7) says that section 5 manuals do not need SYNOPSIS usually,
we have 34 pages that do:

lib/libedit/editrc.5
lib/libkeynote/keynote.5
libexec/getty/gettytab.5
sbin/disklabel/disklabel.5
sbin/mountd/exports.5
share/man/man5/acct.5
share/man/man5/ar.5
share/man/man5/bsd.port.arch.mk.5
share/man/man5/bsd.port.mk.5
share/man/man5/bsd.regress.mk.5
share/man/man5/changelist.5
share/man/man5/core.5
share/man/man5/defaultdomain.5
share/man/man5/dir.5
share/man/man5/disktab.5
share/man/man5/elf.5
share/man/man5/fs.5
share/man/man5/fstab.5
share/man/man5/login.conf.5
share/man/man5/mk.conf.5
share/man/man5/myname.5
share/man/man5/netgroup.5
share/man/man5/printcap.5
share/man/man5/ranlib.5
share/man/man5/spamd.conf.5
share/man/man5/utmp.5
share/termtypes/termcap.5
usr.bin/doas/doas.conf.5
usr.bin/ssh/ssh_config.5
usr.bin/ssh/sshd_config.5
usr.sbin/npppd/npppd/npppd-users.5
usr.sbin/rpc.bootparamd/bootparams.5
usr.sbin/smtpd/aliases.5
usr.sbin/user/usermgmt.conf.5

Two of those contain some complicated stuff in SYNOPSIS:

lib/libkeynote/keynote.5
share/man/man5/bsd.port.arch.mk.5

I think this is okay, since this is the info you likely want to see,
except when opening manual page the first time.

Then, many files do only mention .In or some equivalent in SYNOPSIS:

sbin/disklabel/disklabel.5
share/man/man5/acct.5
share/man/man5/ar.5
share/man/man5/bsd.port.mk.5
share/man/man5/bsd.regress.mk.5
share/man/man5/core.5
share/man/man5/dir.5
share/man/man5/disktab.5
share/man/man5/elf.5
share/man/man5/fs.5
share/man/man5/fstab.5
share/man/man5/mk.conf.5
share/man/man5/ranlib.5
share/man/man5/utmp.5

I'm not sure about those, but those are likely being useful.

Then, we have following files showing their exact placement in SYNOPSIS:

share/man/man5/bootparams.5   .Nm /etc/bootparams
share/man/man5/changelist.5   .Nm /etc/changelist
share/man/man5/defaultdomain.5.Nm /etc/defaultdomain
share/man/man5/login.conf.5   .Nm /etc/login.conf
share/man/man5/mygate.5   .Nm /etc/myname
share/man/man5/myname.5   .Nm /etc/myname
share/man/man5/netgroup.5 .Nm /etc/netgroup
share/man/man5/spamd.conf.5   .Nm /etc/mail/spamd.conf
usr.bin/ssh/ssh_config.5  .Nm /etc/ssh/ssh_config
usr.bin/ssh/ssh_config.5  .Nm ~/.ssh/config
usr.bin/ssh/sshd_config.5 .Nm /etc/ssh/sshd_config
usr.sbin/npppd/npppd/npppd-users.5.Nm /etc/npppd/npppd-users

I'm not sure about these much more. I usually prefer looking at the
FILES section, but maybe this could be put in the first paragraph
of DESCRIPTION, or whatever. Any opinions?

And some files contain totaly extraneous SYNOPSIS lines, either
just ".Nm", or ".Nm foo". Those are totally useless, and the patch
for those is below. Okay?

--
  WBR,
  Vadim Zhukov


Index: lib/libedit/editrc.5
===
RCS file: /cvs/src/lib/libedit/editrc.5,v
retrieving revision 1.28
diff -u -p -r1.28 editrc.5
--- lib/libedit/editrc.515 Dec 2014 22:35:41 -  1.28
+++ lib/libedit/editrc.56 Jan 2016 13:12:46 -
@@ -33,8 +33,6 @@
 .Sh NAME
 .Nm editrc
 .Nd configuration file for editline library
-.Sh SYNOPSIS
-.Nm
 .Sh DESCRIPTION
 The
 .Nm
Index: libexec/getty/gettytab.5
===
RCS file: /cvs/src/libexec/getty/gettytab.5,v
retrieving revision 1.26
diff -u -p -r1.26 gettytab.5
--- libexec/getty/gettytab.56 Nov 2015 16:42:30 -   1.26
+++ libexec/getty/gettytab.56 Jan 2016 13:12:46 -
@@ -34,8 +34,6 @@
 .Sh NAME
 .Nm gettytab
 .Nd terminal configuration database
-.Sh SYNOPSIS
-.Nm gettytab
 .Sh DESCRIPTION
 The
 .Nm
Index: sbin/mountd/exports.5
===
RCS file: /cvs/src/sbin/mountd/exports.5,v
retrieving revision 1.22
diff -u -p -r1.22 exports.5
--- sbin/mountd/exports.5   15 Dec 2015 18:25:28 -  1.22
+++ sbin/mountd/exports.5   6 Jan 2016 13:12:46 -
@@ -36,8 +36,6 @@
 .Sh NAME
 .Nm exports
 .Nd define remote mount points for NFS mount requests
-.Sh SYNOPSIS
-.Nm exports
 .Sh DESCRIPTION
 The
 .Nm
Index: share/man/man5/printcap.5
===
RCS file: /cvs/src/share/man/man5/printcap.5,v
retrieving revision 1.25
diff -u -p -r1.25 printcap.5
--- share/man/man5/printcap.5   17 Nov 2015 17:10:36 -  1.25
+++ share/man/man5/printcap.5   6 Jan 2016 13:12:46 -
@@ -36,8 +36,6 @@
 .Sh NAME
 .Nm printcap
 .Nd printer capability database
-.Sh SYNOPSIS
-.Nm printcap
 .Sh DESCRI

Re: mention /etc/doas.conf instead of plain doas.conf

[Vadim Zhukov]

2016-01-01 2:39 GMT+03:00 Amit Kulkarni <amitk...@gmail.com>:
> I just switched from sudo to doas and was stumped by this.
>
> The doas code expects doas.conf in /etc yet the manpage does not explicitly
> make that clear. I added a SYNOPSIS like in "man login.conf".
>
> Thanks
>
> Index: doas.conf.5
> ===
> RCS file: /cvs/src/usr.bin/doas/doas.conf.5,v
> retrieving revision 1.16
> diff -u -p -u -p -r1.16 doas.conf.5
> --- doas.conf.5 1 Sep 2015 13:20:53 -   1.16
> +++ doas.conf.5 31 Dec 2015 23:39:23 -
> @@ -19,12 +19,14 @@
>  .Sh NAME
>  .Nm doas.conf
>  .Nd doas configuration file
> +.Sh SYNOPSIS
> +.Nm /etc/doas.conf
>  .Sh DESCRIPTION
>  The
>  .Xr doas 1
>  utility executes commands as other users according to the rules
> -in the
> -.Nm
> +in
> +.Nm /etc/doas.conf
>  configuration file.
>  .Pp
>  The rules have the following format:

I think it's rather the login.conf(5) is wrong here... And the default
file placement is usually shown in the FILES section.

On the other side, the /etc is default system configuration directory
anyway, so /etc/doas.conf should be the first, obvious guess... no?

--
  WBR,
  Vadim Zhukov

Re: UTF-8 support for uniq(1)

[Vadim Zhukov]

10 дек. 2015 г. 18:51 пользователь "Ingo Schwarze" <schwa...@usta.de>
написал:
>
> Hi,
>
> here is a simple one.  The uniq(1) utility only needs UTF-8 support
> to distinguish blank and non-blank characters with -f and to skip
> characters with -s.  The former is easy to implement with mbtowc(3)
> and iswblank(3), the latter with mblen(3).  There is no need for
> wrapper functions or a seperate utf8.c file.
>
> OK?
>   Ingo
>
>
> Index: uniq.1
> ===
> RCS file: /cvs/src/usr.bin/uniq/uniq.1,v
> retrieving revision 1.17
> diff -u -p -r1.17 uniq.1
> --- uniq.1  3 Sep 2010 11:09:29 -   1.17
> +++ uniq.1  10 Dec 2015 15:37:02 -
> @@ -114,6 +114,14 @@ A file name of
>  .Ql -
>  denotes the standard input or the standard output
>  .Pq depending on its position on the command line .
> +.Sh ENVIRONMENT
> +.Bl -tag -width LC_CTYPE
> +.It Ev LC_CTYPE
> +The character set
> +.Xr locale 1 .
> +Determines which groups of bytes are treated as characters
> +and which characters are considered blank.
> +.El
>  .Sh EXIT STATUS
>  .Ex -std uniq
>  .Sh SEE ALSO
> Index: uniq.c
> ===
> RCS file: /cvs/src/usr.bin/uniq/uniq.c,v
> retrieving revision 1.23
> diff -u -p -r1.23 uniq.c
> --- uniq.c  2 Nov 2015 20:25:42 -   1.23
> +++ uniq.c  10 Dec 2015 15:37:02 -
> @@ -37,10 +37,13 @@
>  #include 
>  #include 
>  #include 
> +#include 
>  #include 
>  #include 
>  #include 
>  #include 
> +#include 
> +#include 
>
>  #defineMAXLINELEN  (8 * 1024)
>
> @@ -61,6 +64,8 @@ main(int argc, char *argv[])
> int ch;
> char *prevline, *thisline;
>
> +   setlocale(LC_CTYPE, "");
> +
> if (pledge("stdio rpath wpath cpath", NULL) == -1)
> err(1, "pledge");
>
> @@ -176,16 +181,38 @@ show(FILE *ofp, char *str)
>  char *
>  skip(char *str)
>  {
> +   wchar_t wc;
> int nchars, nfields;
> +   int len;
>
> for (nfields = numfields; nfields && *str; nfields--) {
> -   while (isblank((unsigned char)*str))
> -   str++;
> -   while (*str && !isblank((unsigned char)*str))
> -   str++;
> +
> +   /* Skip blanks before the next field. */
> +   do {
> +   if ((len = mbtowc(, str, MB_CUR_MAX)) == -1) {
> +   (void)mbtowc(NULL, NULL, MB_CUR_MAX);
> +   wc = L'?';
> +   len = 1;
> +   }
> +   str += len;
> +   } while (*str != '\0' && iswblank(wc));
> +
> +   /* Skip one field. */
> +   while (*str != '\0' && !iswblank(wc)) {
> +   if ((len = mbtowc(, str, MB_CUR_MAX)) == -1) {
> +   (void)mbtowc(NULL, NULL, MB_CUR_MAX);
> +   wc = L'?';
> +   len = 1;
> +   }
> +   str += len;
> +   }
> }
> -   for (nchars = numchars; nchars-- && *str && *str != '\n'; ++str)
> -   ;
> +
> +   /* Skip some additional characters. */
> +   for (nchars = numchars; nchars-- && *str != '\0'; str += len)
> +   if ((len = mblen(str, MB_CUR_MAX)) == -1)
> +   len = 1;
> +
> return (str);
>  }

Reads good to me, okay zhuk@.

--
Vadim Zhukov

Re: doas password prompt

[Vadim Zhukov]

2015-12-02 12:40 GMT+03:00 Ted Unangst <t...@tedunangst.com>:
> henning points out that if you are seven levels deep when doas asks for a
> password, it can be hard to tell who is asking for what password.
>
> modify the prompt to include the program name and user@host.

The patch itself looks like fine for me, but why not just add '\u@\h'
to PS1 instead?

--
  WBR,
  Vadim Zhukov

Re: mini utf-8 hexdump

[Vadim Zhukov]

2015-10-27 11:51 GMT+01:00 Ted Unangst <t...@tedunangst.com>:
> This adds a quite limited understanding of utf-8 to hexdump. I've found it
> helpful trying to see exactly what's coming out of some utilities instead of
> trying to decode utf-8 by hand.
>
> Index: display.c
> ===
> RCS file: /cvs/src/usr.bin/hexdump/display.c,v
> retrieving revision 1.21
> diff -u -p -r1.21 display.c
> --- display.c   16 Jan 2015 06:40:08 -  1.21
> +++ display.c   27 Oct 2015 10:50:09 -
> @@ -106,6 +106,17 @@ display(void)
> }
>  }
>
> +static int
> +isu8cont(unsigned char c)
> +{
> +   return (c & 0xc0) == 0x80;
> +}
> +static int
> +isu8start(unsigned char c)
> +{
> +   return (c & 0xc0) == 0xc0;
> +}
> +
>  static __inline void
>  print(PR *pr, u_char *bp)
>  {
> @@ -163,7 +174,16 @@ print(PR *pr, u_char *bp)
> }
> break;
> case F_P:
> -   (void)printf(pr->fmt, isprint(*bp) ? *bp : '.');
> +   if (isu8start(*bp)) {
> +   unsigned char *pp = bp + 1;
> +   (void)printf(pr->fmt, *bp);
> +   while (isu8cont(*pp))
> +   (void)printf(pr->fmt, *pp++);
> +   } else if (isu8cont(*bp)) {
> +   (void)printf(pr->fmt, ' ');
> +   } else {
> +   (void)printf(pr->fmt, isprint(*bp) ? *bp : '.');
> +   }
> break;
> case F_STR:
> (void)printf(pr->fmt, (char *)bp);

So in case of the mangled UTF8 codepoint like the following you'll
print more bytes than you should (two at max), no?

110x
10xx
10xx


--
  WBR,
  Vadim Zhukov

Re: clean up mbtowc(3) man page

[Vadim Zhukov]

2015-10-27 13:40 GMT+01:00 Stefan Sperling <s...@stsp.name>:
> Index: mbtowc.3
> ===
> RCS file: /cvs/src/lib/libc/locale/mbtowc.3,v
> retrieving revision 1.4
> diff -u -p -r1.4 mbtowc.3
> --- mbtowc.35 Jun 2013 03:39:22 -   1.4
> +++ mbtowc.327 Oct 2015 12:20:49 -
> @@ -40,27 +40,15 @@
>  .Sh DESCRIPTION
>  The
>  .Fn mbtowc
> -usually converts the multibyte character pointed to by
> +converts the multibyte character pointed to by
>  .Fa s
>  to a wide character, and stores it in the wchar_t object pointed to by
> -.Fa pwc
> -if
> -.Fa pwc
> -is non-null and
> -.Fa s
> -points to a valid character.
> -This function may inspect at most n bytes of the array beginning from
> +.Fa pwc .
> +This function may inspect at most
> +.Fa n
> +bytes of the array pointed to by
>  .Fa s .
>  .Pp
> -In state-dependent encodings,
> -.Fa s
> -may point to the special sequence bytes to change the shift-state.
> -Although such sequence bytes correspond to no individual
> -wide-character code,
> -.Fn mbtowc
> -changes its own state by the sequence bytes and treats them
> -as if they are a part of the subsequence multibyte character.
> -.Pp
>  Unlike
>  .Xr mbrtowc 3 ,
>  the first
> @@ -68,8 +56,24 @@ the first
>  bytes pointed to by
>  .Fa s
>  need to form an entire multibyte character.
> -Otherwise, this function causes an error.
> +Otherwise, this function returns an error and the internal state will
> +be undefined.
> +.Pp
> +If a call to
> +.Fn mbtowc
> +resulted in an undefined internal state,
> +.Fn mbtowc
> +must be called with
> +.Ar s
> +set to
> +.Dv NULL
> +to reset the internal state before being used again for other purposes.
>  .Pp
> +The behaviour of
> +.Fn mbtowc
> +is affected by the
> +.Dv LC_CTYPE
> +category of the current locale.
>  Calling any other functions in
>  .Em libc
>  never change the internal
> @@ -79,41 +83,46 @@ except for calling
>  .Xr setlocale 3
>  with the
>  .Dv LC_CTYPE
> -category changed to that of the current locale.
> +category set to a different locale.
>  Such
>  .Xr setlocale 3
> -calls cause the internal state of this function to be indeterminate.
> +calls cause the internal state of this function to be undefined.
>  .Pp
> -The behaviour of
> +In state-dependent encodings such as Shift-JIS,
> +.Fa s
> +may point to the special sequence of bytes to change the shift-state.
> +Because such sequence bytes correspond to no individual wide-character,
>  .Fn mbtowc
> -is affected by the
> -.Dv LC_CTYPE
> -category of the current locale.
> +treats them as if they were part of the subsequent multibyte character.
>  .Pp
> -These are the special cases:
> +The following special cases apply to the arguments:
>  .Bl -tag -width 012345678901
>  .It s == NULL
>  .Fn mbtowc
>  initializes its own internal state to an initial state, and
>  determines whether the current encoding is state-dependent.
> -This function returns 0 if the encoding is state-independent,
> +.Fn mbtowc
> +returns 0 if the encoding is state-independent,
>  otherwise non-zero.
> -In this case,
>  .Fa pwc
> -is completely ignored.
> +is ignored if
> +.Fa s
> +is
> +.Dv NULL .

Since we're already talking about "s==NULL" case, stating it again is
extraneous. IMHO, better would be:

 .Fa pwc
-is completely ignored.
+is ignored.

>  .It pwc == NULL
>  .Fn mbtowc
>  executes the conversion as if
>  .Fa pwc
> -is non-null, but a result of the conversion is discarded.
> +was not
> +.Dv NULL ,
> +but the result of the conversion is discarded.

IMHO, this should clarify if this would modify internal state.

Other than that, looks okay to me.

--
  WBR,
  Vadim Zhukov

Re: csh: kill profiling & debugging ifdefs

[Vadim Zhukov]

2015-10-26 21:55 GMT+01:00 Christian Weisgerber <na...@mips.inka.de>:
> Remove the profiling and debugging ifdefs.  Most of this is for
> instrumenting the expression evaluator and has been in place for
> 35 years.  I think we're done debugging.
>
> ok?

okay zhuk@
--
  WBR,
  Vadim Zhukov

Spaces are allowed in pdisk partitions, aren't they?

[Vadim Zhukov]

According to miod@, partition name is allowed to have spaces. Thus,
and after reading the code, I suspect the following correction in
the manual page is needed. The "create partition" and "rename
partition" commands share the same name extracting code, so those
should have the same behaviour.

So I'm asking for confirmation (or something I could count like
an appropriate okay). Any suggestions are welcome too, of course;
main point here is to remove conflicting statements.

--
WBR,
  Vadim Zhukov


Index: pdisk.8
===
RCS file: /cvs/src/sbin/pdisk/pdisk.8,v
retrieving revision 1.18
diff -u -p -r1.18 pdisk.8
--- pdisk.8 26 Aug 2010 17:55:10 -  1.18
+++ pdisk.8 26 Oct 2015 18:53:37 -
@@ -141,6 +141,8 @@ respectively.
 The last argument is the name of the partition.
 This can be a single word without quotes, or a string surrounded by
 single or double quotes.
+Note that behaviour of other OSes isn't defined in case of name
+containing space characters.
 The type of the created partition is the correct type for
 .Ox .
 .Pp
@@ -154,7 +156,9 @@ other arguments.
 The
 .Em n
 (name) command allows the name of a partition to be changed.
-The name must not contain any spaces.
+See the description of
+.Sq c
+command for details on naming partitions.
 Note that the various "Apple_Driver" partitions depend
 on the name field for proper functioning.
 I am not aware of any other partition types with this limitation.

Further cleanup of Russian calendars

[Vadim Zhukov]

Further cleanup in Russian calendars:

  * Fix #ifndef safeguards (rename/add where missing);
  * Use consistent spelling for year when it's mentioned in day desc;
  * Tweak some wrong casing cases;
  * Remove calendar.msk since Moscow doesn't have summer time anymore,
and that was the only thing this file was about;
  * A few other corrections.

Still seeking for a way to define a day with fixed shift from the year
start: the Programmer's Day is official in Russia, happens at 256th
day of the year, but calendar(1) handles up to 31 days per month max.

Okay?
--
WBR,
  Vadim Zhukov


Index: calendar.all
===
RCS file: /cvs/src/usr.bin/calendar/calendars/ru_RU.UTF-8/calendar.all,v
retrieving revision 1.1
diff -u -p -r1.1 calendar.all
--- calendar.all23 Oct 2015 09:32:14 -  1.1
+++ calendar.all23 Oct 2015 11:09:26 -
@@ -4,14 +4,13 @@
  * $OpenBSD: calendar.all,v 1.1 2015/10/23 09:32:14 tedu Exp $
  */
 
-#ifndef _ru_RU_KOI8_R_all
-#define _ru_RU_KOI8_R_all
+#ifndef _ru_RU_UTF_8_all
+#define _ru_RU_UTF_8_all
 
 #include 
-#include 
 #include 
 #include 
 #include 
 #include 
 
-#endif /* !_ru_RU_KOI8_R_all */
+#endif /* !_ru_RU_UTF_8_all */
Index: calendar.common
===
RCS file: /cvs/src/usr.bin/calendar/calendars/ru_RU.UTF-8/calendar.common,v
retrieving revision 1.1
diff -u -p -r1.1 calendar.common
--- calendar.common 23 Oct 2015 09:32:14 -  1.1
+++ calendar.common 23 Oct 2015 11:09:26 -
@@ -1,11 +1,12 @@
+B
 /*
  * Русские праздники и памятные даты
  *
  * $OpenBSD: calendar.common,v 1.1 2015/10/23 09:32:14 tedu Exp $
  */
 
-#ifndef _ru_RU_KOI8_R_common_
-#define _ru_RU_KOI8_R_common_
+#ifndef _ru_RU_UTF_8_common_
+#define _ru_RU_UTF_8_common_
 
 LANG=ru_RU.UTF-8
 BODUN=Бодун на утро от:
@@ -15,7 +16,7 @@ BODUN=Бодун на утро от:
 01/12  День работников прокуратуры
 01/13  День Российской печати
 01/14  Старый новый год
-01/25  Татьянин день. Студенческий праздник.
+01/25  Татьянин день. Студенческий праздник
 01/27  Всемирный день таможни
 
 /* Февраль */
@@ -61,7 +62,7 @@ BODUN=Бодун на утро от:
 05/27  Общероссийский день библиотек
 05/28  День пограничника
 05/31  День без табака
-05/Sun+2   День Матери (2-ое воскресенье Мая)
+05/Sun+2   День матери
 05/Sun+5   День химика
 
 /* Июнь */
@@ -84,18 +85,18 @@ BODUN=Бодун на утро от:
 07/11  Всемирный день народонаселения
 07/Sun+2   День российской почты
 07/Sun+3   День металлурга
-07/Sun+4   День Военно-Морского Флота
+07/Sun+4   День военно-морского флота
 
 /* Август */
 08/02  День воздушно-десантных войск России
 08/06  Всемирный день борьбы за запрещение 
ядерного оружия.
 08/09  Всемирный день коренных народов мира
-08/22  День Государственного Флага Р
оссийской Федерации
+08/22  День государственного флага Р
оссийской Федерации
 08/27  День российского кино
 08/SunFirstДень железнодорожника
 08/Sat+2   День физкультурника
 08/Sun+2   День строителя
-08/Sun+3   День Воздушного Флота России
+08/Sun+3   День воздушного флота России
 08/SunLast День шахтера
 
 /* Сентябрь */
@@ -106,7 +107,7 @@ BODUN=Бодун на утро от:
 09/27  Международный день туризма
 09/28  Всемирный день моря
 09/SunFirstДень работников нефтяной и 
газовой промышленности
-09/Sun+2   День танкистов
+09/Sun+2   День танкиста
 09/Sun+3   День работников леса
 09/SunLast День машиностроителя
 
@@ -116,9 +117,9 @@ BODUN=Бодун на утро от:
 10/05  Международный день учителя
 10/16  Всемирный день продовольствия
 10/17  Международный день протеста против 
нищеты
-10/24  Несчастливый день на Байконуре, после 
1960,1963гг
+10/24  Несчастливый день на Байконуре
 10/24  День Организации Объединенных Наций
-10/25  День тамÐ

Re: does anoybody use ul?

[Vadim Zhukov]

2015-10-23 15:38 GMT+02:00 Ted Unangst <t...@tedunangst.com>:
> Christian Weisgerber wrote:
>> Ted Unangst:
>>
>> > --- ul.c10 Oct 2015 16:15:03 -  1.19
>> > +++ ul.c23 Oct 2015 10:29:43 -
>> > @@ -241,6 +241,8 @@ mfilter(FILE *f)
>> > obuf[col].c_mode |= BOLD|mode;
>> > else
>> > obuf[col].c_mode = mode;
>> > +   if ((c & (0x80 | 0x40)) == 0x80 && col > 0)
>> > +   obuf[col].c_mode = obuf[col - 1].c_mode;
>> > col++;
>> > if (col > maxcol)
>> > maxcol = col;
>>
>> That doesn't quite work.  Check out this:
>>
>> mandoc /usr/share/man/man1/ksh.1 | sed -n 1185,1190p | ul
>
> so that works with the diff below. i'm not sure how far down this road we need
> to travel, but i figure it's worth a little exploration.
>
> note that i don't think this handles the case of one character, backspace, a
> different character correctly, though it can asymptotically approach
> correct with some care.
>
> Index: ul.c
> ===
> RCS file: /cvs/src/usr.bin/ul/ul.c,v
> retrieving revision 1.19
> diff -u -p -r1.19 ul.c
> --- ul.c10 Oct 2015 16:15:03 -  1.19
> +++ ul.c23 Oct 2015 13:31:45 -
> @@ -151,6 +151,12 @@ main(int argc, char *argv[])
> exit(0);
>  }
>
> +int
> +isu8cont(unsigned char c)
> +{
> +   return (c & (0x80 | 0x40)) == 0x80;
> +}
> +
>  void
>  mfilter(FILE *f)
>  {
> @@ -158,8 +164,11 @@ mfilter(FILE *f)
>
> while ((c = getc(f)) != EOF && col < MAXBUF) switch(c) {
> case '\b':
> -   if (col > 0)
> +   while (col > 0) {
> col--;
> +   if (!isu8cont(obuf[col].c_char))
> +   break;

Should this check also be run in case of non-UTF-8 locale (read: "C" one)?

> +   }
> continue;
> case '\t':
> col = (col+8) & ~07;
> @@ -241,6 +250,8 @@ mfilter(FILE *f)
> obuf[col].c_mode |= BOLD|mode;
> else
> obuf[col].c_mode = mode;
> +   if ((c & (0x80 | 0x40)) == 0x80 && col > 0)
> +   obuf[col].c_mode = obuf[col - 1].c_mode;
> col++;
> if (col > maxcol)
> maxcol = col;
>

--
  WBR,
  Vadim Zhukov

Re: does anoybody use ul?

[Vadim Zhukov]

[col].c_char == '_') {
> obuf[col].c_char = c;
> obuf[col].c_mode |= UNDERL|mode;
> -   } else if (obuf[col].c_char == c)
> +   } else if (obuf[col].c_char == (char)c)

Did you actuall wanted "(unsigned char)c" here?

> obuf[col].c_mode |= BOLD|mode;
> else
> obuf[col].c_mode = mode;
> +   if (col > 0 && isu8cont(c))
> +   obuf[col].c_mode = obuf[col - 1].c_mode;

Is this right? Shouldn't it just ignore c_mode changing in
isu8cont(c)==1 case, and on isu8cont(c)==0 case go back through all
isu8cont()==1 characters and set their mode based on non-cont
character following them?

> col++;
> if (col > maxcol)
> maxcol = col;
>

--
  WBR,
  Vadim Zhukov

Fwd: Allow bioctl to go through all controllers at once

[Vadim Zhukov]

ping?
--
  WBR,
  Vadim Zhukov



-- Forwarded message --
From: Vadim Zhukov <persg...@gmail.com>
Date: 2015-10-01 21:59 GMT+03:00
Subject: Allow bioctl to go through all controllers at once
To: tech@openbsd.org


Hi all.

I've recently found that this patch still produces M's in my tree.
What it does is going through all bio(4)-enabled controllers in system,
like ifconfig -A does. I didn't add SMALL_KERNEL ifdefs since its very
useful on ramdisks, IMHO, but I don't insist on that.

Any objections/okays/showers?

--
WBR,
  Vadim Zhukov


Index: sys/dev/bio.c
===
RCS file: /cvs/src/sys/dev/bio.c,v
retrieving revision 1.17
diff -u -p -r1.17 bio.c
--- sys/dev/bio.c   26 Aug 2015 22:28:57 -  1.17
+++ sys/dev/bio.c   1 Oct 2015 18:53:18 -
@@ -52,6 +52,7 @@ int   bioopen(dev_t, int, int, struct proc
 intbio_delegate_ioctl(struct bio_mapping *, u_long, caddr_t);
 struct bio_mapping *bio_lookup(char *);
 intbio_validate(void *);
+intbio_listcontrollers(struct bioc_controllerlist *);

 void
 bioattach(int nunits)
@@ -89,6 +90,9 @@ bioioctl(dev_t dev, u_long cmd, caddr_t
return (ENOENT);
break;

+   case BIOCLISTCONTROLLERS:
+   return (bio_listcontrollers((struct bioc_controllerlist*)addr));
+
case BIOCINQ:
case BIOCDISK:
case BIOCVOL:
@@ -138,6 +142,32 @@ bio_unregister(struct device *dev)
free(bm, M_DEVBUF, sizeof(*bm));
}
}
+}
+
+int
+bio_listcontrollers(struct bioc_controllerlist *bcl) {
+   struct bio_mapping *bm;
+   int error, i;
+
+   if (bcl->bcl_size < 0)
+   return EINVAL;
+   if (bcl->bcl_size == 0) {
+   LIST_FOREACH(bm, , bm_link)
+   bcl->bcl_size++;
+   return 0;
+   }
+   i = 0;
+   LIST_FOREACH(bm, , bm_link) {
+   error = copyoutstr(bm->bm_dev->dv_xname,
+   bcl->bcl_list[i].bc_xname,
+   sizeof(bcl->bcl_list[i].bc_xname), NULL);
+   if (error)
+   return (error);
+   if (++i == bcl->bcl_size)
+   break;
+   }
+   bcl->bcl_size = i;
+   return 0;
 }

 struct bio_mapping *
Index: sys/dev/biovar.h
===
RCS file: /cvs/src/sys/dev/biovar.h,v
retrieving revision 1.44
diff -u -p -r1.44 biovar.h
--- sys/dev/biovar.h29 May 2015 00:33:37 -  1.44
+++ sys/dev/biovar.h1 Oct 2015 18:53:18 -
@@ -277,6 +277,15 @@ struct bioc_patrol {
int bp_autonow;
 };

+#define BIOCLISTCONTROLLERS _IOWR('B', 43, struct bioc_controllerlist)
+struct bioc_controller {
+   charbc_xname[16];
+};
+struct bioc_controllerlist {
+   struct bioc_controller  *bcl_list;
+   int  bcl_size;
+};
+
 /* kernel and userspace defines */
 #define BIOC_INQ   0x0001
 #define BIOC_DISK  0x0002
Index: sbin/bioctl/bioctl.c
===
RCS file: /cvs/src/sbin/bioctl/bioctl.c,v
retrieving revision 1.129
diff -u -p -r1.129 bioctl.c
--- sbin/bioctl/bioctl.c18 Jul 2015 23:23:20 -  1.129
+++ sbin/bioctl/bioctl.c1 Oct 2015 18:53:18 -
@@ -69,7 +69,8 @@ void  bio_kdf_generate(struct sr_crypto
 void   derive_key_pkcs(int, u_int8_t *, size_t, u_int8_t *,
size_t, char *, int);

-void   bio_inq(char *);
+void   bio_listall();
+void   bio_inq(char *, int);
 void   bio_alarm(char *);
 intbio_getvolbyname(char *);
 void   bio_setstate(char *, int, char *);
@@ -110,12 +111,17 @@ main(int argc, char *argv[])
u_int16_t   cr_level = 0;
int biodev = 0;

-   if (argc < 2)
-   usage();
+   if (argc < 2) {
+   bio_listall();
+   return 0;
+   }

-   while ((ch = getopt(argc, argv, "a:b:C:c:dH:hik:l:O:Pp:qr:R:st:u:v")) !=
+   while ((ch = getopt(argc, argv,
"Aa:b:C:c:dH:hik:l:O:Pp:qr:R:st:u:v")) !=
-1) {
switch (ch) {
+   case 'A':
+   bio_listall();
+   return 0;
case 'a': /* alarm */
func |= BIOC_ALARM;
al_arg = optarg;
@@ -243,7 +249,7 @@ main(int argc, char *argv[])
} else if (changepass && !biodev) {
bio_changepass(devicename);
} else if (func & BIOC_INQ) {
-   bio_inq(devicename);
+   bio_inq(devicename, 0);
} else if (func == BIOC_ALARM) {
   

Re: iwm: set mbuf pointers to NULL after free

[Vadim Zhukov]

10 окт. 2015 г. 11:45 пользователь "Stefan Sperling" <s...@stsp.name>
написал:
>
> Just in case. NULL derefs are easier to find than use-after-frees.
>
> Index: if_iwm.c
> ===
> RCS file: /cvs/src/sys/dev/pci/if_iwm.c,v
> retrieving revision 1.53
> diff -u -p -r1.53 if_iwm.c
> --- if_iwm.c6 Oct 2015 09:12:00 -   1.53
> +++ if_iwm.c10 Oct 2015 08:35:08 -
> @@ -1062,6 +1062,7 @@ iwm_free_rx_ring(struct iwm_softc *sc, s
> data->map->dm_mapsize, BUS_DMASYNC_POSTREAD);
> bus_dmamap_unload(sc->sc_dmat, data->map);
> m_freem(data->m);
> +   data->m = NULL;
> }
> if (data->map != NULL)
> bus_dmamap_destroy(sc->sc_dmat, data->map);
> @@ -1169,6 +1170,7 @@ iwm_free_tx_ring(struct iwm_softc *sc, s
> data->map->dm_mapsize, BUS_DMASYNC_POSTWRITE);
> bus_dmamap_unload(sc->sc_dmat, data->map);
> m_freem(data->m);
> +   data->m = NULL;
> }
>         if (data->map != NULL)
> bus_dmamap_destroy(sc->sc_dmat, data->map);

Agree. Okay zhuk@.

--
Vadim Zhukov

Re: iwm(4) lladdr tweak

[Vadim Zhukov]

06 окт. 2015 г. 0:00 пользователь "Stefan Sperling" <s...@stsp.name>
написал:
>
> This matches what all other wifi drivers seem to be doing.
> ifconfig iwm0 lladdr random still works.
>
> ok?
>
> Index: if_iwm.c
> ===
> RCS file: /cvs/src/sys/dev/pci/if_iwm.c,v
> retrieving revision 1.52
> diff -u -p -r1.52 if_iwm.c
> --- if_iwm.c5 Oct 2015 13:05:08 -   1.52
> +++ if_iwm.c5 Oct 2015 20:53:23 -
> @@ -5729,11 +5729,6 @@ iwm_ioctl(struct ifnet *ifp, u_long cmd,
> error = 0;
> break;
>
> -   case SIOCSIFLLADDR:
> -   IEEE80211_ADDR_COPY(sc->sc_ic.ic_myaddr,
> -   ((struct arpcom *)ifp)->ac_enaddr);
> -   break;
> -
> default:
> error = ieee80211_ioctl(ifp, cmd, data);
> }
> @@ -6323,8 +6318,12 @@ iwm_preinit(struct iwm_softc *sc)
> return error;
> }
>
> -   if (attached)
> +   if (attached) {
> +   /* Update MAC in case the upper layers changed it. */
> +   IEEE80211_ADDR_COPY(sc->sc_ic.ic_myaddr,
> +   ((struct arpcom *)ifp)->ac_enaddr);
> return 0;
> +   }
>
> if ((error = iwm_start_hw(sc)) != 0) {
> printf("%s: could not initialize hardware\n",
DEVNAME(sc));
>

Looks like a logical things for me.

--
Vadim Zhukov

Re: bsd.port.mk.5: restructure `clean' section

[Vadim Zhukov]

2015-09-26 21:49 GMT+03:00 Michael Reed <m.r...@mykolab.com>:
> The `clean' target takes optional arguments, so denote that in
> the item header.  Additionally, the subtargets are fixed strings,
> not variables, so change the use of Va to Cm to reflect that.

Well, technically "clean=" is not a target, but a hack that looks like
a parametrized target. As a result, the ordering in the following
cases will differ, and that matters:

make package clean
make package clean=work

Even worse, the wording is confusing, because the term "subtarget" is
used in other parts of this manual page with a different semantics.

Can't comment on mdoc macro choosing, sorry.

> Index: bsd.port.mk.5
> ===
> RCS file: /cvs/src/share/man/man5/bsd.port.mk.5,v
> retrieving revision 1.426
> diff -u -p -r1.426 bsd.port.mk.5
> --- bsd.port.mk.5   23 Sep 2015 01:38:36 -  1.426
> +++ bsd.port.mk.5   26 Sep 2015 18:46:05 -
> @@ -178,47 +178,49 @@ will try to fetch a version with the cor
>  main archive site in the case of a checksum mismatch.
>  .Ev NO_CHECKSUM
>  can be used to avoid all checksumming steps.
> -.It Cm clean
> +.It Cm clean Ns Op Pf = Ar subtarget
>  Clean ports contents.
> -By default, it will clean the work directory.
> -It can be invoked as
> -make clean='[depends build bulk work fake flavors dist install sub package
> -packages plist]'.
> +If
> +.Ar subtarget
> +is omitted, it will clean the work directory.
> +Otherwise,
> +.Ar subtarget
> +can be set to one of the following:
>  .Bl -tag -width packages
> -.It Va work
> +.It Cm work
>  Clean work directory.
> -.It Va bulk
> +.It Cm bulk
>  Clean bulk cookie.
> -.It Va build
> +.It Cm build
>  Clean the
>  .Va WRKBUILD
>  directory (only useful if
>  .Va SEPARATE_BUILD
>  is set).
> -.It Va depends
> +.It Cm depends
>  Recurse into dependencies.
> -.It Va dist
> +.It Cm dist
>  Clean distribution files.
> -.It Va fake
> +.It Cm fake
>  Clean fake installation directory.
> -.It Va flavors
> +.It Cm flavors
>  Clean all work directories.
> -.It Va install
> +.It Cm install
>  Uninstall package.
> -.It Va package
> +.It Cm package
>  Remove all copies of package file.
> -.It Va plist
> +.It Cm plist
>  Remove registered packing lists of all subpackages.
> -.It Va sub
> +.It Cm sub
>  With
> -.Va install
> +.Cm install
>  or
> -.Va package ,
> +.Cm package ,
>  clean subpackages as well.
> -.It Va packages
> +.It Cm packages
>  Shorthand for
>  .Sq sub package .
> -.It Va all
> +.It Cm all
>  Shorthand for
>  .Sq work flavors packages plist .
>  .El
>

--
  WBR,
  Vadim Zhukov