less: merge upstream bugfixes

2021-10-09 Thread Tobias Stoeckmann
Hi,

this merges latest bugfixes from upstream to our version of less.
No new features introduced. Upstream commits and issues are linked as
references.

brac.c:
Signed integer overflow with huge files.
https://github.com/gwsw/less/pull/210
https://github.com/gwsw/less/commit/e6eb4c8ddd7f4e7135facad6c30d80886148ca70

command.c:
A prompt should not be shown if explicitly requested to not show one.
Reproducible by entering "-+e" within less. This should
not yield any status output (CTRL + SHIFT + P suppresses the prompt).
https://github.com/gwsw/less/commit/93fee11541b6837a0063e728e60c50da7929924b

decode.c:
Out of boundary accesses and endless loop with user-specified lesskey file
possible (-k option).
https://github.com/gwsw/less/pull/199
https://github.com/gwsw/less/pull/203
https://github.com/gwsw/less/commit/7318ae5ce310fe8a8784a8b0c80132099b11862c
https://github.com/gwsw/less/commit/d07da7152ecc2086809965646e1b8b7a95b6452c

optfunc.c, http to https:
Upstream changed URL to https, we should do the same.
https://github.com/gwsw/less/commit/a8b4980c8403f6f41ef5e534e6b8ad3b919604a3

optfunc.c:
Increase buffer to stay compatible with upstream. Our TABSTOP_MAX is large
enough to prevent overflow of the buffer already, but keep it in sync in
case we reduce TABSTOP_MAX to 32 just like upstream does by default.
https://github.com/gwsw/less/commit/6a860ee977eea7bfa065789ea4319ecab5af703c

option.c:
prchar has a larger buffer than propt uses internally. This does not lead to
an overflow, we could just truncate custom formatter outputs.
https://github.com/gwsw/less/commit/1d95a137938f347c78bdefa91bde6d7e3678bba0

Okay?


Tobias

