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

[Alexandre Ratchov]

On Thu, Nov 12, 2015 at 02:52:01PM +0800, Michael W. Bombardieri wrote:
> > > ok for removing xfree from aucat?
> > > 
> > 
> > yes, ok ratchov; if later this causes me merges i'll find another
> > solution.  Feel free to do the same in usr.bin/sndiod, as it's
> > almost the same.
> > 
> 
> Same thing for sndiod...
> 

ok to replace xfree->free.  But I'd like to keep this constructs:

if (ptr != NULL)
free(ptr);

the reason is that sometimes i add printf()s or additional code
when needed, so i don't want to loose the condition.

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

[Michael McConville]

Alexandre Ratchov wrote:
> On Thu, Nov 12, 2015 at 02:52:01PM +0800, Michael W. Bombardieri wrote:
> > > > ok for removing xfree from aucat?
> > > 
> > > yes, ok ratchov; if later this causes me merges i'll find another
> > > solution.  Feel free to do the same in usr.bin/sndiod, as it's
> > > almost the same.
> > 
> > Same thing for sndiod...
> 
> ok to replace xfree->free.  But I'd like to keep this constructs:
> 
>   if (ptr != NULL)
>   free(ptr);
> 
> the reason is that sometimes i add printf()s or additional code when
> needed, so i don't want to loose the condition.

Its long been protocol to remove this wherever we find it. POSIX
specifies that free() is NULL-safe, and removing the conditions can
greatly improve readability. IMO, this is another instance where a macro
and (Coccinelle|cscope|sed|awk) is your best bet.

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

[Michael McConville]

Michael W. Bombardieri wrote:
> > > ok for removing xfree from aucat?
> > 
> > yes, ok ratchov; if later this causes me merges i'll find another
> > solution.  Feel free to do the same in usr.bin/sndiod, as it's
> > almost the same.
> 
> Same thing for sndiod...

ok mmcc@

> Index: abuf.c
> ===
> RCS file: /cvs/src/usr.bin/sndiod/abuf.c,v
> retrieving revision 1.3
> diff -u -p -u -r1.3 abuf.c
> --- abuf.c16 Feb 2015 06:11:33 -  1.3
> +++ abuf.c12 Nov 2015 07:07:57 -
> @@ -62,7 +62,7 @@ abuf_done(struct abuf *buf)
>   }
>   }
>  #endif
> - xfree(buf->data);
> + free(buf->data);
>   buf->data = (void *)0xdeadbeef;
>  }
>  
> Index: dev.c
> ===
> RCS file: /cvs/src/usr.bin/sndiod/dev.c,v
> retrieving revision 1.18
> diff -u -p -u -r1.18 dev.c
> --- dev.c 5 Sep 2015 11:19:20 -   1.18
> +++ dev.c 12 Nov 2015 07:07:57 -
> @@ -15,6 +15,7 @@
>   * OR IN CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE.
>   */
>  #include 
> +#include 
>  #include 
>  
>  #include "abuf.h"
> @@ -838,10 +839,8 @@ dev_cycle(struct dev *d)
>*/
>   s->pstate = SLOT_INIT;
>   abuf_done(>mix.buf);
> - if (s->mix.decbuf)
> - xfree(s->mix.decbuf);
> - if (s->mix.resampbuf)
> - xfree(s->mix.resampbuf);
> + free(s->mix.decbuf);
> + free(s->mix.resampbuf);
>   s->ops->eof(s->arg);
>   *ps = s->next;
>   dev_mix_adjvol(d);
> @@ -1143,14 +1142,12 @@ dev_close(struct dev *d)
>   d->slot_list = NULL;
>   dev_sio_close(d);
>   if (d->mode & MODE_PLAY) {
> - if (d->encbuf != NULL)
> - xfree(d->encbuf);
> - xfree(d->pbuf);
> + free(d->encbuf);
> + free(d->pbuf);
>   }
>   if (d->mode & MODE_REC) {
> - if (d->decbuf != NULL)
> - xfree(d->decbuf);
> - xfree(d->rbuf);
> + free(d->decbuf);
> + free(d->rbuf);
>   }
>  }
>  
> @@ -1256,7 +1253,7 @@ dev_del(struct dev *d)
>   }
>   midi_del(d->midi);
>   *p = d->next;
> - xfree(d);
> + free(d);
>  }
>  
>  unsigned int
> @@ -1829,16 +1826,12 @@ slot_detach(struct slot *s)
>   }   
>   *ps = s->next;
>   if (s->mode & MODE_RECMASK) {
> - if (s->sub.encbuf)
> - xfree(s->sub.encbuf);
> - if (s->sub.resampbuf)
> - xfree(s->sub.resampbuf);
> + free(s->sub.encbuf);
> + free(s->sub.resampbuf);
>   }
>   if (s->mode & MODE_PLAY) {
> - if (s->mix.decbuf)
> - xfree(s->mix.decbuf);
> - if (s->mix.resampbuf)
> - xfree(s->mix.resampbuf);
> + free(s->mix.decbuf);
> + free(s->mix.resampbuf);
>   dev_mix_adjvol(s->dev);
>   }
>  }
> Index: file.c
> ===
> RCS file: /cvs/src/usr.bin/sndiod/file.c,v
> retrieving revision 1.15
> diff -u -p -u -r1.15 file.c
> --- file.c27 Aug 2015 07:38:38 -  1.15
> +++ file.c12 Nov 2015 07:07:57 -
> @@ -328,7 +328,7 @@ file_poll(void)
>   while ((f = *pf) != NULL) {
>   if (f->state == FILE_ZOMB) {
>   *pf = f->next;
> - xfree(f);
> + free(f);
>   } else
>   pf = >next;
>   }
> Index: listen.c
> ===
> RCS file: /cvs/src/usr.bin/sndiod/listen.c,v
> retrieving revision 1.2
> diff -u -p -u -r1.2 listen.c
> --- listen.c  13 Mar 2013 08:28:33 -  1.2
> +++ listen.c  12 Nov 2015 07:07:57 -
> @@ -70,13 +70,12 @@ listen_close(struct listen *f)
>   }
>   *pf = f->next;
>  
> - if (f->path != NULL) {
> + if (f->path != NULL)
>   unlink(f->path);
> - xfree(f->path);
> - }
> + free(f->path);
>   file_del(f->file);
>   close(f->fd);
> - xfree(f);
> + free(f);
>  }
>  
>  void
> Index: midi.c
> ===
> RCS file: /cvs/src/usr.bin/sndiod/midi.c,v
> retrieving revision 1.10
> diff -u -p -u -r1.10 midi.c
> --- midi.c28 Sep 2013 18:49:32 -  1.10
> +++ midi.c12 Nov 2015 07:07:57 -
> @@ -461,7 +461,7 @@ port_del(struct port *c)
>  #endif
>   }
>   *p = c->next;
> - xfree(c);
> + free(c);
>  }
>  
>  int
> Index: opt.c
> ===
> RCS file: /cvs/src/usr.bin/sndiod/opt.c,v
> retrieving 

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

