[PATCH] etc/ksh.kshrc - unify command substitution
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
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
> 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)
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&T's SVR4 from 1988 had the same behaviour ;) Gerhard
Re: add simple ifstated regression test script
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 working/output
Re: Standard conformance of strtol(3)
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)
I've just committed a fix for this. - todd
Re: Standard conformance of strtol(3)
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
Hi, is/has anyone been working on a diff that would collect&move 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)
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
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
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(&addr) == 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 *)&config); > - > 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(physica
Re: mg backup directory (bump)
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, &fn[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()
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)
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()
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)
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)
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)
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
>> 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&m=144342287923444&w=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
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