Index: brac.c
===
RCS file: /cvs/src/usr.bin/less/brac.c,v
retrieving revision 1.9
diff -u -p -u -p -r1.9 brac.c
--- brac.c  9 Nov 2015 16:39:13 -   1.9
+++ brac.c  9 Oct 2021 10:58:27 -
@@ -75,6 +75,8 @@ match_brac(int obrac, int cbrac, int for
nest = 0;
while ((c = (*chget)()) != EOI) {
if (c == obrac) {
+   if (nest == INT_MAX)
+   break;
nest++;
} else if (c == cbrac && --nest < 0) {
/*
Index: command.c
===
RCS file: /cvs/src/usr.bin/less/command.c,v
retrieving revision 1.32
diff -u -p -u -p -r1.32 command.c
--- command.c   3 Sep 2019 23:08:42 -   1.32
+++ command.c   9 Oct 2021 10:58:28 -
@@ -264,6 +264,7 @@ is_erase_char(int c)
 static int
 mca_opt_first_char(int c)
 {
+   int no_prompt = (optflag & OPT_NO_PROMPT);
int flag = (optflag & ~OPT_NO_PROMPT);
if (flag == OPT_NO_TOGGLE) {
switch (c) {
@@ -277,12 +278,14 @@ mca_opt_first_char(int c)
switch (c) {
case '+':
/* "-+" = UNSET. */
-   optflag = (flag == OPT_UNSET) ? OPT_TOGGLE : OPT_UNSET;
+   optflag = no_prompt |
+   ((flag == OPT_UNSET) ? OPT_TOGGLE : OPT_UNSET);
mca_opt_toggle();
return (MCA_MORE);
case '!':
/* "-!" = SET */
-   optflag = (flag == OPT_SET) ? OPT_TOGGLE : OPT_SET;
+   optflag = no_prompt |
+   ((flag == OPT_SET) ? OPT_TOGGLE : OPT_SET);
mca_opt_toggle();
return (MCA_MORE);
case CONTROL('P'):
Index: decode.c
===
RCS file: /cvs/src/usr.bin/less/decode.c,v
retrieving revision 1.19
diff -u -p -u -p -r1.19 decode.c
--- decode.c28 Jun 2019 13:35:01 -  1.19
+++ decode.c9 Oct 2021 10:58:28 -
@@ -563,6 +563,7 @@ static int
 new_lesskey(char *buf, int len, int sysvar)
 {
char *p;
+   char *end;
int c;
int n;
 
@@ -575,21 +576,28 @@ new_lesskey(char *buf, int len, int sysv
buf[len-1] != C2_END_LESSKEY_MAGIC)
return (-1);
p = buf + 4;
+   end = buf + len;
for (;;) {
c = *p++;
switch (c) {
case CMD_SECTION:
n = gint();
+   if (n < 0 || p + n >= end)
+   return (-1);
add_fcmd_table(p, n);
p += n;
break;
case EDIT_SECTION:
n = gint();
+   if (n < 0 || p + n >= end)
+   return (-1);
add_ecmd_table(p, n);
p += n;
break;
case VAR_SECTION:
n = gint();
+   if (n < 0 || p + n >= end)
+   

less: tighten pledge in secure mode

2021-09-21 Thread Tobias Stoeckmann
Hi,

upstream (greenwood) less has disabled history file support for secure
mode, i.e. LESSSECURE=1: https://github.com/gwsw/less/pull/201

The problem was about permanent marks for which we do not have support
anyway. Users could possibly access files they should not be able to.

Since upstream does not allow history file in secure mode anymore we
could do the same and remove wpath from secure mode pledge.

I have added a note about history file to our manual page.

Comments? Okays?


Tobias

Index: cmdbuf.c
===
RCS file: /cvs/src/usr.bin/less/cmdbuf.c,v
retrieving revision 1.20
diff -u -p -u -p -r1.20 cmdbuf.c
--- cmdbuf.c2 Sep 2019 14:07:45 -   1.20
+++ cmdbuf.c21 Sep 2021 20:16:08 -
@@ -20,6 +20,7 @@
 #include "cmd.h"
 #include "less.h"
 
+extern int secure;
 extern int sc_width;
 extern int utf_mode;
 
@@ -1203,6 +1204,8 @@ init_cmdhist(void)
FILE *f;
char *p;
 
+   if (secure)
+   return;
filename = histfile_name();
if (filename == NULL)
return;
@@ -1274,6 +1277,8 @@ save_cmdhist(void)
struct stat statbuf;
int r;
 
+   if (secure)
+   return;
if (mlist_search.modified)
modified = 1;
if (mlist_shell.modified)
Index: less.1
===
RCS file: /cvs/src/usr.bin/less/less.1,v
retrieving revision 1.57
diff -u -p -u -p -r1.57 less.1
--- less.1  2 Sep 2019 14:07:45 -   1.57
+++ less.1  21 Sep 2021 20:16:09 -
@@ -1697,6 +1697,8 @@ Use of lesskey files.
 .It Fl t
 Use of tags files.
 .It " "
+Use of history file.
+.It " "
 Metacharacters in filenames, such as "*".
 .It " "
 Filename completion (TAB, ^L).
Index: main.c
===
RCS file: /cvs/src/usr.bin/less/main.c,v
retrieving revision 1.37
diff -u -p -u -p -r1.37 main.c
--- main.c  28 Jun 2019 05:44:09 -  1.37
+++ main.c  21 Sep 2021 20:16:09 -
@@ -91,7 +91,7 @@ main(int argc, char *argv[])
secure = 1;
 
if (secure) {
-   if (pledge("stdio rpath wpath tty", NULL) == -1) {
+   if (pledge("stdio rpath tty", NULL) == -1) {
perror("pledge");
exit(1);
}



libevent: prevent integer overflow in kqueue

2019-05-07 Thread Tobias Stoeckmann
This patch avoids a possible integer overflow on excessively large
amount of events added to an event base in kqueue mode (default).

Just as with previous changes, this is very unlikely to trigger and
is a just a defensive measure.

Changes in this diff:

* KNF (sorted imports and added limits.h for INT_MAX)
* recallocarray instead of reallocarray, in sync with minheap change
* adjusted error messages from malloc to recallocarray

Okay?


Tobias

Index: kqueue.c
===
RCS file: /cvs/src/lib/libevent/kqueue.c,v
retrieving revision 1.40
diff -u -p -u -p -r1.40 kqueue.c
--- kqueue.c10 Jul 2017 21:37:26 -  1.40
+++ kqueue.c7 May 2019 19:49:53 -
@@ -32,14 +32,15 @@
 #include 
 #include 
 
+#include 
+#include 
+#include 
+#include 
 #include 
 #include 
 #include 
 #include 
 #include 
-#include 
-#include 
-#include 
 
 #include "event.h"
 #include "event-internal.h"
@@ -133,25 +134,29 @@ kq_insert(struct kqop *kqop, struct keve
struct kevent *newchange;
struct kevent *newresult;
 
+   if (nevents > INT_MAX / 2) {
+   event_warnx("%s: integer overflow", __func__);
+   return (-1);
+   }
nevents *= 2;
 
-   newchange = reallocarray(kqop->changes,
-   nevents, sizeof(struct kevent));
+   newchange = recallocarray(kqop->changes,
+   kqop->nevents, nevents, sizeof(struct kevent));
if (newchange == NULL) {
-   event_warn("%s: malloc", __func__);
+   event_warn("%s: recallocarray", __func__);
return (-1);
}
kqop->changes = newchange;
 
-   newresult = reallocarray(kqop->events,
-   nevents, sizeof(struct kevent));
+   newresult = recallocarray(kqop->events,
+   kqop->nevents, nevents, sizeof(struct kevent));
 
/*
 * If we fail, we don't have to worry about freeing,
 * the next realloc will pick it up.
 */
if (newresult == NULL) {
-   event_warn("%s: malloc", __func__);
+   event_warn("%s: recallocarray", __func__);
return (-1);
}
kqop->events = newresult;



libevent: endless loop on excessively large buffers

2019-05-02 Thread Tobias Stoeckmann
It is possible to trigger an endless loop or out of boundary write
on 64 bit systems with evbuffer_readline calls for buffers which
exceed 4 GB (i.e. overflow uint).

for (i = 0; i < len; i++)

Variable i is unsigned int and len size_t. This leads to an endless
loop if len is larger than UINT_MAX.

If the loop ends exactly at UINT_MAX, then a malloc with additional
space for an ending '\0' leads to an overflow, effectively allocating
0 bytes and therefore an out of boundary write occurs.

This is a proof of concept for an endless loop:

---
#include 
#include 
#include 
#include 
#include 

int
main(void)
{
char *buf;
const size_t len = 0x1UL;
struct evbuffer *buffer;
char *line;

if ((buf = malloc(len)) == NULL)
err(1, "malloc");
memset(buf, 'A', len);

if ((buffer = evbuffer_new()) == NULL)
errx(1, "evbuffer_new");
if (evbuffer_expand(buffer, len + 1))
errx(1, "evbuffer_expand");
evbuffer_add(buffer, buf, len);
evbuffer_add(buffer, "", 1);

printf("%p\n", evbuffer_readline(buffer));
return 0;
}
---

Generally this is a rather theoretical case. Normal users are not
allowed to allocate so much memory. But better be safe than sorry,
especially if login.conf values were adjusted (or the process runs
as root).

This patch completely removes "unsigned int" from buffer.c.


Index: buffer.c
===
RCS file: /cvs/src/lib/libevent/buffer.c,v
retrieving revision 1.31
diff -u -p -u -p -r1.31 buffer.c
--- buffer.c18 Mar 2017 01:48:43 -  1.31
+++ buffer.c1 May 2019 11:00:29 -
@@ -188,7 +188,7 @@ evbuffer_readline(struct evbuffer *buffe
u_char *data = EVBUFFER_DATA(buffer);
size_t len = EVBUFFER_LENGTH(buffer);
char *line;
-   unsigned int i;
+   size_t i;
 
for (i = 0; i < len; i++) {
if (data[i] == '\r' || data[i] == '\n')
@@ -232,7 +232,7 @@ evbuffer_readln(struct evbuffer *buffer,
u_char *start_of_eol, *end_of_eol;
size_t len = EVBUFFER_LENGTH(buffer);
char *line;
-   unsigned int i, n_to_copy, n_to_drain;
+   size_t i, n_to_copy, n_to_drain;
 
if (n_read_out)
*n_read_out = 0;



Re: libevent: remove non-monotonic compat code

2019-05-01 Thread Tobias Stoeckmann
On Tue, Apr 30, 2019 at 07:13:55PM +0200, Jeremie Courreges-Anglas wrote:
> > So the diff below removes the fallback path and all associated code and
> > variables.
> 
> > I have left out some minor cleanups for now to ease reviews.
> 
> Here's a diff that amends the signature of gettime() and makes use of
> TIMESPEC_TO_TIMEVAL().

Okay for both.



Re: libevent: Protect integer multiplications (min_heap)

2019-04-22 Thread Tobias Stoeckmann
On Sun, Apr 21, 2019 at 08:53:11PM +0200, Alexander Bluhm wrote:
> I wonder whether SIZE_T_MAX would be better than -1.  But they seem
> to be equivalent.

I prefer SIZE_T_MAX as well.

> > Index: min_heap.h
> > ===
> > RCS file: /cvs/src/lib/libevent/min_heap.h,v
> > retrieving revision 1.5
> > diff -u -p -r1.5 min_heap.h
> > --- min_heap.h  20 Apr 2019 23:22:28 -  1.5
> > +++ min_heap.h  20 Apr 2019 23:28:09 -
> > @@ -33,7 +33,7 @@
> >
> >  typedef struct min_heap {
> > struct event **p;
> > -   unsigned n, a;
> > +   size_t n, a;
> >  } min_heap_t;

Changing n means that at least timeout_correct in event.c must be
adjusted as well, because it is used as an unsigned:

static void
timeout_correct(struct event_base *base, struct timeval *tv)
{
unsigned int size;
...
size = base->timeheap.n;
for (; size-- > 0; ++pev) {
struct timeval *ev_tv = &(**pev).ev_timeout;
timersub(ev_tv, , ev_tv);
}
...
}

Looking only at min_heap.h it is okay, but takes a bit polishing to be
applicable for all files in libevent.



Tobias



Re: libevent: Protect integer multiplications (min_heap)

2019-04-22 Thread Tobias Stoeckmann
On Mon, Apr 22, 2019 at 01:24:13PM -0400, Ted Unangst wrote:
> ah, good catch. I don't think it's wrong (until it overflows) but needs to be
> fixed. Needs some more review then. Thanks.

I have added following changes:

- converted 0u to 0 as bluhm pointed out
- converted -1 to SIZE_MAX whereever it was used as "illegal index"
  as bluhm mentioned as well
- converted timemap's size usages in event.c from (u)int to size_t

Passes regress tests.

I'll discuss this with libevent maintainers to get these changes into
upstream as well.


Index: event.c
===
RCS file: /cvs/src/lib/libevent/event.c,v
retrieving revision 1.38
diff -u -p -u -p -r1.38 event.c
--- event.c 6 Jan 2015 23:11:23 -   1.38
+++ event.c 22 Apr 2019 17:44:17 -
@@ -163,7 +163,8 @@ event_base_new(void)
 void
 event_base_free(struct event_base *base)
 {
-   int i, n_deleted=0;
+   int i;
+   size_t n_deleted=0;
struct event *ev;
 
if (base == NULL && current_base)
@@ -199,7 +200,7 @@ event_base_free(struct event_base *base)
}
 
if (n_deleted)
-   event_debug(("%s: %d events were still set in base",
+   event_debug(("%s: %z events were still set in base",
__func__, n_deleted));
 
if (base->evsel->dealloc != NULL)
@@ -846,7 +847,7 @@ static void
 timeout_correct(struct event_base *base, struct timeval *tv)
 {
struct event **pev;
-   unsigned int size;
+   size_t size;
struct timeval off;
 
if (use_monotonic)
Index: event.h
===
RCS file: /cvs/src/lib/libevent/event.h,v
retrieving revision 1.30
diff -u -p -u -p -r1.30 event.h
--- event.h 5 Jan 2015 23:14:36 -   1.30
+++ event.h 22 Apr 2019 17:44:17 -
@@ -196,7 +196,7 @@ struct event {
TAILQ_ENTRY (event) ev_next;
TAILQ_ENTRY (event) ev_active_next;
TAILQ_ENTRY (event) ev_signal_next;
-   unsigned int min_heap_idx;  /* for managing timeouts */
+   size_t min_heap_idx;/* for managing timeouts */
 
struct event_base *ev_base;
 
Index: min_heap.h
===
RCS file: /cvs/src/lib/libevent/min_heap.h,v
retrieving revision 1.5
diff -u -p -u -p -r1.5 min_heap.h
--- min_heap.h  20 Apr 2019 23:22:28 -  1.5
+++ min_heap.h  22 Apr 2019 17:44:17 -
@@ -33,7 +33,7 @@
 
 typedef struct min_heap {
struct event **p;
-   unsigned n, a;
+   size_t n, a;
 } min_heap_t;
 
 static inline void min_heap_ctor(min_heap_t * s);
@@ -41,14 +41,14 @@ static inline void min_heap_dtor(min_hea
 static inline void min_heap_elem_init(struct event * e);
 static inline int min_heap_elem_greater(struct event * a, struct event * b);
 static inline int min_heap_empty(min_heap_t * s);
-static inline unsigned min_heap_size(min_heap_t * s);
+static inline size_t min_heap_size(min_heap_t * s);
 static inline struct event *min_heap_top(min_heap_t * s);
-static inline int min_heap_reserve(min_heap_t * s, unsigned n);
+static inline int min_heap_reserve(min_heap_t * s, size_t n);
 static inline int min_heap_push(min_heap_t * s, struct event * e);
 static inline struct event *min_heap_pop(min_heap_t * s);
 static inline int min_heap_erase(min_heap_t * s, struct event * e);
-static inline void min_heap_shift_up_(min_heap_t * s, unsigned hole_index, 
struct event * e);
-static inline void min_heap_shift_down_(min_heap_t * s, unsigned hole_index, 
struct event * e);
+static inline void min_heap_shift_up_(min_heap_t * s, size_t hole_index, 
struct event * e);
+static inline void min_heap_shift_down_(min_heap_t * s, size_t hole_index, 
struct event * e);
 
 int 
 min_heap_elem_greater(struct event * a, struct event * b)
@@ -70,14 +70,14 @@ void min_heap_dtor(min_heap_t * s) {
 void 
 min_heap_elem_init(struct event * e)
 {
-   e->min_heap_idx = -1;
+   e->min_heap_idx = SIZE_MAX;
 }
 int 
 min_heap_empty(min_heap_t * s)
 {
-   return 0u == s->n;
+   return 0 == s->n;
 }
-unsigned 
+size_t 
 min_heap_size(min_heap_t * s)
 {
return s->n;
@@ -102,8 +102,8 @@ min_heap_pop(min_heap_t * s)
 {
if (s->n) {
struct event *e = *s->p;
-   min_heap_shift_down_(s, 0u, s->p[--s->n]);
-   e->min_heap_idx = -1;
+   min_heap_shift_down_(s, 0, s->p[--s->n]);
+   e->min_heap_idx = SIZE_MAX;
return e;
}
return 0;
@@ -112,9 +112,9 @@ min_heap_pop(min_heap_t * s)
 int 
 min_heap_erase(min_heap_t * s, struct event * e)
 {
-   if (((unsigned int)-1) != e->min_heap_idx) {
+   if (e->min_heap_idx != SIZE_MAX) {
struct event *last = s->p[--s->n];
-   unsigned parent = (e->min_heap_idx - 1) / 2;
+   size_t parent = (e->min_heap_idx - 1) / 2;
/*
 * we 

Re: libevent: Protect integer multiplications (min_heap)

2019-04-22 Thread Tobias Stoeckmann
On Sun, Apr 21, 2019 at 08:53:11PM +0200, Alexander Bluhm wrote:
> I wonder whether SIZE_T_MAX would be better than -1.  But they seem
> to be equivalent.

I prefer SIZE_T_MAX as well.

> > Index: min_heap.h
> > ===
> > RCS file: /cvs/src/lib/libevent/min_heap.h,v
> > retrieving revision 1.5
> > diff -u -p -r1.5 min_heap.h
> > --- min_heap.h  20 Apr 2019 23:22:28 -  1.5
> > +++ min_heap.h  20 Apr 2019 23:28:09 -
> > @@ -33,7 +33,7 @@
> >
> >  typedef struct min_heap {
> > struct event **p;
> > -   unsigned n, a;
> > +   size_t n, a;
> >  } min_heap_t;

Changing n means that at least timeout_correct in event.c must be
adjusted as well, because it is used as an unsigned:

static void
timeout_correct(struct event_base *base, struct timeval *tv)
{
unsigned int size;
...
size = base->timeheap.n;
for (; size-- > 0; ++pev) {
struct timeval *ev_tv = &(**pev).ev_timeout;
timersub(ev_tv, , ev_tv);
}
...
}

Looking only at min_heap.h it is okay, but takes a bit polishing to be
applicable for all files in libevent.

I haven't inspected all occurrences, so no updated patch yet.


Tobias



Re: libevent: Protect integer multiplications (min_heap)

2019-04-18 Thread Tobias Stoeckmann
On Wed, Apr 17, 2019 at 11:34:36AM -0400, Ted Unangst wrote:
> (and then reformat to be knf, but after changes that require review.)

Totally agree here. That will help with further changes to this file!

> 
> Index: min_heap.h
> ===
> RCS file: /home/cvs/src/lib/libevent/min_heap.h,v
> retrieving revision 1.3
> diff -u -p -r1.3 min_heap.h
> --- min_heap.h29 Oct 2014 22:47:29 -  1.3
> +++ min_heap.h17 Apr 2019 15:30:02 -
> @@ -112,7 +112,7 @@ int min_heap_reserve(min_heap_t* s, unsi
>  unsigned a = s->a ? s->a * 2 : 8;
>  if(a < n)
>  a = n;
> -if(!(p = (struct event**)realloc(s->p, a * sizeof *p)))
> +if(!(p = realloc(s->p, a * sizeof *p)))
>  return -1;
>  s->p = p;
>  s->a = a;
> @@ -125,11 +125,13 @@ void min_heap_shift_up_(min_heap_t* s, u
>  unsigned parent = (hole_index - 1) / 2;
>  while(hole_index && min_heap_elem_greater(s->p[parent], e))
>  {
> -(s->p[hole_index] = s->p[parent])->min_heap_idx = hole_index;
> +s->p[hole_index] = s->p[parent];
> +s->p[hole_index]->min_heap_idx = hole_index;
>  hole_index = parent;
>  parent = (hole_index - 1) / 2;
>  }
> -(s->p[hole_index] = e)->min_heap_idx = hole_index;
> +e->min_heap_idx = hole_index;
> +s->p[hole_index] = e;
>  }
>  
>  void min_heap_shift_down_(min_heap_t* s, unsigned hole_index, struct event* 
> e)
> @@ -137,10 +139,13 @@ void min_heap_shift_down_(min_heap_t* s,
>  unsigned min_child = 2 * (hole_index + 1);
>  while(min_child <= s->n)
>   {
> -min_child -= min_child == s->n || 
> min_heap_elem_greater(s->p[min_child], s->p[min_child - 1]);
> + if (min_child == s->n ||
> + min_heap_elem_greater(s->p[min_child], s->p[min_child - 1]))
> + min_child -= 1;
>  if(!(min_heap_elem_greater(e, s->p[min_child])))
>  break;
> -(s->p[hole_index] = s->p[min_child])->min_heap_idx = hole_index;
> +s->p[hole_index] = s->p[min_child];
> +s->p[hole_index]->min_heap_idx = hole_index;
>  hole_index = min_child;
>  min_child = 2 * (hole_index + 1);
>   }



libevent: Protect integer multiplications (min_heap)

2019-04-16 Thread Tobias Stoeckmann
I would like to protect min_heap_push against integer overflows,
which could either be triggered on a 64 bit system with massive
amounts of RAM (to overflow s->n) or on a 32 bit system with tight
memory layout (overflowing a * sizeof *p).

Both cases are basically not possible to be triggered, but I prefer
to have reallocarray in place to be sure about it.

Okay?

Index: min_heap.h
===
RCS file: /cvs/src/lib/libevent/min_heap.h,v
retrieving revision 1.3
diff -u -p -u -p -r1.3 min_heap.h
--- min_heap.h  29 Oct 2014 22:47:29 -  1.3
+++ min_heap.h  16 Apr 2019 18:26:11 -
@@ -65,7 +65,7 @@ struct event* min_heap_top(min_heap_t* s
 
 int min_heap_push(min_heap_t* s, struct event* e)
 {
-if(min_heap_reserve(s, s->n + 1))
+if(s->n == UINT32_MAX || min_heap_reserve(s, s->n + 1))
 return -1;
 min_heap_shift_up_(s, s->n++, e);
 return 0;
@@ -112,7 +112,7 @@ int min_heap_reserve(min_heap_t* s, unsi
 unsigned a = s->a ? s->a * 2 : 8;
 if(a < n)
 a = n;
-if(!(p = (struct event**)realloc(s->p, a * sizeof *p)))
+if((p = reallocarray(s->p, a, sizeof *p)) == NULL)
 return -1;
 s->p = p;
 s->a = a;



Re: dd(1): don't cast malloc(3) size to u_int

2018-07-21 Thread Tobias Stoeckmann
On Fri, Jul 20, 2018 at 06:51:33PM -0500, Scott Cheloha wrote:
> tobias@ spotted this one a few months ago when reviewing a different
> diff of mine.

Yeah, I remember that one. Brings up memories of another diff of that
time as well.
 
> ... ok as-is?

Yeah, okay by me as well.


Tobias



ksh: fix buffer overflow in u64ton

2018-04-09 Thread Tobias Stoeckmann
As tb@ pointed out, u64ton can overflow on ULONG_MAX. It could also
happen on systems with 64 bit int and INT_MIN, although we don't have
such a system supported by our code base.

You can reach the u64ton function by printing the length of a string
within a variable like this:

$ a=string
$ echo ${#a}
6
$ _

Technically there is no need to use BITS(uint64_t) because 20+2 would
be enough. But it seems that there is a strong preference towards
going "long long" instead of int64_t due to return types of time()
and strtonum() as well as a highly theoretical support of larger types
than 64 bit in long long. Although that probably will never happen.

So, if we go with a data type like long long, I would prefer to have
the code prepared to scale its buffer with the data type, therefore
BITS(). And let's be honest -- nobody will notice this waste of extra
44 bytes.


Tobias

Index: eval.c
===
RCS file: /cvs/src/bin/ksh/eval.c,v
retrieving revision 1.60
diff -u -p -u -p -r1.60 eval.c
--- eval.c  9 Apr 2018 17:53:36 -   1.60
+++ eval.c  9 Apr 2018 18:47:45 -
@@ -732,7 +732,7 @@ varsub(Expand *xp, char *sp, char *word,
if (Flag(FNOUNSET) && c == 0 && !zero_ok)
errorf("%s: parameter not set", sp);
*stypep = 0; /* unqualified variable/string substitution */
-   xp->str = str_save(u64ton((uint64_t)c, 10), ATEMP);
+   xp->str = str_save(u64ton((uint64_t)c, '\0'), ATEMP);
return XSUB;
}
 
Index: misc.c
===
RCS file: /cvs/src/bin/ksh/misc.c,v
retrieving revision 1.70
diff -u -p -u -p -r1.70 misc.c
--- misc.c  9 Apr 2018 17:53:36 -   1.70
+++ misc.c  9 Apr 2018 18:47:45 -
@@ -56,20 +56,22 @@ initctypes(void)
setctypes(" \n\t\"#$&'()*;<>?[\\`|", C_QUOTE);
 }
 
-/* convert uint64_t to base N string */
+/* convert uint64_t N to a base 10 number as string with optional PREFIX */
 
 char *
-u64ton(uint64_t n, int base)
+u64ton(uint64_t n, char prefix)
 {
char *p;
-   static char buf [20];
+   static char buf [BITS(uint64_t) + 2];
 
p = [sizeof(buf)];
*--p = '\0';
do {
-   *--p = "0123456789ABCDEF"[n%base];
-   n /= base;
+   *--p = "0123456789ABCDEF"[n % 10];
+   n /= 10;
} while (n != 0);
+   if (prefix != '\0')
+   *--p = prefix;
return p;
 }
 
Index: sh.h
===
RCS file: /cvs/src/bin/ksh/sh.h,v
retrieving revision 1.72
diff -u -p -u -p -r1.72 sh.h
--- sh.h9 Apr 2018 17:53:36 -   1.72
+++ sh.h9 Apr 2018 18:47:45 -
@@ -527,7 +527,7 @@ voidcleanup_proc_env(void);
 /* misc.c */
 void   setctypes(const char *, int);
 void   initctypes(void);
-char * u64ton(uint64_t, int);
+char * u64ton(uint64_t, char);
 char * str_save(const char *, Area *);
 char * str_nsave(const char *, int, Area *);
 intoption(const char *);
Index: tree.c
===
RCS file: /cvs/src/bin/ksh/tree.c,v
retrieving revision 1.34
diff -u -p -u -p -r1.34 tree.c
--- tree.c  9 Apr 2018 17:53:36 -   1.34
+++ tree.c  9 Apr 2018 18:47:45 -
@@ -376,9 +376,7 @@ vfptreef(struct shf *shf, int indent, co
case 'd': /* decimal */
n = va_arg(va, int);
neg = n < 0;
-   p = u64ton(neg ? -n : n, 10);
-   if (neg)
-   *--p = '-';
+   p = u64ton(neg ? -n : n, neg ? '-' : '\0');
while (*p)
tputc(*p++, shf);
break;
@@ -392,7 +390,7 @@ vfptreef(struct shf *shf, int indent, co
tputS(p, shf);
break;
case 'u': /* unsigned decimal */
-   p = u64ton(va_arg(va, unsigned int), 10);
+   p = u64ton(va_arg(va, unsigned int), '\0');
while (*p)
tputc(*p++, shf);
break;



Re: ksh: support 64 bit numbers on 32 bit archs

2018-04-07 Thread Tobias Stoeckmann
On Wed, Apr 04, 2018 at 01:30:52PM +0200, Theo Buehler wrote:
> > +/* convert uint64_t to base N string */
> > 
> >  char *
> > -ulton(long unsigned int n, int base)
> > +u64ton(uint64_t n, int base)
> >  {
> > char *p;
> > static char buf [20];
> 
> I don't think it's actually an issue since I don't see how n can ever be
> that large, but 20 is too small to hold a NUL-terminated base 10 version
> of UINT64_MAX, as 18446744073709551615 has 20 digits. Thus,
> u64ton(UINT64_MAX, 10) will write to and return buf[-1].
> 
> With bases < 10 you'll have trouble with buf[21] (base 8 will need
> buf[23] and base 2 will need buf[65]).
> 
> Since all three callers of u64ton() use base 10 anyway, doing buf[21]
> would be enough for now. I wonder if we should get rid of the base
> argument and simplify accordingly (that would be a separate diff).

I fully agree with your spottings. It's actually worse than that.
Take a look at tree.c:

p = ulton((neg) ? -n : n, 10);
if (neg)
*--p = '-';

It means that we would need buf[22]. But I don't want to fix it in this
patch, because this issue is not related to a switch from 32 to 64 bits
but an already existing issue (and it's theoretical, it would take
an int of size 64 bit if you look above that code snippet).

What I want to do with another patch:

- Remove the "n" argument, it's always 10
- Instead, add a "char prefix" argument which could be '-' for negative
  numbers. Nobody should ever change a returned pointer like that.
- Fix the comment above the function

But this is just an issue like the other existing integer overflows
which have already been fixed in test and expr. I want to fix them in
the same way in ksh, thus a unification of the code bases should be
first priority.


Tobias



ksh: support 64 bit numbers on 32 bit archs

2018-04-03 Thread Tobias Stoeckmann
Hi,

this patch increases the number range on 32 bit architectures like
i386 to 64 bit. These are already supported on 64 bit architectures
due to using "long".

The rational behind this patch is to unify test/expr/ksh in allowing
64 bit integers, making variable handling more consistent in base
tools.

Also, keep in mind that 32 bit architectures also support large files,
so 64 bit file sizes in variables aren't totally out of the question.

When this is in, I want to merge the expr integer overflow checks
into ksh.

Verification of this patch is rather easy on amd64, because binary
does not change. I have extended regress tests to show that i386 can
handle 64 bit integers.

My i386 system is running with this patch and nothing broke so far.


Tobias

Index: bin/ksh/c_ksh.c
===
RCS file: /cvs/src/bin/ksh/c_ksh.c,v
retrieving revision 1.59
diff -u -p -u -p -r1.59 c_ksh.c
--- bin/ksh/c_ksh.c 15 Mar 2018 16:51:29 -  1.59
+++ bin/ksh/c_ksh.c 3 Apr 2018 18:00:31 -
@@ -1017,7 +1017,7 @@ int
 c_let(char **wp)
 {
int rv = 1;
-   long val;
+   int64_t val;
 
if (wp[1] == NULL) /* at ksh does this */
bi_errorf("no arguments");
Index: bin/ksh/c_sh.c
===
RCS file: /cvs/src/bin/ksh/c_sh.c,v
retrieving revision 1.62
diff -u -p -u -p -r1.62 c_sh.c
--- bin/ksh/c_sh.c  27 Dec 2017 13:02:57 -  1.62
+++ bin/ksh/c_sh.c  3 Apr 2018 18:00:31 -
@@ -33,7 +33,7 @@ c_shift(char **wp)
 {
struct block *l = genv->loc;
int n;
-   long val;
+   int64_t val;
char *arg;
 
if (ksh_getopt(wp, _opt, null) == '?')
Index: bin/ksh/c_test.c
===
RCS file: /cvs/src/bin/ksh/c_test.c,v
retrieving revision 1.24
diff -u -p -u -p -r1.24 c_test.c
--- bin/ksh/c_test.c26 Dec 2017 19:10:31 -  1.24
+++ bin/ksh/c_test.c3 Apr 2018 18:00:31 -
@@ -308,7 +308,7 @@ test_eval(Test_env *te, Test_op op, cons
case TO_INTLE: /* -le */
case TO_INTLT: /* -lt */
{
-   long v1, v2;
+   int64_t v1, v2;
 
if (!evaluate(opnd1, , KSH_RETURN_ERROR, false) ||
!evaluate(opnd2, , KSH_RETURN_ERROR, false)) {
Index: bin/ksh/c_ulimit.c
===
RCS file: /cvs/src/bin/ksh/c_ulimit.c,v
retrieving revision 1.27
diff -u -p -u -p -r1.27 c_ulimit.c
--- bin/ksh/c_ulimit.c  15 Mar 2018 16:51:29 -  1.27
+++ bin/ksh/c_ulimit.c  3 Apr 2018 18:00:31 -
@@ -22,6 +22,7 @@
 
 #include 
 #include 
+#include 
 #include 
 
 #include "sh.h"
@@ -139,7 +140,7 @@ set_ulimit(const struct limits *l, const
if (strcmp(v, "unlimited") == 0)
val = RLIM_INFINITY;
else {
-   long rval;
+   int64_t rval;
 
if (!evaluate(v, , KSH_RETURN_ERROR, false))
return 1;
@@ -187,6 +188,6 @@ print_ulimit(const struct limits *l, int
shprintf("unlimited\n");
else {
val /= l->factor;
-   shprintf("%ld\n", (long) val);
+   shprintf("%" PRIi64 "\n", (int64_t) val);
}
 }
Index: bin/ksh/edit.c
===
RCS file: /cvs/src/bin/ksh/edit.c,v
retrieving revision 1.64
diff -u -p -u -p -r1.64 edit.c
--- bin/ksh/edit.c  15 Mar 2018 16:51:29 -  1.64
+++ bin/ksh/edit.c  3 Apr 2018 18:00:31 -
@@ -80,10 +80,10 @@ check_sigwinch(void)
ws.ws_col;
 
if ((vp = typeset("COLUMNS", 0, 0, 0, 0)))
-   setint(vp, (long) ws.ws_col);
+   setint(vp, (int64_t) ws.ws_col);
}
if (ws.ws_row && (vp = typeset("LINES", 0, 0, 0, 0)))
-   setint(vp, (long) ws.ws_row);
+   setint(vp, (int64_t) ws.ws_row);
}
}
 }
Index: bin/ksh/eval.c
===
RCS file: /cvs/src/bin/ksh/eval.c,v
retrieving revision 1.59
diff -u -p -u -p -r1.59 eval.c
--- bin/ksh/eval.c  16 Jan 2018 22:52:32 -  1.59
+++ bin/ksh/eval.c  3 Apr 2018 18:00:31 -
@@ -732,7 +732,7 @@ varsub(Expand *xp, char *sp, char *word,
if (Flag(FNOUNSET) && c == 0 && !zero_ok)
errorf("%s: parameter not set", sp);
*stypep = 0; /* unqualified variable/string substitution */
-   xp->str = str_save(ulton((unsigned long)c, 10), ATEMP);
+   xp->str = str_save(u64ton((uint64_t)c, 10), ATEMP);
return XSUB;
}
 
Index: 

Re: expr: Fix integer overflows

2018-03-31 Thread Tobias Stoeckmann
Hi,

On Sat, Mar 31, 2018 at 02:57:45AM +0200, Ingo Schwarze wrote:
> Even though - as discussed previously for test(1) - behaviour is
> undefined by POSIX outside the range 0 to 2147483647, we are allowed
> to handle a larger range, and i agree that handling the range
> -9223372036854775808 to 9223372036854775807 (INT64_MIN to INT64_MAX)
> seems desirable.

I actually ended up in expr(1) after seeing that the test(1) and ksh(1)
debate could be continued here. While expr(1) is int64_t, expressions
in ksh(1) are of type long, i.e. 32/64 bit depending on architecture.

This patch therefore is a preparation to get test(1) and expr(1) changes
into ksh. As both utilities are standalone, it is much easier to discuss
and make modifications in them, first.

> > +   if (c < 0 || INT64_MAX / 10 < i || INT64_MAX - c < i * 10)
> > +   errx(3, "out of range");
> 
> I'm not saying this is an absolute show-stopper, but i consider
> it somewhat ugly.  What do you think about using strtonum(3)
> instead in is_integer?

Totally agree. Patch is adjusted. I tried to keep it as strict as it
was before, but you are correct. We don't violate POSIX with strtonum,
we just accept a few more inputs -- being in sync with ksh and test.

>$ /obin/expr.tobias -36854775808 - \( -9223372036854775807 - 1 \)
>   expr.tobias: overflow
> 
>   if ((l->u.i >= 0 && r->u.i < 0 && res <= 0) ||

I have adjusted the code. Awesome review skills and thanks a lot! Your
example also triggers an overflow in FreeBSD's expr, so it is a good
idea to inform them when we are happy with our fixed version.

> Do you want to check and re-send your patch?

Most certainly. Adjusted patch and extended regress tests in this mail.
Thanks a lot again!


Tobias

Index: bin/expr/expr.c
===
RCS file: /cvs/src/bin/expr/expr.c,v
retrieving revision 1.26
diff -u -p -u -p -r1.26 expr.c
--- bin/expr/expr.c 19 Oct 2016 18:20:25 -  1.26
+++ bin/expr/expr.c 31 Mar 2018 11:22:05 -
@@ -19,8 +19,8 @@
 struct val *make_int(int64_t);
 struct val *make_str(char *);
 voidfree_value(struct val *);
-int is_integer(struct val *, int64_t *);
-int to_integer(struct val *);
+int is_integer(struct val *, int64_t *, const char **);
+int to_integer(struct val *, const char **);
 voidto_string(struct val *);
 int is_zero_or_null(struct val *);
 voidnexttoken(int);
@@ -94,11 +94,9 @@ free_value(struct val *vp)
 
 /* determine if vp is an integer; if so, return it's value in *r */
 int
-is_integer(struct val *vp, int64_t *r)
+is_integer(struct val *vp, int64_t *r, const char **errstr)
 {
-   char   *s;
-   int neg;
-   int64_t i;
+   const char *errstrp;
 
if (vp->type == integer) {
*r = vp->u.i;
@@ -107,43 +105,29 @@ is_integer(struct val *vp, int64_t *r)
 
/*
 * POSIX.2 defines an "integer" as an optional unary minus
-* followed by digits.
+* followed by digits. Other representations are unspecified,
+* which means that strtonum(3) is a viable option here.
 */
-   s = vp->u.s;
-   i = 0;
+   if (errstr == NULL)
+   errstr = 
+   *r = strtonum(vp->u.s, INT64_MIN, INT64_MAX, errstr);
+   if (*errstr != NULL)
+   return 0;
 
-   neg = (*s == '-');
-   if (neg)
-   s++;
-
-   while (*s) {
-   if (!isdigit((unsigned char)*s))
-   return 0;
-
-   i *= 10;
-   i += *s - '0';
-
-   s++;
-   }
-
-   if (neg)
-   i *= -1;
-
-   *r = i;
return 1;
 }
 
 
 /* coerce to vp to an integer */
 int
-to_integer(struct val *vp)
+to_integer(struct val *vp, const char **errstr)
 {
int64_t r;
 
if (vp->type == integer)
return 1;
 
-   if (is_integer(vp, )) {
+   if (is_integer(vp, , errstr)) {
free(vp->u.s);
vp->u.i = r;
vp->type = integer;
@@ -176,7 +160,7 @@ is_zero_or_null(struct val *vp)
if (vp->type == integer)
return vp->u.i == 0;
else
-   return *vp->u.s == 0 || (to_integer(vp) && vp->u.i == 0);
+   return *vp->u.s == 0 || (to_integer(vp, NULL) && vp->u.i == 0);
 }
 
 void
@@ -303,20 +287,25 @@ eval5(void)
 struct val *
 eval4(void)
 {
-   struct val *l, *r;
-   enum token  op;
+   const char  *errstr;
+   struct val  *l, *r;
+   enum token   op;
+   volatile int64_t res;
 
l = eval5();
while ((op = token) == MUL || op == DIV || op == MOD) {
nexttoken(0);
r = eval5();
 
-   if (!to_integer(l) || !to_integer(r)) {
-   errx(2, "non-numeric 

expr: Fix integer overflows

2018-03-30 Thread Tobias Stoeckmann
Our expr implementation supports 64 bit integers, but does not check
for overflows during parsing and arithmetical operations.

This patch fixes the problems based on FreeBSD's implementation, which
is a bit more advanced than NetBSD, which it is based upon.

The added regression test case is taken from NetBSD and passes with
this patch.

Okay?


Tobias

Index: regress/bin/Makefile
===
RCS file: /cvs/src/regress/bin/Makefile,v
retrieving revision 1.13
diff -u -p -u -p -r1.13 Makefile
--- regress/bin/Makefile14 Jan 2018 22:04:47 -  1.13
+++ regress/bin/Makefile30 Mar 2018 15:33:50 -
@@ -1,6 +1,6 @@
 #  $OpenBSD: Makefile,v 1.13 2018/01/14 22:04:47 bluhm Exp $
 
-SUBDIR+= cat chmod csh ed ksh ln md5 pax ps test
+SUBDIR+= cat chmod csh ed expr ksh ln md5 pax ps test
 
 install:
 
Index: bin/expr/expr.c
===
RCS file: /cvs/src/bin/expr/expr.c,v
retrieving revision 1.26
diff -u -p -u -p -r1.26 expr.c
--- bin/expr/expr.c 19 Oct 2016 18:20:25 -  1.26
+++ bin/expr/expr.c 30 Mar 2018 15:33:50 -
@@ -99,6 +99,7 @@ is_integer(struct val *vp, int64_t *r)
char   *s;
int neg;
int64_t i;
+   charc;
 
if (vp->type == integer) {
*r = vp->u.i;
@@ -120,8 +121,12 @@ is_integer(struct val *vp, int64_t *r)
if (!isdigit((unsigned char)*s))
return 0;
 
+   c = *s - '0';
+   if (c < 0 || INT64_MAX / 10 < i || INT64_MAX - c < i * 10)
+   errx(3, "out of range");
+
i *= 10;
-   i += *s - '0';
+   i += c;
 
s++;
}
@@ -303,8 +308,9 @@ eval5(void)
 struct val *
 eval4(void)
 {
-   struct val *l, *r;
-   enum token  op;
+   struct val  *l, *r;
+   enum token   op;
+   volatile int64_t res;
 
l = eval5();
while ((op = token) == MUL || op == DIV || op == MOD) {
@@ -316,7 +322,10 @@ eval4(void)
}
 
if (op == MUL) {
-   l->u.i *= r->u.i;
+   res = l->u.i * r->u.i;
+   if (r->u.i != 0 && l->u.i != res / r->u.i)
+   errx(3, "overflow");
+   l->u.i = res;
} else {
if (r->u.i == 0) {
errx(2, "division by zero");
@@ -324,6 +333,8 @@ eval4(void)
if (op == DIV) {
if (l->u.i != INT64_MIN || r->u.i != -1)
l->u.i /= r->u.i;
+   else
+   errx(3, "overflow");
} else {
if (l->u.i != INT64_MIN || r->u.i != -1)
l->u.i %= r->u.i;
@@ -342,8 +353,9 @@ eval4(void)
 struct val *
 eval3(void)
 {
-   struct val *l, *r;
-   enum token  op;
+   struct val  *l, *r;
+   enum token   op;
+   volatile int64_t res;
 
l = eval4();
while ((op = token) == ADD || op == SUB) {
@@ -355,9 +367,18 @@ eval3(void)
}
 
if (op == ADD) {
-   l->u.i += r->u.i;
+   res = l->u.i + r->u.i;
+   if ((l->u.i > 0 && r->u.i > 0 && res <= 0) ||
+   (l->u.i < 0 && r->u.i < 0 && res >= 0))
+   errx(3, "overflow");
+   l->u.i = res;
} else {
-   l->u.i -= r->u.i;
+   res = l->u.i - r->u.i;
+   if ((r->u.i == INT64_MIN && l->u.i < 0) ||
+   (l->u.i > 0 && r->u.i < 0 && res <= 0) ||
+   (l->u.i < 0 && r->u.i > 0 && res >= 0))
+   errx(3, "overflow");
+   l->u.i = res;
}
 
free_value(r);
Index: regress/bin/expr/Makefile
===
RCS file: regress/bin/expr/Makefile
diff -N regress/bin/expr/Makefile
--- /dev/null   1 Jan 1970 00:00:00 -
+++ regress/bin/expr/Makefile   30 Mar 2018 15:33:50 -
@@ -0,0 +1,8 @@
+# $OpenBSD$
+
+REGRESS_TARGETS = expr
+
+expr:
+   sh ${.CURDIR}/expr.sh
+
+.include 
Index: regress/bin/expr/expr.sh
===
RCS file: regress/bin/expr/expr.sh
diff -N regress/bin/expr/expr.sh
--- /dev/null   1 Jan 1970 00:00:00 -
+++ regress/bin/expr/expr.sh30 Mar 2018 15:33:50 -
@@ -0,0 +1,98 @@
+#!/bin/ksh
+
+: ${EXPR=expr}
+
+function test_expr {
+   #echo "Testing: `eval echo $1`"
+   res=`eval $EXPR $1 2>&1`

Re: disklabel,fsck_ffs: division by zero on invalid frag sizes

2018-03-28 Thread Tobias Stoeckmann
On Wed, Mar 28, 2018 at 12:25:01PM +0200, Otto Moerbeek wrote:
> Hmm, on what platform are you doing this? I could not reproduce your
> problem on arm64 -current,

To sum it up: It was my fault, -current is all fine.

The issue can be reproduced with 6.2-stable amd64 and somewhere between
6.2-release and -current, but not with latest -current.

My i386 system uses a modified kernel (for my touchpad) and due to
being not the fastest system, I am not always keeping the kernel up
to date, as long as everything boots up.

Didn't expect it to be a kernel issue, so after a clean install,
applying the diff krw@ sent me and rebuilding the kernel as well as
disklabel and fsck_ffs, the problem went away. With stock -current
kernel, the issue is also not reproducible.

Sorry for the noise, will give the year 2016 its diff back. ;)


Tobias



Re: cut: Fix segmentation fault

2018-03-28 Thread Tobias Stoeckmann
Hi,

On Wed, Mar 28, 2018 at 12:53:47AM +0200, Ingo Schwarze wrote:
> Ouch, you are right.  But then, the code feels counter-intuitive
> and the error message confusing.

I agree. Talking about a zero out of nothing seems weird, even though
I have learned yesterday that "a=''; echo $((a))" is "0". ;)

As this special zero case is handled by strtonum already, how about we
just stick with "invalid list value" which is true for non-digits as
well?

if (*p != '\0' || !stop || !start)
errx(1, "[-bcf] list: illegal list value");

I've extended the regression tests to cover the overflow part as well
as handling invalid list values.


Tobias

Index: usr.bin/cut/cut.c
===
RCS file: /cvs/src/usr.bin/cut/cut.c,v
retrieving revision 1.23
diff -u -p -u -p -r1.23 cut.c
--- usr.bin/cut/cut.c   2 Dec 2015 00:56:46 -   1.23
+++ usr.bin/cut/cut.c   28 Mar 2018 07:21:28 -
@@ -154,11 +154,32 @@ int autostart, autostop, maxval;
 
 char positions[_POSIX2_LINE_MAX + 1];
 
+int
+read_number(char **p)
+{
+   size_t pos;
+   int dash, n;
+   const char *errstr;
+   char *q;
+
+   q = *p + strcspn(*p, "-");
+   dash = *q == '-';
+   *q = '\0';
+   n = strtonum(*p, 1, _POSIX2_LINE_MAX, );
+   if (errstr != NULL)
+   errx(1, "[-bcf] list: %s %s (allowed 1-%d)", *p, errstr,
+   _POSIX2_LINE_MAX);
+   if (dash)
+   *q = '-';
+   *p = q;
+
+   return n;
+}
+
 void
 get_list(char *list)
 {
int setautostart, start, stop;
-   char *pos;
char *p;
 
/*
@@ -176,30 +197,27 @@ get_list(char *list)
setautostart = 1;
}
if (isdigit((unsigned char)*p)) {
-   start = stop = strtol(p, , 10);
+   start = stop = read_number();
if (setautostart && start > autostart)
autostart = start;
}
if (*p == '-') {
-   if (isdigit((unsigned char)p[1]))
-   stop = strtol(p + 1, , 10);
+   if (isdigit((unsigned char)p[1])) {
+   ++p;
+   stop = read_number();
+   }
if (*p == '-') {
++p;
if (!autostop || autostop > stop)
autostop = stop;
}
}
-   if (*p)
+   if (*p != '\0' || !stop || !start)
errx(1, "[-bcf] list: illegal list value");
-   if (!stop || !start)
-   errx(1, "[-bcf] list: values may not include zero");
-   if (stop > _POSIX2_LINE_MAX)
-   errx(1, "[-bcf] list: %d too large (max %d)",
-   stop, _POSIX2_LINE_MAX);
if (maxval < stop)
maxval = stop;
-   for (pos = positions + start; start++ <= stop; *pos++ = 1)
-   ;
+   if (start <= stop)
+   memset(positions + start, 1, stop - start + 1);
}
 
/* overlapping ranges */
Index: regress/usr.bin/cut/cut.sh
===
RCS file: /cvs/src/regress/usr.bin/cut/cut.sh,v
retrieving revision 1.1
diff -u -p -u -p -r1.1 cut.sh
--- regress/usr.bin/cut/cut.sh  7 Oct 2016 17:22:12 -   1.1
+++ regress/usr.bin/cut/cut.sh  28 Mar 2018 07:21:28 -
@@ -16,15 +16,26 @@
 
 unset LC_ALL
 
+: ${CUT=cut}
+
 test_cut()
 {
-   args=`echo "$1"`
-   stdin=$2
-   expected=`echo "$3"`
+   expected_retval=$1
+   args=`echo "$2"`
+   stdin=$3
+   expected=`echo "$4"`
export LC_CTYPE=en_US.UTF-8
-   result=`echo -n "$stdin" | cut $args`
+   result=`echo -n "$stdin" | $CUT $args 2>/dev/null`
+   retval=$?
+   if [ "$retval" -ne "${expected_retval}" ]; then
+   echo "echo -n \"$stdin\" | $CUT $args"
+   echo -n "$stdin" | hexdump -C
+   echo "expected return value: \"${expected_retval}\""
+   echo "actual return value: \"$retval\""
+   exit 1;
+   fi
if [ "$result" != "${expected}" ]; then
-   echo "echo -n \"$stdin\" | cut $args"
+   echo "echo -n \"$stdin\" | $CUT $args"
echo -n "$stdin" | hexdump -C
echo "expected: \"$expected\""
echo -n "$expected" | hexdump -C
@@ -33,13 +44,20 @@ test_cut()
exit 1;
fi
 
-   if [ -n "$4" ]; then
-   expected=`echo "$4"`
+   if [ -n "$5" ]; then
+   expected=`echo "$5"`
fi
export LC_CTYPE=C
-   result=`echo -n "$stdin" | cut 

Re: cut: Fix segmentation fault

2018-03-27 Thread Tobias Stoeckmann
On Tue, Mar 27, 2018 at 11:47:27PM +0200, Ingo Schwarze wrote:
> See inline for one optional suggestion.
> 
> > if (!stop || !start)
> > errx(1, "[-bcf] list: values may not include zero");
> 
> Consider deleting these two lines, too.
> 
> You new function read_number() already makes sure that neither stop
> nor start can be 0 at this point.

Beware that you can actually reach that point if you call cut with an
empty list argument, like:

$ cut -c '' -
cut: [-bcf] list: values may not include zero

It's therefore still required.

Thinking about it, this might be a good regression candidate as well.



Re: test(1): Fix integer overflows

2018-03-27 Thread Tobias Stoeckmann
On Tue, Mar 27, 2018 at 10:36:17PM +0200, Ingo Schwarze wrote:
> > +   int sig;
> 
> Drop this variable, it does no good but only harm.

Yeah, too bad that I didn't notice this left-over from a previous
development step. Strange enough that regression tests didn't
choke on it...

> > +   /* skip leading zeros */
> > +   while (*p == '0' && isdigit((unsigned char)p[1]))
> > +   p++;
> > +
> > +   /* turn 0 always positive */
> > +   if (*p == '0' && !isdigit((unsigned char)p[1]))
> > +   sig = 1;

As tb@ pointed out, the !isdigit check can be removed as well.

> > +   c = strncmp(p1, p2, len1);
> 
> That's another bug:
> 
>   schwarze@isnote $ /bin/test -1 -gt -2 ; echo $?
>   1
>   schwarze@isnote $ /bin/test 1 -gt 2 ; echo $?
>   1

It's really great to have people who do extensive reviews. Thanks
a lot for spotting this. I have added it to the regression tests.

> With these changes, OK schwarze@.  See below for the patch that
> i tested.

Modified with tb@'s remark. Also I changed the error message
"bad number" to "invalid", so we stay fully in sync with strtonum's
error messages.

> Given that breaking test(1) might cause hard-to-find breakage
> deep down in build systems and scripts, you might with a to
> have a second OK for commit, though.

Definitely, I'm not out to rush this one in.


Index: bin/test/test.c
===
RCS file: /cvs/src/bin/test/test.c,v
retrieving revision 1.18
diff -u -p -u -p -r1.18 test.c
--- bin/test/test.c 24 Jul 2017 22:15:52 -  1.18
+++ bin/test/test.c 27 Mar 2018 20:43:38 -
@@ -16,6 +16,7 @@
 #include 
 #include 
 #include 
+#include 
 #include 
 #include 
 #include 
@@ -145,6 +146,8 @@ static int aexpr(enum token n);
 static int nexpr(enum token n);
 static int binop(void);
 static int primary(enum token n);
+static const char *getnstr(const char *, int *, size_t *);
+static int intcmp(const char *, const char *);
 static int filstat(char *nm, enum token mode);
 static int getn(const char *s);
 static int newerf(const char *, const char *);
@@ -290,6 +293,70 @@ primary(enum token n)
return strlen(*t_wp) > 0;
 }
 
+static const char *
+getnstr(const char *s, int *signum, size_t *len)
+{
+   const char *p, *start;
+
+   /* skip leading whitespaces */
+   p = s;
+   while (isspace((unsigned char)*p))
+   p++;
+
+   /* accept optional sign */
+   if (*p == '-') {
+   *signum = -1;
+   p++;
+   } else {
+   *signum = 1;
+   if (*p == '+')
+   p++;
+   }
+
+   /* skip leading zeros */
+   while (*p == '0' && isdigit((unsigned char)p[1]))
+   p++;
+
+   /* turn 0 always positive */
+   if (*p == '0')
+   *signum = 1;
+
+   start = p;
+   while (isdigit((unsigned char)*p))
+   p++;
+   *len = p - start;
+
+   /* allow trailing whitespaces */
+   while (isspace((unsigned char)*p))
+   p++;
+
+   /* validate number */
+   if (*p != '\0' || *start == '\0')
+   errx(2, "%s: invalid", s);
+
+   return start;
+}
+
+static int
+intcmp(const char *opnd1, const char *opnd2)
+{
+   const char *p1, *p2;
+   size_t len1, len2;
+   int c, sig1, sig2;
+
+   p1 = getnstr(opnd1, , );
+   p2 = getnstr(opnd2, , );
+
+   if (sig1 != sig2)
+   c = sig1;
+   else if (len1 != len2)
+   c = (len1 < len2) ? -sig1 : sig1;
+   else
+   c = strncmp(p1, p2, len1) * sig1;
+
+   return c;
+}
+
 static int
 binop(void)
 {
@@ -313,17 +380,17 @@ binop(void)
case STRGT:
return strcmp(opnd1, opnd2) > 0;
case INTEQ:
-   return getn(opnd1) == getn(opnd2);
+   return intcmp(opnd1, opnd2) == 0;
case INTNE:
-   return getn(opnd1) != getn(opnd2);
+   return intcmp(opnd1, opnd2) != 0;
case INTGE:
-   return getn(opnd1) >= getn(opnd2);
+   return intcmp(opnd1, opnd2) >= 0;
case INTGT:
-   return getn(opnd1) > getn(opnd2);
+   return intcmp(opnd1, opnd2) > 0;
case INTLE:
-   return getn(opnd1) <= getn(opnd2);
+   return intcmp(opnd1, opnd2) <= 0;
case INTLT:
-   return getn(opnd1) < getn(opnd2);
+   return intcmp(opnd1, opnd2) < 0;
case FILNT:
return newerf(opnd1, opnd2);
case FILOT:
@@ -455,22 +522,26 @@ t_lex(char *s)
 static int
 getn(const char *s)
 {
-   char *p;
-   long r;
-
-   errno = 0;
-   r = strtol(s, , 10);
-
-   if (errno != 0)
-   errx(2, "%s: out of range", s);
-
-   while (isspace((unsigned char)*p))
-   p++;
+   char buf[32];
+   const char *errstr, *p;
+   size_t len;
+   int r, sig;
+
+   p = 

Re: test(1): Fix integer overflows

2018-03-27 Thread Tobias Stoeckmann
Hi,

On Tue, Mar 27, 2018 at 09:05:12PM +0200, Klemens Nanni wrote:
> FWIW, see "test: Catch integer overflow, fail on trailing whitespaces"
> from 24.07.2017 on tech@:
>   
>   https://marc.info/?l=openbsd-tech=150091968903042

sorry I didn't intend to ignore your mail. I didn't follow tech@
closely the last months. Your proposed patch is actually close to what
I came up first -- never thought about dropping this whitespace support.

Just as Ingo did, I have checked the specs here closely again and
found this one:

http://pubs.opengroup.org/onlinepubs/9699919799/xrat/V4_xcu_chap02.html

You can find in there this section:

For the commands:
x=010; echo $((x += 1))
the output must be 9.

For the commands:
x=' 1'; echo $((x += 1))
the results are unspecified.

So, POSIX would allow us to do what we want, which means that your
proposal could be done as well.

I tried to see what ksh does, and if there is any way to trigger a
situation in which ' 1' wouldn't be considered a number.

But even this one works:

$ x=' 1 '
$ echo $((x+1))
2
$ _

Here are my two cents and feedback to your mail:

If test(1) stops accepting ' 1 ' as a number, it's in conflict with
ksh. I would prefer if test(1) and built-in test get closer to each
other, not splitting their way of functioning further. But it's no
strong opinion of mine. Having a very strict POSIX test support sounds
appealing as well.


Tobias



test(1): Fix integer overflows

2018-03-27 Thread Tobias Stoeckmann
The test(1) utility is prone to integer overflows on 64 bit systems.
By using strtol and casting the result to an int, it is possible to
run tests which succeed even though they should fail:

$ arch
OpenBSD.amd64
$ /bin/test 4294967296 -eq 0; echo $?
0

I could have chosen the way of FreeBSD, NetBSD, or built-in test of
pdksh and extend the number to intmax_t, but I actually prefered the
way of coreutils, i.e. validating the number as strings.

This version allows numbers based on the rules which I figured out
by reading strtol's man page and POSIX's rather vague specification:

- Numbers are always base 10, regardless of starting with 0
- A number can start with arbitrary amounts of whitespaces
- Then a sign char is allowed (POSIX only mentions -)
- An arbitrary amount of digits follows
- Then an arbitrary amount of whitespaces can follow

Therefore, comparing numbers can be done in a human-understandable
form as strings:

1. If signum differs, the smaller number is the one having a -
2. Leading zeros are skipped
3. If both numbers are positive, the one with more digits is larger
4. If both numbers are negative, the one with more digits is smaller
5. Equality is based on a simple string comparison

I have extended the regression test suite. The last two additions
fail with current implementation.

While at it, -t has a proper 0-INT_MAX limitation now as well.

This is quite a refactoring so tests are very appreciated. Please note
that we have overflows in pdksh's builtin test as well. If bin/test
is refactored, I'll try to unify both implementations much closer to
each other where possible.


Tobias

Index: bin/test/test.c
===
RCS file: /cvs/src/bin/test/test.c,v
retrieving revision 1.18
diff -u -p -u -p -r1.18 test.c
--- bin/test/test.c 24 Jul 2017 22:15:52 -  1.18
+++ bin/test/test.c 27 Mar 2018 17:23:51 -
@@ -16,6 +16,7 @@
 #include 
 #include 
 #include 
+#include 
 #include 
 #include 
 #include 
@@ -145,6 +146,8 @@ static int aexpr(enum token n);
 static int nexpr(enum token n);
 static int binop(void);
 static int primary(enum token n);
+static const char *getnstr(const char *, int *, size_t *);
+static int intcmp(const char *, const char *);
 static int filstat(char *nm, enum token mode);
 static int getn(const char *s);
 static int newerf(const char *, const char *);
@@ -290,6 +293,72 @@ primary(enum token n)
return strlen(*t_wp) > 0;
 }
 
+static const char *
+getnstr(const char *s, int *signum, size_t *len)
+{
+   const char *p, *start;
+   int sig;
+
+   /* skip leading whitespaces */
+   p = s;
+   while (isspace((unsigned char)*p))
+   p++;
+
+   /* accept optional sign */
+   if (*p == '-') {
+   *signum = -1;
+   p++;
+   } else {
+   *signum = 1;
+   if (*p == '+')
+   p++;
+   }
+
+   /* skip leading zeros */
+   while (*p == '0' && isdigit((unsigned char)p[1]))
+   p++;
+
+   /* turn 0 always positive */
+   if (*p == '0' && !isdigit((unsigned char)p[1]))
+   sig = 1;
+
+   start = p;
+   while (isdigit((unsigned char)*p))
+   p++;
+   *len = p - start;
+
+   /* allow trailing whitespaces */
+   while (isspace((unsigned char)*p))
+   p++;
+
+   /* validate number */
+   if (*p != '\0' || *start == '\0')
+   errx(2, "%s: bad number", s);
+
+   *signum = sig;
+   return start;
+}
+
+static int
+intcmp(const char *opnd1, const char *opnd2)
+{
+   const char *p1, *p2;
+   size_t len1, len2;
+   int c, sig1, sig2;
+
+   p1 = getnstr(opnd1, , );
+   p2 = getnstr(opnd2, , );
+
+   if (sig1 != sig2)
+   c = sig1;
+   else if (len1 != len2)
+   c = (len1 < len2) ? -sig1 : sig1;
+   else
+   c = strncmp(p1, p2, len1);
+
+   return c;
+}
+
 static int
 binop(void)
 {
@@ -313,17 +382,17 @@ binop(void)
case STRGT:
return strcmp(opnd1, opnd2) > 0;
case INTEQ:
-   return getn(opnd1) == getn(opnd2);
+   return intcmp(opnd1, opnd2) == 0;
case INTNE:
-   return getn(opnd1) != getn(opnd2);
+   return intcmp(opnd1, opnd2) != 0;
case INTGE:
-   return getn(opnd1) >= getn(opnd2);
+   return intcmp(opnd1, opnd2) >= 0;
case INTGT:
-   return getn(opnd1) > getn(opnd2);
+   return intcmp(opnd1, opnd2) > 0;
case INTLE:
-   return getn(opnd1) <= getn(opnd2);
+   return intcmp(opnd1, opnd2) <= 0;
case INTLT:
-   return getn(opnd1) < getn(opnd2);
+   return intcmp(opnd1, opnd2) < 0;
case FILNT:
return newerf(opnd1, opnd2);
case FILOT:
@@ -455,22 +524,26 @@ t_lex(char *s)
 static int
 

disklabel,fsck_ffs: division by zero on invalid frag sizes

2018-03-27 Thread Tobias Stoeckmann
Resending this old diff. Adjusted to apply with -current source:

Illegal fragmentation block sizes can trigger division by zero in the
disklabel and fsck_ffs tools.

See this sequence of commands to reproduce:

# dd if=/dev/zero of=nofrag.iso bs=1M count=1
# vnconfig vnd0 nofrag.iso
# disklabel -e vnd0

Create 'a' and set fsize = bsize = 1. A line like this one will do
  a: 20480  4.2BSD  1 1 1

# fsck_ffs vnd0a
** /dev/vnd0a (vnd0a)
BAD SUPER BLOCK: MAGIC NUMBER WRONG
Floating point exception (core dumped)
# disklabel -E vnd0
Label editor (enter '?' for help at any prompt)
> m a
offset: [0]
size: [2048]
FS type: [4.2BSD]
Floating point exception (core dumped)
# vnconfig -u vnd0

A fragmentation (block) size smaller than a sector size is not valid
while using "disklabel -E", and really doesn't make sense. While
using "disklabel -e", not all validation checks are performed, which
makes it possible to enter invalid values.

If "disklabel -E" is used without the expert mode, fragmentation sizes
cannot be changed and will be just accepted from the parsed disklabel,
resulting in a division by zero if they are too small.

And the same happens in fsck_ffs. Instead of coming up with a guessed
value in fsck_ffs, I think it's better to simply fail and let the user
fix the disklabel. After all, it shouldn't be fsck_ffs's duty to fix
faulty values outside the filesystem.

Index: sbin/disklabel/disklabel.c
===
RCS file: /cvs/src/sbin/disklabel/disklabel.c,v
retrieving revision 1.227
diff -u -p -u -p -r1.227 disklabel.c
--- sbin/disklabel/disklabel.c  25 Feb 2018 17:24:44 -  1.227
+++ sbin/disklabel/disklabel.c  27 Mar 2018 10:42:59 -
@@ -1100,9 +1100,24 @@ gottype:
 
case FS_BSDFFS:
NXTNUM(fsize, fsize, );
-   if (fsize == 0)
+   if (fsize < lp->d_secsize ||
+   (fsize % lp->d_secsize) != 0) {
+   warnx("line %d: "
+   "bad fragmentation size: %s",
+   lineno, cp);
+   errors++;
break;
+   }
NXTNUM(v, v, );
+   if (v < fsize || (fsize != v &&
+   fsize * 2 != v && fsize * 4 != v &&
+   fsize * 8 != v)) {
+   warnx("line %d: "
+   "bad block size: %s",
+   lineno, cp);
+   errors++;
+   break;
+   }
pp->p_fragblock =
DISKLABELV1_FFS_FRAGBLOCK(fsize, v / fsize);
NXTNUM(pp->p_cpg, pp->p_cpg, );
Index: sbin/disklabel/editor.c
===
RCS file: /cvs/src/sbin/disklabel/editor.c,v
retrieving revision 1.327
diff -u -p -u -p -r1.327 editor.c
--- sbin/disklabel/editor.c 8 Mar 2018 22:05:17 -   1.327
+++ sbin/disklabel/editor.c 27 Mar 2018 10:42:59 -
@@ -2069,16 +2069,16 @@ get_bsize(struct disklabel *lp, int part
if (pp->p_fstype != FS_BSDFFS)
return (0);
 
+   frag = DISKLABELV1_FFS_FRAG(pp->p_fragblock);
+   fsize = DISKLABELV1_FFS_FSIZE(pp->p_fragblock);
+
/* Avoid dividing by zero... */
-   if (pp->p_fragblock == 0)
-   return(1);
+   if (frag * fsize < lp->d_secsize)
+   return (1);
 
if (!expert)
goto align;
 
-   fsize = DISKLABELV1_FFS_FSIZE(pp->p_fragblock);
-   frag = DISKLABELV1_FFS_FRAG(pp->p_fragblock);
-
for (;;) {
ui = getuint64(lp, "block size",
"Size of ffs blocks. 1, 2, 4 or 8 times ffs fragment size.",
@@ -2119,8 +2119,7 @@ align:
orig_size = DL_GETPSIZE(pp);
orig_offset = DL_GETPOFFSET(pp);
 
-   bsize = (DISKLABELV1_FFS_FRAG(pp->p_fragblock) *
-   DISKLABELV1_FFS_FSIZE(pp->p_fragblock)) / lp->d_secsize;
+   bsize = (frag * fsize) / lp->d_secsize;
if (DL_GETPOFFSET(pp) != starting_sector) {
/* Can't change offset of first partition. */
adj = bsize - (DL_GETPOFFSET(pp) % bsize);
Index: sbin/fsck_ffs/setup.c
===
RCS file: /cvs/src/sbin/fsck_ffs/setup.c,v
retrieving revision 1.64
diff -u -p -u -p -r1.64 setup.c
--- sbin/fsck_ffs/setup.c   5 Jan 2018 09:33:47 -   1.64
+++ sbin/fsck_ffs/setup.c   27 Mar 2018 10:42:59 -
@@ -636,6 

cut: Fix segmentation fault

2018-03-27 Thread Tobias Stoeckmann
This patch fixes an out of boundary write in cut:

$ cut -c 2147483648 -
Segmentation fault (core dumped)

The supplied number can be parsed by strtol, but the result is casted
into a signed int, therefore turning negative. Afterwards, it is used
as an offset into a char array to write data at the given position.
This leads to the shown segmentation fault.

I have changed the strtol call to strtonum, making sure that range
specifications still work. Our cut regress tests pass. Also it has a
nicer error message now, thanks to strtonum.

While at it, I replaced a manual for-loop with memset.

Index: usr.bin/cut/cut.c
===
RCS file: /cvs/src/usr.bin/cut/cut.c,v
retrieving revision 1.23
diff -u -p -u -p -r1.23 cut.c
--- usr.bin/cut/cut.c   2 Dec 2015 00:56:46 -   1.23
+++ usr.bin/cut/cut.c   27 Mar 2018 11:24:31 -
@@ -154,11 +154,32 @@ int autostart, autostop, maxval;
 
 char positions[_POSIX2_LINE_MAX + 1];
 
+int
+read_number(char **p)
+{
+   size_t pos;
+   int dash, n;
+   const char *errstr;
+   char *q;
+
+   q = *p + strcspn(*p, "-");
+   dash = *q == '-';
+   *q = '\0';
+   n = strtonum(*p, 1, _POSIX2_LINE_MAX, );
+   if (errstr != NULL)
+   errx(1, "[-bcf] list: %s %s (allowed 1-%d)", *p, errstr,
+   _POSIX2_LINE_MAX);
+   if (dash)
+   *q = '-';
+   *p = q;
+
+   return n;
+}
+
 void
 get_list(char *list)
 {
int setautostart, start, stop;
-   char *pos;
char *p;
 
/*
@@ -176,13 +197,15 @@ get_list(char *list)
setautostart = 1;
}
if (isdigit((unsigned char)*p)) {
-   start = stop = strtol(p, , 10);
+   start = stop = read_number();
if (setautostart && start > autostart)
autostart = start;
}
if (*p == '-') {
-   if (isdigit((unsigned char)p[1]))
-   stop = strtol(p + 1, , 10);
+   if (isdigit((unsigned char)p[1])) {
+   ++p;
+   stop = read_number();
+   }
if (*p == '-') {
++p;
if (!autostop || autostop > stop)
@@ -193,13 +216,10 @@ get_list(char *list)
errx(1, "[-bcf] list: illegal list value");
if (!stop || !start)
errx(1, "[-bcf] list: values may not include zero");
-   if (stop > _POSIX2_LINE_MAX)
-   errx(1, "[-bcf] list: %d too large (max %d)",
-   stop, _POSIX2_LINE_MAX);
if (maxval < stop)
maxval = stop;
-   for (pos = positions + start; start++ <= stop; *pos++ = 1)
-   ;
+   if (start <= stop)
+   memset(positions + start, 1, stop - start + 1);
}
 
/* overlapping ranges */



Re: apply(1): Fix segmentation faults

2018-03-23 Thread Tobias Stoeckmann
On Fri, Mar 23, 2018 at 10:48:03AM +0100, Tobias Stoeckmann wrote:
> My initial mail was sent with quoted-printable mime type, which means
> that it looks quite garbled. Sorry for that, this mail contains the same
> patch but is sent 7bit encoded.

Seems to be an issue with an overly long regress test line.
Here's the pure apply patch, without regression tests:

Index: usr.bin/apply/apply.c
===
RCS file: /cvs/src/usr.bin/apply/apply.c,v
retrieving revision 1.27
diff -u -p -u -p -r1.27 apply.c
--- usr.bin/apply/apply.c   10 Oct 2015 17:48:34 -  1.27
+++ usr.bin/apply/apply.c   21 Mar 2018 20:53:02 -
@@ -44,15 +44,42 @@
 #include 
 #include 
 
+#define ISMAGICNO(p) \
+   (p)[0] == magic && isdigit((unsigned char)(p)[1]) && (p)[1] != '0'
+
 __dead voidusage(void);
 static int mysystem(const char *);
 
+char   *str;
+size_t  sz;
+
+void
+stradd(char *p)
+{
+   size_t n;
+
+   n = strlen(p);
+   if (str == NULL || sz - strlen(str) <= n) {
+   sz += (n / 1024 + 1) * 1024;
+   if ((str = realloc(str, sz)) == NULL)
+   err(1, "realloc");
+   }
+   strlcat(str, p, sz);
+}
+
+void
+strset(char *p)
+{
+   if (str != NULL)
+   str[0] = '\0';
+   stradd(p);
+}
+
 int
 main(int argc, char *argv[])
 {
-   int ch, clen, debug, i, l, magic, n, nargs, rval;
-   char *c, *c2, *cmd, *p, *q;
-   size_t len;
+   int ch, debug, i, magic, n, nargs, rval;
+   char buf[4], *cmd, *p;
 
if (pledge("stdio proc exec", NULL) == -1)
err(1, "pledge");
@@ -63,7 +90,7 @@ main(int argc, char *argv[])
while ((ch = getopt(argc, argv, "a:d0123456789")) != -1)
switch (ch) {
case 'a':
-   if (optarg[1] != '\0')
+   if (optarg[0] == '\0' || optarg[1] != '\0')
errx(1,
"illegal magic character specification.");
magic = optarg[0];
@@ -93,8 +120,7 @@ main(int argc, char *argv[])
 * largest one.
 */
for (n = 0, p = argv[0]; *p != '\0'; ++p)
-   if (p[0] == magic &&
-   isdigit((unsigned char)p[1]) && p[1] != '0') {
+   if (ISMAGICNO(p)) {
++p;
if (p[0] - '0' > n)
n = p[0] - '0';
@@ -106,28 +132,15 @@ main(int argc, char *argv[])
 * the end to consume (nargs) arguments each time round the loop.
 * Allocate enough space to hold the maximum command.
 */
+   strset(argv[0]);
if (n == 0) {
-   len = sizeof("exec ") - 1 +
-   strlen(argv[0]) + 9 * (sizeof(" %1") - 1) + 1;
-   if ((cmd = malloc(len)) == NULL)
-   err(1, NULL);
-
/* If nargs not set, default to a single argument. */
if (nargs == -1)
nargs = 1;
 
-   l = snprintf(cmd, len, "exec %s", argv[0]);
-   if (l >= len || l == -1)
-   errx(1, "error building exec string");
-   len -= l;
-   p = cmd + l;
-   
for (i = 1; i <= nargs; i++) {
-   l = snprintf(p, len, " %c%d", magic, i);
-   if (l >= len || l == -1)
-   errx(1, "error numbering arguments");
-   len -= l;
-   p += l;
+   snprintf(buf, sizeof(buf), " %c%d", magic, i);
+   stradd(buf);
}
 
/*
@@ -136,19 +149,10 @@ main(int argc, char *argv[])
 */
if (nargs == 0)
nargs = 1;
-   } else {
-   if (asprintf(, "exec %s", argv[0]) == -1)
-   err(1, NULL);   
+   } else
nargs = n;
-   }
-
-   /*
-* Grab some space in which to build the command.  Allocate
-* as necessary later, but no reason to build it up slowly
-* for the normal case.
-*/
-   if ((c = malloc(clen = 1024)) == NULL)
-   err(1, NULL);
+   if ((cmd = strdup(str)) == NULL)
+   err(1, "strdup");
 
/*
 * (argc) and (argv) are still offset by one to make it simpler to
@@ -156,35 +160,21 @@ main(int argc, char *argv[])
 * equals 1 means that all the (argv) has been consumed.
 */
for (rval = 0; argc > nargs; argc -= nargs, argv += nargs) {
-   /*
-* Find a max value for the comman

Re: apply(1): Fix segmentation faults

2018-03-23 Thread Tobias Stoeckmann
My initial mail was sent with quoted-printable mime type, which means
that it looks quite garbled. Sorry for that, this mail contains the same
patch but is sent 7bit encoded.

Also, the decision was made to commit this after 6.3 release. I like
that decision, after all we will have more time to give it real testing.

Index: regress/usr.bin/Makefile
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D
RCS file: /cvs/src/regress/usr.bin/Makefile,v
retrieving revision 1.42
diff -u -p -u -p -r1.42 Makefile
--- regress/usr.bin/Makefile28 Jun 2017 15:09:41 -  1.42
+++ regress/usr.bin/Makefile21 Mar 2018 20:53:01 -
@@ -1,7 +1,7 @@
 #  $OpenBSD: Makefile,v 1.42 2017/06/28 15:09:41 anton Exp $
=20
-SUBDIR+=3D basename bc calendar colrm column cut dc diff diff3 dirname doas
-SUBDIR+=3D file fmt fold grep gzip
+SUBDIR+=3D apply basename bc calendar colrm column cut dc diff diff3 dirname
+SUBDIR+=3D doas file fmt fold grep gzip
 SUBDIR+=3D jot lastcomm m4 mail mandoc openssl rev sdiff sed signify sort tsort
 SUBDIR+=3D ul wc xargs
=20
Index: regress/usr.bin/apply/Makefile
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D
RCS file: regress/usr.bin/apply/Makefile
diff -N regress/usr.bin/apply/Makefile
--- /dev/null   1 Jan 1970 00:00:00 -
+++ regress/usr.bin/apply/Makefile  21 Mar 2018 20:53:01 -
@@ -0,0 +1,55 @@
+# $OpenBSD$
+
+APPLY?=3D  apply
+CLEANFILES=3D  *.res
+
+REGRESS_TARGETS=3Dt1 t2 t3 t4 t5 t6
+
+# .in: input file
+# .out: desired output
+
+# t1: uses arguments multiple times (from FreeBSD bin/95079)
+# t2: overflows ARG_MAX (from FreeBSD bin/95079)
+# t3: debugs -0 call
+# t4: debugs -2 call
+# t5: uses magic character '&'
+# t6: uses magic character ' ' with command starting with a number
+
+t1:
+   @echo ${*}
+   @(${APPLY} "echo %1 %1 %1 %1" `cat ${*}.in` > ${*}.res)
+   @cmp -s ${*}.out ${.CURDIR}/${*}.res || \
+   (echo "XXX ${*} failed" && false)
+
+t2:
+   @echo ${*}
+   @ARG_MAX=3D`getconf ARG_MAX`;\
+   ARG_MAX_HALF=3D$$((ARG_MAX / 2)); \
+   ! ${APPLY} "echo %1 %1 %1" \
+   `jot $$ARG_MAX_HALF 1 1 | tr -d '\n'` > ${*}.res 2>&1
+
+t3:
+   @echo ${*}
+   @(${APPLY} -0 -d who 1 2 3 4 5 > ${*}.res)
+   @cmp -s ${*}.out ${.CURDIR}/${*}.res || \
+   (echo "XXX ${*} failed" && false)
+
+t4:
+   @echo ${*}
+   @(${APPLY} -2 -d cmp a1 b1 a2 b2 a3 b3 > ${*}.res)
+   @cmp -s ${*}.out ${.CURDIR}/${*}.res || \
+   (echo "XXX ${*} failed" && false)
+
+t5:
+   @echo ${*}
+   @(${APPLY} -a "&" "echo &2 &1" hello world > ${*}.res)
+   @cmp -s ${*}.out ${.CURDIR}/${*}.res || \
+   (echo "XXX ${*} failed" && false)
+
+t6:
+   @echo ${*}
+   @(${APPLY} -a " " -d "2to3  1" test.py > ${*}.res)
+   @cmp -s ${*}.out ${.CURDIR}/${*}.res || \
+   (echo "XXX ${*} failed" && false)
+
+.include 
Index: regress/usr.bin/apply/t1.in
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D
RCS file: regress/usr.bin/apply/t1.in
diff -N regress/usr.bin/apply/t1.in
--- /dev/null   1 Jan 1970 00:00:00 -
+++ regress/usr.bin/apply/t1.in 21 Mar 2018 20:53:01 -
@@ -0,0 +1 @@
+12345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012
Index: regress/usr.bin/apply/t1.out
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D
RCS file: regress/usr.bin/apply/t1.out
diff -N regress/usr.bin/apply/t1.out
--- /dev/null   1 Jan 1970 00:00:00 -
+++ regress/usr.bin/apply/t1.out21 Mar 2018 20:53:01 -
@@ -0,0 +1 @@

apply(1): Fix segmentation faults

2018-03-21 Thread Tobias Stoeckmann
Hi,

this patch merges FreeBSD bin/95079 into our apply(1) implementation.
As we do not have an sbuf implementation, I have reworked the code
to properly handle string operations. Turns out that this fixes an
out of boundary write as well and made the code much more readable.

While reviewing the code and merging the fix, I have encountered
and fixed another issue when the magic character ' ' is used. A NULL
pointer dereference could occur, like:

$ apply -a ' ' 2to3 test.py
Segmentation fault (core dumped)

This case as well as the ones fixed by FreeBSD are inserted into
our regression tests, for which I have unfortunately already
committed the directory. Oops.

Is it okay to commit at this time of release cycle? If not, I'm
gathering feedback and review anyway. Feel free to comment!


Tobias

Index: regress/usr.bin/Makefile
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D
RCS file: /cvs/src/regress/usr.bin/Makefile,v
retrieving revision 1.42
diff -u -p -u -p -r1.42 Makefile
--- regress/usr.bin/Makefile28 Jun 2017 15:09:41 -  1.42
+++ regress/usr.bin/Makefile21 Mar 2018 20:53:01 -
@@ -1,7 +1,7 @@
 #  $OpenBSD: Makefile,v 1.42 2017/06/28 15:09:41 anton Exp $
=20
-SUBDIR+=3D basename bc calendar colrm column cut dc diff diff3 dirname doas
-SUBDIR+=3D file fmt fold grep gzip
+SUBDIR+=3D apply basename bc calendar colrm column cut dc diff diff3 dirna=
me
+SUBDIR+=3D doas file fmt fold grep gzip
 SUBDIR+=3D jot lastcomm m4 mail mandoc openssl rev sdiff sed signify sort =
tsort
 SUBDIR+=3D ul wc xargs
=20
Index: regress/usr.bin/apply/Makefile
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D
RCS file: regress/usr.bin/apply/Makefile
diff -N regress/usr.bin/apply/Makefile
--- /dev/null   1 Jan 1970 00:00:00 -
+++ regress/usr.bin/apply/Makefile  21 Mar 2018 20:53:01 -
@@ -0,0 +1,55 @@
+# $OpenBSD$
+
+APPLY?=3D  apply
+CLEANFILES=3D  *.res
+
+REGRESS_TARGETS=3Dt1 t2 t3 t4 t5 t6
+
+# .in: input file
+# .out: desired output
+
+# t1: uses arguments multiple times (from FreeBSD bin/95079)
+# t2: overflows ARG_MAX (from FreeBSD bin/95079)
+# t3: debugs -0 call
+# t4: debugs -2 call
+# t5: uses magic character '&'
+# t6: uses magic character ' ' with command starting with a number
+
+t1:
+   @echo ${*}
+   @(${APPLY} "echo %1 %1 %1 %1" `cat ${*}.in` > ${*}.res)
+   @cmp -s ${*}.out ${.CURDIR}/${*}.res || \
+   (echo "XXX ${*} failed" && false)
+
+t2:
+   @echo ${*}
+   @ARG_MAX=3D`getconf ARG_MAX`;\
+   ARG_MAX_HALF=3D$$((ARG_MAX / 2)); \
+   ! ${APPLY} "echo %1 %1 %1" \
+   `jot $$ARG_MAX_HALF 1 1 | tr -d '\n'` > ${*}.res 2>&1
+
+t3:
+   @echo ${*}
+   @(${APPLY} -0 -d who 1 2 3 4 5 > ${*}.res)
+   @cmp -s ${*}.out ${.CURDIR}/${*}.res || \
+   (echo "XXX ${*} failed" && false)
+
+t4:
+   @echo ${*}
+   @(${APPLY} -2 -d cmp a1 b1 a2 b2 a3 b3 > ${*}.res)
+   @cmp -s ${*}.out ${.CURDIR}/${*}.res || \
+   (echo "XXX ${*} failed" && false)
+
+t5:
+   @echo ${*}
+   @(${APPLY} -a "&" "echo &2 &1" hello world > ${*}.res)
+   @cmp -s ${*}.out ${.CURDIR}/${*}.res || \
+   (echo "XXX ${*} failed" && false)
+
+t6:
+   @echo ${*}
+   @(${APPLY} -a " " -d "2to3  1" test.py > ${*}.res)
+   @cmp -s ${*}.out ${.CURDIR}/${*}.res || \
+   (echo "XXX ${*} failed" && false)
+
+.include 
Index: regress/usr.bin/apply/t1.in
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D
RCS file: regress/usr.bin/apply/t1.in
diff -N regress/usr.bin/apply/t1.in
--- /dev/null   1 Jan 1970 00:00:00 -
+++ regress/usr.bin/apply/t1.in 21 Mar 2018 20:53:01 -
@@ -0,0 +1 @@
+12345678901234567890123456789012345678901234567890123456789012345678901234=
567890123456789012345678901234567890123456789012345678901234567890123456789=
012345678901234567890123456789012345678901234567890123456789012345678901234=
567890123456789012345678901234567890123456789012345678901234567890123456789=
012345678901234567890123456789012345678901234567890123456789012345678901234=
567890123456789012345678901234567890123456789012345678901234567890123456789=
012345678901234567890123456789012345678901234567890123456789012
Index: regress/usr.bin/apply/t1.out
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D
RCS file: regress/usr.bin/apply/t1.out
diff -N regress/usr.bin/apply/t1.out
--- /dev/null   1 

Re: less(1): plug memory leak

2017-05-01 Thread Tobias Stoeckmann
Hi,

On Mon, May 01, 2017 at 09:15:45AM +0200, Anton Lindqvist wrote:
> While freeing tag entries, make sure to free the copied strings.

this patch looks good to me.

Have you reported this to the upstream less maintainers, as well?
The original less and the forked one from Garrett D'Amore, which we
use as foundation, have the same issue in them.


Tobias

> Index: tags.c
> ===
> RCS file: /cvs/src/usr.bin/less/tags.c,v
> retrieving revision 1.18
> diff -u -p -r1.18 tags.c
> --- tags.c17 Sep 2016 15:06:41 -  1.18
> +++ tags.c30 Apr 2017 18:13:24 -
> @@ -77,6 +77,8 @@ cleantags(void)
>*/
>   while ((tp = taglist.tl_first) != TAG_END) {
>   TAG_RM(tp);
> + free(tp->tag_file);
> + free(tp->tag_pattern);
>   free(tp);
>   }
>   curtag = NULL;
> 



Re: patch -e reorders line on multiline insert

2016-02-21 Thread Tobias Stoeckmann
On Sun, Feb 21, 2016 at 07:04:27PM +0100, Martin Natano wrote:
> The expected order of lines would be, x, y, z but patch produces y, z, x
> instead. The first line is inserted at the correct position, but then
> the other lines are inserted before the first one. The following diff
> solves that problem by retaining the logic for the first line, but then
> switch to append mode to insert the remaining lines after that one.

Completely agree with the rational, the issue can be triggered with
'i'nsert commands, too. Fixing 'c' (which becomes 'i' eventually)
at that position is the right spot.

As discussed with Martin, this diff does the same, but merges the
very duplicated looking FSM_A/FSM_I code. If nobody objects, I'll use
this one.

And thanks a lot Martin for spotting this.


Tobias

Index: ed.c
===
RCS file: /cvs/src/usr.bin/patch/ed.c,v
retrieving revision 1.1
diff -u -p -u -p -r1.1 ed.c
--- ed.c16 Oct 2015 07:33:47 -  1.1
+++ ed.c21 Feb 2016 18:23:10 -
@@ -121,23 +121,16 @@ do_ed_script(void)
continue;
}
 
-   if (fsm == FSM_A) {
-   nline = create_line(linepos);
-   if (cline == NULL)
-   LIST_INSERT_HEAD(, nline, entries);
-   else
-   LIST_INSERT_AFTER(cline, nline, entries);
-   cline = nline;
-   line_count++;
-   } else if (fsm == FSM_I) {
-   nline = create_line(linepos);
-   if (cline == NULL) {
-   LIST_INSERT_HEAD(, nline, entries);
-   cline = nline;
-   } else
-   LIST_INSERT_BEFORE(cline, nline, entries);
-   line_count++;
-   }
+   nline = create_line(linepos);
+   if (cline == NULL)
+   LIST_INSERT_HEAD(, nline, entries);
+   else if (fsm == FSM_A)
+   LIST_INSERT_AFTER(cline, nline, entries);
+   else
+   LIST_INSERT_BEFORE(cline, nline, entries);
+   cline = nline;
+   line_count++;
+   fsm = FSM_A;
}
 
next_intuit_at(linepos, p_input_line);



Re: diff -e: mishandling of bare dots

2016-02-21 Thread Tobias Stoeckmann
On Sun, Feb 21, 2016 at 05:15:34PM +0100, Martin Natano wrote:
> While testing your suggestion with the example I run into a bug with
> ed-style diff handling in patch(1), resulting in incorrect output
> (fix in the works). Applying the generated diff with ed(1) works fine,
> though.

Yeah, patch has its own ed handling now. And its substitution only
supports s/.// the way it was before (cheating in handling "a"): 

> - diff_output("%ds/.//\n", a);

With this change, the handling of "cline->subst" in ed.c must be adjusted,
which won't be difficult.

Sounds like Martin is already on his way, so I'm lurking to review his
upcoming diff. ;)


Tobias



Re: newfs: avoid oob read on command line argument

2015-12-05 Thread Tobias Stoeckmann
On Sat, Dec 05, 2015 at 06:26:35AM -0500, Ted Unangst wrote:
> may i suggest strlen(s) instead of strchr(s, 0)?

There's actually one part in newfs' code that uses this. And in theory
it has the same issue, not checking if s (which is special, which might
be argv[0]) is empty. I highly doubt this could be reached there, but
I fixed it anyway. Until now it uses strncpy, and with the switch to
strlcpy this is just another additional boundary check in place.


Tobias

Index: sbin/newfs/newfs.c
===
RCS file: /cvs/src/sbin/newfs/newfs.c,v
retrieving revision 1.103
diff -u -p -u -p -r1.103 newfs.c
--- sbin/newfs/newfs.c  25 Nov 2015 19:45:21 -  1.103
+++ sbin/newfs/newfs.c  5 Dec 2015 12:32:07 -
@@ -423,10 +423,11 @@ main(int argc, char *argv[])
warnx("%s: not a character-special device",
special);
}
-   cp = strchr(argv[0], '\0') - 1;
-   if (cp == NULL ||
-   ((*cp < 'a' || *cp > ('a' + maxpartitions - 1))
-   && !isdigit((unsigned char)*cp)))
+   if (*argv[0] == '\0')
+   fatal("empty partition name supplied");
+   cp = argv[0] + strlen(argv[0]) - 1;
+   if ((*cp < 'a' || *cp > ('a' + maxpartitions - 1))
+   && !isdigit((unsigned char)*cp))
fatal("%s: can't figure out file system partition",
argv[0]);
lp = getdisklabel(special, fsi);
@@ -631,8 +632,9 @@ rewritelabel(char *s, int fd, struct dis
/*
 * Make name for 'c' partition.
 */
-   strncpy(specname, s, sizeof(specname) - 1);
-   specname[sizeof(specname) - 1] = '\0';
+   if (*s == '\0' ||
+   strlcpy(specname, s, sizeof(specname)) >= sizeof(specname))
+   fatal("%s: invalid partition name supplied", s);
cp = specname + strlen(specname) - 1;
if (!isdigit((unsigned char)*cp))
*cp = 'c';
Index: sbin/newfs_ext2fs/newfs_ext2fs.c
===
RCS file: /cvs/src/sbin/newfs_ext2fs/newfs_ext2fs.c,v
retrieving revision 1.21
diff -u -p -u -p -r1.21 newfs_ext2fs.c
--- sbin/newfs_ext2fs/newfs_ext2fs.c28 Nov 2015 06:12:09 -  1.21
+++ sbin/newfs_ext2fs/newfs_ext2fs.c5 Dec 2015 12:32:07 -
@@ -529,9 +529,11 @@ getpartition(int fsi, const char *specia
errx(EXIT_FAILURE, "%s: block device", special);
if (!S_ISCHR(st.st_mode))
warnx("%s: not a character-special device", special);
-   cp = strchr(argv[0], '\0') - 1;
-   if (cp == NULL || ((*cp < 'a' || *cp > ('a' + getmaxpartitions() - 1))
-   && !isdigit((unsigned char)*cp)))
+   if (*argv[0] == '\0')
+   errx(EXIT_FAILURE, "empty partition name supplied");
+   cp = argv[0] + strlen(argv[0]) - 1;
+   if ((*cp < 'a' || *cp > ('a' + getmaxpartitions() - 1))
+   && !isdigit((unsigned char)*cp))
errx(EXIT_FAILURE, "%s: can't figure out file system 
partition", argv[0]);
lp = getdisklabel(special, fsi);
if (isdigit((unsigned char)*cp))



Re: libc: getusershell, new implementation

2015-12-05 Thread Tobias Stoeckmann
> Index: gen/getusershell.c
> ===
> RCS file: /cvs/src/lib/libc/gen/getusershell.c,v
> retrieving revision 1.16
> diff -u -p -r1.16 getusershell.c
> --- gen/getusershell.c14 Sep 2015 16:09:13 -  1.16
> +++ gen/getusershell.c5 Dec 2015 16:08:56 -
> @@ -84,48 +86,52 @@ setusershell(void)
>  static char **
>  initshells(void)
>  {
> - char **sp, *cp;
> + char buf[PATH_MAX];
> + int nshells = 0, nalloc;

I would prefer size_t for nshells and nalloc.

> + char *cp;
>   FILE *fp;
> - struct stat statb;
>  
>   free(shells);
>   shells = NULL;
> - free(strings);
> - strings = NULL;
> +
>   if ((fp = fopen(_PATH_SHELLS, "re")) == NULL)
>   return (okshells);
> - if (fstat(fileno(fp), ) == -1) {
> - (void)fclose(fp);
> - return (okshells);
> - }
> - if (statb.st_size > SIZE_MAX) {
> - (void)fclose(fp);
> - return (okshells);
> - }
> - if ((strings = malloc((size_t)statb.st_size)) == NULL) {
> - (void)fclose(fp);
> - return (okshells);
> - }
> - shells = calloc((size_t)(statb.st_size / 3 + 2), sizeof (char *));
> - if (shells == NULL) {
> - (void)fclose(fp);
> - free(strings);
> - strings = NULL;
> - return (okshells);
> - }
> - sp = shells;
> - cp = strings;
> - while (fgets(cp, PATH_MAX + 1, fp) != NULL) {
> +
> + nalloc = 10; // just an initial guess
> + nshells = 0;
> + shells = reallocarray(NULL, nalloc, sizeof (char *));
> + if (shells == NULL)
> + goto fail;
> + while ((cp = fgets(buf, sizeof(buf), fp)) != NULL) {

We already have to dynamically allocate memory anyway, so getline() would
fix some issues we could face while parsing files. The buffer is PATH_MAX
bytes long, which should be sufficient, but if a comment is PATH_MAX + x
bytes in size, we would parse the "x" part as a real path due to fgets'
truncation/wrapping.

>   while (*cp != '#' && *cp != '/' && *cp != '\0')
>   cp++;
>   if (*cp == '#' || *cp == '\0')
>   continue;
> - *sp++ = cp;
> + if (!(shells[nshells] = strdup(cp)))
> + goto fail;
> + cp = shells[nshells];
>   while (!isspace((unsigned char)*cp) && *cp != '#' && *cp != 
> '\0')
>   cp++;
>   *cp++ = '\0';

And I still think that the current code is a bit too permissive in parsing
things. I mean what's the point in allowing lines like:

sometextwithoutspace/bin/ksh should be used for logins # seriously!

Which would result in /bin/ksh, by the way.

Didn't notice the consequences that arise by keeping the descriptor open,
so I'm fine with an alternative approach. Yet we might make the code a
bit easier to review by not allowing such weird lines. What it should
expect and enforce:

- a valid line has to start with a slash
- comments are chopped off
- comments are supposed to be at the beginning of a line

So if somebody writes "/bin/ksh # comment", that actually leads to "/bin/ksh ",
with an additional whitespace at the end. Currently we couldn't even specify a
shell with a whitespace in its path.


Tobias



Re: libc: getusershell, new implementation

2015-12-05 Thread Tobias Stoeckmann
On Sat, Dec 05, 2015 at 01:25:10PM -0500, Ted Unangst wrote:
> ok. i was going to leave the behavior alone, but we can fix that too.
> 
> - use getline to read lines of any length.
> - only consider lines that start with a /.
> - truncate lines after a #, but not after spaces.

ok tobias, thanks for this clean implementation and adjusting the parser
while at it. I guess we wouldn't touch the code otherwise again.



newfs: avoid oob read on command line argument

2015-12-05 Thread Tobias Stoeckmann
Here's the spin-off from previous __progname patch.

It's possible to have an out-of-boundary read in newfs_ext2fs when
supplying an empty partition name. Before calling strchr() - 1, it should
be verified that it's not empty. While at it, the result of the strchr call
will never be NULL, because eventually a '\0' char will be found. Even if
that would not be the case, the "- 1" addition renders the NULL check
pointless.

mmcc@ had the nice idea to split this into an own check, which looks much
better because we avoid saving an illegal pointer, even though it wouldn't
be used.

With applied patch:

$ newfs_ext2fs -N ""   
newfs_ext2fs: /dev/: not a character-special device
newfs_ext2fs: empty partition name supplied
$ _

I think the newfs-part cannot be triggered, but better be safe than
sorry, and stay in sync with newfs_ext2fs.


Tobias

Index: sbin/newfs/newfs.c
===
RCS file: /cvs/src/sbin/newfs/newfs.c,v
retrieving revision 1.103
diff -u -p -u -p -r1.103 newfs.c
--- sbin/newfs/newfs.c  25 Nov 2015 19:45:21 -  1.103
+++ sbin/newfs/newfs.c  5 Dec 2015 10:52:39 -
@@ -423,10 +423,11 @@ main(int argc, char *argv[])
warnx("%s: not a character-special device",
special);
}
+   if (*argv[0] == '\0')
+   fatal("empty partition name supplied");
cp = strchr(argv[0], '\0') - 1;
-   if (cp == NULL ||
-   ((*cp < 'a' || *cp > ('a' + maxpartitions - 1))
-   && !isdigit((unsigned char)*cp)))
+   if ((*cp < 'a' || *cp > ('a' + maxpartitions - 1))
+   && !isdigit((unsigned char)*cp))
fatal("%s: can't figure out file system partition",
argv[0]);
lp = getdisklabel(special, fsi);
Index: sbin/newfs_ext2fs/newfs_ext2fs.c
===
RCS file: /cvs/src/sbin/newfs_ext2fs/newfs_ext2fs.c,v
retrieving revision 1.21
diff -u -p -u -p -r1.21 newfs_ext2fs.c
--- sbin/newfs_ext2fs/newfs_ext2fs.c28 Nov 2015 06:12:09 -  1.21
+++ sbin/newfs_ext2fs/newfs_ext2fs.c5 Dec 2015 10:52:39 -
@@ -529,9 +529,11 @@ getpartition(int fsi, const char *specia
errx(EXIT_FAILURE, "%s: block device", special);
if (!S_ISCHR(st.st_mode))
warnx("%s: not a character-special device", special);
+   if (*argv[0] == '\0')
+   errx(EXIT_FAILURE, "empty partition name supplied");
cp = strchr(argv[0], '\0') - 1;
-   if (cp == NULL || ((*cp < 'a' || *cp > ('a' + getmaxpartitions() - 1))
-   && !isdigit((unsigned char)*cp)))
+   if ((*cp < 'a' || *cp > ('a' + getmaxpartitions() - 1))
+   && !isdigit((unsigned char)*cp))
errx(EXIT_FAILURE, "%s: can't figure out file system 
partition", argv[0]);
lp = getdisklabel(special, fsi);
if (isdigit((unsigned char)*cp))



Re: __progname in base

2015-12-05 Thread Tobias Stoeckmann
On Sat, Dec 05, 2015 at 03:29:06AM -0500, Ted Unangst wrote:
> looks good, but you've got some mostly unrelated changes in here. this should
> be separate, but ok for the rest.

It started with a "check argv" code review and ended up with __progname
adjustments, so I agree here and removed the newfs parts for a separate
commit. Also the gomoku KNF is a bit distracting, so it's gone now, too.

Theo pointed out that tradcpp and lex should stay as they are, because
they are upstream projects.

Joerg mentioned the getprogname function previously, so I left id and nl
alone, too.

The patch touches these programs now:
mt, pax, gomoku, telnet, crunchgen, pppd, and pdisk

As I have removed the concerning parts and got OKs for the rest,
I will commit it later this day if nobody objects.

Index: bin/mt/mt.c
===
RCS file: /cvs/src/bin/mt/mt.c,v
retrieving revision 1.36
diff -u -p -u -p -r1.36 mt.c
--- bin/mt/mt.c 12 Nov 2013 04:36:02 -  1.36
+++ bin/mt/mt.c 5 Dec 2015 10:01:55 -
@@ -88,6 +88,8 @@ int   _rmtmtioctop(int fd, struct mtop *c
 struct mtget   *_rmtstatus(int fd);
 void   _rmtclose(void);
 
+extern char*__progname;
+
 char   *host = NULL;   /* remote host (if any) */
 
 int
@@ -133,7 +135,6 @@ _rmtclose(void)
 #endif
 }
 
-char   *progname;
 inteject = 0;
 
 int
@@ -145,12 +146,7 @@ main(int argc, char *argv[])
char *p, *tape, *realtape, *opts;
size_t len;
 
-   if ((progname = strrchr(argv[0], '/')))
-   progname++;
-   else
-   progname = argv[0];
-
-   if (strcmp(progname, "eject") == 0) {
+   if (strcmp(__progname, "eject") == 0) {
opts = "t";
eject = 1;
tape = NULL;
@@ -320,9 +316,9 @@ void
 usage(void)
 {
if (eject)
-   (void)fprintf(stderr, "usage: %s [-t] device\n", progname);
+   (void)fprintf(stderr, "usage: %s [-t] device\n", __progname);
else
(void)fprintf(stderr,
-   "usage: %s [-f device] command [count]\n", progname);
+   "usage: %s [-f device] command [count]\n", __progname);
exit(X_USAGE);
 }
Index: bin/pax/options.c
===
RCS file: /cvs/src/bin/pax/options.c,v
retrieving revision 1.91
diff -u -p -u -p -r1.91 options.c
--- bin/pax/options.c   18 May 2015 20:26:16 -  1.91
+++ bin/pax/options.c   5 Dec 2015 10:01:56 -
@@ -184,14 +184,12 @@ char *chdname = NULL;
 void
 options(int argc, char **argv)
 {
+   extern char *__progname;
 
/*
 * Are we acting like pax, tar or cpio (based on argv[0])
 */
-   if ((argv0 = strrchr(argv[0], '/')) != NULL)
-   argv0++;
-   else
-   argv0 = argv[0];
+   argv0 = __progname;
 
if (strcmp(NM_TAR, argv0) == 0) {
tar_options(argc, argv);
Index: games/gomoku/main.c
===
RCS file: /cvs/src/games/gomoku/main.c,v
retrieving revision 1.29
diff -u -p -u -p -r1.29 main.c
--- games/gomoku/main.c 30 Nov 2015 08:44:51 -  1.29
+++ games/gomoku/main.c 5 Dec 2015 10:01:56 -
@@ -45,10 +45,11 @@
 #define PROGRAM1   /* get input from program */
 #define INPUTF 2   /* get input from a file */
 
+extern char *__progname;   /* name of program */
+
 intinteractive = 1;/* true if interactive */
 intdebug;  /* true if debugging */
 inttest;   /* both moves come from 1: input, 2: computer */
-char   *prog;  /* name of program */
 FILE   *debugfp;   /* file for debug output */
 FILE   *inputfp;   /* file for debug input */
 
@@ -84,12 +85,6 @@ main(argc, argv)
if (pledge("stdio rpath wpath cpath tty", NULL) == -1)
err(1, "pledge");
 
-   prog = strrchr(argv[0], '/');
-   if (prog)
-   prog++;
-   else
-   prog = argv[0];
-
if ((tmpname = getlogin()) != NULL)
strlcpy(you, tmpname, sizeof(you));
else
@@ -117,7 +112,7 @@ main(argc, argv)
default:
fprintf(stderr,
"usage: %s [-bcdu] [-D debugfile] [inputfile]\n",
-   prog);
+   __progname);
exit(1);
}
}
@@ -194,8 +189,8 @@ again:
}
}
if (interactive) {
-   plyr[BLACK] = input[BLACK] == USER ? you : prog;
-   plyr[WHITE] = input[WHITE] == USER ? you : prog;
+   plyr[BLACK] = input[BLACK] == USER ? you : __progname;
+   plyr[WHITE] = input[WHITE] == USER ? you : __progname;
bdwho(1);
}
 
@@ -222,8 +217,8 @@ again:
  

Re: libc: locale/rune.c input validation

2015-12-04 Thread Tobias Stoeckmann
Hi Ingo,

On Fri, Dec 04, 2015 at 12:27:39PM +0100, Ingo Schwarze wrote:
> uint32_t should be preferred because that's the POSIX type, while
> u_int32_t is not standardized.  If you are working on the file
> anyway, i'd recommend to unify all uses to uint32_t.

done.

> __LP64__ that is overly specific and not standardized.  Something
> like this maybe:
> 
>   #if SIZE_MAX <= UINT32_MAX

That sounds nice and gives a better clue why that #if is in place.
I've applied it.

> > I still cast frl_variable_len to u_int32_t for ntohl, but will cast it
> > back to int32_t for a "less than 0" check. This should properly handle
> > error cases when it's been negative to begin with.
> 
> True.  Slightly ugly, but i don't see a better way either.

Looking at UINT32_MAX usage now, I just test for var_len > INT32_MAX.


Tobias

Index: locale/rune.c
===
RCS file: /cvs/src/lib/libc/locale/rune.c,v
retrieving revision 1.4
diff -u -p -u -p -r1.4 rune.c
--- locale/rune.c   25 May 2014 17:47:04 -  1.4
+++ locale/rune.c   4 Dec 2015 20:48:52 -
@@ -59,23 +59,31 @@
  * SUCH DAMAGE.
  */
 
+#include 
+#include 
 #include 
+#include 
+#include 
 #include 
-#include 
 #include 
-#include 
+#include 
 #include 
-#include 
-#include 
 #include "rune.h"
 #include "rune_local.h"
 
-static int readrange(_RuneLocale *, _RuneRange *, _FileRuneRange *, void *, 
FILE *);
+#define SAFE_ADD(x, y) \
+do {   \
+   if ((x) > SIZE_MAX - (y))   \
+   return NULL;\
+   (x) += (y); \
+} while (0);
+
+static int readrange(_RuneLocale *, _RuneRange *, uint32_t, void *, FILE *);
 static void _freeentry(_RuneRange *);
 static void _wctype_init(_RuneLocale *rl);
 
 static int
-readrange(_RuneLocale *rl, _RuneRange *rr, _FileRuneRange *frr, void *lastp,
+readrange(_RuneLocale *rl, _RuneRange *rr, uint32_t nranges, void *lastp,
FILE *fp)
 {
uint32_t i;
@@ -84,7 +92,7 @@ readrange(_RuneLocale *rl, _RuneRange *r
 
re = (_RuneEntry *)rl->rl_variable;
 
-   rr->rr_nranges = ntohl(frr->frr_nranges);
+   rr->rr_nranges = nranges;
if (rr->rr_nranges == 0) {
rr->rr_rune_ranges = NULL;
return 0;
@@ -92,16 +100,16 @@ readrange(_RuneLocale *rl, _RuneRange *r
 
rr->rr_rune_ranges = re;
for (i = 0; i < rr->rr_nranges; i++) {
+   if ((void *)re >= lastp)
+   return -1;
+
if (fread(, sizeof(fre), 1, fp) != 1)
return -1;
 
-   re->re_min = ntohl((u_int32_t)fre.fre_min);
-   re->re_max = ntohl((u_int32_t)fre.fre_max);
-   re->re_map = ntohl((u_int32_t)fre.fre_map);
+   re->re_min = ntohl((uint32_t)fre.fre_min);
+   re->re_max = ntohl((uint32_t)fre.fre_max);
+   re->re_map = ntohl((uint32_t)fre.fre_map);
re++;
-
-   if ((void *)re > lastp)
-   return -1;
}
rl->rl_variable = re;
return 0;
@@ -121,6 +129,9 @@ readentry(_RuneRange *rr, FILE *fp)
continue;
}
 
+   if (re[i].re_max < re[i].re_min)
+   goto fail;
+
l = re[i].re_max - re[i].re_min + 1;
re[i].re_rune_types = calloc(l, sizeof(_RuneType));
if (!re[i].re_rune_types) {
@@ -151,17 +162,20 @@ fail2:
 }
 
 /* XXX: temporary implementation */
-static void
+static int
 find_codeset(_RuneLocale *rl)
 {
char *top, *codeset, *tail, *ep;
 
+   if (rl->rl_variable == NULL)
+   return 0;
+
/* end of rl_variable region */
ep = (char *)rl->rl_variable;
ep += rl->rl_variable_len;
rl->rl_codeset = NULL;
if (!(top = strstr(rl->rl_variable, _RUNE_CODESET)))
-   return;
+   return 0;
tail = strpbrk(top, " \t");
codeset = top + sizeof(_RUNE_CODESET) - 1;
if (tail) {
@@ -173,6 +187,7 @@ find_codeset(_RuneLocale *rl)
*top = '\0';
rl->rl_codeset = strdup(codeset);
}
+   return (rl->rl_codeset == NULL);
 }
 
 void
@@ -183,8 +198,7 @@ _freeentry(_RuneRange *rr)
 
re = rr->rr_rune_ranges;
for (i = 0; i < rr->rr_nranges; i++) {
-   if (re[i].re_rune_types)
-   free(re[i].re_rune_types);
+   free(re[i].re_rune_types);
re[i].re_rune_types = NULL;
}
 }
@@ -209,6 +223,7 @@ _Read_RuneMagi(FILE *fp)
_RuneLocale *rl;
struct stat sb;
int x;
+   uint32_t runetype_nranges, maplower_nranges, mapupper_nranges, var_len;
 
if (fstat(fileno(fp), ) < 0)
return NULL;
@@ -225,10 +240,26 @@ _Read_RuneMagi(FILE *fp)
if (memcmp(frl.frl_magic, _RUNE_MAGIC_1, 

libc: getusershell, new implementation

2015-12-04 Thread Tobias Stoeckmann
There's still a possible overflow in getusershell.c. We could increase
the buffer allocation yet again, but I have to agree with the glibc
developers here: enough is enough. The code is ugly and has proven to be
difficult to review.

The overflow has been spotted by Paul Pluzhnikov, after I submitted
a bug report of our getusershell fixes to glibc, as seen here:
https://sourceware.org/bugzilla/show_bug.cgi?id=18660

It led to a discussion of the glibc developers. Eventually, Mike Frysinger
came up with a new implementation, which is not in glibc yet. I like it
a lot, and Mike was kind enough to allow an MIT-licensed version for us.
If in doubt, Mike is in CC. It's basically his implementation, plus KNF
and using strcspn for shorter code.

The new code differs from old behavior, but it should be sufficient. We
support /etc/shells as seen in our system and as it is described in
shells(5). But the wording of shells(5) is a bit odd. It makes me believe
that relative paths would be okay, because:

 For each shell a single line should be present, consisting of
 the shell's path, relative to root.

That is not true with the new code, and should be avoided IMHO. If this
implementation encounters a line that does not start with a slash '/',
it will be discarded. Maybe we should adjust the manual page to be more
precise?

If you want to test this code, chpass is the easiest way. Change your
login shell to something that is not in /etc/shells and you should get
a warning. If it's in /etc/shells, no warning shall be seen.

Is it worth it, knowing that it's a deprecated BSD-specific function?
Or maybe just increase buffer allocation (hopefully just) one more time?

Tobias


Index: lib/libc/gen/getusershell.c
===
RCS file: /cvs/src/lib/libc/gen/getusershell.c,v
retrieving revision 1.16
diff -u -p -u -p -r1.16 getusershell.c
--- gen/getusershell.c  14 Sep 2015 16:09:13 -  1.16
+++ gen/getusershell.c  4 Dec 2015 21:39:01 -
@@ -1,131 +1,106 @@
-/* $OpenBSD: getusershell.c,v 1.16 2015/09/14 16:09:13 tedu Exp $ */
+/* $OpenBSD$ */
 /*
- * Copyright (c) 1985, 1993
- * The Regents of the University of California.  All rights reserved.
+ * Copyright (c) 2015 Mike Frysinger 
  *
- * Redistribution and use in source and binary forms, with or without
- * modification, are permitted provided that the following conditions
- * are met:
- * 1. Redistributions of source code must retain the above copyright
- *notice, this list of conditions and the following disclaimer.
- * 2. Redistributions in binary form must reproduce the above copyright
- *notice, this list of conditions and the following disclaimer in the
- *documentation and/or other materials provided with the distribution.
- * 3. Neither the name of the University nor the names of its contributors
- *may be used to endorse or promote products derived from this software
- *without specific prior written permission.
+ * Permission to use, copy, modify, and distribute this software for any
+ * purpose with or without fee is hereby granted, provided that the above
+ * copyright notice and this permission notice appear in all copies.
  *
- * THIS SOFTWARE IS PROVIDED BY THE REGENTS AND CONTRIBUTORS ``AS IS'' AND
- * ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE
- * IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE
- * ARE DISCLAIMED.  IN NO EVENT SHALL THE REGENTS OR CONTRIBUTORS BE LIABLE
- * FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL
- * DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS
- * OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION)
- * HOWEVER CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT
- * LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY
- * OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF
- * SUCH DAMAGE.
+ * THE SOFTWARE IS PROVIDED "AS IS" AND THE AUTHOR DISCLAIMS ALL WARRANTIES
+ * WITH REGARD TO THIS SOFTWARE INCLUDING ALL IMPLIED WARRANTIES OF
+ * MERCHANTABILITY AND FITNESS. IN NO EVENT SHALL THE AUTHOR BE LIABLE FOR
+ * ANY SPECIAL, DIRECT, INDIRECT, OR CONSEQUENTIAL DAMAGES OR ANY DAMAGES
+ * WHATSOEVER RESULTING FROM LOSS OF USE, DATA OR PROFITS, WHETHER IN AN
+ * ACTION OF CONTRACT, NEGLIGENCE OR OTHER TORTIOUS ACTION, ARISING OUT OF
+ * OR IN CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE.
  */
 
-#include 
-
-#include 
-#include 
+#include 
 #include 
-#include 
 #include 
 #include 
+#include 
 #include 
 
-/*
- * Local shells should NOT be added here.  They should be added in
- * /etc/shells.
- */
+/* These functions are not thread safe (by design), so globals are OK. */
+static FILE*shellfp;
+static int  okshell_idx;
+static char*shellbuf;
+static size_t   buf_len;
 
-static char *okshells[] = { _PATH_BSHELL, 

Re: libc: locale/rune.c input validation

2015-12-03 Thread Tobias Stoeckmann
Thanks Ingo for your extensive review! It contains lots of valuable
input for me.

I have applied all your recommendations, they make a lot of sense.

> I would suggest to use uint32_t.

Just while applying this, I noticed that the file has a mix of the
types u_int32_t and uint32_t. I took u_int32_t for now because it was
in there more often than uint32_t. What do you think?

> > +#ifndef __LP64__
> > +   if (runetype_nranges > SIZE_MAX / sizeof(_RuneEntry) ||
> > +   maplower_nranges > SIZE_MAX / sizeof(_RuneEntry) ||
> > +   mapupper_nranges > SIZE_MAX / sizeof(_RuneEntry))
> > +   return NULL;
> > +#endif
> 
> I dislike using #ifdef, it seems error-prone to me.  The checks
> are correct in any case.  Sure, if size_t is much larger than uint32_t,
> these checks can never trigger.  But why do micro-optimization here?
> Why not just delete the #ifndef/#endif?

If I skip this __LP64__ check, it triggers a gcc warning on amd64,
because the compiler tells me that the test is useless. So it's a
cosmetical thing. It's removed for now.

I still cast frl_variable_len to u_int32_t for ntohl, but will cast it
back to int32_t for a "less than 0" check. This should properly handle
error cases when it's been negative to begin with.


Tobias

Index: lib/libc/locale/rune.c
===
RCS file: /cvs/src/lib/libc/locale/rune.c,v
retrieving revision 1.4
diff -u -p -u -p -r1.4 rune.c
--- locale/rune.c   25 May 2014 17:47:04 -  1.4
+++ locale/rune.c   3 Dec 2015 21:55:54 -
@@ -59,32 +59,40 @@
  * SUCH DAMAGE.
  */
 
+#include 
+#include 
 #include 
+#include 
+#include 
 #include 
-#include 
 #include 
-#include 
+#include 
 #include 
-#include 
-#include 
 #include "rune.h"
 #include "rune_local.h"
 
-static int readrange(_RuneLocale *, _RuneRange *, _FileRuneRange *, void *, 
FILE *);
+#define SAFE_ADD(x, y) \
+do {   \
+   if ((x) > SIZE_MAX - (y))   \
+   return NULL;\
+   (x) += (y); \
+} while (0);
+
+static int readrange(_RuneLocale *, _RuneRange *, u_int32_t, void *, FILE *);
 static void _freeentry(_RuneRange *);
 static void _wctype_init(_RuneLocale *rl);
 
 static int
-readrange(_RuneLocale *rl, _RuneRange *rr, _FileRuneRange *frr, void *lastp,
+readrange(_RuneLocale *rl, _RuneRange *rr, u_int32_t nranges, void *lastp,
FILE *fp)
 {
-   uint32_t i;
+   u_int32_t i;
_RuneEntry *re;
_FileRuneEntry fre;
 
re = (_RuneEntry *)rl->rl_variable;
 
-   rr->rr_nranges = ntohl(frr->frr_nranges);
+   rr->rr_nranges = nranges;
if (rr->rr_nranges == 0) {
rr->rr_rune_ranges = NULL;
return 0;
@@ -92,6 +100,9 @@ readrange(_RuneLocale *rl, _RuneRange *r
 
rr->rr_rune_ranges = re;
for (i = 0; i < rr->rr_nranges; i++) {
+   if ((void *)re >= lastp)
+   return -1;
+
if (fread(, sizeof(fre), 1, fp) != 1)
return -1;
 
@@ -99,9 +110,6 @@ readrange(_RuneLocale *rl, _RuneRange *r
re->re_max = ntohl((u_int32_t)fre.fre_max);
re->re_map = ntohl((u_int32_t)fre.fre_map);
re++;
-
-   if ((void *)re > lastp)
-   return -1;
}
rl->rl_variable = re;
return 0;
@@ -121,6 +129,9 @@ readentry(_RuneRange *rr, FILE *fp)
continue;
}
 
+   if (re[i].re_max < re[i].re_min)
+   goto fail;
+
l = re[i].re_max - re[i].re_min + 1;
re[i].re_rune_types = calloc(l, sizeof(_RuneType));
if (!re[i].re_rune_types) {
@@ -151,17 +162,20 @@ fail2:
 }
 
 /* XXX: temporary implementation */
-static void
+static int
 find_codeset(_RuneLocale *rl)
 {
char *top, *codeset, *tail, *ep;
 
+   if (rl->rl_variable == NULL)
+   return 0;
+
/* end of rl_variable region */
ep = (char *)rl->rl_variable;
ep += rl->rl_variable_len;
rl->rl_codeset = NULL;
if (!(top = strstr(rl->rl_variable, _RUNE_CODESET)))
-   return;
+   return 0;
tail = strpbrk(top, " \t");
codeset = top + sizeof(_RUNE_CODESET) - 1;
if (tail) {
@@ -173,18 +187,18 @@ find_codeset(_RuneLocale *rl)
*top = '\0';
rl->rl_codeset = strdup(codeset);
}
+   return (rl->rl_codeset == NULL);
 }
 
 void
 _freeentry(_RuneRange *rr)
 {
_RuneEntry *re;
-   uint32_t i;
+   u_int32_t i;
 
re = rr->rr_rune_ranges;
for (i = 0; i < rr->rr_nranges; i++) {
-   if (re[i].re_rune_types)
-   free(re[i].re_rune_types);
+   free(re[i].re_rune_types);
re[i].re_rune_types = NULL;
}
 }
@@ -209,6 +223,7 @@ 

libc: locale/rune.c input validation

2015-12-02 Thread Tobias Stoeckmann
Hi,

this patch adds a lot of input validation to libc/locale/rune.c.
The kind of validations are borrowed from my nls changes some
weeks ago.

I've contacted stsp@ about this. I think it's ready to get more
review from tech@. Let me know what you think!


Tobias

Index: rune.c
===
RCS file: /cvs/src/lib/libc/locale/rune.c,v
retrieving revision 1.4
diff -u -p -u -p -r1.4 rune.c
--- rune.c  25 May 2014 17:47:04 -  1.4
+++ rune.c  30 Oct 2015 16:13:01 -
@@ -59,23 +59,31 @@
  * SUCH DAMAGE.
  */
 
+#include 
+#include 
 #include 
+#include 
+#include 
 #include 
-#include 
 #include 
-#include 
+#include 
 #include 
-#include 
-#include 
 #include "rune.h"
 #include "rune_local.h"
 
-static int readrange(_RuneLocale *, _RuneRange *, _FileRuneRange *, void *, 
FILE *);
+#define SAFE_ADD(x, y) \
+do {   \
+   if ((x) > SIZE_MAX - (y))   \
+   return NULL;\
+   (x) += (y); \
+} while (0);
+
+static int readrange(_RuneLocale *, _RuneRange *, rune_t, void *, FILE *);
 static void _freeentry(_RuneRange *);
 static void _wctype_init(_RuneLocale *rl);
 
 static int
-readrange(_RuneLocale *rl, _RuneRange *rr, _FileRuneRange *frr, void *lastp,
+readrange(_RuneLocale *rl, _RuneRange *rr, rune_t nranges, void *lastp,
FILE *fp)
 {
uint32_t i;
@@ -84,7 +92,7 @@ readrange(_RuneLocale *rl, _RuneRange *r
 
re = (_RuneEntry *)rl->rl_variable;
 
-   rr->rr_nranges = ntohl(frr->frr_nranges);
+   rr->rr_nranges = nranges;
if (rr->rr_nranges == 0) {
rr->rr_rune_ranges = NULL;
return 0;
@@ -92,6 +100,9 @@ readrange(_RuneLocale *rl, _RuneRange *r
 
rr->rr_rune_ranges = re;
for (i = 0; i < rr->rr_nranges; i++) {
+   if ((void *)re >= lastp)
+   return -1;
+
if (fread(, sizeof(fre), 1, fp) != 1)
return -1;
 
@@ -99,9 +110,6 @@ readrange(_RuneLocale *rl, _RuneRange *r
re->re_max = ntohl((u_int32_t)fre.fre_max);
re->re_map = ntohl((u_int32_t)fre.fre_map);
re++;
-
-   if ((void *)re > lastp)
-   return -1;
}
rl->rl_variable = re;
return 0;
@@ -121,6 +129,9 @@ readentry(_RuneRange *rr, FILE *fp)
continue;
}
 
+   if (re[i].re_max < re[i].re_min)
+   goto fail;
+
l = re[i].re_max - re[i].re_min + 1;
re[i].re_rune_types = calloc(l, sizeof(_RuneType));
if (!re[i].re_rune_types) {
@@ -151,17 +162,20 @@ fail2:
 }
 
 /* XXX: temporary implementation */
-static void
+static int
 find_codeset(_RuneLocale *rl)
 {
char *top, *codeset, *tail, *ep;
 
+   if (rl->rl_variable == NULL)
+   return 0;
+
/* end of rl_variable region */
ep = (char *)rl->rl_variable;
ep += rl->rl_variable_len;
rl->rl_codeset = NULL;
if (!(top = strstr(rl->rl_variable, _RUNE_CODESET)))
-   return;
+   return 0;
tail = strpbrk(top, " \t");
codeset = top + sizeof(_RUNE_CODESET) - 1;
if (tail) {
@@ -173,6 +187,7 @@ find_codeset(_RuneLocale *rl)
*top = '\0';
rl->rl_codeset = strdup(codeset);
}
+   return (rl->rl_codeset == NULL);
 }
 
 void
@@ -183,8 +198,7 @@ _freeentry(_RuneRange *rr)
 
re = rr->rr_rune_ranges;
for (i = 0; i < rr->rr_nranges; i++) {
-   if (re[i].re_rune_types)
-   free(re[i].re_rune_types);
+   free(re[i].re_rune_types);
re[i].re_rune_types = NULL;
}
 }
@@ -209,6 +223,7 @@ _Read_RuneMagi(FILE *fp)
_RuneLocale *rl;
struct stat sb;
int x;
+   rune_t runetype_nranges, maplower_nranges, mapupper_nranges;
 
if (fstat(fileno(fp), ) < 0)
return NULL;
@@ -225,10 +240,24 @@ _Read_RuneMagi(FILE *fp)
if (memcmp(frl.frl_magic, _RUNE_MAGIC_1, sizeof(frl.frl_magic)))
return NULL;
 
-   hostdatalen = sizeof(*rl) + ntohl((u_int32_t)frl.frl_variable_len) +
-   ntohl(frl.frl_runetype_ext.frr_nranges) * sizeof(_RuneEntry) +
-   ntohl(frl.frl_maplower_ext.frr_nranges) * sizeof(_RuneEntry) +
-   ntohl(frl.frl_mapupper_ext.frr_nranges) * sizeof(_RuneEntry);
+   /* XXX assumes rune_t = u_int32_t */
+   runetype_nranges = ntohl(frl.frl_runetype_ext.frr_nranges);
+   maplower_nranges = ntohl(frl.frl_maplower_ext.frr_nranges);
+   mapupper_nranges = ntohl(frl.frl_mapupper_ext.frr_nranges);
+
+#ifndef __LP64__
+   if (runetype_nranges > SIZE_MAX / sizeof(_RuneEntry) ||
+   maplower_nranges > SIZE_MAX / sizeof(_RuneEntry) ||
+   mapupper_nranges > 

Re: unify xmalloc (was Re: [patch] cvs: retire xfree())

2015-11-07 Thread Tobias Stoeckmann
Here's an updated diff:

- use "overflow" error message for snprintf and friends
- use err instead of errx for out of memory conditions
- if fatal() doesn't print error string, use ": %s", strerror(errno)

Is this okay for ssh and tmux, which are out to be very portable?
Nicholas mentioned that malloc is not required to set errno. I've also
checked the standard and it's just an extension. Although at worst,
the user sees a wrong error message...

Index: usr.bin/cvs/xmalloc.c
===
RCS file: /cvs/src/usr.bin/cvs/xmalloc.c,v
retrieving revision 1.12
diff -u -p -u -p -r1.12 xmalloc.c
--- usr.bin/cvs/xmalloc.c   5 Nov 2015 09:48:21 -   1.12
+++ usr.bin/cvs/xmalloc.c   8 Nov 2015 00:27:13 -
@@ -13,6 +13,8 @@
  * called by a name other than "ssh" or "Secure Shell".
  */
 
+#include 
+#include 
 #include 
 #include 
 #include 
@@ -30,7 +32,8 @@ xmalloc(size_t size)
fatal("xmalloc: zero size");
ptr = malloc(size);
if (ptr == NULL)
-   fatal("xmalloc: out of memory (allocating %lu bytes)", (u_long) 
size);
+   fatal("xmalloc: allocating %zu bytes: %s",
+   size, strerror(errno));
return ptr;
 }
 
@@ -41,12 +44,10 @@ xcalloc(size_t nmemb, size_t size)
 
if (size == 0 || nmemb == 0)
fatal("xcalloc: zero size");
-   if (SIZE_MAX / nmemb < size)
-   fatal("xcalloc: nmemb * size > SIZE_MAX");
ptr = calloc(nmemb, size);
if (ptr == NULL)
-   fatal("xcalloc: out of memory (allocating %lu bytes)",
-   (u_long)(size * nmemb));
+   fatal("xcalloc: allocating %zu * %zu bytes: %s",
+   nmemb, size, strerror(errno));
return ptr;
 }
 
@@ -54,28 +55,23 @@ void *
 xreallocarray(void *ptr, size_t nmemb, size_t size)
 {
void *new_ptr;
-   size_t new_size = nmemb * size;
 
-   if (new_size == 0)
-   fatal("xrealloc: zero size");
-   if (SIZE_MAX / nmemb < size)
-   fatal("xrealloc: nmemb * size > SIZE_MAX");
-   new_ptr = realloc(ptr, new_size);
+   if (nmemb == 0 || size == 0)
+   fatal("xreallocarray: zero size");
+   new_ptr = reallocarray(ptr, nmemb, size);
if (new_ptr == NULL)
-   fatal("xrealloc: out of memory (new_size %lu bytes)",
-   (u_long) new_size);
+   fatal("xreallocarray: allocating %zu * %zu bytes: %s",
+   nmemb, size, strerror(errno));
return new_ptr;
 }
 
 char *
 xstrdup(const char *str)
 {
-   size_t len;
char *cp;
 
-   len = strlen(str) + 1;
-   cp = xmalloc(len);
-   strlcpy(cp, str, len);
+   if ((cp = strdup(str)) == NULL)
+   fatal("xstrdup: %s", strerror(errno));
return cp;
 }
 
@@ -90,23 +86,26 @@ xasprintf(char **ret, const char *fmt, .
va_end(ap);
 
if (i < 0 || *ret == NULL)
-   fatal("xasprintf: could not allocate memory");
+   fatal("xasprintf: %s", strerror(errno));
 
-   return (i);
+   return i;
 }
 
 int
-xsnprintf(char *str, size_t size, const char *fmt, ...)
+xsnprintf(char *str, size_t len, const char *fmt, ...)
 {
va_list ap;
int i;
 
+   if (len > INT_MAX)
+   fatal("xsnprintf: len > INT_MAX");
+
va_start(ap, fmt);
-   i = vsnprintf(str, size, fmt, ap);
+   i = vsnprintf(str, len, fmt, ap);
va_end(ap);
 
-   if (i == -1 || i >= (int)size)
+   if (i < 0 || i >= (int)len)
fatal("xsnprintf: overflow");
 
-   return (i);
+   return i;
 }
Index: usr.bin/diff/xmalloc.c
===
RCS file: /cvs/src/usr.bin/diff/xmalloc.c,v
retrieving revision 1.8
diff -u -p -u -p -r1.8 xmalloc.c
--- usr.bin/diff/xmalloc.c  25 Sep 2015 16:16:26 -  1.8
+++ usr.bin/diff/xmalloc.c  8 Nov 2015 00:27:13 -
@@ -27,9 +27,11 @@ xmalloc(size_t size)
 {
void *ptr;
 
+   if (size == 0)
+   errx(2, "xmalloc: zero size");
ptr = malloc(size);
if (ptr == NULL)
-   err(2, "xmalloc %zu", size);
+   err(2, "xmalloc: allocating %zu bytes", size);
return ptr;
 }
 
@@ -40,8 +42,7 @@ xcalloc(size_t nmemb, size_t size)
 
ptr = calloc(nmemb, size);
if (ptr == NULL)
-   err(2, "xcalloc: out of memory (allocating %zu*%zu bytes)",
-   nmemb, size);
+   err(2, "xcalloc: allocating %zu * %zu bytes", nmemb, size);
return ptr;
 }
 
@@ -52,7 +53,8 @@ xreallocarray(void *ptr, size_t nmemb, s
 
new_ptr = reallocarray(ptr, nmemb, size);
if (new_ptr == NULL)
-   err(2, "xrealloc %zu*%zu", nmemb, size);
+   err(2, "xreallocarray: allocating %zu * %zu bytes",
+   nmemb, size);
return 

Re: unify xmalloc (was Re: [patch] cvs: retire xfree())

2015-11-07 Thread Tobias Stoeckmann
On Sat, Nov 07, 2015 at 05:57:55PM -0500, Ted Unangst wrote:
> > Also, I'm seeing a couple "could not allocate memory" messages added to
> > *snprintf() functions. They write to a supplied buffer, no?
> 
> Good catch.

Will update that one, thanks.

> > > > +   i = vsnprintf(str, len, fmt, ap);
> > > > va_end(ap);
> > > >  
> > > > -   if (i == -1 || i >= (int)size)
> > > > -   fatal("xsnprintf: overflow");
> > > > +   if (i < 0 || i >= (int)len)
> > > > +   fatal("xsnprintf: could not allocate memory");
> 
> This change (among a few others) is wrong.

Could you give me a bit of detail what's wrong here?
Can update the diff when you give me details on "a few others", too.



Re: unify xmalloc (was Re: [patch] cvs: retire xfree())

2015-11-05 Thread Tobias Stoeckmann
On Thu, Nov 05, 2015 at 03:57:26PM +, Nicholas Marriott wrote:
> I like this a lot.
> 
> There are some trivial differences in the various xmalloc.h as well, and
> I think you could make the style consistent within the files (eg "return
> i" in xasprintf and xsnprintf).

Oh yes, forgot to check the header files. Updated diff below, including
the return (i) vs. return i change.

Index: usr.bin/cvs/xmalloc.c
===
RCS file: /cvs/src/usr.bin/cvs/xmalloc.c,v
retrieving revision 1.12
diff -u -p -u -p -r1.12 xmalloc.c
--- usr.bin/cvs/xmalloc.c   5 Nov 2015 09:48:21 -   1.12
+++ usr.bin/cvs/xmalloc.c   5 Nov 2015 16:32:21 -
@@ -13,6 +13,7 @@
  * called by a name other than "ssh" or "Secure Shell".
  */
 
+#include 
 #include 
 #include 
 #include 
@@ -30,7 +31,7 @@ xmalloc(size_t size)
fatal("xmalloc: zero size");
ptr = malloc(size);
if (ptr == NULL)
-   fatal("xmalloc: out of memory (allocating %lu bytes)", (u_long) 
size);
+   fatal("xmalloc: out of memory (allocating %zu bytes)", size);
return ptr;
 }
 
@@ -41,12 +42,10 @@ xcalloc(size_t nmemb, size_t size)
 
if (size == 0 || nmemb == 0)
fatal("xcalloc: zero size");
-   if (SIZE_MAX / nmemb < size)
-   fatal("xcalloc: nmemb * size > SIZE_MAX");
ptr = calloc(nmemb, size);
if (ptr == NULL)
-   fatal("xcalloc: out of memory (allocating %lu bytes)",
-   (u_long)(size * nmemb));
+   fatal("xcalloc: out of memory (allocating %zu * %zu bytes)",
+   nmemb, size);
return ptr;
 }
 
@@ -54,28 +53,23 @@ void *
 xreallocarray(void *ptr, size_t nmemb, size_t size)
 {
void *new_ptr;
-   size_t new_size = nmemb * size;
 
-   if (new_size == 0)
-   fatal("xrealloc: zero size");
-   if (SIZE_MAX / nmemb < size)
-   fatal("xrealloc: nmemb * size > SIZE_MAX");
-   new_ptr = realloc(ptr, new_size);
+   if (nmemb == 0 || size == 0)
+   fatal("xreallocarray: zero size");
+   new_ptr = reallocarray(ptr, nmemb, size);
if (new_ptr == NULL)
-   fatal("xrealloc: out of memory (new_size %lu bytes)",
-   (u_long) new_size);
+   fatal("xreallocarray: out of memory "
+   "(allocating %zu * %zu bytes)", nmemb, size);
return new_ptr;
 }
 
 char *
 xstrdup(const char *str)
 {
-   size_t len;
char *cp;
 
-   len = strlen(str) + 1;
-   cp = xmalloc(len);
-   strlcpy(cp, str, len);
+   if ((cp = strdup(str)) == NULL)
+   fatal("xstrdup: could not allocate memory");
return cp;
 }
 
@@ -92,21 +86,24 @@ xasprintf(char **ret, const char *fmt, .
if (i < 0 || *ret == NULL)
fatal("xasprintf: could not allocate memory");
 
-   return (i);
+   return i;
 }
 
 int
-xsnprintf(char *str, size_t size, const char *fmt, ...)
+xsnprintf(char *str, size_t len, const char *fmt, ...)
 {
va_list ap;
int i;
 
+   if (len > INT_MAX)
+   fatal("xsnprintf: len > INT_MAX");
+
va_start(ap, fmt);
-   i = vsnprintf(str, size, fmt, ap);
+   i = vsnprintf(str, len, fmt, ap);
va_end(ap);
 
-   if (i == -1 || i >= (int)size)
-   fatal("xsnprintf: overflow");
+   if (i < 0 || i >= (int)len)
+   fatal("xsnprintf: could not allocate memory");
 
-   return (i);
+   return i;
 }
Index: usr.bin/diff/xmalloc.c
===
RCS file: /cvs/src/usr.bin/diff/xmalloc.c,v
retrieving revision 1.8
diff -u -p -u -p -r1.8 xmalloc.c
--- usr.bin/diff/xmalloc.c  25 Sep 2015 16:16:26 -  1.8
+++ usr.bin/diff/xmalloc.c  5 Nov 2015 16:32:21 -
@@ -27,9 +27,11 @@ xmalloc(size_t size)
 {
void *ptr;
 
+   if (size == 0)
+   errx(2, "xmalloc: zero size");
ptr = malloc(size);
if (ptr == NULL)
-   err(2, "xmalloc %zu", size);
+   errx(2, "xmalloc: out of memory (allocating %zu bytes)", size);
return ptr;
 }
 
@@ -40,7 +42,7 @@ xcalloc(size_t nmemb, size_t size)
 
ptr = calloc(nmemb, size);
if (ptr == NULL)
-   err(2, "xcalloc: out of memory (allocating %zu*%zu bytes)",
+   errx(2, "xcalloc: out of memory (allocating %zu * %zu bytes)",
nmemb, size);
return ptr;
 }
@@ -52,7 +54,8 @@ xreallocarray(void *ptr, size_t nmemb, s
 
new_ptr = reallocarray(ptr, nmemb, size);
if (new_ptr == NULL)
-   err(2, "xrealloc %zu*%zu", nmemb, size);
+   errx(2, "xreallocarray: out of memory "
+   "(allocating %zu * %zu bytes)", nmemb, size);
return new_ptr;
 }
 
@@ -62,7 +65,7 @@ xstrdup(const char *str)
char *cp;
 
   

unify xmalloc (was Re: [patch] cvs: retire xfree())

2015-11-05 Thread Tobias Stoeckmann
On Thu, Nov 05, 2015 at 09:50:48AM +, Nicholas Marriott wrote:
> I don't know why cvs and rcs xmalloc.c has ended up so different.

It's not just about cvs and rcs:

/usr/src/usr.bin/cvs/xmalloc.c
/usr/src/usr.bin/diff/xmalloc.c
/usr/src/usr.bin/file/xmalloc.c
/usr/src/usr.bin/rcs/xmalloc.c
/usr/src/usr.bin/ssh/xmalloc.c
/usr/src/usr.bin/tmux/xmalloc.c (probably not same origin)

All of them share code parts that almost look identical. Some of them
skip tests, do additional tests, test for other return values, or have
typos in their error messages (or call err instead of errx, duplicating
their messages).

This diff would unify them, taking into account that still different
style guides apply (tmux) and some use fatal() or errx() with even
different return values (diff). Ugh...


Index: usr.bin/cvs/xmalloc.c
===
RCS file: /cvs/src/usr.bin/cvs/xmalloc.c,v
retrieving revision 1.12
diff -u -p -u -p -r1.12 xmalloc.c
--- usr.bin/cvs/xmalloc.c   5 Nov 2015 09:48:21 -   1.12
+++ usr.bin/cvs/xmalloc.c   5 Nov 2015 14:42:09 -
@@ -13,6 +13,7 @@
  * called by a name other than "ssh" or "Secure Shell".
  */
 
+#include 
 #include 
 #include 
 #include 
@@ -30,7 +31,7 @@ xmalloc(size_t size)
fatal("xmalloc: zero size");
ptr = malloc(size);
if (ptr == NULL)
-   fatal("xmalloc: out of memory (allocating %lu bytes)", (u_long) 
size);
+   fatal("xmalloc: out of memory (allocating %zu bytes)", size);
return ptr;
 }
 
@@ -41,12 +42,10 @@ xcalloc(size_t nmemb, size_t size)
 
if (size == 0 || nmemb == 0)
fatal("xcalloc: zero size");
-   if (SIZE_MAX / nmemb < size)
-   fatal("xcalloc: nmemb * size > SIZE_MAX");
ptr = calloc(nmemb, size);
if (ptr == NULL)
-   fatal("xcalloc: out of memory (allocating %lu bytes)",
-   (u_long)(size * nmemb));
+   fatal("xcalloc: out of memory (allocating %zu * %zu bytes)",
+   nmemb, size);
return ptr;
 }
 
@@ -54,28 +53,23 @@ void *
 xreallocarray(void *ptr, size_t nmemb, size_t size)
 {
void *new_ptr;
-   size_t new_size = nmemb * size;
 
-   if (new_size == 0)
-   fatal("xrealloc: zero size");
-   if (SIZE_MAX / nmemb < size)
-   fatal("xrealloc: nmemb * size > SIZE_MAX");
-   new_ptr = realloc(ptr, new_size);
+   if (nmemb == 0 || size == 0)
+   fatal("xreallocarray: zero size");
+   new_ptr = reallocarray(ptr, nmemb, size);
if (new_ptr == NULL)
-   fatal("xrealloc: out of memory (new_size %lu bytes)",
-   (u_long) new_size);
+   fatal("xreallocarray: out of memory "
+   "(allocating %zu * %zu bytes)", nmemb, size);
return new_ptr;
 }
 
 char *
 xstrdup(const char *str)
 {
-   size_t len;
char *cp;
 
-   len = strlen(str) + 1;
-   cp = xmalloc(len);
-   strlcpy(cp, str, len);
+   if ((cp = strdup(str)) == NULL)
+   fatal("xstrdup: could not allocate memory");
return cp;
 }
 
@@ -96,17 +90,20 @@ xasprintf(char **ret, const char *fmt, .
 }
 
 int
-xsnprintf(char *str, size_t size, const char *fmt, ...)
+xsnprintf(char *str, size_t len, const char *fmt, ...)
 {
va_list ap;
int i;
 
+   if (len > INT_MAX)
+   fatal("xsnprintf: len > INT_MAX");
+
va_start(ap, fmt);
-   i = vsnprintf(str, size, fmt, ap);
+   i = vsnprintf(str, len, fmt, ap);
va_end(ap);
 
-   if (i == -1 || i >= (int)size)
-   fatal("xsnprintf: overflow");
+   if (i < 0 || i >= (int)len)
+   fatal("xsnprintf: could not allocate memory");
 
return (i);
 }
Index: usr.bin/diff/xmalloc.c
===
RCS file: /cvs/src/usr.bin/diff/xmalloc.c,v
retrieving revision 1.8
diff -u -p -u -p -r1.8 xmalloc.c
--- usr.bin/diff/xmalloc.c  25 Sep 2015 16:16:26 -  1.8
+++ usr.bin/diff/xmalloc.c  5 Nov 2015 14:42:09 -
@@ -27,9 +27,11 @@ xmalloc(size_t size)
 {
void *ptr;
 
+   if (size == 0)
+   errx(2, "xmalloc: zero size");
ptr = malloc(size);
if (ptr == NULL)
-   err(2, "xmalloc %zu", size);
+   errx(2, "xmalloc: out of memory (allocating %zu bytes)", size);
return ptr;
 }
 
@@ -40,7 +42,7 @@ xcalloc(size_t nmemb, size_t size)
 
ptr = calloc(nmemb, size);
if (ptr == NULL)
-   err(2, "xcalloc: out of memory (allocating %zu*%zu bytes)",
+   errx(2, "xcalloc: out of memory (allocating %zu * %zu bytes)",
nmemb, size);
return ptr;
 }
@@ -52,7 +54,8 @@ xreallocarray(void *ptr, size_t nmemb, s
 
new_ptr = reallocarray(ptr, nmemb, size);
if (new_ptr == NULL)
-   err(2, 

Re: cvs(1) simplification

2015-11-02 Thread Tobias Stoeckmann
I wouldn't call this definition readable:

void
cvs_ent_line_str(const char *name, char *rev, char *tstamp, char *opts,
char *sticky, int isdir, int isremoved, char *buf, size_t len)

So what about changing it to return allocated memory by itself,
which would mean changing the internals from xsnprintf() to xasprintf().

Removes the need for two arguments. And the code does no error handling
currently, anyway.

char *
cvs_ent_line_str(const char *name, char *rev, char *tstamp, char *opts,
char *sticky, int isdir, int isremoved)

I can tell that rev can be VERY large... Wait, am I remembering these
(Open)CVS internals all of a sudden again? :P



Re: [PATCH] rcs: buf_free/rcsnum_free

2015-11-01 Thread Tobias Stoeckmann
On Sun, Nov 01, 2015 at 11:17:40AM +, Stuart Henderson wrote:
> On 2015/11/01 08:03, Nicholas Marriott wrote:
> > Some did for a while but it has some nasty bugs and nobody is working on
> > fixing it.
> 
> Some used it on amd64 for a while to avoid checkout failures due to
> running into memory limits, but then I tracked it down and increased
> the limit in CVSROOT/options to get around this.
> 
> Switching between cvs and opencvs upstreams without making a fresh
> checkout caused some problems so it's better if mirrors all just use
> cvs.

I think it should be moved into Attic. It's not like we've been nice to
the pcc tree-import either after it lacked attention.



Re: [PATCH] rcs: buf_free/rcsnum_free

2015-10-30 Thread Tobias Stoeckmann
On Fri, Oct 30, 2015 at 08:52:02AM +0800, Michael W. Bombardieri wrote:
> Sorry. Here is new diff. Hopefully I haven't missed anything else.

You missed OpenCVS, which shares the same code base.

But is OpenCVS worth it anymore?
Even a harsher question: Is it time to tedu it?



Re: sed: better error handling

2015-10-26 Thread Tobias Stoeckmann
> Comments / oks?

Looks much cleaner, okay for me.



sed: better error message

2015-10-25 Thread Tobias Stoeckmann
$ sed s/a/b/ /nofile
sed: 0: /nofile: /nofile

That message doesn't tell me a lot, let's better write
strerror(errno) if fopen returns NULL:

$ ./sed s/a/b/ /nofile
sed: 0: /nofile: No such file or directory

Index: main.c
===
RCS file: /cvs/src/usr.bin/sed/main.c,v
retrieving revision 1.27
diff -u -p -u -p -r1.27 main.c
--- main.c  10 Oct 2015 20:18:30 -  1.27
+++ main.c  25 Oct 2015 20:01:19 -
@@ -389,7 +389,7 @@ mf_fgets(SPACE *sp, enum e_spflag spflag
outfname = "stdout";
}
if ((infile = fopen(fname, "r")) == NULL) {
-   err(WARNING, "%s", fname);
+   err(WARNING, "%s", strerror(errno));
rval = 1;
continue;
}



Re: catopen/catgets: out of boundary access

2015-10-23 Thread Tobias Stoeckmann
On Fri, Oct 23, 2015 at 09:19:34PM +0200, Stefan Sperling wrote:
> Now that this is committed, do you intend to audit the runes code as
> well? :-)

Hah, yeah that's the next logical step to do. Except you are faster
than me, then I would probably okay it. ;)



nlist(3): out of boundary access

2015-10-15 Thread Tobias Stoeckmann
The library function nlist(3) does not properly validate parsed ELF
binary files, which can lead to out of boundary accesses.

Also, nlist will return -1 for stripped binary files, because eventually
it will try to mmap 0 bytes. Instead of returning the amount of symbols
we tried to look up, -1 suggests that the file was not there or invalid.
I moved that check a bit up so it behaves as expected.

What my diff does to prevent out of boundary accesses:

- Prevent mmap to map memory from outside file boundaries. A broken
  file could contain invalid values for ehdr.e_shoff and shdr_size.
  Please note that shdr_size calculation is safe ((2^16 - 1)^2 < 2^32).
- Verify that shdr[i].sh_link contains a valid index.
- While iterating through symbols, make sure that symsize actually
  references enough memory for an Elf_Sym. Imagine that only 1 byte is
  left in file. Current code would read that 1 byte into an Elf_Sym
  struct and processes its content. That's definitely undefined
  behaviour.
- Verify that Elf symbol references a valid string offset.
- When comparing symbol names, make sure that strcmp() won't access
  memory outside the allocated space. A symbol name must \0 terminate,
  and that byte must be in the ELF file.


Index: lib/libc/gen/nlist.c
===
RCS file: /cvs/src/lib/libc/gen/nlist.c,v
retrieving revision 1.59
diff -u -p -r1.59 nlist.c
--- lib/libc/gen/nlist.c6 Feb 2015 23:21:58 -   1.59
+++ lib/libc/gen/nlist.c15 Oct 2015 09:03:07 -
@@ -101,6 +101,7 @@ __fdnlist(int fd, struct nlist *list)
Elf_Word shdr_size;
struct stat st;
int usemalloc = 0;
+   size_t left, len;
 
/* Make sure obj is OK */
if (pread(fd, , sizeof(Elf_Ehdr), (off_t)0) != sizeof(Elf_Ehdr) ||
@@ -111,7 +112,8 @@ __fdnlist(int fd, struct nlist *list)
shdr_size = ehdr.e_shentsize * ehdr.e_shnum;
 
/* Make sure it's not too big to mmap */
-   if (shdr_size > SIZE_MAX) {
+   if (SIZE_MAX - ehdr.e_shoff < shdr_size ||
+   ehdr.e_shoff + shdr_size > st.st_size) {
errno = EFBIG;
return (-1);
}
@@ -138,6 +140,8 @@ __fdnlist(int fd, struct nlist *list)
 */
for (i = 0; i < ehdr.e_shnum; i++) {
if (shdr[i].sh_type == SHT_SYMTAB) {
+   if (shdr[i].sh_link >= ehdr.e_shnum)
+   continue;
symoff = shdr[i].sh_offset;
symsize = shdr[i].sh_size;
symstroff = shdr[shdr[i].sh_link].sh_offset;
@@ -152,12 +156,37 @@ __fdnlist(int fd, struct nlist *list)
else
munmap((caddr_t)shdr, shdr_size);
 
+   /*
+* clean out any left-over information for all valid entries.
+* Type and value defined to be 0 if not found; historical
+* versions cleared other and desc as well.  Also figure out
+* the largest string length so don't read any more of the
+* string table than we have to.
+*
+* XXX clearing anything other than n_type and n_value violates
+* the semantics given in the man page.
+*/
+   nent = 0;
+   for (p = list; !ISLAST(p); ++p) {
+   p->n_type = 0;
+   p->n_other = 0;
+   p->n_desc = 0;
+   p->n_value = 0;
+   ++nent;
+   }
+
+   /* Don't process any further if object is stripped. */
+   /* ELFism - dunno if stripped by looking at header */
+   if (symoff == 0)
+   return nent;
+
/* Check for files too large to mmap. */
-   /* XXX is this really possible? */
-   if (symstrsize > SIZE_MAX) {
+   if (SIZE_MAX - symstrsize < symstroff ||
+   symstrsize + symstroff > st.st_size) {
errno = EFBIG;
return (-1);
}
+
/*
 * Map string table into our address space.  This gives us
 * an easy way to randomly access all the strings, without
@@ -177,41 +206,20 @@ __fdnlist(int fd, struct nlist *list)
if (strtab == MAP_FAILED)
return (-1);
}
-   /*
-* clean out any left-over information for all valid entries.
-* Type and value defined to be 0 if not found; historical
-* versions cleared other and desc as well.  Also figure out
-* the largest string length so don't read any more of the
-* string table than we have to.
-*
-* XXX clearing anything other than n_type and n_value violates
-* the semantics given in the man page.
-*/
-   nent = 0;
-   for (p = list; !ISLAST(p); ++p) {
-   p->n_type = 0;
-   p->n_other = 0;
-   p->n_desc = 0;
-   p->n_value = 0;
-   ++nent;
-   }
 
-   /* Don't process any further if object is stripped. */
- 

Re: nlist(3): out of boundary access

2015-10-15 Thread Tobias Stoeckmann
On Thu, Oct 15, 2015 at 11:28:07AM -0600, Todd C. Miller wrote:
> Those checks all look good.  The only thing I had a question
> about is the:
> 
> len = strlen(sym);
> 
> Would it be better to use memchr to search for the NUL terminator
> to avoid going past the end?  E.g.
> 
> if (memchr(sym, 0, left) == NULL)
>   continue;

__fdnlist(int fd, struct nlist *list)
...
for (p = list; !ISLAST(p); p++) {
...
sym = p->n_un.n_name;

I consider sym to be trustworthy here, because it's supplied by the
caller. It's not a pointer into the binary file.



Re: patch: native ed support

2015-10-13 Thread Tobias Stoeckmann
On Mon, Oct 12, 2015 at 12:01:49PM -0600, Todd C. Miller wrote:
> I think this is the correct approach.  I've only taken a brief look
> so far but I think you should make write_lines() static in its
> declaration to match the prototype.

Thanks for pointing this out. Updated diff below:

- Make write_lines static
- Fixed typo in comment (finit_e_ state machine)
- Check for our new /.// substitution expression
- Be more restrictive about command parsing, so we do not
  allow commands like "10insert" instead of "10i".
- The variable p in get_command was not initialized when calling
  isidigit, my bad...


Tobias

Index: Makefile
===
RCS file: /cvs/src/usr.bin/patch/Makefile,v
retrieving revision 1.4
diff -u -p -r1.4 Makefile
--- Makefile16 May 2005 15:22:46 -  1.4
+++ Makefile13 Oct 2015 16:33:09 -
@@ -1,6 +1,6 @@
 #  $OpenBSD: Makefile,v 1.4 2005/05/16 15:22:46 espie Exp $
 
 PROG=  patch
-SRCS=  patch.c pch.c inp.c util.c backupfile.c mkpath.c
+SRCS=  patch.c pch.c inp.c util.c backupfile.c mkpath.c ed.c
 
 .include 
Index: ed.c
===
RCS file: ed.c
diff -N ed.c
--- /dev/null   1 Jan 1970 00:00:00 -
+++ ed.c13 Oct 2015 16:33:09 -
@@ -0,0 +1,340 @@
+/* $OpenBSD$ */
+
+/*
+ * Copyright (c) 2015 Tobias Stoeckmann <tob...@openbsd.org>
+ *
+ * Permission to use, copy, modify, and distribute this software for any
+ * purpose with or without fee is hereby granted, provided that the above
+ * copyright notice and this permission notice appear in all copies.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS" AND THE AUTHOR DISCLAIMS ALL WARRANTIES
+ * WITH REGARD TO THIS SOFTWARE INCLUDING ALL IMPLIED WARRANTIES OF
+ * MERCHANTABILITY AND FITNESS. IN NO EVENT SHALL THE AUTHOR BE LIABLE FOR
+ * ANY SPECIAL, DIRECT, INDIRECT, OR CONSEQUENTIAL DAMAGES OR ANY DAMAGES
+ * WHATSOEVER RESULTING FROM LOSS OF USE, DATA OR PROFITS, WHETHER IN AN
+ * ACTION OF CONTRACT, NEGLIGENCE OR OTHER TORTIOUS ACTION, ARISING OUT OF
+ * OR IN CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE.
+ */
+
+#include 
+#include 
+
+#include 
+#include 
+#include 
+#include 
+
+#include "common.h"
+#include "util.h"
+#include "pch.h"
+#include "inp.h"
+
+/* states of finite state machine */
+#define FSM_CMD1
+#define FSM_A  2
+#define FSM_C  3
+#define FSM_D  4
+#define FSM_I  5
+#define FSM_S  6
+
+#define SRC_INP1   /* line's origin is input file */
+#define SRC_PCH2   /* line's origin is patch file */
+
+static voidinit_lines(void);
+static voidfree_lines(void);
+static struct ed_line  *get_line(LINENUM);
+static struct ed_line  *create_line(off_t);
+static int valid_addr(LINENUM, LINENUM);
+static int get_command(void);
+static voidwrite_lines(char *);
+
+LIST_HEAD(ed_head, ed_line) head;
+struct ed_line {
+   LIST_ENTRY(ed_line) entries;
+   int src;
+   unsigned long   subst;
+   union {
+   LINENUM lineno;
+   off_t   seek;
+   } pos;
+};
+
+static LINENUM first_addr;
+static LINENUM second_addr;
+static LINENUM line_count;
+static struct ed_line  *cline; /* current line */
+
+void
+do_ed_script(void)
+{
+   off_t linepos;
+   struct ed_line *nline;
+   LINENUM i, range;
+   int fsm;
+
+   init_lines();
+   cline = NULL;
+   fsm = FSM_CMD;
+
+   for (;;) {
+   linepos = ftello(pfp);
+   if (pgets(buf, sizeof buf, pfp) == NULL)
+   break;
+   p_input_line++;
+
+   if (fsm == FSM_CMD) {
+   if ((fsm = get_command()) == -1)
+   break;
+
+   switch (fsm) {
+   case FSM_C:
+   case FSM_D:
+   /* delete lines in specified range */
+   if (second_addr == -1)
+   range = 1;
+   else
+   range = second_addr - first_addr + 1;
+   for (i = 0; i < range; i++) {
+   nline = LIST_NEXT(cline, entries);
+   LIST_REMOVE(cline, entries);
+   free(cline);
+   cline = nline;
+   line_count--;
+   }
+   fsm = (fsm == FSM_C) ? FSM_I : FSM_CMD;
+   break;
+   case FS

Re: patch: native ed support

2015-10-13 Thread Tobias Stoeckmann
On Tue, Oct 13, 2015 at 06:43:31PM +0200, Tobias Stoeckmann wrote:
> - Check for our new /.// substitution expression
> - Be more restrictive about command parsing, so we do not
>   allow commands like "10insert" instead of "10i".

And now, fix regression which I introduced with that diff, of course
it takes strncmp() now instead of strcmp() for s-subsitution. The
latest regress test, t17, covers this. This version passes just fine.

Thought about changing the fatal-calls to return -1 in get_command,
so we would be more relaxed towards garbage at the end of file that
looks like ed commands, e.g. "1,1a" (illegal, "a" takes only one
address, not a range).

The problem would be though that patch gets straight back into the
ed routine because it says "looks like an ed command", which basically
means an endless loop. Let's see if there is an ed-user who'll
complain about fatal() usage here.

Q: What does patch currently do with these illegal ed commands?
A: It passes it to ed. And ed continues after printing "?"...

Index: Makefile
===
RCS file: /cvs/src/usr.bin/patch/Makefile,v
retrieving revision 1.4
diff -u -p -r1.4 Makefile
--- Makefile16 May 2005 15:22:46 -  1.4
+++ Makefile13 Oct 2015 21:31:52 -
@@ -1,6 +1,6 @@
 #  $OpenBSD: Makefile,v 1.4 2005/05/16 15:22:46 espie Exp $
 
 PROG=  patch
-SRCS=  patch.c pch.c inp.c util.c backupfile.c mkpath.c
+SRCS=  patch.c pch.c inp.c util.c backupfile.c mkpath.c ed.c
 
 .include 
Index: ed.c
===
RCS file: ed.c
diff -N ed.c
--- /dev/null   1 Jan 1970 00:00:00 -
+++ ed.c13 Oct 2015 21:31:52 -
@@ -0,0 +1,342 @@
+/* $OpenBSD$ */
+
+/*
+ * Copyright (c) 2015 Tobias Stoeckmann <tob...@openbsd.org>
+ *
+ * Permission to use, copy, modify, and distribute this software for any
+ * purpose with or without fee is hereby granted, provided that the above
+ * copyright notice and this permission notice appear in all copies.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS" AND THE AUTHOR DISCLAIMS ALL WARRANTIES
+ * WITH REGARD TO THIS SOFTWARE INCLUDING ALL IMPLIED WARRANTIES OF
+ * MERCHANTABILITY AND FITNESS. IN NO EVENT SHALL THE AUTHOR BE LIABLE FOR
+ * ANY SPECIAL, DIRECT, INDIRECT, OR CONSEQUENTIAL DAMAGES OR ANY DAMAGES
+ * WHATSOEVER RESULTING FROM LOSS OF USE, DATA OR PROFITS, WHETHER IN AN
+ * ACTION OF CONTRACT, NEGLIGENCE OR OTHER TORTIOUS ACTION, ARISING OUT OF
+ * OR IN CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE.
+ */
+
+#include 
+#include 
+
+#include 
+#include 
+#include 
+#include 
+
+#include "common.h"
+#include "util.h"
+#include "pch.h"
+#include "inp.h"
+
+/* states of finite state machine */
+#define FSM_CMD1
+#define FSM_A  2
+#define FSM_C  3
+#define FSM_D  4
+#define FSM_I  5
+#define FSM_S  6
+
+#define SRC_INP1   /* line's origin is input file */
+#define SRC_PCH2   /* line's origin is patch file */
+
+#define S_PATTERN  "/.//"
+
+static voidinit_lines(void);
+static voidfree_lines(void);
+static struct ed_line  *get_line(LINENUM);
+static struct ed_line  *create_line(off_t);
+static int valid_addr(LINENUM, LINENUM);
+static int get_command(void);
+static voidwrite_lines(char *);
+
+LIST_HEAD(ed_head, ed_line) head;
+struct ed_line {
+   LIST_ENTRY(ed_line) entries;
+   int src;
+   unsigned long   subst;
+   union {
+   LINENUM lineno;
+   off_t   seek;
+   } pos;
+};
+
+static LINENUM first_addr;
+static LINENUM second_addr;
+static LINENUM line_count;
+static struct ed_line  *cline; /* current line */
+
+void
+do_ed_script(void)
+{
+   off_t linepos;
+   struct ed_line *nline;
+   LINENUM i, range;
+   int fsm;
+
+   init_lines();
+   cline = NULL;
+   fsm = FSM_CMD;
+
+   for (;;) {
+   linepos = ftello(pfp);
+   if (pgets(buf, sizeof buf, pfp) == NULL)
+   break;
+   p_input_line++;
+
+   if (fsm == FSM_CMD) {
+   if ((fsm = get_command()) == -1)
+   break;
+
+   switch (fsm) {
+   case FSM_C:
+   case FSM_D:
+   /* delete lines in specified range */
+   if (second_addr == -1)
+   range = 1;
+   else
+   range = second_addr - first_addr + 1;
+   for (i = 0; i < range; i

patch: native ed support

2015-10-11 Thread Tobias Stoeckmann
Currently, patch calls /bin/ed to support ed-formatted diffs, which
means that patch needs to pledge proc and exec. As discussed, it would
be a huge benefit if we can remove that.

Our patch implementation and GNU patch are already very restrictive in
which patch lines are sent to ed, so don't expect a full-blown ed here:

- Address ranges must be supplied in absolute line numbers. No search
  regular expressions, no relative positioning etc.
- Allowed commands are a, c, d, i, and s.
  - s only works for one address, not a range.
  - s only works with /\.\././ (want to change this to /.//)
- No support for binary files, because our patch lacks that in general

If you ever happened to work with ed-formatted diffs, no news so far.
What's new is this:

- Invalid address ranges lead to fatal instead of silently accepting
  them. This behaviour is in sync with unified diff handling now. If
  garbage at the end of a diff file starts with a number, it might be
  affected.
- No ed output on your console, because... there is no ed running. :P
- The ed code also uses plan A or plan B now. The code benefits from
  this because it renders the needed scratch file of /bin/ed obsolete.

I decided to store the ed code in its own file to reduce complexity
in pch.c. If ed-diffs ever become obsolete, simply remove ed.{c,h}.

So... anyone with ed-diffs out there? This diff wants some tests and
reviews.


Tobias

PS: No, I won't send this in ed-format.
PPS: Yes, I tried and it applies just fine after "touch ed.{c,h}". ;)

Index: Makefile
===
RCS file: /cvs/src/usr.bin/patch/Makefile,v
retrieving revision 1.4
diff -u -p -u -p -r1.4 Makefile
--- Makefile16 May 2005 15:22:46 -  1.4
+++ Makefile11 Oct 2015 09:43:59 -
@@ -1,6 +1,6 @@
 #  $OpenBSD: Makefile,v 1.4 2005/05/16 15:22:46 espie Exp $
 
 PROG=  patch
-SRCS=  patch.c pch.c inp.c util.c backupfile.c mkpath.c
+SRCS=  patch.c pch.c inp.c util.c backupfile.c mkpath.c ed.c
 
 .include 
Index: ed.c
===
RCS file: ed.c
diff -N ed.c
--- /dev/null   1 Jan 1970 00:00:00 -
+++ ed.c11 Oct 2015 09:43:59 -
@@ -0,0 +1,335 @@
+/* $OpenBSD$ */
+
+/*
+ * Copyright (c) 2015 Tobias Stoeckmann <tob...@openbsd.org>
+ *
+ * Permission to use, copy, modify, and distribute this software for any
+ * purpose with or without fee is hereby granted, provided that the above
+ * copyright notice and this permission notice appear in all copies.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS" AND THE AUTHOR DISCLAIMS ALL WARRANTIES
+ * WITH REGARD TO THIS SOFTWARE INCLUDING ALL IMPLIED WARRANTIES OF
+ * MERCHANTABILITY AND FITNESS. IN NO EVENT SHALL THE AUTHOR BE LIABLE FOR
+ * ANY SPECIAL, DIRECT, INDIRECT, OR CONSEQUENTIAL DAMAGES OR ANY DAMAGES
+ * WHATSOEVER RESULTING FROM LOSS OF USE, DATA OR PROFITS, WHETHER IN AN
+ * ACTION OF CONTRACT, NEGLIGENCE OR OTHER TORTIOUS ACTION, ARISING OUT OF
+ * OR IN CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE.
+ */
+
+#include 
+#include 
+
+#include 
+#include 
+#include 
+#include 
+
+#include "common.h"
+#include "util.h"
+#include "pch.h"
+#include "inp.h"
+
+/* states of finit state machine */
+#define FSM_CMD1
+#define FSM_A  2
+#define FSM_C  3
+#define FSM_D  4
+#define FSM_I  5
+#define FSM_S  6
+
+#define SRC_INP1   /* line's origin is input file */
+#define SRC_PCH2   /* line's origin is patch file */
+
+static voidinit_lines(void);
+static voidfree_lines(void);
+static struct ed_line  *get_line(LINENUM);
+static struct ed_line  *create_line(off_t);
+static int valid_addr(LINENUM, LINENUM);
+static int get_command(void);
+static voidwrite_lines(char *);
+
+LIST_HEAD(ed_head, ed_line) head;
+struct ed_line {
+   LIST_ENTRY(ed_line) entries;
+   int src;
+   unsigned long   subst;
+   union {
+   LINENUM lineno;
+   off_t   seek;
+   } pos;
+};
+
+static LINENUM first_addr;
+static LINENUM second_addr;
+static LINENUM line_count;
+static struct ed_line  *cline; /* current line */
+
+void
+do_ed_script(void)
+{
+   off_t linepos;
+   struct ed_line *nline;
+   LINENUM i, range;
+   int fsm;
+
+   init_lines();
+   cline = NULL;
+   fsm = FSM_CMD;
+
+   for (;;) {
+   linepos = ftello(pfp);
+   if (pgets(buf, sizeof buf, pfp) == NULL)
+   break;
+   p_input_line++;
+
+   if (fsm == FSM_CMD) {
+   if ((fsm = get_command()) == -1)
+   break;
+
+   switch (fsm) {
+   

queue.3: missing curly bracket

2015-10-10 Thread Tobias Stoeckmann
That's what I get for copy out of a manual...

The LIST_EMPTY example lacks an opening curly bracket. The other
examples have it, so it's a pretty obvious fix.


Index: queue.3
===
RCS file: /cvs/src/share/man/man3/queue.3,v
retrieving revision 1.60
diff -u -p -u -p -r1.60 queue.3
--- queue.3 13 Sep 2014 01:09:31 -  1.60
+++ queue.3 10 Oct 2015 12:53:54 -
@@ -593,7 +593,7 @@ LIST_INSERT_BEFORE(n1, n2, entries);
 LIST_FOREACH(np, , entries)
np-> ...
 
-while (!LIST_EMPTY()) /* Delete. */
+while (!LIST_EMPTY()) {   /* Delete. */
n1 = LIST_FIRST();
LIST_REMOVE(n1, entries);
free(n1);



diff: use s/.// ed-substitution

2015-10-10 Thread Tobias Stoeckmann
GNU patch only allows s/.// as a regular expression in substitutions.
Our diff implementation writes s/^\.\././ which is basically the same,
because they are used to change ".." lines into ".".

This is required if an ed-formatted diff tries to create a line that
only has a dot in it. Normally, that means end of line input for ed
commands. Inserting ".." and then changing it to "." is a common trick
here.

Compatibility with restrictive GNU patch is a good reason already, but
the stronger reason for this is my current work to support ed-style
diffs in patch directly.

If we limit support of substitutions to s/.//, then it is easily
transformed into code, e.g. p++. No need for any form of regexp
support, at all... and we are GNU patch compatible. ;)

Okay?


Index: diffreg.c
===
RCS file: /cvs/src/usr.bin/diff/diffreg.c,v
retrieving revision 1.88
diff -u -p -u -p -r1.88 diffreg.c
--- diffreg.c   5 Oct 2015 20:15:00 -   1.88
+++ diffreg.c   10 Oct 2015 23:17:58 -
@@ -1084,7 +1084,7 @@ proceed:
 * back and restart where we left off.
 */
diff_output(".\n");
-   diff_output("%ds/^\\.\\././\n", a);
+   diff_output("%ds/.//\n", a);
a += i;
c += i;
goto restart;



Re: catopen/catgets: out of boundary access

2015-10-06 Thread Tobias Stoeckmann
By the way, this is the second version with miod's feedback. Time to
send it to tech@ now, too.

Fixed one issue due to missing braces and less ntohl() calls, which
makes the code easier to read.

Index: catopen.c
===
RCS file: /cvs/src/lib/libc/nls/catopen.c,v
retrieving revision 1.17
diff -u -p -r1.17 catopen.c
--- catopen.c   5 Sep 2015 11:25:30 -   1.17
+++ catopen.c   14 Sep 2015 18:27:00 -
@@ -30,20 +30,24 @@
 
 #define _NLS_PRIVATE
 
-#include 
-#include 
-#include 
 #include 
 #include 
 #include 
-#include 
+#include 
 #include 
+#include 
 #include 
+#include 
+#include 
+#include 
+
+#define MAXIMUM(a, b)  (((a) > (b)) ? (a) : (b))
 
 #define NLS_DEFAULT_PATH 
"/usr/share/nls/%L/%N.cat:/usr/share/nls/%l.%c/%N.cat:/usr/share/nls/%l/%N.cat"
 #define NLS_DEFAULT_LANG "C"
 
-static nl_catd load_msgcat(const char *);
+static nl_catd load_msgcat(const char *);
+static int verify_msgcat(nl_catd);
 
 nl_catd
 catopen(const char *name, int oflag)
@@ -165,6 +169,8 @@ load_msgcat(const char *path)
void *data;
int fd;
 
+   catd = NULL;
+
if ((fd = open(path, O_RDONLY|O_CLOEXEC)) == -1)
return (nl_catd) -1;
 
@@ -173,24 +179,106 @@ load_msgcat(const char *path)
return (nl_catd) -1;
}
 
-   data = mmap(0, (size_t) st.st_size, PROT_READ, MAP_SHARED, fd, 
(off_t)0);
-   close (fd);
-
-   if (data == MAP_FAILED) {
+   if (st.st_size > INT_MAX || st.st_size < sizeof (struct _nls_cat_hdr)) {
+   errno = EINVAL;
+   close (fd);
return (nl_catd) -1;
}
 
-   if (ntohl(((struct _nls_cat_hdr *) data)->__magic) != _NLS_MAGIC) {
-   munmap(data, (size_t) st.st_size);
-   return (nl_catd) -1;
-   }
+   data = mmap(0, (size_t)st.st_size, PROT_READ, MAP_SHARED, fd, (off_t)0);
+   close (fd);
 
-   if ((catd = malloc(sizeof (*catd))) == 0) {
-   munmap(data, (size_t) st.st_size);
+   if (data == MAP_FAILED)
return (nl_catd) -1;
-   }
+
+   if (ntohl(((struct _nls_cat_hdr *) data)->__magic) != _NLS_MAGIC)
+   goto invalid;
+
+   if ((catd = malloc(sizeof (*catd))) == 0)
+   goto invalid;
 
catd->__data = data;
catd->__size = st.st_size;
+
+   if (verify_msgcat(catd))
+   goto invalid;
+
return catd;
+
+invalid:
+   free(catd);
+   munmap(data, (size_t) st.st_size);
+   errno = EINVAL;
+   return (nl_catd) -1;
 }
+
+static int
+verify_msgcat(nl_catd catd)
+{
+   struct _nls_cat_hdr *cat;
+   struct _nls_set_hdr *set;
+   struct _nls_msg_hdr *msg;
+   size_t remain;
+   int hdr_offset, i, index, j, msgs, nmsgs, nsets, off, txt_offset;
+
+   remain = catd->__size;
+   cat = (struct _nls_cat_hdr *) catd->__data;
+
+   hdr_offset = ntohl(cat->__msg_hdr_offset);
+   nsets = ntohl(cat->__nsets);
+   txt_offset = ntohl(cat->__msg_txt_offset);
+
+   /* catalog must contain at least one set and no negative offsets */
+   if (nsets < 1 || hdr_offset < 0 || txt_offset < 0)
+   return (1);
+
+   remain -= sizeof (*cat);
+
+   /* check if offsets or set size overflow */
+   if (remain <= hdr_offset || remain <= ntohl(cat->__msg_txt_offset) ||
+   remain / sizeof (*set) < nsets)
+   return (1);
+
+   set = (struct _nls_set_hdr *) ((char *) catd->__data + sizeof (*cat));
+
+   /* make sure that msg has space for at least one index */
+   if (remain - hdr_offset < sizeof(*msg))
+   return (1);
+
+   msg = (struct _nls_msg_hdr *) ((char *) catd->__data + sizeof (*cat)
+   + hdr_offset);
+
+   /* validate and retrieve largest string offset from sets */
+   off = 0;
+   for (i = 0; i < nsets; i++) {
+   index = ntohl(set[i].__index);
+   nmsgs = ntohl(set[i].__nmsgs);
+   /* set must contain at least one message */
+   if (index < 0 || nmsgs < 1)
+   return (1);
+
+   if (INT_MAX - nmsgs < index)
+   return (1);
+   msgs = index + nmsgs;
+
+   /* avoid msg index overflow */
+   if ((remain - hdr_offset) / sizeof(*msg) < msgs)
+   return (1);
+
+   /* retrieve largest string offset */
+   for (j = index; j < nmsgs; j++) {
+   if (ntohl(msg[j].__offset) < 0)
+   return (1);
+   off = MAXIMUM(off, ntohl(msg[j].__offset));
+   }
+   }
+
+   /* check if largest string offset is nul-terminated */
+   if (remain - txt_offset < off ||
+   memchr((char *) catd->__data + sizeof(*cat) + txt_offset + off,
+   '\0', remain - txt_offset - off) == NULL)
+

Re: Drop a distracting function from locate(1)

2015-09-19 Thread Tobias Stoeckmann
On Sat, Sep 19, 2015 at 05:57:23PM -0400, Michael McConville wrote:
> If you're abstracting something into a function, it definitely shouldn't
> be creating more code.

Yet this shouldn't stop you from performing "divide and conquer". It's
not just about reducing lines of code when abstracting logical blocks,
i.e. functions.

If you want an example of what I mean, please look at the main function
of src/sbin/newfs_msdos/newfs_msdos.c.

> When you create functions for things like if (z) errx(...); conditions,
> it makes the code much harder to read.

Is that statement true for xmalloc(), too? ;)

On Sat, Sep 19, 2015 at 02:29:56PM -0400, Michael McConville wrote:
> - sane_count(count);
> + if (count < 0 || count >= PATH_MAX)
> + errx(1, "corrupted database");
>   for (p = path + count; (c = getc(fp)) > SWITCH; size++)

> - sane_count(count);
> + if (count < 0 || count >= PATH_MAX)
> + errx(1, "corrupted database");
>   /* overlay old path */
>   p = path + count;

I would recommend to replace PATH_MAX with sizeof (path) here, so it
looks less magical and adapts whenever someone changes path's size. I've
kept the very next line after your patches in my quote, which shows that
count is used as an index into path, therefore the sizeof (path) range
check.


Tobias



ping6: out of boundary access with invalid packets

2015-09-08 Thread Tobias Stoeckmann
The function pr_pack does not properly check boundaries before
accessing packet data. This could happen on short network reads or
when we receive packets that are addressed for another running ping6
instance (see pr_pack comment for more information).

Index: ping6.c
===
RCS file: /cvs/src/sbin/ping6/ping6.c,v
retrieving revision 1.112
diff -u -p -r1.112 ping6.c
--- ping6.c 1 Sep 2015 19:53:23 -   1.112
+++ ping6.c 8 Sep 2015 19:29:29 -
@@ -1222,6 +1222,11 @@ pr_pack(u_char *buf, int cc, struct msgh
SIPHASH_CTX ctx;
u_int8_t mac[SIPHASH_DIGEST_LENGTH];
 
+   if (cc - sizeof(*cp) < sizeof(payload)) {
+   (void)printf("signature missing!\n");
+   return;
+   }
+
memcpy(, icp + 1, sizeof(payload));
tv64 = 
 
@@ -1306,7 +1311,8 @@ pr_pack(u_char *buf, int cc, struct msgh
}
}
}
-   } else if (icp->icmp6_type == ICMP6_NI_REPLY && mynireply(ni)) {
+   } else if (icp->icmp6_type == ICMP6_NI_REPLY &&
+   cc >= sizeof(*ni) && mynireply(ni)) {
seq = ntohs(*(u_int16_t *)ni->icmp6_ni_nonce);
++nreceived;
if (TST(seq % mx_dup_ck)) {
@@ -1351,7 +1357,7 @@ pr_pack(u_char *buf, int cc, struct msgh
case NI_QTYPE_FQDN:
default:/* XXX: for backward compatibility */
cp = (u_char *)ni + ICMP6_NIRLEN;
-   if (buf[off + ICMP6_NIRLEN] ==
+   if (off + ICMP6_NIRLEN < cc && buf[off + ICMP6_NIRLEN] 
==
cc - off - ICMP6_NIRLEN - 1)
oldfqdn = 1;
else
@@ -1438,7 +1444,8 @@ pr_pack(u_char *buf, int cc, struct msgh
}
}
 
-   if (buf[off + ICMP6_NIRLEN] !=
+   if (off + ICMP6_NIRLEN < cc &&
+   buf[off + ICMP6_NIRLEN] !=
cc - off - ICMP6_NIRLEN - 1 && oldfqdn) {
if (comma)
printf(",");



wsfontload: avoid floating point exception

2015-09-06 Thread Tobias Stoeckmann
Don't allow negative numbers for font width, because it could lead to a
floating point exception (and wouldn't make sense anyway).

# wsfontload -w -7 /dev/null
Floating point exception (core dumped)
# _

While at it, use strtonum for proper error messages; also check font height.

Current error message includes previous value of font width/height
and therefore doesn't help spotting the error at all.

Index: wsfontload.c
===
RCS file: /cvs/src/usr.sbin/wsfontload/wsfontload.c,v
retrieving revision 1.14
diff -u -p -u -p -r1.14 wsfontload.c
--- wsfontload.c9 Feb 2015 23:00:15 -   1.14
+++ wsfontload.c6 Sep 2015 19:21:35 -
@@ -33,17 +33,18 @@
  *
  */
 
-#include 
-#include 
 #include 
 #include 
+#include 
+#include 
 
+#include 
+#include 
+#include 
 #include 
 #include 
-#include 
-#include 
 #include 
-#include 
+#include 
 
 #include 
 
@@ -90,6 +91,7 @@ main(int argc, char *argv[])
struct stat stat;
size_t len;
void *buf;
+   const char *errstr;
 
wsdev = DEFDEV;
memset(, 0, sizeof f);
@@ -103,14 +105,14 @@ main(int argc, char *argv[])
wsdev = optarg;
break;
case 'w':
-   if (sscanf(optarg, "%d", ) != 1)
-   errx(1, "invalid font width of %d",
-   f.fontwidth);
+   f.fontwidth = strtonum(optarg, 1, UINT_MAX, );
+   if (errstr)
+   errx(1, "font width is %s: %s", errstr, optarg);
break;
case 'h':
-   if (sscanf(optarg, "%d", ) != 1)
-   errx(1, "invalid font height of %d",
-   f.fontheight);
+   f.fontheight = strtonum(optarg, 1, UINT_MAX, );
+   if (errstr)
+   errx(1, "font height is %s: %s", errstr, 
optarg);
break;
case 'e':
f.encoding = getencoding(optarg);



ldconfig: missing strdup checks

2015-09-05 Thread Tobias Stoeckmann
Okay to add missing strdup checks to ldconfig?

Index: libexec/ld.so/ldconfig/library.c
===
RCS file: /cvs/src/libexec/ld.so/ldconfig/library.c,v
retrieving revision 1.9
diff -u -p -r1.9 library.c
--- libexec/ld.so/ldconfig/library.c10 Jun 2015 20:50:05 -  1.9
+++ libexec/ld.so/ldconfig/library.c5 Sep 2015 12:21:53 -
@@ -60,6 +60,8 @@ load_lib(const char *name, struct elf_ob
char *lpath, *lname;
 
lpath = strdup(name);
+   if (lpath == NULL)
+   return (1);
lname = strrchr(lpath, '/');
if (lname == NULL || lname[1] == '\0') {
free(lpath);
Index: libexec/ld.so/ldconfig/prebind.c
===
RCS file: /cvs/src/libexec/ld.so/ldconfig/prebind.c,v
retrieving revision 1.29
diff -u -p -r1.29 prebind.c
--- libexec/ld.so/ldconfig/prebind.c3 Jun 2015 02:24:36 -   1.29
+++ libexec/ld.so/ldconfig/prebind.c5 Sep 2015 12:21:53 -
@@ -518,6 +518,10 @@ elf_load_object(void *pexe, const char *
 
object->load_base = lbase;
object->load_name = strdup(name);
+   if (object->load_name == NULL) {
+   printf("unable to allocate object for %s\n", name);
+   exit(10);
+   }
 
phdr = (Elf_Phdr *)((char *)pexe + ehdr->e_phoff);
for (i = 0; i < ehdr->e_phnum; i++) {
@@ -529,6 +533,11 @@ elf_load_object(void *pexe, const char *
/* XXX can only occur in programs */
curbin->interp = strdup((char *)((char *)pexe +
phdr[i].p_offset));
+   if (curbin->interp == NULL) {
+   printf("unable to allocate object for %s\n",
+   name);
+   exit(10);
+   }
break;
default:
break;



catopen/catgets: out of boundary access

2015-09-03 Thread Tobias Stoeckmann
Hi,

our catopen implementation does not check the parsed message catalog,
making it vulnerable to all sorts of out of boundary accesses.

Take this minimalistic proof of concept file:

$ printf '\xff\x88\xff\x89\x01\x00\x00\x00' > poc.cat

If you are too lazy to write code to open it yourself, take this one:

---poc.c---
#include 
#include 
#include 

int
main(int argc, char *argv[])
{
nl_catd cat;

if (argc != 2) {
fprintf(stderr, "usage: poc file.cat\n");
return (1);
}

if ((cat = catopen(argv[1], 0)) == (nl_catd) -1)
err(2, "catopen");
printf("%s\n", catgets(cat, 1, 1, "default text"));
catclose(cat);
return (0);
}
---poc.c---

$ ./poc $PWD/poc.cat # yes, it takes an absolute path
Segmentation fault (core dumped)
$ _

I've added all sorts of checks, making sure that whatever offset and
index is inside the catalog is actually valid. Unfortunately it looks
rather messy, because I even have to check if there are negative
values in it -- it's all int32_t.

There are also cases in which catopen() returns -1 but does not set
errno properly. I took the glibc approach and set errno to EINVAL
whenever we encounter an invalid value.

Also, make sure that we directly ignore files which are too small or
too large.

Successfully passes the libc.cat files we have in base, so I'm rather
confident that there are no false positives.

Any advices to make this look nicer though? Or how to handle INT_MAX
and int32_t types? They are basically the same, can I trust that it's
true on all our archs?


Tobias

Index: lib/libc/nls/catopen.c
===
RCS file: /cvs/src/lib/libc/nls/catopen.c,v
retrieving revision 1.16
diff -u -p -u -p -r1.16 catopen.c
--- catopen.c   16 Jan 2015 16:48:51 -  1.16
+++ catopen.c   3 Sep 2015 20:48:07 -
@@ -30,20 +30,24 @@
 
 #define _NLS_PRIVATE
 
-#include 
-#include 
-#include 
 #include 
 #include 
 #include 
-#include 
+#include 
 #include 
+#include 
 #include 
+#include 
+#include 
+#include 
+
+#define MAXIMUM(a, b)  (((a) > (b)) ? (a) : (b))
 
 #define NLS_DEFAULT_PATH 
"/usr/share/nls/%L/%N.cat:/usr/share/nls/%l.%c/%N.cat:/usr/share/nls/%l/%N.cat"
 #define NLS_DEFAULT_LANG "C"
 
-static nl_catd load_msgcat(const char *);
+static nl_catd load_msgcat(const char *);
+static int verify_msgcat(nl_catd);
 
 /* ARGSUSED */
 nl_catd
@@ -173,24 +177,106 @@ load_msgcat(const char *path)
return (nl_catd) -1;
}
 
+   if (st.st_size > INT_MAX || st.st_size < sizeof (struct _nls_cat_hdr)) {
+   errno = EINVAL;
+   close (fd);
+   return (nl_catd) -1;
+   }
+
data = mmap(0, (size_t) st.st_size, PROT_READ, MAP_SHARED, fd, 
(off_t)0);
close (fd);
 
-   if (data == MAP_FAILED) {
+   if (data == MAP_FAILED)
return (nl_catd) -1;
-   }
 
-   if (ntohl(((struct _nls_cat_hdr *) data)->__magic) != _NLS_MAGIC) {
-   munmap(data, (size_t) st.st_size);
-   return (nl_catd) -1;
-   }
+   if (ntohl(((struct _nls_cat_hdr *) data)->__magic) != _NLS_MAGIC)
+   goto invalid;
 
-   if ((catd = malloc(sizeof (*catd))) == 0) {
-   munmap(data, (size_t) st.st_size);
-   return (nl_catd) -1;
-   }
+   if ((catd = malloc(sizeof (*catd))) == 0)
+   goto invalid;
 
catd->__data = data;
catd->__size = st.st_size;
+
+   if (verify_msgcat(catd)) {
+   free(catd);
+   goto invalid;
+   }
+
return catd;
+
+invalid:
+   munmap(data, (size_t) st.st_size);
+   errno = EINVAL;
+   return (nl_catd) -1;
 }
+
+static int
+verify_msgcat(nl_catd catd)
+{
+   struct _nls_cat_hdr *cat;
+   struct _nls_set_hdr *set;
+   struct _nls_msg_hdr *msg;
+   size_t remain;
+   int i, j, msgs, off;
+
+   remain = catd->__size;
+   cat = (struct _nls_cat_hdr *) catd->__data;
+
+   /* catalog must contain at least one set and no negative offsets */
+   if (ntohl(cat->__nsets) < 1 ||
+   ntohl(cat->__msg_hdr_offset) < 0 ||
+   ntohl(cat->__msg_txt_offset) < 0)
+   return (1);
+
+   remain -= sizeof (*cat);
+
+   /* check if offsets or set size overflow */
+   if (remain <= ntohl(cat->__msg_hdr_offset) ||
+   remain <= ntohl(cat->__msg_txt_offset) ||
+   remain / sizeof (*set) < ntohl(cat->__nsets))
+   return (1);
+
+   set = (struct _nls_set_hdr *) ((char *) catd->__data + sizeof (*cat));
+
+   /* make sure that msg has space for at least one index */
+   if (remain - ntohl(cat->__msg_hdr_offset) < sizeof(*msg))
+   return (1);
+
+   msg = (struct _nls_msg_hdr *) ((char *) catd->__data + sizeof (*cat)
+   + ntohl(cat->__msg_hdr_offset));
+
+   /* validate and retrieve 

Re: [patch] return instead of exit(3) in src/bin/

2015-08-30 Thread Tobias Stoeckmann
On Sun, Aug 30, 2015 at 12:18:12PM -0600, Theo de Raadt wrote:
 In a more complex program with a large main() function, a call to
 exit() is an explicit statement about termination, so that even if
 someone refactors code to a subfunction, they must consider that
 it carefully.
 
 The return is not as explicit.

In general these are error conditions. We can accept exit() there
just like err() etc.

12 out of these bin/ programs in this patch call exit() right at
the end of main, as the very last line. I think it's still worth
it to cover these obvious cases, i.e. successful termination.

Makes them rather easy to review (and test), too.


Tobias



Re: [patch] cat's main never return

2015-08-30 Thread Tobias Stoeckmann
On Sat, Aug 29, 2015 at 05:02:33PM -0600, Theo de Raadt wrote:
 It really does not matter.  Coder's choice.  The result is the same.
 You could hunt them all down, change them all, save a few code bytes,
 but don't you dare introduce any bugs...

The main function is called by crt0 like
exit(main(argc, argv, envp));

Which means that return 0; and exit(0); in main lead to the same
result.

But there is a subtle difference.

If main calls exit, its own stack protector will never be validated,
which means that a previous overflow of main's stack is not spotted.
return on the other hand would trigger the machine code to check.

I would prefer a proper return. Who knows if we spot an issue?  But on
the other hand, take Theo's statement into consideration:

 but don't you dare introduce any bugs...


Tobias



getusershell: possible overflow

2015-08-11 Thread Tobias Stoeckmann
Hi,

I know this will be the third commit to fix the overflow situation in
getusershell if /etc/shells is malformed. Hopefully it will be the last
adjustment.

I submitted a bug report to glibc devs after noticing that they use the
same implementation like we do (more or less). Paul Pluzhnikov reviewed
my diff and noticed that it can still overflow:

From https://sourceware.org/bugzilla/show_bug.cgi?id=18660
 It seems to me that even adding 2 * sizeof (char *) is insufficient:
 if /etc/shells contains 10 two-byte lines /\n, then shells will
 point to an array of 20/3 + 2 == 8 pointers, and we'll try to write
 11 entries into it.

So, let's reserve more memory. Or actually implement this code to only
reserve as much as needed...


Tobias

Index: getusershell.c
===
RCS file: /cvs/src/lib/libc/gen/getusershell.c,v
retrieving revision 1.15
diff -u -p -r1.15 getusershell.c
--- getusershell.c  6 Feb 2015 23:21:58 -   1.15
+++ getusershell.c  11 Aug 2015 17:33:58 -
@@ -110,7 +110,7 @@ initshells(void)
(void)fclose(fp);
return (okshells);
}
-   shells = calloc((size_t)(statb.st_size / 3 + 2), sizeof (char *));
+   shells = calloc((size_t)(statb.st_size / 2 + 2), sizeof (char *));
if (shells == NULL) {
(void)fclose(fp);
free(strings);



sort: another mkstemp use case

2015-04-01 Thread Tobias Stoeckmann
When creating a new temporary file name, use mkstemp instead of just
taking a rather predictable path, which could even be a symlink by
a malicious user (granted, that is very unlikely).

Index: file.c
===
RCS file: /cvs/src/usr.bin/sort/file.c,v
retrieving revision 1.6
diff -u -p -r1.6 file.c
--- file.c  1 Apr 2015 19:06:18 -   1.6
+++ file.c  1 Apr 2015 19:48:25 -
@@ -167,12 +167,13 @@ file_is_tmp(const char *fn)
 char *
 new_tmp_file_name(void)
 {
-   static size_t tfcounter = 0;
-   static const char *fn = .bsdsort.;
char *ret;
+   int fd;
 
-   sort_asprintf(ret, %s/%s%d.%lu, tmpdir, fn, (int)getpid(),
-   (unsigned long)(tfcounter++));
+   sort_asprintf(ret, %s/.bsdsort.XX, tmpdir);
+   if ((fd = mkstemp(ret)) == -1)
+   err(2, %s, ret);
+   close(fd);
tmp_file_atexit(ret);
return ret;
 }



sort: useless use of cat

2015-03-31 Thread Tobias Stoeckmann
I see a useless use of cat in here.

Manual page:
When called with the -d option, it must decompress standard input
to standard output.

Index: file.c
===
RCS file: /cvs/src/usr.bin/sort/file.c,v
retrieving revision 1.4
diff -u -p -u -p -r1.4 file.c
--- file.c  30 Mar 2015 19:59:07 -  1.4
+++ file.c  31 Mar 2015 18:35:01 -
@@ -526,8 +526,8 @@ openfile(const char *fn, const char *mod
fflush(stdout);
 
if (mode[0] == 'r')
-   len = sort_asprintf(cmd, cat %s | %s -d,
-   fn, compress_program);
+   len = sort_asprintf(cmd, %s -d  %s,
+   compress_program, fn);
else if (mode[0] == 'w')
len = sort_asprintf(cmd, %s  %s,
compress_program, fn);



Re: sort output file permissions

2015-03-31 Thread Tobias Stoeckmann
Setting permissions without setting owner and group can have rather
inconvenient results. See this example with latest code:

$ touch test
$ chgrp wheel test
$ chmod g+w test
$ ls -l test
-rw-rw-r--  1 tobias  wheel  0 Mar 31 20:05 test
$ ./sort -o test test
$ ls -l test
-rw-rw-r--  1 tobias  tobias  0 Mar 31 20:05 test

It changed group from wheel to tobias, giving that group write access.


Tobias



patch: safer temp file handling

2015-03-29 Thread Tobias Stoeckmann
);
+   ofp = open_temp_file(TMP_OUT, w, TMPOUTNAME);
 }
 
 /*
  * Open a file to put hunks we can't locate.
  */
 static void
-init_reject(const char *name)
+init_reject(void)
 {
-   rejfp = fopen(name, w);
-   if (rejfp == NULL)
-   pfatal(can't create %s, name);
+   rejfp = open_temp_file(TMP_REJ, w, TMPREJNAME);
 }
 
 /*
Index: pch.c
===
RCS file: /cvs/src/usr.bin/patch/pch.c,v
retrieving revision 1.51
diff -u -p -r1.51 pch.c
--- pch.c   5 Feb 2015 12:59:57 -   1.51
+++ pch.c   29 Mar 2015 12:18:21 -
@@ -42,6 +42,7 @@
 #include util.h
 #include pch.h
 #include pathnames.h
+#include temp.h
 
 /* Patch (diff listing) abstract type. */
 
@@ -103,17 +104,15 @@ open_patch_file(const char *filename)
struct stat filestat;
 
if (filename == NULL || *filename == '\0' || strEQ(filename, -)) {
-   pfp = fopen(TMPPATNAME, w);
-   if (pfp == NULL)
-   pfatal(can't create %s, TMPPATNAME);
+   pfp = open_temp_file(TMP_PAT, r+, (char **)filename);
while (fgets(buf, sizeof buf, stdin) != NULL)
fputs(buf, pfp);
-   fclose(pfp);
-   filename = TMPPATNAME;
+   fseeko(pfp, 0, SEEK_SET);
+   } else {
+   pfp = fopen(filename, r);
+   if (pfp == NULL)
+   pfatal(patch file %s not found, filename);
}
-   pfp = fopen(filename, r);
-   if (pfp == NULL)
-   pfatal(patch file %s not found, filename);
if (fstat(fileno(pfp), filestat))
pfatal(can't stat %s, filename);
p_filesize = filestat.st_size;
@@ -1378,6 +1377,8 @@ do_ed_script(void)
char*t;
off_t   beginning_of_this_line;
FILE*pipefp = NULL;
+
+   fclose(open_temp_file(TMP_OUT, w, TMPOUTNAME));
 
if (!skip_rest_of_patch) {
if (copy_file(filearg[0], TMPOUTNAME)  0) {
Index: temp.c
===
RCS file: temp.c
diff -N temp.c
--- /dev/null   1 Jan 1970 00:00:00 -
+++ temp.c  29 Mar 2015 12:18:21 -
@@ -0,0 +1,112 @@
+/* $OpenBSD$   */
+/*
+ * Copyright (c) 2015 Tobias Stoeckmann tob...@openbsd.org
+ *
+ * Permission to use, copy, modify, and distribute this software for any
+ * purpose with or without fee is hereby granted, provided that the above
+ * copyright notice and this permission notice appear in all copies.
+ *
+ * THE SOFTWARE IS PROVIDED AS IS AND THE AUTHOR DISCLAIMS ALL WARRANTIES
+ * WITH REGARD TO THIS SOFTWARE INCLUDING ALL IMPLIED WARRANTIES OF
+ * MERCHANTABILITY AND FITNESS. IN NO EVENT SHALL THE AUTHOR BE LIABLE FOR
+ * ANY SPECIAL, DIRECT, INDIRECT, OR CONSEQUENTIAL DAMAGES OR ANY DAMAGES
+ * WHATSOEVER RESULTING FROM LOSS OF USE, DATA OR PROFITS, WHETHER IN AN
+ * ACTION OF CONTRACT, NEGLIGENCE OR OTHER TORTIOUS ACTION, ARISING OUT OF
+ * OR IN CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE.
+ */
+
+#include paths.h
+#include stdio.h
+#include stdlib.h
+#include string.h
+#include unistd.h
+
+#include common.h
+#include util.h
+#include temp.h
+
+static char *pattern[] = {
+   patchoXX,
+   patchiXX,
+   patchrXX,
+   patchpXX
+};
+
+static char*path[4];
+static char*tmpdir;
+
+static void temp_init(void);
+
+static void
+temp_init(void)
+{
+   int i;
+
+   /* Cons up the names of the temporary files. */
+   if ((tmpdir = getenv(TMPDIR)) == NULL || *tmpdir == '\0')
+   tmpdir = _PATH_TMP;
+   for (i = strlen(tmpdir) - 1; i  0  tmpdir[i] == '/'; i--)
+   ;
+   i++;
+
+   if (asprintf(tmpdir, %.*s, i, tmpdir) == -1)
+   fatal(can't allocate memory);
+}
+
+FILE *
+open_temp_file(unsigned int type, const char *mode, char **name)
+{
+   FILE *fp;
+   int fd;
+
+   if (type  3)
+   fatal(invalid temp type %u, type);
+
+   if (tmpdir == NULL)
+   temp_init();
+
+   close_temp_file(type);
+
+   if (asprintf(path[type], %s/%s, tmpdir, pattern[type]) == -1)
+   fatal(can't allocate memory);
+   if ((fd = mkstemp(path[type]))  0) {
+   free(path[type]);
+   path[type] = NULL;
+   pfatal(can't create %s/%s, tmpdir, pattern[type]);
+   }
+
+   if ((fp = fdopen(fd, mode)) == NULL) {
+   close(fd);
+   pfatal(can't fdopen %s, path[type]);
+   }
+   if (name != NULL)
+   *name = path[type];
+   return fp;
+}
+
+void
+close_temp_file(unsigned int type)
+{
+   if (type  3)
+   fatal(invalid temp type %u, type);
+
+   if (path[type] != NULL) {
+   switch (type) {
+   case TMP_OUT:
+   if (!toutkeep)
+   unlink(path[type

Re: tail: -r mem leak with non-regular files

2015-03-27 Thread Tobias Stoeckmann
On Thu, Mar 26, 2015 at 11:41:23PM +0100, Tobias Stoeckmann wrote:
 The less obvious one is in an error path.

As tl-l is always of fixed size (BSZ), we can just change the struct
to have a BSZ sized array in it. This removes the need to do checks
in the error path completely.

While at it, there is no use to have a typedef in the code, so I just
removed it, too.

Index: reverse.c
===
RCS file: /cvs/src/usr.bin/tail/reverse.c,v
retrieving revision 1.19
diff -u -p -r1.19 reverse.c
--- reverse.c   27 Oct 2009 23:59:44 -  1.19
+++ reverse.c   27 Mar 2015 11:19:14 -
@@ -147,12 +147,13 @@ r_reg(FILE *fp, enum STYLE style, off_t 
return (0);
 }
 
-typedef struct bf {
+#defineBSZ (128 * 1024)
+struct bf {
struct bf *next;
struct bf *prev;
size_t len;
-   char *l;
-} BF;
+   char l[BSZ];
+};
 
 /*
  * r_buf -- display a non-regular file in reverse order by line.
@@ -167,21 +168,19 @@ typedef struct bf {
 static void
 r_buf(FILE *fp)
 {
-   BF *mark, *tr, *tl = NULL;
+   struct bf *mark, *tr, *tl = NULL;
int ch;
size_t len, llen;
char *p;
off_t enomem;
 
-#defineBSZ (128 * 1024)
for (mark = NULL, enomem = 0;;) {
/*
 * Allocate a new block and link it into place in a doubly
 * linked list.  If out of memory, toss the LRU block and
 * keep going.
 */
-   if (enomem || (tl = malloc(sizeof(BF))) == NULL ||
-   (tl-l = malloc(BSZ)) == NULL) {
+   if (enomem || (tl = malloc(sizeof(*tl))) == NULL) {
if (!mark)
err(1, NULL);
tl = enomem ? tl-next : mark;
@@ -259,5 +258,12 @@ r_buf(FILE *fp)
while ((tl = tl-next)-len) {
WR(tl-l, tl-len);
tl-len = 0;
+   }
+
+   tl-prev-next = NULL;
+   while (tl != NULL) {
+   tr = tl-next;
+   free(tl);
+   tl = tr;
}
 }



tail: -r mem leak with non-regular files

2015-03-26 Thread Tobias Stoeckmann
Hi,

tail -r has two memory leaks when handling non-regular files. You can
easily see memory usage increasing with commands like

$ mknod pipe p
$ tail -r pipe pipe pipe  /dev/null

And then writing a large file into the pipe three times. top shows the
memory usage of tail increasing with each consecutive read.

The obvious memory leak is at the end of reverse(): the doubly linked
list (actually circular list) is never freed. To fix this, I break the
circle and iterate through it, freeing items until I hit NULL.

The less obvious one is in an error path. If tail fails to allocate an
item (tl) or space for its content (tl-l), it won't simply err() out
but tries to recover. Yet, it doesn't free tl if allocation of tl-l
failed.


Tobias

Index: reverse.c
===
RCS file: /cvs/src/usr.bin/tail/reverse.c,v
retrieving revision 1.19
diff -u -p -U4 -r1.19 reverse.c
--- reverse.c   27 Oct 2009 23:59:44 -  1.19
+++ reverse.c   26 Mar 2015 22:31:38 -
@@ -183,8 +183,10 @@ r_buf(FILE *fp)
if (enomem || (tl = malloc(sizeof(BF))) == NULL ||
(tl-l = malloc(BSZ)) == NULL) {
if (!mark)
err(1, NULL);
+   if (!enomem)
+   free(tl);
tl = enomem ? tl-next : mark;
enomem += tl-len;
} else if (mark) {
tl-next = mark;
@@ -258,6 +260,14 @@ r_buf(FILE *fp)
}
while ((tl = tl-next)-len) {
WR(tl-l, tl-len);
tl-len = 0;
+   }
+
+   tl-prev-next = NULL;
+   while (tl != NULL) {
+   tr = tl-next;
+   free(tl-l);
+   free(tl);
+   tl = tr;
}
 }



libssl: signal races in capability checks

2015-03-14 Thread Tobias Stoeckmann
Hi,

grep'ed the tree for siglongjmp calls, and spotted possible offenders in
libssl's code. The code in question checks hardware capabilities for
ARM, S390x, and SPARCv9.

The code will call some routines that could trigger SIGILL (or SIGBUS),
which is caught with an own signal handler. This signal handler will
perform the previously mentioned siglongjmp and jumps out of the
capability testing.

Unfortunately, the signal handlers are registered and activated before
the jump environment is properly initialized. If the program receives
SIGILL (or SIGBUS in case of SPARCv9) from another source, the behavior
of the program will be undefined.

Fix for ARM and S390x is simple: Register the signal handlers after the
environment has been properly set up. Which means right after sigsetjmp.

The SPARCv9 code is more complicated: It registers a signal handler for
SIGILL and SIGBUS. These signal handlers don't block each other, which
means that SIGILL performing a siglongjmp could be interrupted by SIGBUS
which will perform a siglongjmp, too. I haven't seen these jump methods
as signal-safe in signal(3), so I assume that they are not. The first
part of the fix is to block SIGILL and SIGBUS while performing the
signal handler.

Also, SPARCv9 code overrides the environment between two independent
tests, still allowing these signals. It means that the setting of the
jump environment can be interrupted with a jump... That sounds like
asking for trouble. Therefore, I block these two signals during call of
sigsetjmp.

Last remaining bits are KNF and removing unused code.

It's difficult to write proper signal handlers, and even worse, I lack
machines of these types. gcc -c compiles the S390x and SPARCv9 files
for me, so there shouldn't be syntax errors. Yet I really need peer
reviews. If somebody dares to look at signals: Now is the time! :)


Tobias

Index: armcap.c
===
RCS file: /cvs/src/lib/libssl/src/crypto/armcap.c,v
retrieving revision 1.6
diff -u -p -r1.6 armcap.c
--- armcap.c20 Jun 2014 21:00:46 -  1.6
+++ armcap.c14 Mar 2015 17:52:50 -
@@ -14,7 +14,10 @@ unsigned int OPENSSL_armcap_P;
 static sigset_t all_masked;
 
 static sigjmp_buf ill_jmp;
-   static void ill_handler (int sig) { siglongjmp(ill_jmp, sig);
+static void
+ill_handler(int sig)
+{
+   siglongjmp(ill_jmp, sig);
 }
 
 /*
@@ -31,9 +34,6 @@ void OPENSSL_cpuid_setup(void) __attribu
 void
 OPENSSL_cpuid_setup(void)
 {
-#ifndef __OpenBSD__
-   char *e;
-#endif
 #if __ARM_ARCH__ = 7
struct sigactionill_oact, ill_act;
sigset_toset;
@@ -59,14 +59,14 @@ OPENSSL_cpuid_setup(void)
ill_act.sa_mask = all_masked;
 
sigprocmask(SIG_SETMASK, ill_act.sa_mask, oset);
-   sigaction(SIGILL, ill_act, ill_oact);
 
if (sigsetjmp(ill_jmp, 1) == 0) {
+   sigaction(SIGILL, ill_act, ill_oact);
_armv7_neon_probe();
OPENSSL_armcap_P |= ARMV7_NEON;
}
 
-   sigaction (SIGILL, ill_oact, NULL);
+   sigaction(SIGILL, ill_oact, NULL);
sigprocmask(SIG_SETMASK, oset, NULL);
 #endif
 }
Index: s390xcap.c
===
RCS file: /cvs/src/lib/libssl/src/crypto/s390xcap.c,v
retrieving revision 1.3
diff -u -p -r1.3 s390xcap.c
--- s390xcap.c  12 Jun 2014 15:49:27 -  1.3
+++ s390xcap.c  14 Mar 2015 17:52:50 -
@@ -8,7 +8,8 @@
 extern unsigned long OPENSSL_s390xcap_P[];
 
 static sigjmp_buf ill_jmp;
-static void ill_handler (int sig)
+static void
+ill_handler(int sig)
 {
siglongjmp(ill_jmp, sig);
 }
@@ -32,12 +33,13 @@ OPENSSL_cpuid_setup(void)
sigdelset(ill_act.sa_mask, SIGILL);
sigdelset(ill_act.sa_mask, SIGTRAP);
sigprocmask(SIG_SETMASK, ill_act.sa_mask, oset);
-   sigaction (SIGILL, ill_act, oact);
 
/* protection against missing store-facility-list-extended */
-   if (sigsetjmp(ill_jmp, 1) == 0)
+   if (sigsetjmp(ill_jmp, 1) == 0) {
+   sigaction(SIGILL, ill_act, oact);
OPENSSL_s390x_facilities();
+   }
 
-   sigaction (SIGILL, oact, NULL);
+   sigaction(SIGILL, oact, NULL);
sigprocmask(SIG_SETMASK, oset, NULL);
 }
Index: sparcv9cap.c
===
RCS file: /cvs/src/lib/libssl/src/crypto/sparcv9cap.c,v
retrieving revision 1.7
diff -u -p -r1.7 sparcv9cap.c
--- sparcv9cap.c20 Jun 2014 21:00:46 -  1.7
+++ sparcv9cap.c14 Mar 2015 17:52:50 -
@@ -44,9 +44,8 @@ common_handler(int sig)
 void
 OPENSSL_cpuid_setup(void)
 {
-   char *e;
struct sigactioncommon_act, ill_oact, bus_oact;
-   sigset_tall_masked, oset;
+   sigset_tothers_masked, all_masked, oset;
static int trigger = 0;
 
if (trigger)
@@ -56,25 +55,33 @@ OPENSSL_cpuid_setup(void)
/* 

df: division by zero on invalid ext2fs

2015-03-01 Thread Tobias Stoeckmann
Hi,

it is possible to trigger a floating point exception in df when it is
used to retrieve information from a raw device with a broken ext2
file system.

These are steps to prepare a file system with an invalid entry for
e2fs_log_bsize (0xFF):

$ dd if=/dev/zero of=ext2.fs bs=1K count=1440
# vnconfig vnd0c ext2.fs
# newfs_ext2fs -I vnd0c
$ dd if=/dev/zero bs=1 count=4 | tr '\0' '\777' | \
dd of=ext2.fs conv=notrunc seek=1048 bs=1 count=4

If this raw device is accessed with df, the call will result in a
floating point exception:

$ df /dev/vnd0c
Floating point exception (core dumped)

With applied diff, the raw device is considered invalid, which leads to
same behavior as with unknown file systems:

$ /usr/obj/bin/df/df /dev/vnd0c
$ echo $?
1

The fix is simple: Avoid division by zero by explicitly checking that
we won't divide by zero. The results can always be weird with broken
file systems, but at least df won't crash.


Tobias

Index: ext2fs_df.c
===
RCS file: /cvs/src/bin/df/ext2fs_df.c,v
retrieving revision 1.12
diff -u -p -r1.12 ext2fs_df.c
--- bin/df/ext2fs_df.c  16 Jan 2015 06:39:31 -  1.12
+++ bin/df/ext2fs_df.c  1 Mar 2015 11:09:42 -
@@ -77,8 +77,9 @@ e2fs_df(int rfd, char *file, struct stat
sfsp-f_bsize = 1024  sblock.e2fs_log_bsize;
sfsp-f_iosize = 1024  sblock.e2fs_log_bsize;
 
-   ipb = sfsp-f_bsize / sizeof(struct ext2fs_dinode);
-   itpg = sblock.e2fs_ipg/ipb;
+   if ((ipb = sfsp-f_bsize / sizeof(struct ext2fs_dinode)) == 0)
+   return (-1);
+   itpg = sblock.e2fs_ipg / ipb;
 
ncg = howmany(sblock.e2fs_bcount - sblock.e2fs_first_dblock,
sblock.e2fs_bpg);



compress: funopen error handling

2015-01-31 Thread Tobias Stoeckmann
Hi,

this diff adds error handling for funopen() failure.  I have also fixed
a whitespace issue for proper intendation.


Tobias

Index: usr.bin/compress/zopen.c
===
RCS file: /cvs/src/usr.bin/compress/zopen.c,v
retrieving revision 1.19
diff -u -p -r1.19 zopen.c
--- usr.bin/compress/zopen.c16 Jan 2015 06:40:06 -  1.19
+++ usr.bin/compress/zopen.c31 Jan 2015 15:36:23 -
@@ -278,7 +278,7 @@ zwrite(void *cookie, const char *wbp, in
/* Secondary hash (after G. Knott). */
disp = zs-zs_hsize_reg - i;
if (i == 0)
-   disp = 1;
+   disp = 1;
 probe: if ((i -= disp)  0)
i += zs-zs_hsize_reg;
 
@@ -738,6 +738,7 @@ cl_hash(struct s_zstate *zs, count_int c
 FILE *
 zopen(const char *name, const char *mode, int bits)
 {
+   FILE *fp;
int fd;
void *cookie;
if ((fd = open(name, (*mode=='r'? O_RDONLY:O_WRONLY|O_CREAT),
@@ -747,8 +748,13 @@ zopen(const char *name, const char *mode
close(fd);
return NULL;
}
-   return funopen(cookie, (*mode == 'r'?zread:NULL),
-   (*mode == 'w'?zwrite:NULL), NULL, zclose);
+   if ((fp = funopen(cookie, (*mode == 'r'?zread:NULL),
+   (*mode == 'w'?zwrite:NULL), NULL, zclose)) == NULL) {
+   close(fd);
+   free(cookie);
+   return NULL;
+   }
+   return fp;
 }
 
 void *



patch: fix arbitrary ed command allowance

2014-12-13 Thread Tobias Stoeckmann
Hi,

patch accepts arbitrary ed commands after encountering s.  The s
ed command does not expect any further input, which makes it a one line
command like d.  Yet, patch sends any lines until . unchecked to ed
through its pipe, allowing command execution.

Example:

$ ls
ed.diff
$ cat ed.diff
0a
some text.
.
1s/.//
!/usr/bin/touch file.txt
$ touch a
$ patch a  ed.diff
Hmm...  Looks like an ed script to me...
0
!
10
done
$ ls
a   a.orig  ed.diff file.txt
$ _


Tobias

Index: pch.c
===
RCS file: /cvs/src/usr.bin/patch/pch.c,v
retrieving revision 1.49
diff -u -p -u -p -r1.49 pch.c
--- pch.c   13 Dec 2014 10:31:07 -  1.49
+++ pch.c   13 Dec 2014 15:25:51 -
@@ -1398,10 +1398,10 @@ do_ed_script(void)
;
/* POSIX defines allowed commands as {a,c,d,i,s} */
if (isdigit((unsigned char)*buf) 
-   (*t == 'a' || *t == 'c' || *t == 'd' || *t == 'i' || *t == 
's')) {
+   strchr(acdis, *t) != NULL) {
if (pipefp != NULL)
fputs(buf, pipefp);
-   if (*t != 'd') {
+   if (*t != 'd'  *t != 's') {
while (pgets(buf, sizeof buf, pfp) != NULL) {
p_input_line++;
if (pipefp != NULL)



Re: patch: fix arbitrary ed command allowance

2014-12-13 Thread Tobias Stoeckmann
On Sat, Dec 13, 2014 at 10:57:42AM -0500, Daniel Dickman wrote:
  -   (*t == 'a' || *t == 'c' || *t == 'd' || *t == 'i' || *t 
  == 's')) {
  +   strchr(acdis, *t) != NULL) {
 
 
 doesn't this change the semantics slightly? i haven't looked at the
 context beyond what's in your patch but if *t is somehow equal to NUL,
 won't strchr return the position of the terminating NUL since The
 terminating NUL character is considered to be part of the string.?

Indeed, thanks for pointing it out.

Updated diff below:


Index: pch.c
===
RCS file: /cvs/src/usr.bin/patch/pch.c,v
retrieving revision 1.49
diff -u -p -u -p -r1.49 pch.c
--- pch.c   13 Dec 2014 10:31:07 -  1.49
+++ pch.c   13 Dec 2014 16:17:01 -
@@ -1398,10 +1398,10 @@ do_ed_script(void)
;
/* POSIX defines allowed commands as {a,c,d,i,s} */
if (isdigit((unsigned char)*buf) 
-   (*t == 'a' || *t == 'c' || *t == 'd' || *t == 'i' || *t == 
's')) {
+   *t != '\0'  strchr(acdis, *t) != NULL) {
if (pipefp != NULL)
fputs(buf, pipefp);
-   if (*t != 'd') {
+   if (*t != 'd'  *t != 's') {
while (pgets(buf, sizeof buf, pfp) != NULL) {
p_input_line++;
if (pipefp != NULL)



patch: safer temp file handling

2014-12-13 Thread Tobias Stoeckmann
);
 }
 
 /*
  * Open a file to put hunks we can't locate.
  */
 static void
-init_reject(const char *name)
+init_reject(void)
 {
-   rejfp = fopen(name, w);
-   if (rejfp == NULL)
-   pfatal(can't create %s, name);
+   rejfp = open_temp_file(TMP_REJ, w, TMPREJNAME);
 }
 
 /*
Index: pch.c
===
RCS file: /cvs/src/usr.bin/patch/pch.c,v
retrieving revision 1.49
diff -u -p -u -p -r1.49 pch.c
--- pch.c   13 Dec 2014 10:31:07 -  1.49
+++ pch.c   13 Dec 2014 19:38:20 -
@@ -41,6 +41,7 @@
 #include util.h
 #include pch.h
 #include pathnames.h
+#include temp.h
 
 /* Patch (diff listing) abstract type. */
 
@@ -102,17 +103,15 @@ open_patch_file(const char *filename)
struct stat filestat;
 
if (filename == NULL || *filename == '\0' || strEQ(filename, -)) {
-   pfp = fopen(TMPPATNAME, w);
-   if (pfp == NULL)
-   pfatal(can't create %s, TMPPATNAME);
+   pfp = open_temp_file(TMP_PAT, r+, (char **)filename);
while (fgets(buf, sizeof buf, stdin) != NULL)
fputs(buf, pfp);
-   fclose(pfp);
-   filename = TMPPATNAME;
+   fseeko(pfp, 0, SEEK_SET);
+   } else {
+   pfp = fopen(filename, r);
+   if (pfp == NULL)
+   pfatal(patch file %s not found, filename);
}
-   pfp = fopen(filename, r);
-   if (pfp == NULL)
-   pfatal(patch file %s not found, filename);
if (fstat(fileno(pfp), filestat))
pfatal(can't stat %s, filename);
p_filesize = filestat.st_size;
@@ -1377,6 +1376,8 @@ do_ed_script(void)
char*t;
off_t   beginning_of_this_line;
FILE*pipefp = NULL;
+
+   fclose(open_temp_file(TMP_OUT, w, TMPOUTNAME));
 
if (!skip_rest_of_patch) {
if (copy_file(filearg[0], TMPOUTNAME)  0) {
Index: temp.c
===
RCS file: temp.c
diff -N temp.c
--- /dev/null   1 Jan 1970 00:00:00 -
+++ temp.c  13 Dec 2014 19:38:20 -
@@ -0,0 +1,110 @@
+/* $OpenBSD$   */
+/*
+ * Copyright (c) 2014 Tobias Stoeckmann tob...@openbsd.org
+ *
+ * Permission to use, copy, modify, and distribute this software for any
+ * purpose with or without fee is hereby granted, provided that the above
+ * copyright notice and this permission notice appear in all copies.
+ *
+ * THE SOFTWARE IS PROVIDED AS IS AND THE AUTHOR DISCLAIMS ALL WARRANTIES
+ * WITH REGARD TO THIS SOFTWARE INCLUDING ALL IMPLIED WARRANTIES OF
+ * MERCHANTABILITY AND FITNESS. IN NO EVENT SHALL THE AUTHOR BE LIABLE FOR
+ * ANY SPECIAL, DIRECT, INDIRECT, OR CONSEQUENTIAL DAMAGES OR ANY DAMAGES
+ * WHATSOEVER RESULTING FROM LOSS OF USE, DATA OR PROFITS, WHETHER IN AN
+ * ACTION OF CONTRACT, NEGLIGENCE OR OTHER TORTIOUS ACTION, ARISING OUT OF
+ * OR IN CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE.
+ */
+
+#include paths.h
+#include stdio.h
+#include stdlib.h
+#include string.h
+#include unistd.h
+
+#include common.h
+#include util.h
+#include temp.h
+
+static char *pattern[] = {
+   patchoXX,
+   patchiXX,
+   patchrXX,
+   patchpXX
+};
+
+static char*path[4];
+static char*tmpdir;
+
+static void temp_init(void);
+
+static void
+temp_init(void)
+{
+   int i;
+
+   /* Cons up the names of the temporary files. */
+   if ((tmpdir = getenv(TMPDIR)) == NULL || *tmpdir == '\0')
+   tmpdir = _PATH_TMP;
+   for (i = strlen(tmpdir) - 1; i  0  tmpdir[i] == '/'; i--)
+   ;
+   i++;
+
+   if (asprintf(tmpdir, %.*s, i, tmpdir) == -1)
+   fatal(can't allocate memory);
+}
+
+FILE *
+open_temp_file(unsigned int type, const char *mode, char **name)
+{
+   FILE *fp;
+   int fd;
+
+   if (type  3)
+   fatal(invalid temp type %u, type);
+
+   if (tmpdir == NULL)
+   temp_init();
+
+   close_temp_file(type);
+
+   if (asprintf(path[type], %s/%s, tmpdir, pattern[type]) == -1)
+   fatal(can't allocate memory);
+   if ((fd = mkstemp(path[type]))  0) {
+   free(path[type]);
+   path[type] = NULL;
+   pfatal(can't create %s/%s, tmpdir, pattern[type]);
+   }
+
+   if ((fp = fdopen(fd, mode)) == NULL)
+   pfatal(can't fdopen %s, path[type]);
+   if (name != NULL)
+   *name = path[type];
+   return fp;
+}
+
+void
+close_temp_file(unsigned int type)
+{
+   if (type  3)
+   fatal(invalid temp type %u, type);
+
+   if (path[type] != NULL) {
+   switch (type) {
+   case TMP_OUT:
+   if (!toutkeep)
+   unlink(path[type]);
+   toutkeep = false;
+   break;
+   case

chmod: range checks

2014-12-12 Thread Tobias Stoeckmann
Hi,

chmod doesn't check if the program name is at least 3 characters long
before checking its index 2.

Also, there is a compiler warning about signed vs unsigned when val
is used.  In one instance, it's used with strtoul, in another with strtol,
checking its ranges.  It's okay due to automatic casting but definitely
no clean code.


Tobias

Index: chmod.c
===
RCS file: /cvs/src/bin/chmod/chmod.c,v
retrieving revision 1.31
diff -u -p -u -p -r1.31 chmod.c
--- chmod.c 6 Oct 2014 17:37:34 -   1.31
+++ chmod.c 12 Dec 2014 17:10:29 -
@@ -58,7 +58,7 @@ main(int argc, char *argv[])
FTS *ftsp;
FTSENT *p;
void *set;
-   long val;
+   unsigned long val;
int oct;
mode_t omode;
int Hflag, Lflag, Rflag, ch, fflag, fts_options, hflag, rval;
@@ -69,10 +69,12 @@ main(int argc, char *argv[])
 
setlocale(LC_ALL, );
 
-   ischown = __progname[2] == 'o';
-   ischgrp = __progname[2] == 'g';
-   ischmod = __progname[2] == 'm';
-   ischflags = __progname[2] == 'f';
+   if (strlen(__progname)  2) {
+   ischown = __progname[2] == 'o';
+   ischgrp = __progname[2] == 'g';
+   ischmod = __progname[2] == 'm';
+   ischflags = __progname[2] == 'f';
+   }
 
uid = (uid_t)-1;
gid = (gid_t)-1;
@@ -171,8 +173,8 @@ done:
mode = *argv;
if (*mode = '0'  *mode = '7') {
errno = 0;
-   val = strtol(mode, ep, 8);
-   if (val  INT_MAX || val  0)
+   val = strtoul(mode, ep, 8);
+   if (val  INT_MAX)
errno = ERANGE;
if (errno)
err(1, invalid file mode: %s, mode);



Re: chmod: range checks

2014-12-12 Thread Tobias Stoeckmann
On Fri, Dec 12, 2014 at 10:42:21AM -0800, patrick keshishian wrote:
 Just throwing this out there: will this program ever get
 installed with filename shorter than ch{grp,mod,own,flags}?

No.

It's still a form of input validation.  Therefore, it should be done.
And a user can create such a link (even though behavior is pretty
obfuscated then).

$ cd ~/bin
$ which x
which: x: Command not found.
$ ln -s /bin/chmod x
$ x
usage: x [-h] [-R [-H | -L | -P]] group file ...
$ _


Tobias



Re: skgpio crash on i386 (#604)

2014-12-11 Thread Tobias Stoeckmann
On Thu, Dec 11, 2014 at 08:21:12PM +, Miod Vallat wrote:
  Hi,
  
  I'm encountering system crash during boot with latest snapshot.  Turns
  out that it works fine when I disable skgpio through UKC.
 
 Try this.

Okay tobias@, too. ;)

The system boots nicely again, thanks!



Re: patch: Support long lines in Plan B

2014-12-07 Thread Tobias Stoeckmann
Anyone?

On Sat, Nov 29, 2014 at 05:40:31PM +0100, Tobias Stoeckmann wrote:
 Hi,
 
 this diff doesn't just fix the division by zero for input files with
 lines longer than 1023 chars in Plan B mode, it actually removes this
 line limit!
 
 Before:
 
 $ dd if=/dev/zero bs=1 count=1024 | tr '\0' a  a
 $ dd if=/dev/zero bs=1 count=1024 | tr '\0' b  b
 $ diff -u a b  a.diff
 $ patch -x 8 a a.diff
 Hmm...  Looks like a unified diff to me...
 The text leading up to this was:
 --
 |--- a  Sat Nov 29 17:34:15 2014
 |+++ b  Sat Nov 29 17:34:19 2014
 --
 Floating point exception (core dumped)
 $_
 
 After:
 
 $ patch -x 8 a a.diff
 Hmm...  Looks like a unified diff to me...
 The text leading up to this was:
 --
 |--- a  Sat Nov 29 17:34:15 2014
 |+++ b  Sat Nov 29 17:34:19 2014
 --
 Patching file a using Plan B...
 Hunk #1 succeeded at 1.
 done
 
 With this diff, patch uses fgetln to read very long lines.  One could argue
 that this limits patch to the amount of RAM it has available -- but
 that will happen anyway because at least twice as much memory as the
 longest line has to be allocated.  fgetln is therefore a good choice to
 easily parse the file.
 
 If you want to regress-test, you can supply -x 8 in PATCHOPTIONS to
 enforce Plan B.  Keep in mind that test5 will fail because this debug
 option skips a check if the input file is available.
 
 
 Tobias
 
 Index: inp.c
 ===
 RCS file: /cvs/src/usr.bin/patch/inp.c,v
 retrieving revision 1.41
 diff -u -p -u -p -r1.41 inp.c
 --- inp.c 25 Nov 2014 10:22:08 -  1.41
 +++ inp.c 29 Nov 2014 16:14:45 -
 @@ -55,8 +55,9 @@ static char **i_ptr;/* pointers to line
  static int   tifd = -1;  /* plan b virtual string array */
  static char  *tibuf[2];  /* plan b buffers */
  static LINENUM   tiline[2] = {-1, -1};   /* 1st line in each buffer */
 -static LINENUM   lines_per_buf;  /* how many lines per buffer */
 -static int   tireclen;   /* length of records in tmp file */
 +static size_tlines_per_buf;  /* how many lines per buffer */
 +static size_ttibuflen;   /* plan b buffer length */
 +static size_ttireclen;   /* length of records in tmp file */
  
  static bool  rev_in_string(const char *);
  static bool  reallocate_lines(size_t *);
 @@ -333,8 +334,8 @@ static void
  plan_b(const char *filename)
  {
   FILE*ifp;
 - size_t  i = 0, j, maxlen = 1;
 - char*p;
 + size_t  i = 0, j, len, maxlen = 1;
 + char*lbuf = NULL, *p;
   boolfound_revision = (revision == NULL);
  
   using_plan_a = false;
 @@ -343,15 +344,28 @@ plan_b(const char *filename)
   (void) unlink(TMPINNAME);
   if ((tifd = open(TMPINNAME, O_EXCL | O_CREAT | O_WRONLY, 0666))  0)
   pfatal(can't open file %s, TMPINNAME);
 - while (fgets(buf, sizeof buf, ifp) != NULL) {
 - if (revision != NULL  !found_revision  rev_in_string(buf))
 + while ((p = fgetln(ifp, len)) != NULL) {
 + if (p[len - 1] == '\n')
 + p[len - 1] = '\0';
 + else {
 + /* EOF without EOL, copy and add the NUL */
 + if ((lbuf = malloc(len + 1)) == NULL)
 + fatal(out of memory\n);
 + memcpy(lbuf, p, len);
 + lbuf[len] = '\0';
 + p = lbuf;
 +
 + last_line_missing_eol = true;
 + len++;
 + }
 + if (revision != NULL  !found_revision  rev_in_string(p))
   found_revision = true;
 - if ((i = strlen(buf))  maxlen)
 - maxlen = i; /* find longest line */
 + if (len  maxlen)
 + maxlen = len;   /* find longest line */
   }
 - last_line_missing_eol = i  0  buf[i - 1] != '\n';
 - if (last_line_missing_eol  maxlen == i)
 - maxlen++;
 + free(lbuf);
 + if (ferror(ifp))
 + pfatal(can't read file %s, filename);
  
   if (revision != NULL) {
   if (!found_revision) {
 @@ -376,23 +390,26 @@ plan_b(const char *filename)
   revision);
   }
   fseek(ifp, 0L, SEEK_SET);   /* rewind file */
 - lines_per_buf = BUFFERSIZE / maxlen;
   tireclen = maxlen;
 - tibuf[0] = malloc(BUFFERSIZE + 1);
 + tibuflen = maxlen  BUFFERSIZE ? maxlen : BUFFERSIZE;
 + lines_per_buf = tibuflen / maxlen;
 + tibuf[0] = malloc(tibuflen + 1);
   if (tibuf[0] == NULL)
   fatal(out of memory\n);
 - tibuf[1] = malloc(BUFFERSIZE + 1);
 + tibuf[1] = malloc(tibuflen + 1);
   if (tibuf[1] == NULL)
   fatal(out of memory\n);
   for (i = 1;; i++) {
   p = tibuf[0] + maxlen * (i % lines_per_buf

patch: Support long lines in Plan B

2014-11-29 Thread Tobias Stoeckmann
Hi,

this diff doesn't just fix the division by zero for input files with
lines longer than 1023 chars in Plan B mode, it actually removes this
line limit!

Before:

$ dd if=/dev/zero bs=1 count=1024 | tr '\0' a  a
$ dd if=/dev/zero bs=1 count=1024 | tr '\0' b  b
$ diff -u a b  a.diff
$ patch -x 8 a a.diff
Hmm...  Looks like a unified diff to me...
The text leading up to this was:
--
|--- a  Sat Nov 29 17:34:15 2014
|+++ b  Sat Nov 29 17:34:19 2014
--
Floating point exception (core dumped)
$_

After:

$ patch -x 8 a a.diff
Hmm...  Looks like a unified diff to me...
The text leading up to this was:
--
|--- a  Sat Nov 29 17:34:15 2014
|+++ b  Sat Nov 29 17:34:19 2014
--
Patching file a using Plan B...
Hunk #1 succeeded at 1.
done

With this diff, patch uses fgetln to read very long lines.  One could argue
that this limits patch to the amount of RAM it has available -- but
that will happen anyway because at least twice as much memory as the
longest line has to be allocated.  fgetln is therefore a good choice to
easily parse the file.

If you want to regress-test, you can supply -x 8 in PATCHOPTIONS to
enforce Plan B.  Keep in mind that test5 will fail because this debug
option skips a check if the input file is available.


Tobias

Index: inp.c
===
RCS file: /cvs/src/usr.bin/patch/inp.c,v
retrieving revision 1.41
diff -u -p -u -p -r1.41 inp.c
--- inp.c   25 Nov 2014 10:22:08 -  1.41
+++ inp.c   29 Nov 2014 16:14:45 -
@@ -55,8 +55,9 @@ static char   **i_ptr;/* pointers to line
 static int tifd = -1;  /* plan b virtual string array */
 static char*tibuf[2];  /* plan b buffers */
 static LINENUM tiline[2] = {-1, -1};   /* 1st line in each buffer */
-static LINENUM lines_per_buf;  /* how many lines per buffer */
-static int tireclen;   /* length of records in tmp file */
+static size_t  lines_per_buf;  /* how many lines per buffer */
+static size_t  tibuflen;   /* plan b buffer length */
+static size_t  tireclen;   /* length of records in tmp file */
 
 static boolrev_in_string(const char *);
 static boolreallocate_lines(size_t *);
@@ -333,8 +334,8 @@ static void
 plan_b(const char *filename)
 {
FILE*ifp;
-   size_t  i = 0, j, maxlen = 1;
-   char*p;
+   size_t  i = 0, j, len, maxlen = 1;
+   char*lbuf = NULL, *p;
boolfound_revision = (revision == NULL);
 
using_plan_a = false;
@@ -343,15 +344,28 @@ plan_b(const char *filename)
(void) unlink(TMPINNAME);
if ((tifd = open(TMPINNAME, O_EXCL | O_CREAT | O_WRONLY, 0666))  0)
pfatal(can't open file %s, TMPINNAME);
-   while (fgets(buf, sizeof buf, ifp) != NULL) {
-   if (revision != NULL  !found_revision  rev_in_string(buf))
+   while ((p = fgetln(ifp, len)) != NULL) {
+   if (p[len - 1] == '\n')
+   p[len - 1] = '\0';
+   else {
+   /* EOF without EOL, copy and add the NUL */
+   if ((lbuf = malloc(len + 1)) == NULL)
+   fatal(out of memory\n);
+   memcpy(lbuf, p, len);
+   lbuf[len] = '\0';
+   p = lbuf;
+
+   last_line_missing_eol = true;
+   len++;
+   }
+   if (revision != NULL  !found_revision  rev_in_string(p))
found_revision = true;
-   if ((i = strlen(buf))  maxlen)
-   maxlen = i; /* find longest line */
+   if (len  maxlen)
+   maxlen = len;   /* find longest line */
}
-   last_line_missing_eol = i  0  buf[i - 1] != '\n';
-   if (last_line_missing_eol  maxlen == i)
-   maxlen++;
+   free(lbuf);
+   if (ferror(ifp))
+   pfatal(can't read file %s, filename);
 
if (revision != NULL) {
if (!found_revision) {
@@ -376,23 +390,26 @@ plan_b(const char *filename)
revision);
}
fseek(ifp, 0L, SEEK_SET);   /* rewind file */
-   lines_per_buf = BUFFERSIZE / maxlen;
tireclen = maxlen;
-   tibuf[0] = malloc(BUFFERSIZE + 1);
+   tibuflen = maxlen  BUFFERSIZE ? maxlen : BUFFERSIZE;
+   lines_per_buf = tibuflen / maxlen;
+   tibuf[0] = malloc(tibuflen + 1);
if (tibuf[0] == NULL)
fatal(out of memory\n);
-   tibuf[1] = malloc(BUFFERSIZE + 1);
+   tibuf[1] = malloc(tibuflen + 1);
if (tibuf[1] == NULL)
fatal(out of memory\n);
for (i = 1;; i++) {
p = tibuf[0] + maxlen * (i % lines_per_buf);
if (i % lines_per_buf == 0) /* new block */
-   if (write(tifd, tibuf[0], 

syslogd: properly validate config

2014-11-27 Thread Tobias Stoeckmann
Hi,

the facility number is not properly validated while parsing the
configuration file -- it is possible to supply a number which is
larger than LOG_NFACILITIES, therefore accessing memory outside
of f_pmask's boundaries.

# echo 10.debug;syslog,user.info /var/log/messages  my.conf
# syslogd -d -f $PWD/my.conf
off  running
init
[priv]: msg PRIV_CONFIG_MODIFIED received
[priv]: msg PRIV_OPEN_CONFIG received
cfline(10.debug;syslog,user.info /var/log/messages, f, *)
# _ (this one just segfaulted)


Tobias

Index: syslogd.c
===
RCS file: /cvs/src/usr.sbin/syslogd/syslogd.c,v
retrieving revision 1.131
diff -u -p -u -p -U8 -r1.131 syslogd.c
--- syslogd.c   26 Nov 2014 18:34:52 -  1.131
+++ syslogd.c   27 Nov 2014 19:30:56 -
@@ -1512,17 +1512,17 @@ cfline(char *line, char *prog)
for (bp = buf; *p  !strchr(\t,;., *p); )
*bp++ = *p++;
*bp = '\0';
if (*buf == '*')
for (i = 0; i  LOG_NFACILITIES; i++)
f-f_pmask[i] = pri;
else {
i = decode(buf, facilitynames);
-   if (i  0) {
+   if (i  0 || (i  3)  LOG_NFACILITIES) {
(void)snprintf(ebuf, sizeof(ebuf),
unknown facility name \%s\,
buf);
logerror(ebuf);
free(f);
return (NULL);
}
f-f_pmask[i  3] = pri;



Re: syslogd: properly validate config

2014-11-27 Thread Tobias Stoeckmann
On Thu, Nov 27, 2014 at 01:29:48PM -0700, Todd C. Miller wrote:
 I think it would be better for decode() to just return -1 in this
 case.

The validation looks a bit like a magic number there, but this could
prevent issues of other decode()-users, too...  So yeah, I think that
is worth it:

Index: syslogd.c
===
RCS file: /cvs/src/usr.sbin/syslogd/syslogd.c,v
retrieving revision 1.131
diff -u -p -r1.131 syslogd.c
--- syslogd.c   26 Nov 2014 18:34:52 -  1.131
+++ syslogd.c   27 Nov 2014 20:46:47 -
@@ -1762,10 +1762,15 @@ int
 decode(const char *name, const CODE *codetab)
 {
const CODE *c;
+   int val;
char *p, buf[40];
 
-   if (isdigit((unsigned char)*name))
-   return (atoi(name));
+   if (isdigit((unsigned char)*name)) {
+   val = atoi(name);
+   if ((val  3)  LOG_NFACILITIES)
+   return (-1);
+   return (val);
+   }
 
for (p = buf; *name  p  buf[sizeof(buf) - 1]; p++, name++) {
if (isupper((unsigned char)*name))



Re: patch: properly check NULL return values

2014-11-26 Thread Tobias Stoeckmann
Hi,

turns out that there are a few more bad savestr calls even inside of pch.c.
Some of the code pathes are quite obvious that a NULL return value would
lead to a null pointer dereference, others would last longer before
dereferencing.

Carefully reviewing the savestr calls, the rule of thumb seems to be:
If savestr is used, immediately check out_of_mem, otherwise use xstrdup.

Theo pointed out that this one is very ugly (indeed):

+   if (!s)
+   s = Oops;

Yet it's current behavior of savestr.  I want to remove that in another
commit.  So for now, xstrdup behaves like savestr, calling fatal in case
of out of memory situation -- regardless of plan a or plan b.


Tobias

Index: patch.c
===
RCS file: /cvs/src/usr.bin/patch/patch.c,v
retrieving revision 1.51
diff -u -p -r1.51 patch.c
--- patch.c 26 Nov 2013 13:19:07 -  1.51
+++ patch.c 26 Nov 2014 11:46:20 -
@@ -213,7 +213,7 @@ main(int argc, char *argv[])
warn_on_invalid_line = true;
 
if (outname == NULL)
-   outname = savestr(filearg[0]);
+   outname = xstrdup(filearg[0]);
 
/* for ed script just up and do it and exit */
if (diff_type == ED_DIFF) {
@@ -491,10 +491,10 @@ get_some_switches(void)
/* FALLTHROUGH */
case 'z':
/* must directly follow 'b' case for backwards compat */
-   simple_backup_suffix = savestr(optarg);
+   simple_backup_suffix = xstrdup(optarg);
break;
case 'B':
-   origprae = savestr(optarg);
+   origprae = xstrdup(optarg);
break;
case 'c':
diff_type = CONTEXT_DIFF;
@@ -532,7 +532,7 @@ get_some_switches(void)
case 'i':
if (++filec == MAXFILEC)
fatal(too many file arguments\n);
-   filearg[filec] = savestr(optarg);
+   filearg[filec] = xstrdup(optarg);
break;
case 'l':
canonicalize = true;
@@ -544,7 +544,7 @@ get_some_switches(void)
noreverse = true;
break;
case 'o':
-   outname = savestr(optarg);
+   outname = xstrdup(optarg);
break;
case 'p':
strippath = atoi(optarg);
@@ -588,12 +588,12 @@ get_some_switches(void)
Argv += optind;
 
if (Argc  0) {
-   filearg[0] = savestr(*Argv++);
+   filearg[0] = xstrdup(*Argv++);
Argc--;
while (Argc  0) {
if (++filec == MAXFILEC)
fatal(too many file arguments\n);
-   filearg[filec] = savestr(*Argv++);
+   filearg[filec] = xstrdup(*Argv++);
Argc--;
}
}
Index: pch.c
===
RCS file: /cvs/src/usr.bin/patch/pch.c,v
retrieving revision 1.46
diff -u -p -r1.46 pch.c
--- pch.c   26 Nov 2014 10:11:21 -  1.46
+++ pch.c   26 Nov 2014 11:46:21 -
@@ -215,14 +215,14 @@ there_is_another_patch(void)
while (filearg[0] == NULL) {
if (force || batch) {
say(No file to patch.  Skipping...\n);
-   filearg[0] = savestr(bestguess);
+   filearg[0] = xstrdup(bestguess);
skip_rest_of_patch = true;
return true;
}
ask(File to patch: );
if (*buf != '\n') {
free(bestguess);
-   bestguess = savestr(buf);
+   bestguess = xstrdup(buf);
filearg[0] = fetchname(buf, exists, 0);
}
if (!exists) {
@@ -310,7 +310,7 @@ intuit_diff_type(void)
else if (strnEQ(s, Prereq:, 7)) {
for (t = s + 7; isspace((unsigned char)*t); t++)
;
-   revision = savestr(t);
+   revision = xstrdup(t);
for (t = revision;
*t  !isspace((unsigned char)*t); t++)
;
@@ -389,7 +389,7 @@ scan_exit:
free(bestguess);
bestguess = NULL;
if (filearg[0] != NULL)
-   bestguess = savestr(filearg[0]);
+   bestguess = xstrdup(filearg[0]);
else if (!ok_to_create_file) {
/*
 * We don't want to create a new file but we need a
@@ -1473,7 

Re: locate(1): ignore paths longer than MAXPATHLEN

2014-11-26 Thread Tobias Stoeckmann
On Wed, Nov 26, 2014 at 09:48:15PM +0100, Nicolas Bedos wrote:
 Last update, with Tobias's help.
 
 The following diff 
 - changes MAXPATHLEN from sys/param.h to PATH_MAX from limits.h
 - adds a missing prototype for sane_count
 - locate.bigram and locate.code now abort when reading a pathname
   exceeding PATH_MAX bytes on stdin

If somebody else agrees with this diff, I'll adjust the last
style-deviations and commit it.


Tobias



patch: integer overflows and oob memory access

2014-11-25 Thread Tobias Stoeckmann
Hi,

it is possible to overflow line numbers in patch; this diff cares about
the lines specified in diff files.  If such an overflow happens with
unified diffs, out of bound memory access can occur.

If you have a 32 bit system, take this one (LONG_MAX = 2^31 - 1):
--- a   Sat Nov 15 00:25:29 2014
+++ b   Sat Nov 15 00:06:50 2014
@@ -1 +2147483647,2147483647 @@
-a
+b

If you have a 64 bit system, take this one, untested (LONG_MAX = 2^63 - 1):
--- a   Sat Nov 15 00:25:29 2014
+++ b   Sat Nov 15 00:06:50 2014
@@ -1 +9223372036854775807,9223372036854775807 @@
-a
+b

$ arch
OpenBSD.i386
$ touch a
$ patch a arch.diff
Hmm...  Looks like a unified diff to me...
The text leading up to this was:
--
|--- a   Sat Nov 15 00:25:29 2014
|+++ b   Sat Nov 15 00:06:50 2014
--
Patching file a using Plan A...
Segmentation fault (core dumped)
$ _


Tobias

Index: pch.c
===
RCS file: /cvs/src/usr.bin/patch/pch.c,v
retrieving revision 1.45
diff -u -p -u -p -r1.45 pch.c
--- pch.c   25 Nov 2014 10:26:07 -  1.45
+++ pch.c   25 Nov 2014 15:19:59 -
@@ -585,12 +585,17 @@ another_hunk(void)
if (!*s)
malformed();
p_ptrn_lines = strtolinenum(s, s) - 
p_first + 1;
+   if (p_ptrn_lines  0)
+   malformed();
} else if (p_first)
p_ptrn_lines = 1;
else {
p_ptrn_lines = 0;
p_first = 1;
}
+   if (p_first = LINENUM_MAX - p_ptrn_lines ||
+   p_ptrn_lines = LINENUM_MAX - 6)
+   malformed();
 
/* we need this much at least */
p_max = p_ptrn_lines + 6;
@@ -652,12 +657,17 @@ another_hunk(void)
malformed();
p_repl_lines = strtolinenum(s, 
s) -
p_newfirst + 1;
+   if (p_repl_lines  0)
+   malformed();
} else if (p_newfirst)
p_repl_lines = 1;
else {
p_repl_lines = 0;
p_newfirst = 1;
}
+   if (p_newfirst = LINENUM_MAX - 
p_repl_lines ||
+   p_repl_lines = LINENUM_MAX - p_end)
+   malformed();
p_max = p_repl_lines + p_end;
if (p_max  MAXHUNKSIZE)
fatal(hunk too large (%ld 
lines) at line %ld: %s,
@@ -868,6 +878,10 @@ hunk_done:
s++;
if (*s != '@')
malformed();
+   if (p_first = LINENUM_MAX - p_ptrn_lines ||
+   p_newfirst  LINENUM_MAX - p_repl_lines ||
+   p_ptrn_lines = LINENUM_MAX - p_repl_lines - 1)
+   malformed();
if (!p_ptrn_lines)
p_first++;  /* do append rather than insert */
p_max = p_ptrn_lines + p_repl_lines + 1;
@@ -1010,8 +1024,12 @@ hunk_done:
p_first = strtolinenum(buf, s);
if (*s == ',') {
p_ptrn_lines = strtolinenum(s + 1, s) - p_first + 1;
+   if (p_ptrn_lines  0)
+   malformed();
} else
p_ptrn_lines = (*s != 'a');
+   if (p_first = LINENUM_MAX - p_ptrn_lines)
+   malformed();
hunk_type = *s;
if (hunk_type == 'a')
p_first++;  /* do append rather than insert */
@@ -1020,16 +1038,21 @@ hunk_done:
max = strtolinenum(s + 1, s);
else
max = min;
+   if (min  0 || min  max || max - min == LINENUM_MAX)
+   malformed();
if (hunk_type == 'd')
min++;
-   p_end = p_ptrn_lines + 1 + max - min + 1;
+   p_newfirst = min;
+   p_repl_lines = max - min + 1;
+   if (p_newfirst  LINENUM_MAX - p_repl_lines ||

patch: fix segfault on revision + empty file

2014-11-24 Thread Tobias Stoeckmann
Hi,

patch will fail with a segmentation fault in plan a if it encounters a
diff with a revision (Prereq line) when the input file is empty.

i_womp will be set to NULL to avoid mmapping 0 bytes, but later on it
will be scanned for the supplied revision.

The fix is simple: avoid scanning i_womp if it's NULL.  Just enter the
if-condition that the revision is not in there.

How to reproduce:

$ echo a  a
$ echo b  b
$ diff -u a b | sed '3i\
 Prereq: 1\
 '  b.diff
$ cat b.diff
--- a   Mon Nov 24 16:31:24 2014
+++ b   Mon Nov 24 16:31:26 2014
Prereq: 1
@@ -1 +1 @@
-a
+b
$ rm a b; touch a b;
$ patch -i b.diff
Hmm...  Looks like a unified diff to me...
The text leading up to this was:
--
|--- a  Mon Nov 24 16:31:24 2014
|+++ b  Mon Nov 24 16:31:26 2014
|Prereq: 1
--
Segmentation fault (core dumped)


Tobias

Index: inp.c
===
RCS file: /cvs/src/usr.bin/patch/inp.c,v
retrieving revision 1.40
diff -u -p -r1.40 inp.c
--- inp.c   22 Nov 2014 15:49:28 -  1.40
+++ inp.c   24 Nov 2014 15:36:37 -
@@ -303,7 +303,7 @@ plan_a(const char *filename)
/* now check for revision, if any */
 
if (revision != NULL) {
-   if (!rev_in_string(i_womp)) {
+   if (i_womp == NULL || !rev_in_string(i_womp)) {
if (force) {
if (verbose)
say(Warning: this file doesn't appear 



paste: release file descriptors earlier

2014-11-24 Thread Tobias Stoeckmann
Hi,

the function parallel() should release file descriptors just like
sequential() does:  If we reach EOF, close it -- except we were
reading stdin.


Tobias

Index: paste.c
===
RCS file: /cvs/src/usr.bin/paste/paste.c,v
retrieving revision 1.18
diff -u -p -r1.18 paste.c
--- paste.c 12 Aug 2010 05:02:52 -  1.18
+++ paste.c 24 Nov 2014 18:04:16 -
@@ -130,6 +130,8 @@ parallel(char **argv)
if (!(buf = fgetln(lp-fp, len))) {
if (!--opencnt)
break;
+   if (lp-fp != stdin)
+   (void)fclose(lp-fp);
lp-fp = NULL;
if (output  lp-cnt 
(ch = delim[(lp-cnt - 1) % delimcnt]))



Re: locate(1): ignore paths longer than MAXPATHLEN

2014-11-24 Thread Tobias Stoeckmann
On Mon, Nov 24, 2014 at 08:37:47PM +0100, Nicolas Bedos wrote:
 In locate.code.c 'mbuf' is never free()d: it is only allocated for the
 last line of input and after processing this line the program ends. I
 hope it is ok.

I would free() it nontheless outside the while loop.  For the sake of
faster review.  But that's just my opinion.

Also, it would be nice if there is only one len/sizeof() check after
fgetln.  Which means that the check should be done after the
if/else-block.  Could happen that we malloc memory for nothing, but hey:
It happens only once and just if there is a missing newline.

And while at it, silently ignoring lines doesn't sound like a good idea.
I would even go for an err() call; or at least warn().


Tobias



Re: locate(1): ignore paths longer than MAXPATHLEN

2014-11-23 Thread Tobias Stoeckmann
On Sun, Nov 23, 2014 at 06:59:59PM +0100, Nicolas Bedos wrote:
 Index: src/usr.bin/locate//code/locate.code.c
 ===
 RCS file: /cvs/src/usr.bin/locate/code/locate.code.c,v
 retrieving revision 1.17
 diff -u -p -u -r1.17 locate.code.c
 --- src/usr.bin/locate//code/locate.code.c17 Nov 2013 20:19:36 -  
 1.17
 +++ src/usr.bin/locate//code/locate.code.c23 Nov 2014 16:49:47 -
 @@ -171,6 +171,14 @@ main(int argc, char *argv[])
   /* chop newline */
   if (*cp == '\n')
   *cp = '\0';
 + }
 +
 + /* skip truncated lines */
 + if (cp == path + sizeof(buf2) - 1  *(cp-1) != '\0') {
 + while (fgets(path, sizeof(buf2), stdin) != NULL)
 + if (strchr(path, '\n') != NULL)
 + break;
 + continue;
   }
  
   /* Skip longest common prefix. */

I would recommend using fgetln for the actual line parsing.  Then this
kinda fragile code can be avoided (fragile: fgets and its users have a
hard time to properly handle '\0' chars inside a file).

See also:
- fgetln(3)
- http://undeadly.org/cgi?action=articlesid=20061027031811


Tobias



rcs: uninitialized pointer leads to segfault

2014-11-22 Thread Tobias Stoeckmann
Hi,

as jsg@ pointed out, rcs will segfault reliably when using malloc.conf with
'J' (the pointer in question is filled with d0's).

The pointer rp_delta is checked at the end of rcsparse_delta.  If it's
non-NULL, it will be included into a linked list; line 1181 of rcsparse.c.

This RCS file triggers the segfault, as supplied by jsg@:
--
head1.1;
access;
symbols
OPENBSD_5_6_BASE:1.1;
locks; strict;
comment @# @;


@.1
date95.12.18.15.18.15;  author deraadt; state Exp;
branches:
n
--

$ rlog foo,v
rlog: foo,v:9: no newline at end of file
Segmentation fault (core dumped)


Tobias

Index: usr.bin/cvs/rcsparse.c
===
RCS file: /cvs/src/usr.bin/cvs/rcsparse.c,v
retrieving revision 1.8
diff -u -p -u -p -r1.8 rcsparse.c
--- usr.bin/cvs/rcsparse.c  16 Nov 2014 19:14:34 -  1.8
+++ usr.bin/cvs/rcsparse.c  22 Nov 2014 10:32:32 -
@@ -228,6 +228,7 @@ rcsparse_init(RCSFILE *rfp)
pdp-rp_buf = xmalloc(RCS_BUFSIZE);
pdp-rp_blen = RCS_BUFSIZE;
pdp-rp_bufend = pdp-rp_buf + pdp-rp_blen - 1;
+   pdp-rp_delta = NULL;
pdp-rp_token = -1;
pdp-rp_lineno = 1;
pdp-rp_msglineno = 1;
Index: usr.bin/rcs/rcsparse.c
===
RCS file: /cvs/src/usr.bin/rcs/rcsparse.c,v
retrieving revision 1.11
diff -u -p -u -p -r1.11 rcsparse.c
--- usr.bin/rcs/rcsparse.c  16 Nov 2014 19:14:34 -  1.11
+++ usr.bin/rcs/rcsparse.c  22 Nov 2014 10:32:32 -
@@ -227,6 +227,7 @@ rcsparse_init(RCSFILE *rfp)
pdp-rp_buf = xmalloc(RCS_BUFSIZE);
pdp-rp_blen = RCS_BUFSIZE;
pdp-rp_bufend = pdp-rp_buf + pdp-rp_blen - 1;
+   pdp-rp_delta = NULL;
pdp-rp_token = -1;
pdp-rp_lineno = 1;
pdp-rp_msglineno = 1;



Re: rcs: uninitialized pointer leads to segfault

2014-11-22 Thread Tobias Stoeckmann
On Sat, Nov 22, 2014 at 11:45:02AM +0100, Tobias Stoeckmann wrote:
 as jsg@ pointed out, rcs will segfault reliably when using malloc.conf with
 'J' (the pointer in question is filled with d0's).

As Theo suggested, xcalloc will take care of this pointer and other
struct entries which are not initialized right at the start.


Index: usr.bin/cvs/rcsparse.c
===
RCS file: /cvs/src/usr.bin/cvs/rcsparse.c,v
retrieving revision 1.8
diff -u -p -r1.8 rcsparse.c
--- usr.bin/cvs/rcsparse.c  16 Nov 2014 19:14:34 -  1.8
+++ usr.bin/cvs/rcsparse.c  22 Nov 2014 15:36:47 -
@@ -224,7 +224,7 @@ rcsparse_init(RCSFILE *rfp)
if (rfp-rf_flags  RCS_PARSED)
return (0);
 
-   pdp = xmalloc(sizeof(*pdp));
+   pdp = xcalloc(sizeof(*pdp));
pdp-rp_buf = xmalloc(RCS_BUFSIZE);
pdp-rp_blen = RCS_BUFSIZE;
pdp-rp_bufend = pdp-rp_buf + pdp-rp_blen - 1;
Index: usr.bin/rcs/rcsparse.c
===
RCS file: /cvs/src/usr.bin/rcs/rcsparse.c,v
retrieving revision 1.11
diff -u -p -r1.11 rcsparse.c
--- usr.bin/rcs/rcsparse.c  16 Nov 2014 19:14:34 -  1.11
+++ usr.bin/rcs/rcsparse.c  22 Nov 2014 15:36:48 -
@@ -223,7 +223,7 @@ rcsparse_init(RCSFILE *rfp)
if (rfp-rf_flags  RCS_PARSED)
return (0);
 
-   pdp = xmalloc(sizeof(*pdp));
+   pdp = xcalloc(sizeof(*pdp));
pdp-rp_buf = xmalloc(RCS_BUFSIZE);
pdp-rp_blen = RCS_BUFSIZE;
pdp-rp_bufend = pdp-rp_buf + pdp-rp_blen - 1;



Re: patch: tedu sccs support

2014-11-20 Thread Tobias Stoeckmann
On Wed, Nov 19, 2014 at 10:02:33PM -0500, Ted Unangst wrote:
 Are we still tracking upstream patch in any way? I think we've
 diverged in a few places, but there still is an upstream, yes?

I don't think so.  Looking at the past, the first version was written
by Larry Wall who licensed it to FSF in GPL and to BSD in, well, BSD
license.

The README's commit states: patch(1) is now free, thanks Larry!

Which means that there are at least two license-incompatible versions,
the GNU patch and the BSD patch.  As we can't use the former, let's look
at the latter:

FreeBSD stays in sync with our implementation.
DragonflyBSD stays in sync with FreeBSD.
NetBSD stays in sync with DragonflyBSD/FreeBSD.

So, if there is any form of upstream among the larger BSDs, it's our
implementation.

GNU patch and our patch are kinda sibblings, yet GNU patch has more
features than our patch has.  The patches I've sent lately mostly apply
to our patch only, but there are also bugs that hit the GNU patch
version (and some that are exclusive to GNU patch, like using off_t for
indexing memory).

I've sent one larger diff to GNU patch and got asked if I would want
to sign a contributor agreement, but no... I'm not interested in that.
So before I send these common bug fixes to them, I would like to have
them published in our patch, first.  So they are clearly licensed in
our terms and there is no further discussion about it.

So whoever reads this:  Reviewing my diffs would help me to make faster
progress.  Will have quite some time for patch development this week
and next one, so the amount of diffs will be very likely higher than
reviewing can happen.  But every small review helps. :)


Tobias



patch: tedu sccs support

2014-11-19 Thread Tobias Stoeckmann
Hi,

as we don't even have the get command in base, there is no need to
support SCCS files anymore.  I also remember that we removed the
SCCS IDs from source files so...

Time to tedu sccs?


Tobias

Index: common.h
===
RCS file: /cvs/src/usr.bin/patch/common.h,v
retrieving revision 1.26
diff -u -p -r1.26 common.h
--- common.h11 Mar 2006 19:41:30 -  1.26
+++ common.h19 Nov 2014 18:23:01 -
@@ -39,10 +39,6 @@
 #define MAXLINELEN 8192
 #define BUFFERSIZE 1024
 
-#define SCCSPREFIX s.
-#define GET get -e %s
-#define SCCSDIFF get -p %s | diff - %s /dev/null
-
 #define RCSSUFFIX ,v
 #define CHECKOUT co -l %s
 #define RCSDIFF rcsdiff %s  /dev/null
Index: inp.c
===
RCS file: /cvs/src/usr.bin/patch/inp.c,v
retrieving revision 1.39
diff -u -p -r1.39 inp.c
--- inp.c   15 Nov 2014 16:35:47 -  1.39
+++ inp.c   19 Nov 2014 18:23:01 -
@@ -163,7 +163,7 @@ plan_a(const char *filename)
}
if (statfailed  check_only)
fatal(%s not found, -C mode, can't probe further\n, filename);
-   /* For nonexistent or read-only files, look for RCS or SCCS versions.  
*/
+   /* For nonexistent or read-only files, look for RCS versions.  */
if (statfailed ||
/* No one can write to it.  */
(filestat.st_mode  0222) == 0 ||
@@ -187,11 +187,6 @@ plan_a(const char *filename)
snprintf(buf, sizeof buf, CHECKOUT, filename);
snprintf(lbuf, sizeof lbuf, RCSDIFF, filename);
cs = RCS;
-   } else if (try(%s/SCCS/%s%s, filedir, SCCSPREFIX, filebase) ||
-   try(%s/%s%s, filedir, SCCSPREFIX, filebase)) {
-   snprintf(buf, sizeof buf, GET, s);
-   snprintf(lbuf, sizeof lbuf, SCCSDIFF, s, filename);
-   cs = SCCS;
} else if (statfailed)
fatal(can't find %s\n, filename);
/*
Index: patch.1
===
RCS file: /cvs/src/usr.bin/patch/patch.1,v
retrieving revision 1.27
diff -u -p -r1.27 patch.1
--- patch.1 15 Apr 2014 06:26:54 -  1.27
+++ patch.1 19 Nov 2014 18:23:01 -
@@ -481,8 +481,8 @@ the shortest basename, and the shortest 
 .It
 If no file exists,
 .Nm
-checks for the existence of the files in an SCCS or RCS directory
-(using the appropriate prefix or suffix) using the criteria specified
+checks for the existence of the files in an RCS directory
+(using the appropriate suffix) using the criteria specified
 above.
 If found,
 .Nm
Index: pch.c
===
RCS file: /cvs/src/usr.bin/patch/pch.c,v
retrieving revision 1.43
diff -u -p -r1.43 pch.c
--- pch.c   18 Nov 2014 17:03:35 -  1.43
+++ pch.c   19 Nov 2014 18:23:02 -
@@ -1451,7 +1451,7 @@ posix_name(const struct file_name *names
if (path == NULL  !assume_exists) {
/*
 * No files found, look for something we can checkout from
-* RCS/SCCS dirs.  Same order as above.
+* RCS dirs.  Same order as above.
 */
for (i = 0; i  MAX_FILE; i++) {
if (names[i].path != NULL 
Index: util.c
===
RCS file: /cvs/src/usr.bin/patch/util.c,v
retrieving revision 1.36
diff -u -p -r1.36 util.c
--- util.c  26 Nov 2013 13:19:07 -  1.36
+++ util.c  19 Nov 2014 18:23:02 -
@@ -374,7 +374,7 @@ fetchname(const char *at, bool *exists, 
 }
 
 /*
- * Takes the name returned by fetchname and looks in RCS/SCCS directories
+ * Takes the name returned by fetchname and looks in RCS directory
  * for a checked in version.
  */
 char *
@@ -391,9 +391,7 @@ checked_in(char *file)
 
if (try(%s/RCS/%s%s, filedir, filebase, RCSSUFFIX) ||
try(%s/RCS/%s%s, filedir, filebase, ) ||
-   try(%s/%s%s, filedir, filebase, RCSSUFFIX) ||
-   try(%s/SCCS/%s%s, filedir, SCCSPREFIX, filebase) ||
-   try(%s/%s%s, filedir, SCCSPREFIX, filebase))
+   try(%s/%s%s, filedir, filebase, RCSSUFFIX))
return file;
 
return NULL;



patch: avoid reading after end of string

2014-11-18 Thread Tobias Stoeckmann
Hi,

on a diff with a missing new line, it is possible that patch will read
past the terminating NUL character.


Tobias

Index: pch.c
===
RCS file: /cvs/src/usr.bin/patch/pch.c,v
retrieving revision 1.42
diff -u -p -r1.42 pch.c
--- pch.c   17 Nov 2014 10:58:09 -  1.42
+++ pch.c   18 Nov 2014 14:25:31 -
@@ -344,9 +344,9 @@ intuit_diff_type(void)
ok_to_create_file = true;
/*
 * If this is a new context diff the character just
-* before the newline is a '*'.
+* at the end of the line is a '*'.
 */
-   while (*s != '\n')
+   while (*s  *s != '\n')
s++;
p_indent = indent;
p_start = previous_line;



Re: patch: avoid reading after end of string

2014-11-18 Thread Tobias Stoeckmann
On Tue, Nov 18, 2014 at 04:32:17PM +0100, Otto Moerbeek wrote:
 I seem to remember other code guarantees that lines are always
 termined by '\n'. Specifically plan_a(), plan_b() and ifetch();

Not in this case.  We use the output of fgets.

This would be a valid contextual diff:

$ echo a  a
$ echo b  b
$ diff -c a b  a.diff
$ cat a.diff
*** a   Tue Nov 18 16:36:31 2014
--- b   Tue Nov 18 16:36:34 2014
***
*** 1 
! a
--- 1 
! b

If we remove the last 3 lines and also remove the newline of
*** 1 , then fgets will return it without a \n.  I've inserted
a printf into the loop to verify that we get past the \0.

In general, nothing much will happen, because we won't get out of buf[]
due to the previous lines.  But it's still a bug.


Tobias



patch: introduce strtolinenum (instead of atol)

2014-11-18 Thread Tobias Stoeckmann
Hi,

the numbers in patch files are parsed with atol(), which is not properly
checked for all cases:

- atol() accepts leading spaces, whereas the surrounding code will
  iterate through digits to skip that number... If there is a plus or
  minus sign in front, this for-loop would fail, too.
- Ranges are not properly checked, allowing negative values
  and overflowing numbers.

I have introduced the method strtolinenum, because the internal format
of line numbers is LINENUM (typedef of long).  It behaves like strtol
but uses our strtonum internally.  In case of an unparsable file, it
bails out with a fatal.  Either it writes the offending number on
terminal or uses the generic malformed function to bail out.

If you want to play around with the number parsing, take this example:

$ echo a  a
$ echo b  b
$ diff -u a b  a.diff
$ cat a.diff
--- a   Tue Nov 18 19:57:47 2014
+++ b   Tue Nov 18 19:57:49 2014
@@ -1 +1 @@
-a
+b

You can adjust the @@ numbers.  Remove one, make it too large, etc.


Tobias

Index: common.h
===
RCS file: /cvs/src/usr.bin/patch/common.h,v
retrieving revision 1.26
diff -u -p -r1.26 common.h
--- common.h11 Mar 2006 19:41:30 -  1.26
+++ common.h18 Nov 2014 18:54:42 -
@@ -28,6 +28,7 @@
 
 #include sys/types.h
 
+#include limits.h
 #include stdbool.h
 
 #define DEBUGGING
@@ -38,6 +39,7 @@
 #define INITHUNKMAX 125/* initial dynamic allocation size */
 #define MAXLINELEN 8192
 #define BUFFERSIZE 1024
+#define LINENUM_MAX LONG_MAX
 
 #define SCCSPREFIX s.
 #define GET get -e %s
Index: pch.c
===
RCS file: /cvs/src/usr.bin/patch/pch.c,v
retrieving revision 1.43
diff -u -p -r1.43 pch.c
--- pch.c   18 Nov 2014 17:03:35 -  1.43
+++ pch.c   18 Nov 2014 18:54:43 -
@@ -76,6 +76,7 @@ static char   *pgets(char *, int, FILE *);
 static char*best_name(const struct file_name *, bool);
 static char*posix_name(const struct file_name *, bool);
 static size_t  num_components(const char *);
+static LINENUM strtolinenum(char *, char **);
 
 /*
  * Prepare to look for the next patch in the patch file.
@@ -340,7 +341,7 @@ intuit_diff_type(void)
stars_this_line = strnEQ(s, , 8);
if ((!diff_type || diff_type == CONTEXT_DIFF)  
stars_last_line 
strnEQ(s, *** , 4)) {
-   if (atol(s + 4) == 0)
+   if (strtolinenum(s + 4, s) == 0)
ok_to_create_file = true;
/*
 * If this is a new context diff the character just
@@ -577,15 +578,13 @@ another_hunk(void)
malformed();
if (strnEQ(s, 0,0, 3))
memmove(s, s + 2, strlen(s + 2) + 1);
-   p_first = (LINENUM) atol(s);
-   while (isdigit((unsigned char)*s))
-   s++;
+   p_first = strtolinenum(s, s);
if (*s == ',') {
for (; *s  !isdigit((unsigned 
char)*s); s++)
;
if (!*s)
malformed();
-   p_ptrn_lines = ((LINENUM) atol(s)) - 
p_first + 1;
+   p_ptrn_lines = strtolinenum(s, s) - 
p_first + 1;
} else if (p_first)
p_ptrn_lines = 1;
else {
@@ -645,15 +644,13 @@ another_hunk(void)
;
if (!*s)
malformed();
-   p_newfirst = (LINENUM) atol(s);
-   while (isdigit((unsigned char)*s))
-   s++;
+   p_newfirst = strtolinenum(s, s);
if (*s == ',') {
for (; *s  !isdigit((unsigned 
char)*s); s++)
;
if (!*s)
malformed();
-   p_repl_lines = ((LINENUM) 
atol(s)) -
+   p_repl_lines = strtolinenum(s, 
s) -
p_newfirst + 1;
} else if (p_newfirst)
p_repl_lines = 1;
@@ -853,26 +850,18 @@ 

Re: less strlen in bgpctl

2014-11-17 Thread Tobias Stoeckmann
On Sun, Nov 16, 2014 at 06:38:24PM -0500, Ted Unangst wrote:
 Not sure how I ended up reading this file, but all the redundant
 strlen calls make me twitchy.

I agree, but have you considered the other parser.c files with
match_token and same use of strlen(word), too?

src/usr.sbin/dvmrpctl/parser.c
src/usr.sbin/ikectl/parser.c
src/usr.sbin/iscsictl/parser.c
src/usr.sbin/ldpctl/parser.c
src/usr.sbin/npppctl/parser.c
src/usr.sbin/ospf6ctl/parser.c
src/usr.sbin/ospfctl/parser.c
src/usr.sbin/relayctl/parser.c
src/usr.sbin/ripctl/parser.c
src/usr.sbin/snmpctl/parser.c

 @@ -744,13 +746,14 @@ parse_prefix(const char *word, struct bg
   bzero(addr, sizeof(struct bgpd_addr));
  
   if ((p = strrchr(word, '/')) != NULL) {
 + size_t plen = strlen(p);
   mask = strtonum(p + 1, 0, 128, errstr);
   if (errstr)
   errx(1, netmask %s, errstr);
  
 - if ((ps = malloc(strlen(word) - strlen(p) + 1)) == NULL)
 + if ((ps = malloc(wordlen - plen + 1)) == NULL)
   err(1, parse_prefix: malloc);
 - strlcpy(ps, word, strlen(word) - strlen(p) + 1);
 + strlcpy(ps, word, wordlen - plen + 1);
  
   if (parse_addr(ps, addr) == 0) {
   free(ps);

While at it, the argument of malloc and strlcpy could be synced by
using a variable pslen.  It's easier to review that way:

Index: parser.c
===
RCS file: /cvs/src/usr.sbin/bgpctl/parser.c,v
retrieving revision 1.68
diff -u -p -r1.68 parser.c
--- parser.c5 Jan 2014 20:53:56 -   1.68
+++ parser.c17 Nov 2014 10:20:10 -
@@ -744,13 +746,14 @@ parse_prefix(const char *word, struct bg
bzero(addr, sizeof(struct bgpd_addr));
 
if ((p = strrchr(word, '/')) != NULL) {
+   size_t pslen = wordlen - strlen(p) + 1;
mask = strtonum(p + 1, 0, 128, errstr);
if (errstr)
errx(1, netmask %s, errstr);
 
-   if ((ps = malloc(strlen(word) - strlen(p) + 1)) == NULL)
+   if ((ps = malloc(pslen)) == NULL)
err(1, parse_prefix: malloc);
-   strlcpy(ps, word, strlen(word) - strlen(p) + 1);
+   strlcpy(ps, word, pslen);
 
if (parse_addr(ps, addr) == 0) {
free(ps);



ospf6ctl: memleak in parser

2014-11-17 Thread Tobias Stoeckmann
Hi,

after using the temporary buffer ps for parse_addr, it is not
released after successful operation.


Tobias

Index: parser.c
===
RCS file: /cvs/src/usr.sbin/ospf6ctl/parser.c,v
retrieving revision 1.12
diff -u -p -r1.12 parser.c
--- parser.c21 Oct 2012 21:30:44 -  1.12
+++ parser.c17 Nov 2014 10:09:55 -
@@ -334,6 +334,7 @@ parse_prefix(const char *word, struct in
free(ps);
return (0);
}
+   free(ps);
 
inet6applymask(addr, addr, mask);
*prefixlen = mask;



  1   2   >