[Michael W. Bombardieri]

> > ok for removing xfree from aucat?
> > 
> 
> yes, ok ratchov; if later this causes me merges i'll find another
> solution.  Feel free to do the same in usr.bin/sndiod, as it's
> almost the same.
> 

Same thing for sndiod...


Index: abuf.c
===
RCS file: /cvs/src/usr.bin/sndiod/abuf.c,v
retrieving revision 1.3
diff -u -p -u -r1.3 abuf.c
--- abuf.c  16 Feb 2015 06:11:33 -  1.3
+++ abuf.c  12 Nov 2015 07:07:57 -
@@ -62,7 +62,7 @@ abuf_done(struct abuf *buf)
}
}
 #endif
-   xfree(buf->data);
+   free(buf->data);
buf->data = (void *)0xdeadbeef;
 }
 
Index: dev.c
===
RCS file: /cvs/src/usr.bin/sndiod/dev.c,v
retrieving revision 1.18
diff -u -p -u -r1.18 dev.c
--- dev.c   5 Sep 2015 11:19:20 -   1.18
+++ dev.c   12 Nov 2015 07:07:57 -
@@ -15,6 +15,7 @@
  * OR IN CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE.
  */
 #include 
+#include 
 #include 
 
 #include "abuf.h"
@@ -838,10 +839,8 @@ dev_cycle(struct dev *d)
 */
s->pstate = SLOT_INIT;
abuf_done(>mix.buf);
-   if (s->mix.decbuf)
-   xfree(s->mix.decbuf);
-   if (s->mix.resampbuf)
-   xfree(s->mix.resampbuf);
+   free(s->mix.decbuf);
+   free(s->mix.resampbuf);
s->ops->eof(s->arg);
*ps = s->next;
dev_mix_adjvol(d);
@@ -1143,14 +1142,12 @@ dev_close(struct dev *d)
d->slot_list = NULL;
dev_sio_close(d);
if (d->mode & MODE_PLAY) {
-   if (d->encbuf != NULL)
-   xfree(d->encbuf);
-   xfree(d->pbuf);
+   free(d->encbuf);
+   free(d->pbuf);
}
if (d->mode & MODE_REC) {
-   if (d->decbuf != NULL)
-   xfree(d->decbuf);
-   xfree(d->rbuf);
+   free(d->decbuf);
+   free(d->rbuf);
}
 }
 
@@ -1256,7 +1253,7 @@ dev_del(struct dev *d)
}
midi_del(d->midi);
*p = d->next;
-   xfree(d);
+   free(d);
 }
 
 unsigned int
