Re: malloc: 1st small step in long way to multiple pools

[Michael McConville]

Otto Moerbeek wrote:
> a future goal for malloc is to use multiple pools in threaded
> environments, to reduce lock contention. 
> 
> This is a small first step towards that goal: move two globals to the
> pool-specific struct dir_info. Currently there's only a single pool,
> but that will change one day.
> 
> Lightly tested by myself on amd64, you can help by reviewing and
> testing this.

Thanks for this - I've been meaning to study the pool logic more.

One minor thing I noticed:

> Index: malloc.c
> ===
> RCS file: /cvs/src/lib/libc/stdlib/malloc.c,v
> retrieving revision 1.182
> diff -u -p -r1.182 malloc.c
> --- malloc.c  25 Feb 2016 00:38:51 -  1.182
> +++ malloc.c  9 Mar 2016 08:31:52 -
> @@ -93,13 +93,13 @@
>  #define MQUERY(a, sz)mquery((a), (size_t)(sz), PROT_READ | 
> PROT_WRITE, \
>  MAP_ANON | MAP_PRIVATE | MAP_FIXED, -1, (off_t)0)
>  
> -#define _MALLOC_LEAVE() if (__isthreaded) do { \
> - malloc_active--; \
> +#define _MALLOC_LEAVE(d) if (__isthreaded) do { \
> + (d)->active--; \
>   _MALLOC_UNLOCK(); \
>  } while (0)
> -#define _MALLOC_ENTER() if (__isthreaded) do { \
> +#define _MALLOC_ENTER(d) if (__isthreaded) do { \
>   _MALLOC_LOCK(); \
> - malloc_active++; \
> + (d)->active++; \
>  } while (0)

Are you sure these are guarded properly? I think the do-while block
should wrap the entire thing, including the if condition. Otherwise,
else conditions can still associate with the if.

Re: head(1) -c

[lists]

> To repeat myself, the addition of this rather silly option is supposed
> to reduce differences from other implementations so that we can stop
> wasting time about it.  

It should be cool to be able to run scripts that are expected to run so
fine on the other POSIX targeting systems.  So, push them to follow the
standard or even better fix the standard.  Why deviate from the rest of
the hurd if it's POSIX that's at fault?  And changes are changeable too.

Re: opendev(3) tweak

[Theo de Raadt]

I am compelled to add two throughts about opendev() and pledge:

Beforehands, please read src/lib/libutil/opendev.c

I am not saying opendev is wrong, the design of opening a master
device, doing an ioctl, and then finding the correct device to
actually open was very expedient, DUID development was taking a long
time, and we needed to get this step in place quickly.

Before opendev, all these programs would get to their device by calling
open().  It was atomic, but there was all sorts of gunk around the device
naming and handling.  So opendev() arrived, moving a lot of boilerplate
into one library function, and it was still atomic.

Then this diskmap / ioctl got added, and it was... no longer atomic.

Reminds me of something from a long time ago!

http://homepage.ntlworld.com/jonathan.deboynepollard/FGA/bernstein-on-ttys/security.html
then SVR4 did something to "fix it"...
and then history repeated itself, just a month ago
http://marc.info/?t=14562294971=1=2

Now please read fdisk and disklabel source.  With pledge, both follow
the pattern of:

pledge
[a bunch of getopt]
opendev

I would argue the pledge call in most of those programs is premature,
and should be placed after the opendev() call.  I don't see much being
protected by the early pledge, because that opendev is going to open a
raw disk device.  "Premature protection"?

(Years ago, at least fdisk and disklabel had a need to open disks "late",
I think Ken fixed that in all these programs, to allow them to be pledged).

Anyways, I think a study of the programs in the tree that call opendev
is due, and maybe just moving the pledge calls is the solution?  Dunno.

Re: head(1) -c

[Jeremie Courreges-Anglas]

"Dmitrij D. Czarkoff"  writes:

> Jeremie Courreges-Anglas said:
>> To repeat myself, the addition of this rather silly option is supposed
>> to reduce differences from other implementations so that we can stop
>> wasting time about it.
>
> There is a big difference between providing a switch for compatibility
> and following specific behavior resulting from outright stupid use of
> utility.

Quite strong words for such a tiny detail.

> There is a limit even to compatibility effort

Whose effort is it?  The code is there and is dead simple.  I spent
perhaps 5mn writing it and I feel like I've already wasted hours in
this bikeshed session.

> - we aren't
> going to implement GNU coreutils' long options, right?

What is it, some slight variation on Godwin's law?

> In this particular case trying to pick between "-n" and "-c" flags is
> just wrong: you can't want both X lines and Y bytes at the same time.
> If your script does this, you should fix the script.  If port's script
> does it, fix it and upstream the fix.  We are already doing that for
> similar bugs.

I say you're over-thinking it.

Thanks for the advice about pushing patches upstream. :)

-- 
jca | PGP : 0x1524E7EE / 5135 92C1 AD36 5293 2BDF  DDCC 0DFA 74AE 1524 E7EE

Probable logic error in st(4)

[Michael McConville]

sys/scsi/st.c:1024 uses:

> cmd->len != 0

as a condition. However, cmd is of type struct scsi_rw_tape, and its len
field is of type u_int8_t len[3]. Therefore, the condition is always
true because len is treated as a pointer.

Clang warns about this.

Thanks for your time,
Mike

Re: opendev(3) tweak

[Theo de Raadt]

> On Thu, Mar 10, 2016 at 08:48:21AM -0700, Theo de Raadt wrote:
> > The reason for these checks is because they protect the kernel,
> > and they identify a program that does the wrong thing.  Here, a
> > program did the wrong thing.  I am 100% in agreement that opendev
> > may not be the right place to do this.  That kind of stems from
> > the design of opendev regarding DUID conversion  I think we all
> > knew that wasn't the best design early on, but we needed to get
> > that going, before the rest of the DUID subsystem could work...
> 
> 
> 
> If it's opendev that does the check, where should it be done ? Right now
> krw added checks in both disklabel AND fdisk to prevent the issue from
> coming up.

