[PATCH] etc/ksh.kshrc - unify command substitution

[Raf Czlonka]

Hi all,

I've noticed that etc/ksh.kshrc uses both types of command substitution
`command` and $(command). The below diff unifies it and uses
$(command) notation consistently.

While there:

- remove ':' (null utility) from the very first line of the file -
  I *do* understand what it does but it doesn't seem like it's needed
  at all, unless I'm missing something (as is the case with some idioms)
- remove basename(1) invocation and use parameter expansion instead
- simplify one if conditional by replacing it with && and grouping
  commands

Regards,

Raf

Index: etc/ksh.kshrc
===
RCS file: /cvs/src/etc/ksh.kshrc,v
retrieving revision 1.27
diff -u -p -u -r1.27 ksh.kshrc
--- etc/ksh.kshrc   14 Sep 2016 18:34:51 -  1.27
+++ etc/ksh.kshrc   7 Jul 2017 04:38:58 -
@@ -1,4 +1,3 @@
-:
 #  $OpenBSD: ksh.kshrc,v 1.27 2016/09/14 18:34:51 rpe Exp $
 #
 # NAME:
@@ -39,7 +38,7 @@ case "$-" in
0) PS1S='# ';;
esac
PS1S=${PS1S:-'$ '}
-   HOSTNAME=${HOSTNAME:-`uname -n`}
+   HOSTNAME=${HOSTNAME:-$(uname -n)}
HOST=${HOSTNAME%%.*}
 
PROMPT="$USER:!$PS1S"
@@ -49,8 +48,8 @@ case "$-" in
PS1=$PPROMPT
# $TTY is the tty we logged in on,
# $tty is that which we are in now (might by pty)
-   tty=`tty`
-   tty=`basename $tty`
+   tty=$(tty)
+   tty=${tty##*/}
TTY=${TTY:-$tty}
# $console is the system console device
console=$(sysctl kern.consdev)
@@ -74,9 +73,8 @@ case "$-" in
xterm*)
ILS='\033]1;'; ILE='\007'
WLS='\033]2;'; WLE='\007'
-   if ps -p $PPID -o command | grep -q telnet; then
+   { ps -p $PPID -o command | grep -q telnet; } &&
export TERM=xterms
-   fi
;;
*)  ;;
esac
@@ -117,7 +115,7 @@ case "$-" in
alias o='fg %-'
alias df='df -k'
alias du='du -k'
-   alias rsize='eval `resize`'
+   alias rsize='eval $(resize)'
 ;;
 *) # non-interactive
 ;;
@@ -142,6 +140,6 @@ function pre_path {
 }
 # if $1 is in path, remove it
 function del_path {
-   no_path $* || eval ${2:-PATH}=`eval echo :'$'${2:-PATH}: |
-   sed -e "s;:$1:;:;g" -e "s;^:;;" -e "s;:\$;;"`
+   no_path $* || eval ${2:-PATH}=$(eval echo :'$'${2:-PATH}: |
+   sed -e "s;:$1:;:;g" -e "s;^:;;" -e "s;:\$;;")
 }

[PATCH] clarify history of htpasswd(1) in its manpage

[Raf Czlonka]

Hi all,

Just gone through htpasswd(1)'s man page and noticed this in the AUTHORS 
section:

Florian Obser  implemented htpasswd from scratch
after httpd was removed from OpenBSD base. 

To those not familiar with OpenBSD's release history, this may seem
wrong (after all, httpd *is* in base) or misleading at the very least.

What do you think about clarifying it ever so slightly?

Regards,

Raf

Index: usr.bin/htpasswd/htpasswd.1
===
RCS file: /cvs/src/usr.bin/htpasswd/htpasswd.1,v
retrieving revision 1.7
diff -u -p -r1.7 htpasswd.1
--- usr.bin/htpasswd/htpasswd.1 26 Aug 2014 21:50:38 -  1.7
+++ usr.bin/htpasswd/htpasswd.1 7 Jul 2017 02:24:05 -
@@ -69,6 +69,6 @@ has been available since
 .An Florian Obser Aq Mt flor...@openbsd.org
 implemented
 .Nm
-from scratch after httpd was removed from
+from scratch after Apache httpd was removed from
 .Ox
 base.

Re: lock(1): use crypt_checkpass(3) for one-off keys

[Scott Cheloha]

> On Jun 26, 2017, at 10:49 PM, Ted Unangst  wrote:
> 
> [...]
> 
>> 
>> CC'd tedu@ because I'm not sure if I'm using crypt_newhash(3)
>> correctly.
>> 
>> Ted: In other places people use _PASSWORD_LEN for the length
>>  of the hash buffer.  Clearly this works, but it feels off.
>>  _PASSWORD_LEN is meant to be an upper bound on length of
>>  the plaintext, not the hash output, right?
>> 
>>  Is there a better way to size my buffer for use with
>>  crypt_newhash(3)?
> 
> yes, but there is no better define for the output buffer. perhaps PASS_MAX,
> i'm not sure why everything settled on _PASSWORD_LEN.

fwiw, PASS_MAX is now deprecated by POSIX.  I think leaving it as _PASSWORD_LEN
for consistency with the rest of the tree for now is probably better.