@@ -1829,16 +1826,12 @@ slot_detach(struct slot *s)
}   
*ps = s->next;
if (s->mode & MODE_RECMASK) {
-   if (s->sub.encbuf)
-   xfree(s->sub.encbuf);
-   if (s->sub.resampbuf)
-   xfree(s->sub.resampbuf);
+   free(s->sub.encbuf);
+   free(s->sub.resampbuf);
}
if (s->mode & MODE_PLAY) {
-   if (s->mix.decbuf)
-   xfree(s->mix.decbuf);
-   if (s->mix.resampbuf)
-   xfree(s->mix.resampbuf);
+   free(s->mix.decbuf);
+   free(s->mix.resampbuf);
dev_mix_adjvol(s->dev);
}
 }
Index: file.c
===
RCS file: /cvs/src/usr.bin/sndiod/file.c,v
retrieving revision 1.15
diff -u -p -u -r1.15 file.c
--- file.c  27 Aug 2015 07:38:38 -  1.15
+++ file.c  12 Nov 2015 07:07:57 -
@@ -328,7 +328,7 @@ file_poll(void)
while ((f = *pf) != NULL) {
if (f->state == FILE_ZOMB) {
*pf = f->next;
-   xfree(f);
+   free(f);
} else
pf = >next;
}
Index: listen.c
===
RCS file: /cvs/src/usr.bin/sndiod/listen.c,v
retrieving revision 1.2
diff -u -p -u -r1.2 listen.c
--- listen.c13 Mar 2013 08:28:33 -  1.2
+++ listen.c12 Nov 2015 07:07:57 -
@@ -70,13 +70,12 @@ listen_close(struct listen *f)
}
*pf = f->next;
 
-   if (f->path != NULL) {
+   if (f->path != NULL)
unlink(f->path);
-   xfree(f->path);
-   }
+   free(f->path);
file_del(f->file);
close(f->fd);
-   xfree(f);
+   free(f);
 }
 
 void
Index: midi.c
===
RCS file: /cvs/src/usr.bin/sndiod/midi.c,v
retrieving revision 1.10
diff -u -p -u -r1.10 midi.c
--- midi.c  28 Sep 2013 18:49:32 -  1.10
+++ midi.c  12 Nov 2015 07:07:57 -
@@ -461,7 +461,7 @@ port_del(struct port *c)
 #endif
}
*p = c->next;
-   xfree(c);
+   free(c);
 }
 
 int
Index: opt.c
===
RCS file: /cvs/src/usr.bin/sndiod/opt.c,v
retrieving revision 1.2
diff -u -p -u -r1.2 opt.c
--- opt.c   7 Dec 2012 08:04:58 -   1.2
+++ opt.c   12 Nov 2015 07:07:57 -
@@ -136,5 +136,5 @@ opt_del(struct 

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

[Tobias Stöckmann]

> On November 9, 2015 at 5:04 AM Michael McConville  wrote:
> Tobias, could you split your latest diff into separate diffs for each
> function type (xmalloc, xcalloc, etc.)? It'd make it easier to zero in
> on the problematic hunks and fast-track the rest.

I don't really see the point splitting it up.
The scope of the diff is "merge xmalloc.{c,h} files".

If you still see problematic hunks, tell me. I don't know one.
In fact, I'm just waiting for feedback from djm.

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

[Alexandre Ratchov]

On Sun, Nov 08, 2015 at 07:43:10PM -0500, Michael McConville wrote:
> Maybe we should pick this off in smaller chunks so that we don't get
> immobilized by a few scattered issues.
> 
> ok for removing xfree from aucat?
> 

yes, ok ratchov; if later this causes me merges i'll find another
solution.  Feel free to do the same in usr.bin/sndiod, as it's
almost the same.

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

[Nicholas Marriott]

Hmm yes you are right, in that case, go for it.


On Sun, Nov 08, 2015 at 02:00:17PM +0100, Joerg Sonnenberger wrote:
> On Sun, Nov 08, 2015 at 08:36:52AM +, Nicholas Marriott wrote:
> > On Sat, Nov 07, 2015 at 08:42:14PM -0500, Ted Unangst wrote:
> > > Tobias Stoeckmann wrote:
> > > > 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...
> > > 
> > > Are they portable to not-posix? posix dictates that malloc set errno.
> > 
> > It is optional in SUSv3:
> > 
> > RETURN VALUE
> > 
> >  Upon successful completion with size not equal to 0, malloc() shall
> >  return a pointer to the allocated space. If size is 0, either
> >  a null pointer or a unique pointer that can be successfully
> >  passed to free() shall be returned. Otherwise, it shall return
> >  a null pointer ^[CX] [Option Start] and set errno to indicate
> >  the error. [Option End]
> 
> Not really, that just means that the "and set errno" part is not
> included in ISO C itself. The markup is a bit confusing, but CX is not
> an option in the "pick it or not sense".
> 
> Joerg
> 

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

[Joerg Sonnenberger]

On Sun, Nov 08, 2015 at 08:36:52AM +, Nicholas Marriott wrote:
> On Sat, Nov 07, 2015 at 08:42:14PM -0500, Ted Unangst wrote:
> > Tobias Stoeckmann wrote:
> > > 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...
> > 
> > Are they portable to not-posix? posix dictates that malloc set errno.
> 
> It is optional in SUSv3:
> 
> RETURN VALUE
> 
>  Upon successful completion with size not equal to 0, malloc() shall
>  return a pointer to the allocated space. If size is 0, either
>  a null pointer or a unique pointer that can be successfully
>  passed to free() shall be returned. Otherwise, it shall return
>  a null pointer ^[CX] [Option Start] and set errno to indicate
>  the error. [Option End]

Not really, that just means that the "and set errno" part is not
included in ISO C itself. The markup is a bit confusing, but CX is not
an option in the "pick it or not sense".

Joerg

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

[Michael McConville]

Alexandre Ratchov wrote:
> On Sun, Nov 08, 2015 at 09:56:23AM +0800, Michael W. Bombardieri wrote:
> > On Thu, Nov 05, 2015 at 03:50:29PM +0100, Tobias Stoeckmann wrote:
> > > 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)
> > > 
> > 
> > 
> > Also note that aucat(1)'s utils.c contains xmalloc() and xfree().
> > Its version of xfree() contains no special logic so remove it?
> 
> i'd prefer we keep xfree(), as sometimes i replace it with a
> instrumented version that detects leaks and gathers statistics