Is it ugly for a program to check it is doing an ioctl against a
device before it does so?  Perhaps.

Perhaps we can delete the other 40 or so fstat calls in the tree
which are done to verify these circumstances?  Perhaps not.

I don't have an answer for you.

> BUT both those do opendev upfront, so it's really duplicated code.

Hang on, I am not arguing against doing it in opendev(3).  I don't
where to do it.

I will argue that what opendev(3) does - with ioctl DIOCMAP -- is
non-atomic and ugly.  But I recall why we did this, we needed a quick
step to make DUIDs work, and this was the easiest way to do it.  Maybe
we are ready for a different way.

So why is DIOCMAP whitelisted to a kill like that?  ioctl is a system
call backed by asstons of kernel code, essentially any ioctl operation
can be attempted against any fd and will call through at minimum 2
layers of kernel code, probably maximum 7, before unwinding if the
subsystem does not support that "32 bit request".  So that is why
pledge is so aggressively whitelisting, and blocks towards a kill --
so we can FIND THESE SITUATIONS rather than watching errors being
ignored, so that we can identify problems, and learn how to adjust the
entire system towards the new "subset of POSIX" frame of mind.

Maybe this ioctl should return an error instead of killing.  But we
would not have known that this one should return an error, BEFORE
the first instance reporting it.

The handful of people who worked on the pledge kernel code just cannot
anticipate everything, and 5.9 (and probably 6.0) will ship with a few
issues like this.  (this is a termite hill sized for 6 inhabitants)

> The other thing that (in my opinion) makes sense would be to duplicate the
> way ttys are handled, e.g., we've got a isatty  library call that validates
> an fd so that further tty(4) ioctl will work.   Maybe this is what's needed,
> or something similar.

Hang on, isatty is a very special case. For those who don't know isatty(3)
is backed by fcntl(2) using F_ISATTY, which is always allowed by pledge(2)
with the "stdio" request, so it is not an accurate example.  Moving it to
fcntl(2) was a foundation of pledge...

Yes in a few places we put a isatty(3) before a tty ioctl, but this is
because the legacy code we were dealing with shares lower level functions
between "file" and "tty" ops.  The "tty" vs "stdout" abstraction is a
big part of unix.

DIOCMAP?  Not so much.

> I don't think leaving the check to the individual programs is a good solution.

It is proposed for 2 programs now.  There are 20+? users of opendev?
I suggest checking a few more, then we'll know for sure.

> I just stumbled on fdisk/disklabel being slightly broken by accident... after
> a few months of pledge(2) testing.

And what did you expect?  It shows that there is insufficient testing
of the entire utility base, to verify all semantics are correct.  We are
talking almost 800 programs, probably with argument choices there are
10,000 usage paterns.  Some of the issues we fix in programs, some of them
we fix in library routines, and some we fix in the pledge layer.  We'll
get there.

I still don't know which this is, but /dev/diskmap + DIOCMAP is still
a gross artifact.

Re: opendev(3) tweak

[Marc Espie]

On Thu, Mar 10, 2016 at 08:48:21AM -0700, Theo de Raadt wrote:
> The reason for these checks is because they protect the kernel,
> and they identify a program that does the wrong thing.  Here, a
> program did the wrong thing.  I am 100% in agreement that opendev
> may not be the right place to do this.  That kind of stems from
> the design of opendev regarding DUID conversion  I think we all
> knew that wasn't the best design early on, but we needed to get
> that going, before the rest of the DUID subsystem could work...



If it's opendev that does the check, where should it be done ? Right now
krw added checks in both disklabel AND fdisk to prevent the issue from
coming up.

BUT both those do opendev upfront, so it's really duplicated code.

The other thing that (in my opinion) makes sense would be to duplicate the
way ttys are handled, e.g., we've got a isatty  library call that validates
an fd so that further tty(4) ioctl will work.   Maybe this is what's needed,
or something similar.

I don't think leaving the check to the individual programs is a good solution.

I just stumbled on fdisk/disklabel being slightly broken by accident... after
a few months of pledge(2) testing.

Re: head(1) -c

[Theo de Raadt]

> Jeremie Courreges-Anglas said:
> > To repeat myself, the addition of this rather silly option is supposed
> > to reduce differences from other implementations so that we can stop
> > wasting time about it.
> 
> There is a big difference between providing a switch for compatibility
> and following specific behavior resulting from outright stupid use of
> utility.  There is a limit even to compatibility effort - we aren't
> going to implement GNU coreutils' long options, right?
> 
> In this particular case trying to pick between "-n" and "-c" flags is
> just wrong: you can't want both X lines and Y bytes at the same time.
> If your script does this, you should fix the script.  If port's script
> does it, fix it and upstream the fix.  We are already doing that for
> similar bugs.

One could argue the existance of various systems which handle this
differently...

Helps find such misuses...

And keeps scripts portable, so that systems which handle this
differently don't get burned.

So the question becomes:  What is the PURE BENEFIT of being
purist here, relative to the possibilty of breaking a script
used in other systems?

The argument is a lot like putting #!/bin/bash at the top of
scripts.

Re: head(1) -c

[Dmitrij D. Czarkoff]

Jeremie Courreges-Anglas said:
> To repeat myself, the addition of this rather silly option is supposed
> to reduce differences from other implementations so that we can stop
> wasting time about it.

There is a big difference between providing a switch for compatibility
and following specific behavior resulting from outright stupid use of
utility.  There is a limit even to compatibility effort - we aren't
going to implement GNU coreutils' long options, right?

In this particular case trying to pick between "-n" and "-c" flags is
just wrong: you can't want both X lines and Y bytes at the same time.
If your script does this, you should fix the script.  If port's script
does it, fix it and upstream the fix.  We are already doing that for
similar bugs.

-- 
Dmitrij D. Czarkoff

Re: hang with processes in fltamap: how can I identify running out of RAM?

[Stefan Kempf]