Would a later patch exposing something like, I dunno, "BCRYPT_HASHMAX" to
includers of unistd.h be welcome?  A documented define would round out the API.

--
Scott Cheloha

Re: Standard conformance of strtol(3)

[Gerhard Roth]

On 06.07.2017 17:53, Todd C. Miller wrote:

On Thu, 06 Jul 2017 07:37:19 -0600, "Todd C. Miller" wrote:

glibc strtol() behavior:
 AIX
 FreeBSD
 GNU/Linux
 Solaris
 macOS


SunOS 4.1.3 has the same behavior as Solaris.  That's as far back
as I care to go.

  - todd



FWIW: AT's SVR4 from 1988 had the same behaviour ;)


Gerhard

Re: add simple ifstated regression test script

[Rob Pierce]

On Sun, Jul 02, 2017 at 06:29:07PM +0200, Sebastian Benoit wrote:
> Rob Pierce(r...@2keys.ca) on 2017.07.02 12:06:25 -0400:
> > I am currently using this regression script for basic ifstated sanity 
> > testing.
> > 
> > Still a work in progress. Requesting commit for safe keeping.
> 
> 
> Hi,
> 
> this should go into /usr/src/regress/usr.sbin/ifstated
> (which does not esist yet).
> 
> Also, it should hook into the regress framework (bsd.regress.mk(5)).
> 
> As it needs some network configuration, maybe it should be similar to
> relayd regress tests.
> 
> Happy to work with you on that.
> 
> /B.

I have updated the ifstated regression scripts based on your feedback.

I also added a script to test drive the state machine.

Both should be more systematic in coverage, but hopefully it is a good start.

Regards,

Rob

Index: regress/usr.sbin/ifstated/Makefile
===
RCS file: regress/usr.sbin/ifstated/Makefile
diff -N regress/usr.sbin/ifstated/Makefile
--- /dev/null   1 Jan 1970 00:00:00 -
+++ regress/usr.sbin/ifstated/Makefile  6 Jul 2017 16:55:57 -
@@ -0,0 +1,13 @@
+# $OpenBSD$
+
+# Regress tests for ifstated
+
+REGRESS_TARGETS =  run-regress-statemachine run-regress-ifstated
+
+run-regress-statemachine:
+   sh ${.CURDIR}/statemachine
+
+run-regress-ifstated:
+   sh ${.CURDIR}/ifstated
+
+.include 

Index: regress/usr.sbin/ifstated/ifstated
===
RCS file: regress/usr.sbin/ifstated/ifstated
diff -N regress/usr.sbin/ifstated/ifstated
--- /dev/null   1 Jan 1970 00:00:00 -
+++ regress/usr.sbin/ifstated/ifstated  6 Jul 2017 16:55:57 -
@@ -0,0 +1,148 @@
+# $OpenBSD$
+
+# Basic ifstated regression script to test interface changes.
+
+# Golbal variables
+VHIDA=252
+VHIDB=253
+PREFIX=172.16.0
+DEMOTE=ifconfig
+PROMOTE=ifconfig
+EVERY=5
+SLEEP=10
+
+cleanup() {
+   ifconfig carp${VHIDA} destroy > /dev/null 2>&1
+   ifconfig carp${VHIDB} destroy > /dev/null 2>&1
+   rm working/ifstated.conf >/dev/null 2>&1
+   rm working/ifstated.log >/dev/null 2>&1
+   rm working/output.test >/dev/null 2>&1
+   rm working/output.new >/dev/null 2>&1
+   rm working/nohup.out >/dev/null 2>&1
+   rmdir working >/dev/null 2>&1
+}
+
+fail() {
+   echo FAILED
+   cleanup
+   exit 1
+}
+
+skip() {
+   echo SKIPPED
+   cleanup
+   exit 0
+}
+
+trap 'skip' INT
+
+# look for a suitable physical interface for carp
+NIC="$(netstat -rn -finet | grep ^default | awk '{ print $8 }')"
+STATUS="$(ifconfig | grep -A5 ^${NIC} | grep status: | awk '{ print $2 }')"
+
+if [ "$STATUS" != "active" ]
+then
+   echo "No suitable physical interface found."
+   echo SKIPPED
+   exit 0
+fi
+
+if [ "$(pgrep ifstated)" ]
+then
+   echo "The ifstated daemon is already running."
+   echo SKIPPED
+   exit 0
+fi
+
+for interface in carp${VHIDA} carp${VHIDB}
+do
+   ifconfig ${interface} > /dev/null 2>&1
+   if [ $? -eq 0 ]
+   then
+   echo "Interface $interface already exists."
+   echo SKIPPED
+   exit 0
+   fi
+done
+
+mkdir -p working
+
+cat > working/ifstated.conf < working/output.test < ifstated.log 2>&1) &
+
+sleep ${SLEEP}
+ifconfig carp${VHIDA} down
+sleep ${SLEEP}
+ifconfig carp${VHIDA} up
+sleep ${SLEEP}
+ifconfig carp${VHIDB} destroy
+sleep ${SLEEP}
+ifconfig carp${VHIDB} inet ${PREFIX}.${VHIDB} netmask 255.255.255.0 broadcast \
+   ${PREFIX}.255 vhid ${VHIDB} carpdev ${NIC}
+sleep ${SLEEP}
+ifconfig carp${VHIDA} down
+sleep ${SLEEP}
+ifconfig carp${VHIDB} destroy
+sleep ${SLEEP}
+ifconfig carp${VHIDA} up
+sleep ${SLEEP}
+ifconfig carp${VHIDB} inet ${PREFIX}.${VHIDB} netmask 255.255.255.0 broadcast \
+   ${PREFIX}.255 vhid ${VHIDB} carpdev ${NIC}
+sleep ${SLEEP}
+kill -HUP $(pgrep ifstated) >/dev/null 2>&1
+sleep ${SLEEP}
+
+grep ^changing working/ifstated.log > working/output.new
+
+kill $(pgrep ifstated) >/dev/null 2>&1
+
+diff working/output.test working/output.new
+case $? in
+0) echo PASSED
+   cleanup
+   exit 0
+   ;;
+1) fail
+   ;;
+esac