I don't think this is a good reason, honestly. It's easy to add a
wrapper function/macro while debugging and replace all free() calls with
it.

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

[Michael McConville]

Maybe we should pick this off in smaller chunks so that we don't get
immobilized by a few scattered issues.

ok for removing xfree from aucat?


Index: abuf.c
===
RCS file: /cvs/src/usr.bin/aucat/abuf.c,v
retrieving revision 1.26
diff -u -p -r1.26 abuf.c
--- abuf.c  21 Jan 2015 08:43:55 -  1.26
+++ abuf.c  9 Nov 2015 00:40:36 -
@@ -62,7 +62,7 @@ abuf_done(struct abuf *buf)
}
}
 #endif
-   xfree(buf->data);
+   free(buf->data);
buf->data = (void *)0xdeadbeef;
 }
 
Index: aucat.c
===
RCS file: /cvs/src/usr.bin/aucat/aucat.c,v
retrieving revision 1.149
diff -u -p -r1.149 aucat.c
--- aucat.c 27 Aug 2015 07:25:56 -  1.149
+++ aucat.c 9 Nov 2015 00:40:36 -
@@ -214,7 +214,7 @@ slot_new(char *path, int mode, struct ap
if (!afile_open(>afile, path, hdr,
mode == SIO_PLAY ? AFILE_FREAD : AFILE_FWRITE,
par, rate, cmax - cmin + 1)) {
-   xfree(s);
+   free(s);
return 0;
}
s->cmin = cmin;
@@ -413,15 +413,13 @@ slot_del(struct slot *s)
}
 #endif
abuf_done(>buf);
-   if (s->resampbuf)
-   xfree(s->resampbuf);
-   if (s->convbuf)
-   xfree(s->convbuf);
+   free(s->resampbuf);
+   free(s->convbuf);
}
for (ps = _list; *ps != s; ps = &(*ps)->next)
; /* nothing */
*ps = s->next;
-   xfree(s);
+   free(s);
 }
 
 static int 
@@ -672,9 +670,9 @@ dev_close(void)
if (dev_mh)
mio_close(dev_mh);
if (dev_mode & SIO_PLAY)
-   xfree(dev_pbuf);
+   free(dev_pbuf);
if (dev_mode & SIO_REC)
-   xfree(dev_rbuf);
+   free(dev_rbuf);
 }
 
 static void
@@ -999,7 +997,7 @@ offline(void)
slot_list_copy(todo, dev_pchan, dev_pbuf);
slot_list_iodo();
}
-   xfree(dev_pbuf);
+   free(dev_pbuf);
while (slot_list)
slot_del(slot_list);
return 1;
@@ -1148,7 +1146,7 @@ playrec(char *dev, int mode, int bufsz, 
 
if (dev_pstate == DEV_START)
dev_mmcstop();
-   xfree(pfds);
+   free(pfds);
dev_close();
while (slot_list)
slot_del(slot_list);
Index: utils.c
===
RCS file: /cvs/src/usr.bin/aucat/utils.c,v
retrieving revision 1.1
diff -u -p -r1.1 utils.c
--- utils.c 21 Jan 2015 08:43:55 -  1.1
+++ utils.c 9 Nov 2015 00:40:36 -
@@ -158,15 +158,6 @@ xmalloc(size_t size)
 }
 
 /*
- * free memory allocated with xmalloc()
- */
-void
-xfree(void *p)
-{
-   free(p);
-}
-
-/*
  * xmalloc-style strdup(3)
  */
 char *