Stefan Kempf wrote:
> Stuart Henderson wrote:
> > On 2016/03/09 20:49, Stefan Kempf wrote:
> > > Here's a diff that allocates the most commonly used amap slot sizes
> > > (between 1 and 16) using pool_get(9) instead of malloc(9). That should
> > > reduce the pressure on kernel virtual address space somewhat (on amd64
> > > at least),
> > 
> > Thanks for the useful information and diff.
> > 
> > I'm running a full ports build on i386 now with this in the kernel,
> > and will run some more of my memory-hungry jobs on amd64 tomorrow.
>  
> I looked at the diff again and unfortunately there is a bug in the
> first version. slotalloc was computed incorrectly in amap_extend()
> when using pool(9).
> 
> That code path seems to not be executed often though
> and can only be triggered with certain allocation sizes (maybe the
> i386 ports build survives even with the first diff).
> 
> But please try the one below on amd64 if possible.

There's still at least one issue with the diff. Again in amap_extend().
The slotalloc computation was still off :-(

Index: uvm/uvm_amap.c
===
RCS file: /cvs/src/sys/uvm/uvm_amap.c,v
retrieving revision 1.60
diff -u -p -r1.60 uvm_amap.c
--- uvm/uvm_amap.c  6 Mar 2016 14:47:07 -   1.60
+++ uvm/uvm_amap.c  10 Mar 2016 18:15:17 -
@@ -52,8 +52,13 @@
 
 struct pool uvm_amap_pool;
 
+/* Pools for amap slots for the most common amap slot sizes */
+struct pool uvm_amap_slot_pools[UVM_AMAP_CHUNK];
+
 LIST_HEAD(, vm_amap) amap_list;
 
+static char amap_slot_pool_names[UVM_AMAP_CHUNK][13];
+
 #define MALLOC_SLOT_UNIT (2 * sizeof(int) + sizeof(struct vm_anon *))
 
 /*
@@ -151,10 +156,20 @@ pp_setreflen(int *ppref, int offset, int
 void
 amap_init(void)
 {
+   int i;
+
/* Initialize the vm_amap pool. */
pool_init(_amap_pool, sizeof(struct vm_amap), 0, 0, PR_WAITOK,
"amappl", NULL);
pool_sethiwat(_amap_pool, 4096);
+
+   for (i = 0; i < nitems(uvm_amap_slot_pools); i++) {
+   snprintf(amap_slot_pool_names[i],
+   sizeof(amap_slot_pool_names[0]), "amapslotpl%d", i + 1);
+   pool_init(_amap_slot_pools[i], (i + 1) * MALLOC_SLOT_UNIT,
+   0, 0, PR_WAITOK, amap_slot_pool_names[i], NULL);
+   pool_sethiwat(_amap_slot_pools[i], 4096);
+   }
 }
 
 /*
@@ -172,8 +187,13 @@ amap_alloc1(int slots, int padslots, int
if (amap == NULL)
return(NULL);
 
-   totalslots = malloc_roundup((slots + padslots) * MALLOC_SLOT_UNIT) /
-   MALLOC_SLOT_UNIT;
+   totalslots = slots + padslots;
+   KASSERT(totalslots > 0);
+
+   if (totalslots > UVM_AMAP_CHUNK)
+   totalslots = malloc_roundup(totalslots * MALLOC_SLOT_UNIT) /
+   MALLOC_SLOT_UNIT;
+
amap->am_ref = 1;
amap->am_flags = 0;
 #ifdef UVM_AMAP_PPREF
@@ -183,8 +203,14 @@ amap_alloc1(int slots, int padslots, int
amap->am_nslot = slots;
amap->am_nused = 0;
 
-   amap->am_slots = malloc(totalslots * MALLOC_SLOT_UNIT, M_UVMAMAP,
-   waitf);
+   if (totalslots > UVM_AMAP_CHUNK)
+   amap->am_slots = malloc(totalslots * MALLOC_SLOT_UNIT,
+   M_UVMAMAP, waitf);
+   else
+   amap->am_slots = pool_get(
+   _amap_slot_pools[totalslots - 1],
+   (waitf == M_WAITOK) ? PR_WAITOK : PR_NOWAIT);
+
if (amap->am_slots == NULL)
goto fail1;
 
@@ -238,7 +264,12 @@ amap_free(struct vm_amap *amap)
KASSERT(amap->am_ref == 0 && amap->am_nused == 0);
KASSERT((amap->am_flags & AMAP_SWAPOFF) == 0);
 
-   free(amap->am_slots, M_UVMAMAP, 0);
+   if (amap->am_maxslot > UVM_AMAP_CHUNK)
+   free(amap->am_slots, M_UVMAMAP, 0);
+   else
+   pool_put(_amap_slot_pools[amap->am_maxslot - 1],
+   amap->am_slots);
+
 #ifdef UVM_AMAP_PPREF
if (amap->am_ppref && amap->am_ppref != PPREF_NONE)
free(amap->am_ppref, M_UVMAMAP, 0);
@@ -324,8 +355,12 @@ amap_extend(struct vm_map_entry *entry, 
if (slotneed >= UVM_AMAP_LARGE)
return E2BIG;
 
-   slotalloc = malloc_roundup(slotneed * MALLOC_SLOT_UNIT) /
-   MALLOC_SLOT_UNIT;
+   if (slotneed > UVM_AMAP_CHUNK)
+   slotalloc = malloc_roundup(slotneed * MALLOC_SLOT_UNIT) /
+   MALLOC_SLOT_UNIT;
+   else
+   slotalloc = slotneed;
+
 #ifdef UVM_AMAP_PPREF
newppref = NULL;
if (amap->am_ppref && amap->am_ppref != PPREF_NONE) {
@@ -338,8 +373,12 @@ amap_extend(struct vm_map_entry *entry, 
}
}
 #endif
-   newsl = malloc(slotalloc * MALLOC_SLOT_UNIT, M_UVMAMAP,
-   M_WAITOK | M_CANFAIL);
+   if (slotneed > UVM_AMAP_CHUNK)
+   newsl = malloc(slotalloc * MALLOC_SLOT_UNIT, M_UVMAMAP,
+   M_WAITOK | M_CANFAIL);
+ 

Re: Xorg stipple

[Aaron Bieber]

joshua stein writes:

> Is anyone seriously finding video/Xorg bugs through the default X
> stipple pattern anymore?  Xorg changed the default to draw a black
> background a while ago (with stipple enabled using the -retro flag),
> but we have this local change that reverted it while adding a silly
> -retard flag in order to show the black background.
>
> I think we can finally stop partying like it's 1989 (vax is dead,
> after all) and have X show a solid black background by default.

While on this topic, I recently "synced" the look of ssh-askpass with
the semi-recent changes for the XDM login screen:

ssh-askpass*font:   
-adobe-helvetica-medium-r-normal--18-180-75-75-p-98-iso8859-1
ssh-askpass*foreground: #ff
ssh-askpass*background: #798a99
ssh-askpass.*.borderWidth:  1
ssh-askpass.*.borderColor:  black
ssh-askpass.*.shadowThickness:  0
ssh-askpass.*Button.borderWidth:0
ssh-askpass.*Button.background: #99aab9
ssh-askpass.indicator.borderWidth:  0
ssh-askpass.indicator.background:   #99aab9
ssh-askpass.indicator.foreground:   #fffa06

Not suggesting we modify it, but figured someone else might be
interested :D

OpenSSH Security Advisory: xauth command injection

[Damien Miller]

OpenSSH Security Advisory: x11fwd.adv

This document may be found at: http://www.openssh.com/txt/x11fwd.adv

1. Affected configurations

All versions of OpenSSH prior to 7.2p2 with X11Forwarding
enabled.

2. Vulnerability

Missing sanitisation of untrusted input allows an
authenticated user who is able to request X11 forwarding
to inject commands to xauth(1).

Injection of xauth commands grants the ability to read
arbitrary files under the authenticated user's privilege,
Other xauth commands allow limited information leakage,
file overwrite, port probing and generally expose xauth(1),
which was not written with a hostile user in mind, as an
attack surface.

xauth(1) is run under the user's privilege, so this
vulnerability offers no additional access to unrestricted
accounts, but could circumvent key or account restrictions
such as sshd_config ForceCommand, authorized_keys
command="..." or restricted shells.

3. Mitigation

Set X11Forwarding=no in sshd_config. This is the default.

For authorized_keys that specify a "command" restriction,
also set the "restrict" (available in OpenSSH >=7.2) or
"no-x11-forwarding" restrictions.

4. Details

As part of establishing an X11 forwarding session, sshd(8)
accepts an X11 authentication credential from the client.
This credential is supplied to the xauth(1) utility to
establish it for X11 applications that the user subsequently
runs.

The contents of the credential's components (authentication
scheme and credential data) were not sanitised to exclude
meta-characters such as newlines. An attacker could
therefore supply a credential that injected commands to
xauth(1). The attacker could then use a number of xauth
commands to read or overwrite arbitrary files subject to
file permissions, connect to local ports or perform attacks
on xauth(1) itself.

OpenSSH 7.2p2 implements a whitelist of characters that
are permitted to appear in X11 authentication credentials.

5. Credit

This issue was identified by github.com/tintinweb and
communicated to the OpenSSH developers on March 3rd, 2016.

6. Fix

Portable OpenSSH 7.2p2 contains a fix for this vulnerability.

Patches for supported OpenBSD releases (5.7, 5.8 and 5.9) have
been committed to the -STABLE branches and are available on the
errata pages:

http://www.openbsd.org/errata57.html
http://www.openbsd.org/errata58.html
http://www.openbsd.org/errata59.html

Re: Xorg stipple

[lists]

Wed, 9 Mar 2016 19:40:35 -0600 joshua stein 
> On Thu, 10 Mar 2016 at 03:32:53 +0200, li...@wrant.com wrote:
> > Apparently all you guys use laptops with LVDS, but TFT/LCD panels on
> > VGA are still widely common in use.  You remember the automatic adjust
> > functional button on these monitors, right?  It does phase and sync
> > adjust that saves a lot of fiddling.  It works best on a fine pattern
> > exactly like the stipple which we know is from old times but is that
> > pattern for the same reason, help you focus you CRT tubes.  Also, think
> > about focusing projectors that use VGA.  Same thing, focus is good to
> > have and the pattern still helps.  I'll comment on floppies another day.  
> 
> So start X with -retro for those specific times you need it.  This
> is a weak argument for forcing this default on everyone, every time.

Sure, no problem.  I'm not a conference presenter at the time.  Also,
we have xtestpicture in ports.  So, I agree with your proposal (though
you don't need it to proceed).

Re: Xorg stipple

[lists]

Wed, 9 Mar 2016 17:10:07 -0800 Michael McConville 
> Theo de Raadt wrote:
> > > > Is anyone seriously finding video/Xorg bugs through the default X
> > > > stipple pattern anymore?  Xorg changed the default to draw a black
> > > > background a while ago (with stipple enabled using the -retro flag),
> > > > but we have this local change that reverted it while adding a silly
> > > > -retard flag in order to show the black background.
> > > > 
> > > > I think we can finally stop partying like it's 1989 (vax is dead,
> > > > after all) and have X show a solid black background by default.  
> > > 
> > > The reason for this is to be able distinguish between:
> > > 
> > >   "I get a black screen because X is not working"
> > > 
> > > and
> > > 
> > >   "I get a black screen because my desktop environment is not working"  
> > 
> > And as we all can observe, this problem has become more severe over
> > the last decade.  
> 
> Can anyone think of a good option that isn't black but is less hideous
> than stipple? Even a dark navy or something like that.

Now mailing also the list, sorry for the triple message, Joshua.

Apparently all you guys use laptops with LVDS, but TFT/LCD panels on
VGA are still widely common in use.  You remember the automatic adjust
functional button on these monitors, right?  It does phase and sync
adjust that saves a lot of fiddling.  It works best on a fine pattern
exactly like the stipple which we know is from old times but is that
pattern for the same reason, help you focus you CRT tubes.  Also, think
about focusing projectors that use VGA.  Same thing, focus is good to
have and the pattern still helps.  I'll comment on floppies another day.

Re: Xorg stipple

[lists]

Thu, 10 Mar 2016 13:37:50 +0100 Matthieu Herrb 
> On Wed, Mar 09, 2016 at 05:09:13PM -0600, joshua stein wrote:
> > Is anyone seriously finding video/Xorg bugs through the default X
> > stipple pattern anymore?  Xorg changed the default to draw a black
> > background a while ago (with stipple enabled using the -retro flag),
> > but we have this local change that reverted it while adding a silly
> > -retard flag in order to show the black background.
> > 
> > I think we can finally stop partying like it's 1989 (vax is dead,
> > after all) and have X show a solid black background by default.  
> 
> I don't have a strong preference. If the team who prefer to get rid
> of the stipple (and seem to be the majority of expressed opinons)
> agree to take the extra step to try with -retro and/or check
> Xorg.0.log carefully before reporting problems to bugs@, I'm ok with
> dumping this extra local change.
> 
> And eventually the -retro option will be removed upstreams...

As long as the pattern is still available and usable, I'd prefer it
over any dark background which is worse.  Dark screens don't save
electricity, and we're not running CRTs daily, so our eyes are OK even
when using the pattern.  For this one can run xsetroot -def and over
time learn to appreciate IPS (or better) panels that don't flicker on
their PWM (free) brightness & contrast control and work OK on lower
settings.  In fact it is considered an important monitor testing and
selection feature, a quick glimpse and you can tell which is TN and PWM
flickering and not settle for it at all at right away, instead of years
after purchase.  Then, after all the test patterns are considered the
full time usage and the talking head is a minor interruption (and not
the other way round).  If you want to not ever see the pattern, OK,
just please don't rip out the way to have if for others that like it.

So, I don't have a strong preference either, as long as the pattern is
available and can be opted for after starting the server and cwm (twm),
or whatever.

> 
> > 
> > 
> > Index: dix/globals.c
> > ===
> > RCS file: /var/cvs/xenocara/xserver/dix/globals.c,v
> > retrieving revision 1.11
> > diff -u -p -u -p -r1.11 globals.c
> > --- dix/globals.c   16 Sep 2015 19:10:20 -  1.11
> > +++ dix/globals.c   9 Mar 2016 22:57:54 -
> > @@ -118,11 +118,7 @@ const char *defaultCursorFont = COMPILED
> >  FontPtr defaultFont;/* not declared in dix.h to avoid 
> > including font.h in
> > every compilation of dix code */
> >  CursorPtr rootCursor;
> > -#ifndef __OpenBSD__
> >  Bool party_like_its_1989 = FALSE;
> > -#else
> > -Bool party_like_its_1989 = TRUE;
> > -#endif
> >  Bool whiteRoot = FALSE;
> >  
> >  TimeStamp currentTime;
> > Index: os/utils.c
> > ===
> > RCS file: /var/cvs/xenocara/xserver/os/utils.c,v
> > retrieving revision 1.21
> > diff -u -p -u -p -r1.21 utils.c
> > --- os/utils.c  7 Nov 2015 16:48:53 -   1.21
> > +++ os/utils.c  9 Mar 2016 22:57:37 -
> > @@ -579,11 +579,7 @@ UseMsg(void)
> >  ErrorF("-r turns off auto-repeat\n");
> >  ErrorF("r  turns on auto-repeat \n");
> >  ErrorF("-render [default|mono|gray|color] set render color alloc 
> > policy\n");
> > -#ifndef __OpenBSD__
> >  ErrorF("-retro start with classic stipple and 
> > cursor\n");
> > -#else
> > -ErrorF("-retard   start with black background and no 
> > cursor\n");
> > -#endif
> >  ErrorF("-s #   screen-saver timeout (minutes)\n");
> >  ErrorF("-seat string   seat to run on\n");
> >  ErrorF("-t #   default pointer threshold 
> > (pixels/t)\n");
> > @@ -918,13 +914,8 @@ ProcessCommandLine(int argc, char *argv[
> >  defaultKeyboardControl.autoRepeat = TRUE;
> >  else if (strcmp(argv[i], "-r") == 0)
> >  defaultKeyboardControl.autoRepeat = FALSE;
> > -#ifndef __OpenBSD__
> > else if ( strcmp( argv[i], "-retro") == 0)
> >  party_like_its_1989 = TRUE;
> > -#else
> > -   else if ( strcmp( argv[i], "-retard") == 0)
> > -   party_like_its_1989 = FALSE;
> > -#endif
> >  else if (strcmp(argv[i], "-s") == 0) {
> >  if (++i < argc)
> >  defaultScreenSaverTime = ((CARD32) atoi(argv[i])) *  
> 

Re: Xorg stipple

[Artturi Alm]

On Wed, Mar 09, 2016 at 07:40:35PM -0600, joshua stein wrote:
> On Thu, 10 Mar 2016 at 03:32:53 +0200, li...@wrant.com wrote:
> > Apparently all you guys use laptops with LVDS, but TFT/LCD panels on
> > VGA are still widely common in use.  You remember the automatic adjust
> > functional button on these monitors, right?  It does phase and sync
> > adjust that saves a lot of fiddling.  It works best on a fine pattern
> > exactly like the stipple which we know is from old times but is that
> > pattern for the same reason, help you focus you CRT tubes.  Also, think
> > about focusing projectors that use VGA.  Same thing, focus is good to
> > have and the pattern still helps.  I'll comment on floppies another day.
> 
> So start X with -retro for those specific times you need it.  This
> is a weak argument for forcing this default on everyone, every time.
> 

So "$ startx -- -retro".

Solid black is the worst w/black background on xterm etc. when stacking,
not saying i don't feel embarrassed about it whenever anyone does see
the stipple, but it is also colored enough to use the display for some
light in a dark room.

If it is not going to be black, i hope atleast cwm default border colors
will be taken into consideration when choosing it.

-Artturi

Re: Xorg stipple

[Alexander Bluhm]

On Thu, Mar 10, 2016 at 08:00:43AM +0100, Martin Pieuchot wrote:
> The current status is just a bad software experience from the 80s.

Usually you complain about the 90s :-)

Personally I like the traditional X11 appearance.

bluhm

Re: opendev(3) tweak

[Theo de Raadt]

> So I think we need to narrow down the pledge(2) semantics a bit more
> with respect to ioctls.  I'm inclined to say that if a certain ioctl
> is allowed by pledge(2) it should not abort the program anymore but
> return an error like it would do if unpledged.  But perhaps we need to
> make that decision on a per-ioctl basis.

With tty ioctls, there are a few circumstances where the subsystem
returns error rather than killing.  Most of those are to satisfy
library routines which do the operation.

There was some significant grumpyniness when I added the first one of
those.  Now the error condition is visible to a caller, can be
checked, and what we have is a run-time deviation from "POSIX".

The reason for these checks is because they protect the kernel,
and they identify a program that does the wrong thing.  Here, a
program did the wrong thing.  I am 100% in agreement that opendev
may not be the right place to do this.  That kind of stems from
the design of opendev regarding DUID conversion  I think we all
knew that wasn't the best design early on, but we needed to get
that going, before the rest of the DUID subsystem could work...

Re: opendev(3) tweak

[Theo de Raadt]

> Checks like the one you introduce here suffer from TOCTOU.

I don't see that.  It is not a stat, it is an fstat.  The descriptor
opened early, remains the same type through the whole operation.

Re: opendev(3) tweak

[Mark Kettenis]

> Date: Thu, 10 Mar 2016 12:52:35 +0100
> From: Marc Espie 
> 
> Already shown to a few people, but since pledge(2) aborts on non-dev, let's
> check upfront that we're of the right type.
> 
> I don't think this requires a bump. It doesn't really change the interface,
> just makes it stricter.

EFTYPE really isn't the appropriate errno value to use.  I'd say it
should return ENOTTY just as the failed ioctl would do in the
unpledged case.

I do believe that this needs to be fixed in pledge(2).  Checks like
the one you introduce here suffer from TOCTOU.  Perhaps that isn't
such a serious issue in this case, but there will be cases where it
does matter.

So I think we need to narrow down the pledge(2) semantics a bit more
with respect to ioctls.  I'm inclined to say that if a certain ioctl
is allowed by pledge(2) it should not abort the program anymore but
return an error like it would do if unpledged.  But perhaps we need to
make that decision on a per-ioctl basis.


> Index: opendev.3
> ===
> RCS file: /build/data/openbsd/cvs/src/lib/libutil/opendev.3,v
> retrieving revision 1.22
> diff -u -p -r1.22 opendev.3
> --- opendev.3 15 Jan 2015 19:06:32 -  1.22
> +++ opendev.3 10 Mar 2016 11:51:27 -
> @@ -93,7 +93,9 @@ it is modified to point at the fully exp
>  The
>  .Fn opendev
>  return value and errors are the same as the return value and errors of
> -.Xr open 2 .
> +.Xr open 2 ,
> +plus
> +.Er EFTYPE .
>  .Sh SEE ALSO
>  .Xr open 2 ,
>  .Xr getrawpartition 3 ,
> Index: opendev.c
> ===
> RCS file: /build/data/openbsd/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 8 Mar 2016 22:30:44 -
> @@ -38,6 +38,7 @@
>  #include 
>  #include 
>  #include 
> +#include 
>  
>  #include "util.h"
>  
> @@ -50,6 +51,7 @@ opendev(const char *path, int oflags, in
>  {
>   static char namebuf[PATH_MAX];
>   struct dk_diskmap dm;
> + struct stat st;
>   char *slash, *prefix;
>   int fd;
>  
> @@ -106,5 +108,17 @@ opendev(const char *path, int oflags, in
>   if (realpath)
>   *realpath = namebuf;
>  
> + if (fd != -1) {
> + if (fstat(fd, ) == -1) {
> + close(fd);
> + return (-1);
> + }
> + if ((dflags & OPENDEV_BLCK) ? !S_ISBLK(st.st_mode) :
> + !S_ISCHR(st.st_mode)) {
> + close(fd);
> + errno = EFTYPE;
> + return (-1);
> + }
> + }
>   return (fd);
>  }
> 
> 

Re: head(1) -c

[Jeremie Courreges-Anglas]

Ingo Schwarze  writes:

> Hi,

Hi Ingo,

> two general remarks:
>
>  1) The head(1) utility is supposed to handle text files.  Our
> manual page doesn't mention that technicality - in general, our
> manuals avoid excessive technicality in favour of readability -
> but POSIX is explicit:
>   "Input files shall be text files, but the line length
>is not restricted to {LINE_MAX} bytes."
> So, the -c option is badly designed.  It requires a text file
> as input, but usually produces something on output that will
> no longer be a text file, because the output won't usually end
> in a newline character.  Adding a trailing newline in case a
> line is cut in the middle would also be a bad idea.  Cutting
> at the last line break before the character count would have
> been better design, but it's not an option for us because it's
> not what everyone else does.  Note that tail(1) does not suffer
> from the same ailment.  tail -c does not turn text files into
> non-text files, it does preserve the trailing newline.  Of
> course, tail(1) -c cuts characters in half, so it isn't stellar
> design either...
>
>  2) In a text utility, it feels quite strange to add an option
> counting bytes in 2016 when there is no way to count characters
> or display columns.  But given that's what everyone else does,
> so be it.  If the GNU folks ever notice this, we'll very
> probably get option inflation here.  But we can deal with
> that when it happens.
>
> Dmitrij D. Czarkoff wrote on Thu, Mar 10, 2016 at 09:27:05AM +0100:
>> Jeremie Courreges-Anglas said:
>
>>> The situation is a bit muddy. :)
>>> 1. GNU head obeys the last command line option
>>> 2. FreeBSD errors out if both -c and -n are specified
>>> 3. NetBSD always follows -c if it has been specified, probably mixing -c
>>>and -n was overlooked
>>> 4. busybox is a bit more broken:
>>> 
>>>   $ printf '%s\n' a b c d e | busybox head -c 2 -n 5
>>>   a
>>>   b
>>>   c$
>>> 
>>>   ie if -c is passed it always specifies the byte-counting behavior, but
>>>   the actual byte count can be modified by subsequent -n options...
>>> 
>>> I prefer 1. 'cause I see no reason to do 2.
>
> There are three good reasons for 2.:
>
>  1)
>
>> FWIW our tail(2) does 2, so IMO head should as well.
>
> I agree with Dmitrij, for two more reasons in addition to the already
> quite good one Dmitrij mentions - which, by the way, is not just us,
> but POSIX, too, in the case of tail(1).

GNU tail busybox tail, FreeBSD tail and NetBSD tail all allow mixing -c
and -n options.  Our tail(1) is the one that is different here, so it's
not clear to me that the right move is to follow FreeBSD regarding
head(1) and to differ from them regarding tail(1).

To repeat myself, the addition of this rather silly option is supposed
to reduce differences from other implementations so that we can stop
wasting time about it.

>  2) POSIX says:  http://pubs.opengroup.org/onlinepubs/9699919799/
>
> Guideline 11:
> The order of different options relative to one another should
> not matter, unless the options are documented as mutually-exclusive
> and such an option is documented to override any incompatible
> options preceding it.
>
> So, Jeremie, as it stands, your diff is outright wrong.  You
> document the two options to conflict (by marking them with "|"
> in the SYNOPSIS) but implement them as silently overriding
> each other.  That's NOT ok.

Yeah, that came to my mind earlier today.  So, to make things clear: if
those options were explicitely documented as mutually-exclusive *and*
that the last option passed wins, that point would be addressed.  Right?

>  3) When different implementations conflict, let us be conservative.
> It doesn't help users if a non-standard option behaves in some
> random way conflicting with what other systems do.  If somebody
> uses a non-standard option in a way treated differently by
> different systems, they will get bitten somewhere no matter what.
> So it's better to error out and tell them they are doing it wrong.

Among the head(1) implementations I mentioned, only one errors out if
both -c and -n are passed.  If FreeBSD wants to be different fine,
that's their problem; maybe they'd change their implementation if this
difference was reported.  So I really don't find your argument to be
compelling here.

> I suggest you rework the diff to error out.

I don't want to sound dense but I think that I'd rather tweak the
manpage than change the proposed implementation.

> I don't like this
> option even then, it's badly designed, but given how widespread it
> is and how little bloat it causes, i doubt that shooting this diff
> down and forcing porters to waste their time fighting this particular
> windmill is a wise course.

-- 
jca | PGP : 0x1524E7EE / 5135 92C1 AD36 5293 2BDF  DDCC 0DFA 74AE 1524 E7EE

Re: Xorg stipple

[Matthieu Herrb]

On Wed, Mar 09, 2016 at 05:09:13PM -0600, joshua stein wrote:
> Is anyone seriously finding video/Xorg bugs through the default X
> stipple pattern anymore?  Xorg changed the default to draw a black
> background a while ago (with stipple enabled using the -retro flag),
> but we have this local change that reverted it while adding a silly
> -retard flag in order to show the black background.
> 
> I think we can finally stop partying like it's 1989 (vax is dead,
> after all) and have X show a solid black background by default.

I don't have a strong preference. If the team who prefer to get rid
of the stipple (and seem to be the majority of expressed opinons)
agree to take the extra step to try with -retro and/or check
Xorg.0.log carefully before reporting problems to bugs@, I'm ok with
dumping this extra local change.

And eventually the -retro option will be removed upstreams...

> 
> 
> Index: dix/globals.c
> ===
> RCS file: /var/cvs/xenocara/xserver/dix/globals.c,v
> retrieving revision 1.11
> diff -u -p -u -p -r1.11 globals.c
> --- dix/globals.c 16 Sep 2015 19:10:20 -  1.11
> +++ dix/globals.c 9 Mar 2016 22:57:54 -
> @@ -118,11 +118,7 @@ const char *defaultCursorFont = COMPILED
>  FontPtr defaultFont;/* not declared in dix.h to avoid including 
> font.h in
> every compilation of dix code */
>  CursorPtr rootCursor;
> -#ifndef __OpenBSD__
>  Bool party_like_its_1989 = FALSE;
> -#else
> -Bool party_like_its_1989 = TRUE;
> -#endif
>  Bool whiteRoot = FALSE;
>  
>  TimeStamp currentTime;
> Index: os/utils.c
> ===
> RCS file: /var/cvs/xenocara/xserver/os/utils.c,v
> retrieving revision 1.21
> diff -u -p -u -p -r1.21 utils.c
> --- os/utils.c7 Nov 2015 16:48:53 -   1.21
> +++ os/utils.c9 Mar 2016 22:57:37 -
> @@ -579,11 +579,7 @@ UseMsg(void)
>  ErrorF("-r turns off auto-repeat\n");
>  ErrorF("r  turns on auto-repeat \n");
>  ErrorF("-render [default|mono|gray|color] set render color alloc 
> policy\n");
> -#ifndef __OpenBSD__
>  ErrorF("-retro start with classic stipple and cursor\n");
> -#else
> -ErrorF("-retard start with black background and no 
> cursor\n");
> -#endif
>  ErrorF("-s #   screen-saver timeout (minutes)\n");
>  ErrorF("-seat string   seat to run on\n");
>  ErrorF("-t #   default pointer threshold (pixels/t)\n");
> @@ -918,13 +914,8 @@ ProcessCommandLine(int argc, char *argv[
>  defaultKeyboardControl.autoRepeat = TRUE;
>  else if (strcmp(argv[i], "-r") == 0)
>  defaultKeyboardControl.autoRepeat = FALSE;
> -#ifndef __OpenBSD__
>   else if ( strcmp( argv[i], "-retro") == 0)
>  party_like_its_1989 = TRUE;
> -#else
> - else if ( strcmp( argv[i], "-retard") == 0)
> - party_like_its_1989 = FALSE;
> -#endif
>  else if (strcmp(argv[i], "-s") == 0) {
>  if (++i < argc)
>  defaultScreenSaverTime = ((CARD32) atoi(argv[i])) *

-- 
Matthieu Herrb


pgpvMXM2ziNi2.pgp
Description: PGP signature

opendev(3) tweak

[Marc Espie]

Already shown to a few people, but since pledge(2) aborts on non-dev, let's
check upfront that we're of the right type.

I don't think this requires a bump. It doesn't really change the interface,
just makes it stricter.

Index: opendev.3
===
RCS file: /build/data/openbsd/cvs/src/lib/libutil/opendev.3,v
retrieving revision 1.22
diff -u -p -r1.22 opendev.3
--- opendev.3   15 Jan 2015 19:06:32 -  1.22
+++ opendev.3   10 Mar 2016 11:51:27 -
@@ -93,7 +93,9 @@ it is modified to point at the fully exp
 The
 .Fn opendev
 return value and errors are the same as the return value and errors of
-.Xr open 2 .
+.Xr open 2 ,
+plus
+.Er EFTYPE .
 .Sh SEE ALSO
 .Xr open 2 ,
 .Xr getrawpartition 3 ,
Index: opendev.c
===
RCS file: /build/data/openbsd/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   8 Mar 2016 22:30:44 -
@@ -38,6 +38,7 @@
 #include 
 #include 
 #include 
+#include 
 
 #include "util.h"
 
@@ -50,6 +51,7 @@ opendev(const char *path, int oflags, in
 {
static char namebuf[PATH_MAX];
struct dk_diskmap dm;
+   struct stat st;
char *slash, *prefix;
int fd;
 
@@ -106,5 +108,17 @@ opendev(const char *path, int oflags, in
if (realpath)
*realpath = namebuf;
 
+   if (fd != -1) {
+   if (fstat(fd, ) == -1) {
+   close(fd);
+   return (-1);
+   }
+   if ((dflags & OPENDEV_BLCK) ? !S_ISBLK(st.st_mode) :
+   !S_ISCHR(st.st_mode)) {
+   close(fd);
+   errno = EFTYPE;
+   return (-1);
+   }
+   }
return (fd);
 }

Re: head(1) -c

[Ingo Schwarze]

Hi,

two general remarks:

 1) The head(1) utility is supposed to handle text files.  Our
manual page doesn't mention that technicality - in general, our
manuals avoid excessive technicality in favour of readability -
but POSIX is explicit:
  "Input files shall be text files, but the line length
   is not restricted to {LINE_MAX} bytes."
So, the -c option is badly designed.  It requires a text file
as input, but usually produces something on output that will
no longer be a text file, because the output won't usually end
in a newline character.  Adding a trailing newline in case a
line is cut in the middle would also be a bad idea.  Cutting
at the last line break before the character count would have
been better design, but it's not an option for us because it's
not what everyone else does.  Note that tail(1) does not suffer
from the same ailment.  tail -c does not turn text files into
non-text files, it does preserve the trailing newline.  Of
course, tail(1) -c cuts characters in half, so it isn't stellar
design either...

 2) In a text utility, it feels quite strange to add an option
counting bytes in 2016 when there is no way to count characters
or display columns.  But given that's what everyone else does,
so be it.  If the GNU folks ever notice this, we'll very
probably get option inflation here.  But we can deal with
that when it happens.

Dmitrij D. Czarkoff wrote on Thu, Mar 10, 2016 at 09:27:05AM +0100:
> Jeremie Courreges-Anglas said:

>> The situation is a bit muddy. :)
>> 1. GNU head obeys the last command line option
>> 2. FreeBSD errors out if both -c and -n are specified
>> 3. NetBSD always follows -c if it has been specified, probably mixing -c
>>and -n was overlooked
>> 4. busybox is a bit more broken:
>> 
>>   $ printf '%s\n' a b c d e | busybox head -c 2 -n 5
>>   a
>>   b
>>   c$
>> 
>>   ie if -c is passed it always specifies the byte-counting behavior, but
>>   the actual byte count can be modified by subsequent -n options...
>> 
>> I prefer 1. 'cause I see no reason to do 2.

There are three good reasons for 2.:

 1)

> FWIW our tail(2) does 2, so IMO head should as well.

I agree with Dmitrij, for two more reasons in addition to the already
quite good one Dmitrij mentions - which, by the way, is not just us,
but POSIX, too, in the case of tail(1).

 2) POSIX says:  http://pubs.opengroup.org/onlinepubs/9699919799/

Guideline 11:
The order of different options relative to one another should
not matter, unless the options are documented as mutually-exclusive
and such an option is documented to override any incompatible
options preceding it.

So, Jeremie, as it stands, your diff is outright wrong.  You
document the two options to conflict (by marking them with "|"
in the SYNOPSIS) but implement them as silently overriding
each other.  That's NOT ok.

 3) When different implementations conflict, let us be conservative.
It doesn't help users if a non-standard option behaves in some
random way conflicting with what other systems do.  If somebody
uses a non-standard option in a way treated differently by
different systems, they will get bitten somewhere no matter what.
So it's better to error out and tell them they are doing it wrong.

I suggest you rework the diff to error out.  I don't like this
option even then, it's badly designed, but given how widespread it
is and how little bloat it causes, i doubt that shooting this diff
down and forcing porters to waste their time fighting this particular
windmill is a wise course.

Yours,
  Ingo

Re: head(1) -c

[Dmitrij D. Czarkoff]

Jeremie Courreges-Anglas said:
> The situation is a bit muddy. :)
> 1. GNU head obeys the last command line option
> 2. FreeBSD errors out if both -c and -n are specified
> 3. NetBSD always follows -c if it has been specified, probably mixing -c
>and -n was overlooked
> 4. busybox is a bit more broken:
> 
>   $ printf '%s\n' a b c d e | busybox head -c 2 -n 5
>   a
>   b
>   c$
> 
>   ie if -c is passed it always specifies the byte-counting behavior, but
>   the actual byte count can be modified by subsequent -n options...
> 
> I prefer 1. 'cause I see no reason to do 2.

FWIW our tail(2) does 2, so IMO head should as well.

-- 
Dmitrij D. Czarkoff