Index: regress/usr.sbin/ifstated/statemachine
===
RCS file: regress/usr.sbin/ifstated/statemachine
diff -N regress/usr.sbin/ifstated/statemachine
--- /dev/null   1 Jan 1970 00:00:00 -
+++ regress/usr.sbin/ifstated/statemachine  6 Jul 2017 16:55:57 -
@@ -0,0 +1,183 @@
+# $OpenBSD$
+
+# Basic ifstated regression script to test the finite state machine.
+
+#
+# NOTE: Increase LSLEEP as required when adding additional test states.
+#
+
+# Golbal variables
+FILE1="truth1.test"
+FILE2="truth2.test"
+EVERY=2
+SLEEP=5
+LSLEEP=35
+
+cleanup() {
+   rm working/$FILE1 >/dev/null 2>&1
+   rm working/$FILE2 >/dev/null 2>&1
+   rm working/ifstated.conf >/dev/null 2>&1
+   rm working/ifstated.log >/dev/null 2>&1
+   rm 

Re: Standard conformance of strtol(3)

[Marc Espie]

On Thu, Jul 06, 2017 at 06:06:09PM +0200, Joerg Sonnenberger wrote:
> On Thu, Jul 06, 2017 at 03:42:19PM +0200, Marc Espie wrote:
> > 7.20.1.4 (3) If the value of base is zero, the expected form of the subject
> > sequence is that of an integer constant *as described in 6.4.4.1*, 
> > optionally
> > preceded by a plus or minus sign but not including an integer suffix [...]
> > 
> > 6.4.4.1 is a grammar
> > 
> > 
> > integer-constant:
> > [...]
> > hexadecimal-constant  integer-suffix_opt
> 
> You have skipped with [...] the part that actually matches "0". So yes,
> the ISO C grammar does require strtol and friends to handle "0xx" and
> similar as just "0".

Yep, since the goal was just to show that 0x was not a valid number in ISO C.

Duh!

Re: Standard conformance of strtol(3)

[Todd C. Miller]

I've just committed a fix for this.

 - todd

Re: Standard conformance of strtol(3)

[Joerg Sonnenberger]

On Thu, Jul 06, 2017 at 03:42:19PM +0200, Marc Espie wrote:
> 7.20.1.4 (3) If the value of base is zero, the expected form of the subject
> sequence is that of an integer constant *as described in 6.4.4.1*, optionally
> preceded by a plus or minus sign but not including an integer suffix [...]
> 
> 6.4.4.1 is a grammar
> 
> 
> integer-constant:
>   [...]
>   hexadecimal-constant  integer-suffix_opt

You have skipped with [...] the part that actually matches "0". So yes,
the ISO C grammar does require strtol and friends to handle "0xx" and
similar as just "0".

Joerg

armv7 bootstrap-only variables

[Artturi Alm]

Hi,

is/has anyone been working on a diff that would collect these
into a structure, so that those could easier get gotten rid of once
bootstrap is done? Or have i missed something about this new bootstrap
split-up to locore/0.S, i mean, is the gap alone good enough to leave
these around, or is this still WIP on arm, or considered cleanup hence
not to be done, or?

-Artturi

Re: Standard conformance of strtol(3)

[Todd C. Miller]

On Thu, 06 Jul 2017 07:37:19 -0600, "Todd C. Miller" wrote:

> Sorry, HP-UX actually has the same behavior as us.  Here's what I
> have so far:
> 
> 4.4BSD strtol() behavior:
> HP-UX
> NetBSD
> OpenBSD

strtol(3) was added to BSD in 4.3-Reno along with the rest of the
Torek libc.

> glibc strtol() behavior:
> AIX
> FreeBSD
> GNU/Linux
> Solaris
> macOS

SunOS 4.1.3 has the same behavior as Solaris.  That's as far back
as I care to go.

 - todd

Re: ifstated unused variable

[Lee Clagett]

On Sun, 2 Jul 2017 16:31:29 +0200
Sebastian Benoit  wrote:

> Thanks, i commited all three.
> 
> /Benno
> 
> Rob Pierce(r...@2keys.ca) on 2017.07.02 00:32:27 -0400:
> > Remove unused variable from header file.
> > 
> > Index: ifstated.h
> > ===
> > RCS file: /cvs/src/usr.sbin/ifstated/ifstated.h,v
> > retrieving revision 1.12
> > diff -u -p -r1.12 ifstated.h
> > --- ifstated.h  28 Jun 2017 11:10:08 -  1.12
> > +++ ifstated.h  2 Jul 2017 04:30:01 -
> > @@ -70,7 +70,6 @@ struct ifsd_action {
> > struct {
> > struct ifsd_action_list  actions;
> > struct ifsd_expression  *expression;
> > -   u_int8_t ignore_init;
> > } c;
> > } act;
> > u_int32_ttype;
> > 
> 

Re: armv7 small bootstrap improvement/simplification

[Artturi Alm]

On Wed, Jul 05, 2017 at 09:40:41PM +0300, Artturi Alm wrote:
> 
> diff --git a/sys/arch/armv7/armv7/armv7_machdep.c 
> b/sys/arch/armv7/armv7/armv7_machdep.c
> index aa1c549b29b..105fbf1 100644
> --- a/sys/arch/armv7/armv7/armv7_machdep.c
> +++ b/sys/arch/armv7/armv7/armv7_machdep.c
> @@ -356,6 +356,30 @@ copy_io_area_map(pd_entry_t *new_pd)
>   }
>  }
>  
> +static inline paddr_t
> +_bs_alloc(size_t sz)
> +{
> + paddr_t addr, pa = 0;
> +
> + for (sz = round_page(sz); sz > 0; sz -= PAGE_SIZE) {
> + if (uvm_page_physget() == FALSE)
> + panic("uvm_page_physget() failed");
> + memset((char *)addr, 0, PAGE_SIZE);
> + if (pa == 0)
> + pa = addr;
> + }
> + return pa;
> +}
> +
> +/* RelativePA 2 KVA */
> +#define  _BS_RPA2KVA(x, y)   (KERNEL_BASE + (x) - (y))
> +static inline void
> +_bs_valloc(pv_addr_t *pv, vsize_t sz, paddr_t off)
> +{
> + pv->pv_pa = _bs_alloc(sz);
> + pv->pv_va = _BS_RPA2KVA(pv->pv_pa, off);
> +}
> +
>  /*
>   * u_int initarm(...)
>   *
> @@ -379,7 +403,7 @@ initarm(void *arg0, void *arg1, void *arg2, paddr_t 
> loadaddr)
>   paddr_t memstart;
>   psize_t memsize;
>   paddr_t memend;
> - void *config;
> + void *config = arg2;
>   size_t size;
>   void *node;
>   extern uint32_t esym; /* &_end if no symbols are loaded */
> @@ -420,18 +444,8 @@ initarm(void *arg0, void *arg1, void *arg2, paddr_t 
> loadaddr)
>   tmp_bs_tag.bs_map = bootstrap_bs_map;
>  
>   /*
> -  * Now, map the FDT area.
> -  *
> -  * As we don't know the size of a possible FDT, map the size of a
> -  * typical bootstrap bs map.  The FDT might not be aligned, so this
> -  * might take up to two L1_S_SIZEd mappings.
> -  *
> -  * XXX: There's (currently) no way to unmap a bootstrap mapping, so
> -  * we might lose a bit of the bootstrap address space.
> +  * Now, init the FDT @ PA, reloc and reinit to KVA later.
>*/
> - bootstrap_bs_map(NULL, (bus_addr_t)arg2, L1_S_SIZE, 0,
> - (bus_space_handle_t *));
> -
>   if (!fdt_init(config) || fdt_get_size(config) == 0)
>   panic("initarm: no FDT");
>  
> @@ -477,6 +491,33 @@ initarm(void *arg0, void *arg1, void *arg2, paddr_t 
> loadaddr)
>  
>   physmem = (physical_end - physical_start) / PAGE_SIZE;
>  
> + /* Load memory into UVM. */
> +#ifdef VERBOSE_INIT_ARM
> + printf("page ");
> +#endif
> + uvm_setpagesize();/* initialize PAGE_SIZE-dependent variables */
> + uvm_page_physload(atop(physical_freestart), atop(physical_freeend),
> + atop(physical_freestart), atop(physical_freeend), 0);
> +
> + if (physical_start < loadaddr) {
> + uvm_page_physload(atop(physical_start), atop(loadaddr),
> + atop(physical_start), atop(loadaddr), 0);
> + physsegs--;
> + }
> +
> + for (i = 1; i < physsegs; i++) {
> + if (fdt_get_reg(node, i, ))
> + break;
> + if (reg.size == 0)
> + continue;
> +
> + memstart = reg.addr;
> + memend = MIN(reg.addr + reg.size, (paddr_t)-PAGE_SIZE);
> + physmem += atop(memend - memstart);
> + uvm_page_physload(atop(memstart), atop(memend),
> + atop(memstart), atop(memend), 0);
> + }
> +
>  #ifdef DEBUG
>   /* Tell the user about the memory */
>   printf("physmemory: %d pages at 0x%08lx -> 0x%08lx\n", physmem,
> @@ -514,27 +555,27 @@ initarm(void *arg0, void *arg1, void *arg2, paddr_t 
> loadaddr)
>  
>   /* Define a macro to simplify memory allocation */
>  #define  valloc_pages(var, np)   \
> - alloc_pages((var).pv_pa, (np)); \
> - (var).pv_va = KERNEL_BASE + (var).pv_pa - loadaddr;
> + _bs_valloc(&(var), ptoa((np)), loadaddr)
>  
>  #define alloc_pages(var, np) \
> - (var) = physical_freestart; \
> - physical_freestart += ((np) * PAGE_SIZE);   \
> - if (physical_freeend < physical_freestart)  \
> - panic("initarm: out of memory");\
> - free_pages -= (np); \
> - memset((char *)(var), 0, ((np) * PAGE_SIZE));
> + (var) = _bs_alloc(ptoa((np)))
>  
>   loop1 = 0;
>   kernel_l1pt.pv_pa = 0;
> + physical_freestart = _bs_alloc(ptoa(NUM_KERNEL_PTS) + L1_TABLE_SIZE);
>   for (loop = 0; loop <= NUM_KERNEL_PTS; ++loop) {
>   /* Are we 16KB aligned for an L1 ? */
>   if (((physical_freestart) & (L1_TABLE_SIZE - 1)) == 0
>   && kernel_l1pt.pv_pa == 0) {
> - valloc_pages(kernel_l1pt, L1_TABLE_SIZE / PAGE_SIZE);
> + kernel_l1pt.pv_pa = physical_freestart;
> + kernel_l1pt.pv_va =
> + _BS_RPA2KVA(physical_freestart, 

Re: mg backup directory (bump)

[Lucas Gabriel Vuotto]

Bump.

Emacs gets HOME from environment here. I think that getting it from pw entry is 
more correct, but I can make a patch to behave like emacs if needed.

On 19/05/17 14:11, Lucas Gabriel Vuotto wrote:
> Previous patch shall be ignored, as it was an ugly hack. Below is a patch 
> that is simpler and fixes expandtilde instead, so it fixes the problem in 
> other situations (writing files to ~, for example). The only thing that I'm 
> not sure is whether to use getuid or geteuid. Any suggestion / explanation is 
> welcome.
> 
> Index: usr.bin/mg/fileio.c
> ===
> RCS file: /cvs/src/usr.bin/mg/fileio.c,v
> retrieving revision 1.103
> diff -u -p -u -p -r1.103 fileio.c
> --- usr.bin/mg/fileio.c28 Jul 2016 21:40:25 -1.103
> +++ usr.bin/mg/fileio.c18 May 2017 17:53:03 -
> @@ -723,15 +723,12 @@ expandtilde(const char *fn)
>  return(ret);
>  }
>  if (ulen == 0) { /* ~/ or ~ */
> -if ((un = getlogin()) != NULL)
> -(void)strlcpy(user, un, sizeof(user));
> -else
> -user[0] = '\0';
> +pw = getpwuid(getuid());
>  } else { /* ~user/ or ~user */
>  memcpy(user, [1], ulen);
>  user[ulen] = '\0';
> +pw = getpwnam(user);
>  }
> -pw = getpwnam(user);
>  if (pw != NULL) {
>  plen = strlcpy(path, pw->pw_dir, sizeof(path));
>  if (plen == 0 || path[plen - 1] != '/') {
> 

Re: [patch] mg: fix overflow on vteeol()

[Florian Obser]

Hi Ingo,

thanks for your detailed analysis, makes sense to me OK florian@

Apologies to Hiltjo for slacking on this. I looked at it multiple
times but couldn't make any progress on it.

Florian

On Thu, Jul 06, 2017 at 04:08:09PM +0200, Ingo Schwarze wrote:
> Hi,
> 
> Hiltjo Posthuma wrote on Sun, Jun 18, 2017 at 03:04:31PM +0200:
> 
> > mg crashes with certain (unicode) characters and moving the cursor to the
> > end of the line.
> 
> Even though i failed to reproduce the crash, even with MALLOC_OPTIONS=S,
> i see how it may happen.  Your analysis looks at the right functions,
> but part of it is inaccurate.
> 
> > The characters are printed to the screen as \nnn in vtpute()
> 
> No, they are not, which is another bug that needs fixing.
> 
> > and vtcol is updated,
> 
> No, not sufficiently, which is exactly the problem.
> 
> > however vteeol() will write beyond the buffer.
> 
> More precisely, before the beginning of the buffer.
> 
> > A test file contains the data:
> >
> > ??
> 
> [ Regarding vteeol() ]
> > It is safer to use vtpute() here because it checks if vtcol >= 0.
> 
> No, that is not needed.  Apart from incrementing vtcol, the only
> place where vtcol is changed is vtmove().  The only place where
> vtmove() is called with a non-zero second argument is in updext().
> In updext(), when calculating lbound, both numbers subtracted are
> clearly non-negative, so we have lbound <= curcol.  So after the
> loop calling vtpute() repeatedly, vtcol >= 0 ought to be guaranteed.
> To summarize, vtcol >= 0 is an invariant except for a very short
> section of code inside updext(), and the check in vtpute() is only
> needed for the call from that section.  In vteeol(), no such check
> is needed.
> 
> The problem is that vtpute() is not handling bytes correctly that
> are neither printable nor control characters.  It does not print
> anything for them and it increments vtcol only by one.  Consequently,
> whenn there are many bytes with the high bit set, vtcol may remain
> negative after finishing the loop in updext(), and vteeol() gets
> fatally called with negative vtcol.
> 
> The patch below fixes the root cause by adding the same logic for
> handling special characters to vtpute() that you can already see
> in vtputc().  I tried to keep the patch minimal, and to also mimick
> the existing style.
> 
> OK to commit that?
> 
> > Below is a patch:
> 
> Your patch would merely hide the bug, and the editor still wouldn't
> operate correctly.
> 
> Yours,
>   Ingo
> 
> 
> Index: display.c
> ===
> RCS file: /cvs/src/usr.bin/mg/display.c,v
> retrieving revision 1.47
> diff -u -p -r1.47 display.c
> --- display.c 3 Apr 2015 22:10:29 -   1.47
> +++ display.c 6 Jul 2017 13:55:43 -
> @@ -366,10 +366,16 @@ vtpute(int c)
>   } else if (ISCTRL(c) != FALSE) {
>   vtpute('^');
>   vtpute(CCHR(c));
> - } else {
> + } else if (isprint(c)) {
>   if (vtcol >= 0)
>   vp->v_text[vtcol] = c;
>   ++vtcol;
> + } else {
> + char bf[5], *cp;
> +
> + snprintf(bf, sizeof(bf), "\\%o", c);
> + for (cp = bf; *cp != '\0'; cp++)
> + vtpute(*cp);
>   }
>  }
>  

-- 
I'm not entirely sure you are real.

Re: Standard conformance of strtol(3)

[Bryan Steele]

On Thu, Jul 06, 2017 at 07:37:19AM -0600, Todd C. Miller wrote:
> Sorry, HP-UX actually has the same behavior as us.  Here's what I
> have so far:
> 
> 4.4BSD strtol() behavior:
> HP-UX
> NetBSD
> OpenBSD

I had discovered with awolk@ and Dragonfly also shares this
behaviour:

http://gitweb.dragonflybsd.org/dragonfly.git/blob/HEAD:/lib/libc/stdlib/_strtol.h

> glibc strtol() behavior:
> AIX
> FreeBSD
> GNU/Linux
> Solaris
> macOS
> 
>  - todd

Re: [patch] mg: fix overflow on vteeol()

[Ingo Schwarze]

Hi,

Hiltjo Posthuma wrote on Sun, Jun 18, 2017 at 03:04:31PM +0200:

> mg crashes with certain (unicode) characters and moving the cursor to the
> end of the line.

Even though i failed to reproduce the crash, even with MALLOC_OPTIONS=S,
i see how it may happen.  Your analysis looks at the right functions,
but part of it is inaccurate.

> The characters are printed to the screen as \nnn in vtpute()

No, they are not, which is another bug that needs fixing.

> and vtcol is updated,

No, not sufficiently, which is exactly the problem.

> however vteeol() will write beyond the buffer.

More precisely, before the beginning of the buffer.

> A test file contains the data:
>
> ——

[ Regarding vteeol() ]
> It is safer to use vtpute() here because it checks if vtcol >= 0.

No, that is not needed.  Apart from incrementing vtcol, the only
place where vtcol is changed is vtmove().  The only place where
vtmove() is called with a non-zero second argument is in updext().
In updext(), when calculating lbound, both numbers subtracted are
clearly non-negative, so we have lbound <= curcol.  So after the
loop calling vtpute() repeatedly, vtcol >= 0 ought to be guaranteed.
To summarize, vtcol >= 0 is an invariant except for a very short
section of code inside updext(), and the check in vtpute() is only
needed for the call from that section.  In vteeol(), no such check
is needed.

The problem is that vtpute() is not handling bytes correctly that
are neither printable nor control characters.  It does not print
anything for them and it increments vtcol only by one.  Consequently,
whenn there are many bytes with the high bit set, vtcol may remain
negative after finishing the loop in updext(), and vteeol() gets
fatally called with negative vtcol.

The patch below fixes the root cause by adding the same logic for
handling special characters to vtpute() that you can already see
in vtputc().  I tried to keep the patch minimal, and to also mimick
the existing style.

OK to commit that?

> Below is a patch:

Your patch would merely hide the bug, and the editor still wouldn't
operate correctly.

Yours,
  Ingo


Index: display.c
===
RCS file: /cvs/src/usr.bin/mg/display.c,v
retrieving revision 1.47
diff -u -p -r1.47 display.c
--- display.c   3 Apr 2015 22:10:29 -   1.47
+++ display.c   6 Jul 2017 13:55:43 -
@@ -366,10 +366,16 @@ vtpute(int c)
} else if (ISCTRL(c) != FALSE) {
vtpute('^');
vtpute(CCHR(c));
-   } else {
+   } else if (isprint(c)) {
if (vtcol >= 0)
vp->v_text[vtcol] = c;
++vtcol;
+   } else {
+   char bf[5], *cp;
+
+   snprintf(bf, sizeof(bf), "\\%o", c);
+   for (cp = bf; *cp != '\0'; cp++)
+   vtpute(*cp);
}
 }
 

Re: Standard conformance of strtol(3)

[Marc Espie]

On Wed, Jul 05, 2017 at 07:12:28PM -0400, Ted Unangst wrote:
> Olivier Antoine wrote:
> > Hi all,
> > 
> > Recently a bug has been identified in Tor:
> > 
> > https://trac.torproject.org/projects/tor/ticket/22789
> > 
> > As comments were made, questions were raised about the use of strtol(3),
> > the different interpretations of the standard and their implementation.
> > 
> > To summarize, the question revolves around the processing of strings in
> > base=16 and with the optional prefix '0x'.
> > 
> > l = strtol ("0xquux", & rest, 16);
> > 
> > Produce
> > l=0 rest=0xquux on OpenBSD
> > l=0 rest=xquux on Linux
> > 
> > Do specialists of the standard or developers have an opinion on this point
> > of detail?
> > Is there a defined behavior?
> 
> My opinion is that well written code would avoid feeding ambigious strings to
> strtol. Today's it's 0xquux and tomorrow it's 0xaquux and now you have a
> problem.
> 
> But, let's read 
> http://pubs.opengroup.org/onlinepubs/9699919799/functions/strtol.html
> 
> It's actually unclear IMO. But I don't see anything prohibiting interpreting
> the string as an optional prefix with an empty body.

Well:

The functionality described on this reference page is aligned with the ISO C 
standard. Any conflict between the requirements described here and the ISO C 
standard is unintentional. This volume of POSIX.1-2008 defers to the ISO C 
standard.


This is Sparta^WISO C we're talking about.  The wording of posix is actually
irrelevant.

ISO C 99 is waaays clearer.

7.20.1.4 (3) If the value of base is zero, the expected form of the subject
sequence is that of an integer constant *as described in 6.4.4.1*, optionally
preceded by a plus or minus sign but not including an integer suffix [...]

7.20.1.4 (4) The subject sequence is defined as the longest intial subsequence
of the input string [...] *that is of the expected form*.


6.4.4.1 is a grammar


integer-constant:
[...]
hexadecimal-constant  integer-suffix_opt

hexadecimal-constant:
hexadecimal-prefix hexadecimal-digit
hexadecimal-constant hexadecimal-digit

There is no wiggle room there.


That grammar is explicit that there must be at least one hexadecimal digit
after the prefix.

Re: Standard conformance of strtol(3)

[Todd C. Miller]

Sorry, HP-UX actually has the same behavior as us.  Here's what I
have so far:

4.4BSD strtol() behavior:
HP-UX
NetBSD
OpenBSD

glibc strtol() behavior:
AIX
FreeBSD
GNU/Linux
Solaris
macOS

 - todd

Re: Standard conformance of strtol(3)

[Todd C. Miller]

Solaris, AIX, and HP-UX all have the same behavior as glibc.
We are the outlier.

Since the standard is clear that the 0x/0X prefix is optional I
believe our behavior is wrong.

 - todd

Re: patch: fix inteldrm for Intel P4000 graphics

[Joe Gidi]

>> Date: Tue, 4 Jul 2017 17:24:27 -0400
>> From: "Joe Gidi" 
>>
>> I have a machine with a Xeon E3-1225v2 CPU, which includes integrated
>> Intel P4000 graphics. This required a patch back in 2015 to avoid
>> matching
>> on the mythical "Intel Quanta Transcode" device, which kettenis@
>> committed
>> here:
>>
>> http://marc.info/?l=openbsd-cvs=144342287923444=2
>>
>> The recent update to inteldrm broke X on this system, because this patch
>> got lost.
>>
>> The patch below has been tested and restores inteldrm functionality with
>> P4000 graphics.
>
> Thanks.  Fixed this in a different way such that it hopefully doesn't
> happen again.  Can you let me know if the problem persists?

The fix you committed is working for me. Thank you!

-- 

Joe Gidi
j...@entropicblur.com

"You cannot buy skill." -- Ross Seyfried

Re: dhcpd: don't reject DHCPINFORM from behind relay

[Landry Breuil]

On Wed, Jul 05, 2017 at 04:37:39PM +0200, Reyk Floeter wrote:
> Hi,
> 
> landry@ sees many log messages 'DHCPINFORM from xx but ciaddr yy is
> not consistent with actual address' in a setup where dhcpd runs behind
> dhcrelay.
> 
> The code in dhcpd's dhcpinform() seems wrong - it assumes that ciaddr
> (the client IP) is identical to the packet source address and it
> doesn't consider a relay, indicated by giaddr (gateway).
> 
> I looked at isc-dhcpd code and, omg my eyes are bleeding, it seems to
> handle DHCPINFORM from relayed clients with giaddr set.
> 
> So it seems that dhcpd never accepted DHCPINFORM from clients behind a
> relay, and the diff below changes it and stops the log spam.  But it
> also changes behavior, so it needs some testing and maybe feedback
> from DHCP experts (prodding krw).

I've put this on a 6.1 vm behind a dhcrelay at work, previously the
w7 clients were apparently sending DHCPINFORM every ~10mn, and in bursts:

Jul  5 14:52:42 oberalp dhcpd[92834]: DHCPINFORM from 172.20.85.254 but ciaddr 
172.20.85.124 is not consistent with actual address
Jul  5 14:52:45 oberalp dhcpd[92834]: DHCPINFORM from 172.20.85.254 but ciaddr 
172.20.85.124 is not consistent with actual address
Jul  5 14:54:11 oberalp last message repeated 2 times
Jul  5 14:55:23 oberalp dhcpd[92834]: DHCPINFORM from 172.20.85.254 but ciaddr 
172.20.85.107 is not consistent with actual address
Jul  5 14:55:26 oberalp dhcpd[92834]: DHCPINFORM from 172.20.85.254 but ciaddr 
172.20.85.107 is not consistent with actual address
Jul  5 14:56:50 oberalp last message repeated 2 times

With this diff, now the clients still send DHCPINFORM, some every ~10mn,
some every ~30m, still in bursts, and the DHCPACK is sent back:

Jul  6 11:08:06 oberalp dhcpd[42317]: DHCPACK to 172.20.85.107 
() via 172.20.85.254
Jul  6 11:08:09 oberalp dhcpd[42317]: DHCPINFORM from 172.20.85.254
Jul  6 11:08:09 oberalp dhcpd[42317]: DHCPACK to 172.20.85.107 
() via 172.20.85.254
Jul  6 11:18:13 oberalp dhcpd[42317]: DHCPINFORM from 172.20.85.254
Jul  6 11:18:13 oberalp dhcpd[42317]: DHCPACK to 172.20.85.107 
() via 172.20.85.254
Jul  6 11:18:16 oberalp dhcpd[42317]: DHCPINFORM from 172.20.85.254
Jul  6 11:18:16 oberalp dhcpd[42317]: DHCPACK to 172.20.85.107 
() via 172.20.85.254
Jul  6 11:19:13 oberalp dhcpd[42317]: DHCPINFORM from 172.20.85.254
Jul  6 11:19:13 oberalp dhcpd[42317]: DHCPACK to 172.20.85.122 
() via 172.20.85.254
Jul  6 11:19:16 oberalp dhcpd[42317]: DHCPINFORM from 172.20.85.254
Jul  6 11:19:16 oberalp dhcpd[42317]: DHCPACK to 172.20.85.122 
() via 172.20.85.254
Jul  6 11:20:58 oberalp dhcpd[42317]: DHCPINFORM from 172.20.85.254
Jul  6 11:20:58 oberalp dhcpd[42317]: DHCPACK to 172.20.85.122 
() via 172.20.85.254
Jul  6 11:21:01 oberalp dhcpd[42317]: DHCPINFORM from 172.20.85.254
Jul  6 11:21:01 oberalp dhcpd[42317]: DHCPACK to 172.20.85.122 
() via 172.20.85.254

dhcpdump of the DHCPINFORM request:

OPTION:  53 (  1) DHCP message type 8 (DHCPINFORM)
OPTION:  61 (  7) Client-identifier 
OPTION:  12 (  7) Host name 
OPTION:  60 (  8) Vendor class identifier   MSFT 5.0
OPTION:  55 ( 13) Parameter Request List  1 (Subnet mask)
 15 (Domainname)
  3 (Routers)
  6 (DNS server)
 44 (NetBIOS name server)
 46 (NetBIOS node type)
 47 (NetBIOS scope)
 31 (Perform router discovery)
 33 (Static route)
121 (Classless Static Route)
249 (MSFT - Classless route)
 43 (Vendor specific info)
252 (MSFT - WinSock Proxy Auto 
Detect)

So i dunno if the clients behavious will adapt over time (searching for
'windows7 dhcpinform' on the interwebs leads to tons of pages..), or if it's
even worse in terms of syslog spam, but at least it seems more 'correct'
dhcp-wise, and apparently the client still has a working network.

Can't really okay it as i dont have all the big picture details,
but at least it doesnt break this setup.

Landry

Re: ifstated remove unused logging code

[Tim Hoddy]

On 6 July 2017 02:11:03 BST, Rob Pierce  wrote:
>This code has been here since version 1.1/1.2, but never used.
>
>Rob
>
>Index: ifstated.c
>===
>RCS file: /cvs/src/usr.sbin/ifstated/ifstated.c,v
>retrieving revision 1.50
>diff -u -p -r1.50 ifstated.c
>--- ifstated.c 4 Jul 2017 21:09:52 -   1.50
>+++ ifstated.c 6 Jul 2017 01:04:40 -
>@@ -656,9 +656,6 @@ remove_action(struct ifsd_action *action
>   return;
> 
>   switch (action->type) {
>-  case IFSD_ACTION_LOG:
>-  free(action->act.logmessage);
>-  break;
>   case IFSD_ACTION_COMMAND:
>   free(action->act.command);
>   break;
>
>Index: ifstated.h
>===
>RCS file: /cvs/src/usr.sbin/ifstated/ifstated.h,v
>retrieving revision 1.16
>diff -u -p -r1.16 ifstated.h
>--- ifstated.h 4 Jul 2017 21:04:14 -   1.16
>+++ ifstated.h 6 Jul 2017 01:04:40 -
>@@ -63,7 +63,6 @@ struct ifsd_action {
>   TAILQ_ENTRY(ifsd_action) entries;
>   struct ifsd_action  *parent;
>   union {
>-  char*logmessage;
>   char*command;
>   struct ifsd_state   *nextstate;
>   char*statename;
>@@ -73,7 +72,6 @@ struct ifsd_action {
>   } c;
>   } act;
>   u_int32_ttype;
>-#define IFSD_ACTION_LOG   0
> #define IFSD_ACTION_COMMAND   1
> #define IFSD_ACTION_CHANGESTATE   2
> #define IFSD_ACTION_CONDITION 3