Index: utils.h
===
RCS file: /cvs/src/usr.bin/aucat/utils.h,v
retrieving revision 1.1
diff -u -p -r1.1 utils.h
--- utils.h 21 Jan 2015 08:43:55 -  1.1
+++ utils.h 9 Nov 2015 00:40:36 -
@@ -29,7 +29,6 @@ void log_flush(void);
 
 void *xmalloc(size_t);
 char *xstrdup(char *);
-void xfree(void *);
 
 /*
  * Log levels:

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

[Michael McConville]

Michael McConville wrote:
> Maybe we should pick this off in smaller chunks so that we don't get
> immobilized by a few scattered issues.
> 
> ok for removing xfree from aucat?

I just realized that this one was already submitted separately.

Tobias, could you split your latest diff into separate diffs for each
function type (xmalloc, xcalloc, etc.)? It'd make it easier to zero in
on the problematic hunks and fast-track the rest.

> Index: abuf.c
> ===
> RCS file: /cvs/src/usr.bin/aucat/abuf.c,v
> retrieving revision 1.26
> diff -u -p -r1.26 abuf.c
> --- abuf.c21 Jan 2015 08:43:55 -  1.26
> +++ abuf.c9 Nov 2015 00:40:36 -
> @@ -62,7 +62,7 @@ abuf_done(struct abuf *buf)
>   }
>   }
>  #endif
> - xfree(buf->data);
> + free(buf->data);
>   buf->data = (void *)0xdeadbeef;
>  }
>  
> Index: aucat.c
> ===
> RCS file: /cvs/src/usr.bin/aucat/aucat.c,v
> retrieving revision 1.149
> diff -u -p -r1.149 aucat.c
> --- aucat.c   27 Aug 2015 07:25:56 -  1.149
> +++ aucat.c   9 Nov 2015 00:40:36 -
> @@ -214,7 +214,7 @@ slot_new(char *path, int mode, struct ap
>   if (!afile_open(>afile, path, hdr,
>   mode == SIO_PLAY ? AFILE_FREAD : AFILE_FWRITE,
>   par, rate, cmax - cmin + 1)) {
> - xfree(s);
> + free(s);
>   return 0;
>   }
>   s->cmin = cmin;
> @@ -413,15 +413,13 @@ slot_del(struct slot *s)
>   }
>  #endif
>   abuf_done(>buf);
> - if (s->resampbuf)
> - xfree(s->resampbuf);
> - if (s->convbuf)
> - xfree(s->convbuf);
> + free(s->resampbuf);
> + free(s->convbuf);
>   }
>   for (ps = _list; *ps != s; ps = &(*ps)->next)
>   ; /* nothing */
>   *ps = s->next;
> - xfree(s);
> + free(s);
>  }
>  
>  static int 
> @@ -672,9 +670,9 @@ dev_close(void)
>   if (dev_mh)
>   mio_close(dev_mh);
>   if (dev_mode & SIO_PLAY)
> - xfree(dev_pbuf);
> + free(dev_pbuf);
>   if (dev_mode & SIO_REC)
> - xfree(dev_rbuf);
> + free(dev_rbuf);
>  }
>  
>  static void
> @@ -999,7 +997,7 @@ offline(void)
>   slot_list_copy(todo, dev_pchan, dev_pbuf);
>   slot_list_iodo();
>   }
> - xfree(dev_pbuf);
> + free(dev_pbuf);
>   while (slot_list)
>   slot_del(slot_list);
>   return 1;
> @@ -1148,7 +1146,7 @@ playrec(char *dev, int mode, int bufsz, 
>  
>   if (dev_pstate == DEV_START)
>   dev_mmcstop();
> - xfree(pfds);
> + free(pfds);
>   dev_close();
>   while (slot_list)
>   slot_del(slot_list);
> Index: utils.c
> ===
> RCS file: /cvs/src/usr.bin/aucat/utils.c,v
> retrieving revision 1.1
> diff -u -p -r1.1 utils.c
> --- utils.c   21 Jan 2015 08:43:55 -  1.1
> +++ utils.c   9 Nov 2015 00:40:36 -
> @@ -158,15 +158,6 @@ xmalloc(size_t size)
>  }
>  
>  /*
> - * free memory allocated with xmalloc()
> - */
> -void
> -xfree(void *p)
> -{
> - free(p);
> -}
> -
> -/*
>   * xmalloc-style strdup(3)
>   */
>  char *
> Index: utils.h
> ===
> RCS file: /cvs/src/usr.bin/aucat/utils.h,v
> retrieving revision 1.1
> diff -u -p -r1.1 utils.h
> --- utils.h   21 Jan 2015 08:43:55 -  1.1
> +++ utils.h   9 Nov 2015 00:40:36 -
> @@ -29,7 +29,6 @@ void log_flush(void);
>  
>  void *xmalloc(size_t);
>  char *xstrdup(char *);
> -void xfree(void *);
>  
>  /*
>   * Log levels:
> 

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

[Nicholas Marriott]

This is fine with me, I think it was better without errno, but using it
can't do any harm. It is an extension TO set it, not to not set it, but
I am pretty sure it only happens on platforms I don't care about :-).

I suggest you check with djm or dtucker for ssh in case they do care, or
there are any other problem for portable.



On Sun, Nov 08, 2015 at 01:39:32AM +0100, Tobias Stoeckmann wrote:
> 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.c25 Sep 2015 16:16:26 -  1.8
> +++ usr.bin/diff/xmalloc.c8 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 

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

[Nicholas Marriott]

On Sat, Nov 07, 2015 at 08:42:14PM -0500, Ted Unangst wrote:
> Tobias Stoeckmann wrote:
> > 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...
> 
> Are they portable to not-posix? posix dictates that malloc set errno.

It is optional in SUSv3:

RETURN VALUE

 Upon successful completion with size not equal to 0, malloc() shall
 return a pointer to the allocated space. If size is 0, either
 a null pointer or a unique pointer that can be successfully
 passed to free() shall be returned. Otherwise, it shall return
 a null pointer ^[CX] [Option Start] and set errno to indicate
 the error. [Option End]

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

[Nicholas Marriott]

Hi

On Sat, Nov 07, 2015 at 04:39:09PM -0500, Michael McConville wrote:
> Nicholas Marriott wrote:
> > Looks good, ok nicm
> 
> Reviewing now, generally looks good.
> 
> A few things:
> 
> I don't understand the motive for all the err() -> errx() and fatal() ->
> fatalx() changes. If I came across these, I probably would have

malloc and friends are not guaranteed to set errno (they do on OpenBSD
but it is not required).

> suggested the reverse. err(1, "xstrdup") is a lot cleaner than a long
> custom error message, IMO. I don't know how much value is in showing the
> size of the failed allocation, either - thoughts on that? I'm fine with
> a little less uniformity for simplicity's sake.

I think it is better if they are all the same, and showing the size is
useful (for example, if the size is crazy it could be an overflow bug
rather than out of memory).

> 
> Also, I'm seeing a couple "could not allocate memory" messages added to
> *snprintf() functions. They write to a supplied buffer, no?

Yes you are right, it should be a different message.

> 
> We should probably pass the SSH changes by open...@openssh.com.
> 
> This is valuable work - thanks.
> 
> > On Thu, Nov 05, 2015 at 05:35:22PM +0100, Tobias Stoeckmann wrote:
> > > 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)
> > > - 

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

[Michael McConville]

Nicholas Marriott wrote:
> Looks good, ok nicm

Reviewing now, generally looks good.

A few things:

I don't understand the motive for all the err() -> errx() and fatal() ->
fatalx() changes. If I came across these, I probably would have
suggested the reverse. err(1, "xstrdup") is a lot cleaner than a long
custom error message, IMO. I don't know how much value is in showing the
size of the failed allocation, either - thoughts on that? I'm fine with
a little less uniformity for simplicity's sake.

Also, I'm seeing a couple "could not allocate memory" messages added to
*snprintf() functions. They write to a supplied buffer, no?

We should probably pass the SSH changes by open...@openssh.com.

This is valuable work - thanks.

> On Thu, Nov 05, 2015 at 05:35:22PM +0100, Tobias Stoeckmann wrote:
> > 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 @@ 

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

[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())

[Ted Unangst]

Michael McConville wrote:
> Nicholas Marriott wrote:
> > Looks good, ok nicm
> 
> Reviewing now, generally looks good.
> 
> A few things:
> 
> I don't understand the motive for all the err() -> errx() and fatal() ->
> fatalx() changes. If I came across these, I probably would have
> suggested the reverse. err(1, "xstrdup") is a lot cleaner than a long
> custom error message, IMO. I don't know how much value is in showing the
> size of the failed allocation, either - thoughts on that? I'm fine with
> a little less uniformity for simplicity's sake.

Showing the size lets you determine if something has gone terribly
wrong (can't allocate 3840284093284092840928402 bytes) versus a more mundane
out of memory condition. Allocation failure is usually the final symptom of
some other disease.

But agreed that always printing "out of memory" seems like a step backwards
from printing what errno tells us. It's discarding information.

> Also, I'm seeing a couple "could not allocate memory" messages added to
> *snprintf() functions. They write to a supplied buffer, no?

Good catch.

> > > + 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.

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

[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())

[Ted Unangst]

Tobias Stoeckmann wrote:
> > > > > + 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.

Anything that doesn't allocate memory can't fail to allocate memory. This
would be the sprintf variants that are not asprintf.

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

[Ted Unangst]

Tobias Stoeckmann wrote:
> 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...

Are they portable to not-posix? posix dictates that malloc set errno.

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

[Michael W. Bombardieri]

On Thu, Nov 05, 2015 at 03:50:29PM +0100, Tobias Stoeckmann wrote:
> 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)
> 


Also note that aucat(1)'s utils.c contains xmalloc() and xfree().
Its version of xfree() contains no special logic so remove it?


Index: abuf.c
===
RCS file: /cvs/src/usr.bin/aucat/abuf.c,v
retrieving revision 1.26
diff -u -p -u -r1.26 abuf.c
--- abuf.c  21 Jan 2015 08:43:55 -  1.26
+++ abuf.c  8 Nov 2015 02:16:41 -
@@ -62,7 +62,7 @@ abuf_done(struct abuf *buf)
}
}
 #endif
-   xfree(buf->data);
+   free(buf->data);
buf->data = (void *)0xdeadbeef;
 }
 
Index: aucat.c
===
RCS file: /cvs/src/usr.bin/aucat/aucat.c,v
retrieving revision 1.149
diff -u -p -u -r1.149 aucat.c
--- aucat.c 27 Aug 2015 07:25:56 -  1.149
+++ aucat.c 8 Nov 2015 02:16:42 -
@@ -214,7 +214,7 @@ slot_new(char *path, int mode, struct ap
if (!afile_open(>afile, path, hdr,
mode == SIO_PLAY ? AFILE_FREAD : AFILE_FWRITE,
par, rate, cmax - cmin + 1)) {
-   xfree(s);
+   free(s);
return 0;
}
s->cmin = cmin;
@@ -413,15 +413,13 @@ slot_del(struct slot *s)
}
 #endif
abuf_done(>buf);
-   if (s->resampbuf)
-   xfree(s->resampbuf);
-   if (s->convbuf)
-   xfree(s->convbuf);
+   free(s->resampbuf);
+   free(s->convbuf);
}
for (ps = _list; *ps != s; ps = &(*ps)->next)
; /* nothing */
*ps = s->next;
-   xfree(s);
+   free(s);
 }
 
 static int 
@@ -672,9 +670,9 @@ dev_close(void)
if (dev_mh)
mio_close(dev_mh);
if (dev_mode & SIO_PLAY)
-   xfree(dev_pbuf);
+   free(dev_pbuf);
if (dev_mode & SIO_REC)
-   xfree(dev_rbuf);
+   free(dev_rbuf);
 }
 
 static void
@@ -999,7 +997,7 @@ offline(void)
slot_list_copy(todo, dev_pchan, dev_pbuf);
slot_list_iodo();
}
-   xfree(dev_pbuf);
+   free(dev_pbuf);
while (slot_list)
slot_del(slot_list);
return 1;
@@ -1148,7 +1146,7 @@ playrec(char *dev, int mode, int bufsz, 
 
if (dev_pstate == DEV_START)
dev_mmcstop();
-   xfree(pfds);
+   free(pfds);
dev_close();
while (slot_list)
slot_del(slot_list);
Index: utils.c
===
RCS file: /cvs/src/usr.bin/aucat/utils.c,v
retrieving revision 1.1
diff -u -p -u -r1.1 utils.c
--- utils.c 21 Jan 2015 08:43:55 -  1.1
+++ utils.c 8 Nov 2015 02:16:42 -
@@ -158,15 +158,6 @@ xmalloc(size_t size)
 }
 
 /*
- * free memory allocated with xmalloc()
- */
-void
-xfree(void *p)
-{
-   free(p);
-}
-
-/*
  * xmalloc-style strdup(3)
  */
 char *
Index: utils.h
===
RCS file: /cvs/src/usr.bin/aucat/utils.h,v
retrieving revision 1.1
diff -u -p -u -r1.1 utils.h
--- utils.h 21 Jan 2015 08:43:55 -  1.1
+++ utils.h 8 Nov 2015 02:16:42 -
@@ -29,7 +29,6 @@ void log_flush(void);
 
 void *xmalloc(size_t);
 char *xstrdup(char *);
-void xfree(void *);
 
 /*
  * Log levels:

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

[Michael McConville]

Michael W. Bombardieri wrote:
> On Thu, Nov 05, 2015 at 03:50:29PM +0100, Tobias Stoeckmann wrote:
> > 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)
> 
> Also note that aucat(1)'s utils.c contains xmalloc() and xfree().
> Its version of xfree() contains no special logic so remove it?

ok mmcc@

> Index: abuf.c
> ===
> RCS file: /cvs/src/usr.bin/aucat/abuf.c,v
> retrieving revision 1.26
> diff -u -p -u -r1.26 abuf.c
> --- abuf.c21 Jan 2015 08:43:55 -  1.26
> +++ abuf.c8 Nov 2015 02:16:41 -
> @@ -62,7 +62,7 @@ abuf_done(struct abuf *buf)
>   }
>   }
>  #endif
> - xfree(buf->data);
> + free(buf->data);
>   buf->data = (void *)0xdeadbeef;
>  }
>  
> Index: aucat.c
> ===
> RCS file: /cvs/src/usr.bin/aucat/aucat.c,v
> retrieving revision 1.149
> diff -u -p -u -r1.149 aucat.c
> --- aucat.c   27 Aug 2015 07:25:56 -  1.149
> +++ aucat.c   8 Nov 2015 02:16:42 -
> @@ -214,7 +214,7 @@ slot_new(char *path, int mode, struct ap
>   if (!afile_open(>afile, path, hdr,
>   mode == SIO_PLAY ? AFILE_FREAD : AFILE_FWRITE,
>   par, rate, cmax - cmin + 1)) {
> - xfree(s);
> + free(s);
>   return 0;
>   }
>   s->cmin = cmin;
> @@ -413,15 +413,13 @@ slot_del(struct slot *s)
>   }
>  #endif
>   abuf_done(>buf);
> - if (s->resampbuf)
> - xfree(s->resampbuf);
> - if (s->convbuf)
> - xfree(s->convbuf);
> + free(s->resampbuf);
> + free(s->convbuf);
>   }
>   for (ps = _list; *ps != s; ps = &(*ps)->next)
>   ; /* nothing */
>   *ps = s->next;
> - xfree(s);
> + free(s);
>  }
>  
>  static int 
> @@ -672,9 +670,9 @@ dev_close(void)
>   if (dev_mh)
>   mio_close(dev_mh);
>   if (dev_mode & SIO_PLAY)
> - xfree(dev_pbuf);
> + free(dev_pbuf);
>   if (dev_mode & SIO_REC)
> - xfree(dev_rbuf);
> + free(dev_rbuf);
>  }
>  
>  static void
> @@ -999,7 +997,7 @@ offline(void)
>   slot_list_copy(todo, dev_pchan, dev_pbuf);
>   slot_list_iodo();
>   }
> - xfree(dev_pbuf);
> + free(dev_pbuf);
>   while (slot_list)
>   slot_del(slot_list);
>   return 1;
> @@ -1148,7 +1146,7 @@ playrec(char *dev, int mode, int bufsz, 
>  
>   if (dev_pstate == DEV_START)
>   dev_mmcstop();
> - xfree(pfds);
> + free(pfds);
>   dev_close();
>   while (slot_list)
>   slot_del(slot_list);
> Index: utils.c
> ===
> RCS file: /cvs/src/usr.bin/aucat/utils.c,v
> retrieving revision 1.1
> diff -u -p -u -r1.1 utils.c
> --- utils.c   21 Jan 2015 08:43:55 -  1.1
> +++ utils.c   8 Nov 2015 02:16:42 -
> @@ -158,15 +158,6 @@ xmalloc(size_t size)
>  }
>  
>  /*
> - * free memory allocated with xmalloc()
> - */
> -void
> -xfree(void *p)
> -{
> - free(p);
> -}
> -
> -/*
>   * xmalloc-style strdup(3)
>   */
>  char *
> Index: utils.h
> ===
> RCS file: /cvs/src/usr.bin/aucat/utils.h,v
> retrieving revision 1.1
> diff -u -p -u -r1.1 utils.h
> --- utils.h   21 Jan 2015 08:43:55 -  1.1
> +++ utils.h   8 Nov 2015 02:16:42 -
> @@ -29,7 +29,6 @@ void log_flush(void);
>  
>  void *xmalloc(size_t);
>  char *xstrdup(char *);
> -void xfree(void *);
>  
>  /*
>   * Log levels:
> 

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

[Nicholas Marriott]

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



On Thu, Nov 05, 2015 at 03:50:29PM +0100, Tobias Stoeckmann wrote:
> 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.c25 Sep 2015 16:16:26 -  1.8
> +++ usr.bin/diff/xmalloc.c5 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)
>  
>   

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

[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;
 
   

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

[Nicholas Marriott]

Looks good, ok nicm


On Thu, Nov 05, 2015 at 05:35:22PM +0100, Tobias Stoeckmann wrote:
> 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.c25 Sep 2015 16:16:26 -  1.8
> +++ usr.bin/diff/xmalloc.c5 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",