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 +0000, 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 -0000 1.12
> > +++ usr.bin/cvs/xmalloc.c 5 Nov 2015 16:32:21 -0000
> > @@ -13,6 +13,7 @@
> > * called by a name other than "ssh" or "Secure Shell".
> > */
> >
> > +#include <limits.h>
> > #include <stdint.h>
> > #include <stdio.h>
> > #include <stdlib.h>
> > @@ -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 -0000 1.8
> > +++ usr.bin/diff/xmalloc.c 5 Nov 2015 16:32:21 -0000
> > @@ -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;
> >
> > if ((cp = strdup(str)) == NULL)
> > - err(1, "xstrdup");
> > + errx(2, "xstrdup: could not allocate memory");
> > return cp;
> > }
> >
> > @@ -77,7 +80,7 @@ xasprintf(char **ret, const char *fmt, .
> > va_end(ap);
> >
> > if (i < 0 || *ret == NULL)
> > - err(2, "xasprintf");
> > + errx(2, "xasprintf: could not allocate memory");
> >
> > - return (i);
> > + return i;
> > }
> > Index: usr.bin/diff/xmalloc.h
> > ===================================================================
> > RCS file: /cvs/src/usr.bin/diff/xmalloc.h,v
> > retrieving revision 1.3
> > diff -u -p -u -p -r1.3 xmalloc.h
> > --- usr.bin/diff/xmalloc.h 29 Apr 2015 04:00:25 -0000 1.3
> > +++ usr.bin/diff/xmalloc.h 5 Nov 2015 16:32:21 -0000
> > @@ -22,7 +22,6 @@
> > void *xmalloc(size_t);
> > void *xcalloc(size_t, size_t);
> > void *xreallocarray(void *, size_t, size_t);
> > -void xfree(void *);
> > char *xstrdup(const char *);
> > int xasprintf(char **, const char *, ...)
> > __attribute__((__format__ (printf, 2, 3)))
> > Index: usr.bin/file/xmalloc.c
> > ===================================================================
> > RCS file: /cvs/src/usr.bin/file/xmalloc.c,v
> > retrieving revision 1.2
> > diff -u -p -u -p -r1.2 xmalloc.c
> > --- usr.bin/file/xmalloc.c 17 Jun 2015 18:51:11 -0000 1.2
> > +++ usr.bin/file/xmalloc.c 5 Nov 2015 16:32:21 -0000
> > @@ -31,9 +31,7 @@ xmalloc(size_t size)
> > errx(1, "xmalloc: zero size");
> > ptr = malloc(size);
> > if (ptr == NULL)
> > - errx(1,
> > - "xmalloc: out of memory (allocating %zu bytes)",
> > - size);
> > + errx(1, "xmalloc: out of memory (allocating %zu bytes)", size);
> > return ptr;
> > }
> >
> > @@ -44,12 +42,10 @@ xcalloc(size_t nmemb, size_t size)
> >
> > if (size == 0 || nmemb == 0)
> > errx(1, "xcalloc: zero size");
> > - if (SIZE_MAX / nmemb < size)
> > - errx(1, "xcalloc: nmemb * size > SIZE_MAX");
> > ptr = calloc(nmemb, size);
> > if (ptr == NULL)
> > - errx(1, "xcalloc: out of memory (allocating %zu bytes)",
> > - (size * nmemb));
> > + errx(1, "xcalloc: out of memory (allocating %zu * %zu bytes)",
> > + nmemb, size);
> > return ptr;
> > }
> >
> > @@ -60,8 +56,8 @@ xreallocarray(void *ptr, size_t nmemb, s
> >
> > new_ptr = reallocarray(ptr, nmemb, size);
> > if (new_ptr == NULL)
> > - errx(1, "xreallocarray: out of memory (new_size %zu bytes)",
> > - nmemb * size);
> > + errx(1, "xreallocarray: out of memory "
> > + "(allocating %zu * %zu bytes)", nmemb, size);
> > return new_ptr;
> > }
> >
> > @@ -71,7 +67,7 @@ xstrdup(const char *str)
> > char *cp;
> >
> > if ((cp = strdup(str)) == NULL)
> > - err(1, "xstrdup");
> > + errx(1, "xstrdup: could not allocate memory");
> > return cp;
> > }
> >
> > @@ -88,5 +84,5 @@ xasprintf(char **ret, const char *fmt, .
> > if (i < 0 || *ret == NULL)
> > errx(1, "xasprintf: could not allocate memory");
> >
> > - return (i);
> > + return i;
> > }
> > Index: usr.bin/file/xmalloc.h
> > ===================================================================
> > RCS file: /cvs/src/usr.bin/file/xmalloc.h,v
> > retrieving revision 1.2
> > diff -u -p -u -p -r1.2 xmalloc.h
> > --- usr.bin/file/xmalloc.h 20 Jul 2015 08:51:41 -0000 1.2
> > +++ usr.bin/file/xmalloc.h 5 Nov 2015 16:32:21 -0000
> > @@ -27,4 +27,4 @@ int xasprintf(char **, const char *, ..
> > __attribute__((__format__ (printf, 2, 3)))
> > __attribute__((__nonnull__ (2)));
> >
> > -#endif /* XMALLOC_H */
> > +#endif /* XMALLOC_H */
> > Index: usr.bin/rcs/xmalloc.c
> > ===================================================================
> > RCS file: /cvs/src/usr.bin/rcs/xmalloc.c,v
> > retrieving revision 1.10
> > diff -u -p -u -p -r1.10 xmalloc.c
> > --- usr.bin/rcs/xmalloc.c 17 Jun 2015 20:50:10 -0000 1.10
> > +++ usr.bin/rcs/xmalloc.c 5 Nov 2015 16:32:21 -0000
> > @@ -31,9 +31,7 @@ xmalloc(size_t size)
> > errx(1, "xmalloc: zero size");
> > ptr = malloc(size);
> > if (ptr == NULL)
> > - errx(1,
> > - "xmalloc: out of memory (allocating %zu bytes)",
> > - size);
> > + errx(1, "xmalloc: out of memory (allocating %zu bytes)", size);
> > return ptr;
> > }
> >
> > @@ -44,12 +42,10 @@ xcalloc(size_t nmemb, size_t size)
> >
> > if (size == 0 || nmemb == 0)
> > errx(1, "xcalloc: zero size");
> > - if (SIZE_MAX / nmemb < size)
> > - errx(1, "xcalloc: nmemb * size > SIZE_MAX");
> > ptr = calloc(nmemb, size);
> > if (ptr == NULL)
> > - errx(1, "xcalloc: out of memory (allocating %zu bytes)",
> > - (size * nmemb));
> > + errx(1, "xcalloc: out of memory (allocating %zu * %zu bytes)",
> > + nmemb, size);
> > return ptr;
> > }
> >
> > @@ -60,8 +56,8 @@ xreallocarray(void *ptr, size_t nmemb, s
> >
> > new_ptr = reallocarray(ptr, nmemb, size);
> > if (new_ptr == NULL)
> > - errx(1, "xreallocarray: out of memory (new_size %zu bytes)",
> > - nmemb * size);
> > + errx(1, "xreallocarray: out of memory "
> > + "(allocating %zu * %zu bytes)", nmemb, size);
> > return new_ptr;
> > }
> >
> > @@ -71,7 +67,7 @@ xstrdup(const char *str)
> > char *cp;
> >
> > if ((cp = strdup(str)) == NULL)
> > - err(1, "xstrdup");
> > + errx(1, "xstrdup: could not allocate memory");
> > return cp;
> > }
> >
> > @@ -88,5 +84,5 @@ xasprintf(char **ret, const char *fmt, .
> > if (i < 0 || *ret == NULL)
> > errx(1, "xasprintf: could not allocate memory");
> >
> > - return (i);
> > + return i;
> > }
> > Index: usr.bin/rcs/xmalloc.h
> > ===================================================================
> > RCS file: /cvs/src/usr.bin/rcs/xmalloc.h,v
> > retrieving revision 1.3
> > diff -u -p -u -p -r1.3 xmalloc.h
> > --- usr.bin/rcs/xmalloc.h 13 Jun 2015 20:15:21 -0000 1.3
> > +++ usr.bin/rcs/xmalloc.h 5 Nov 2015 16:32:21 -0000
> > @@ -27,4 +27,4 @@ int xasprintf(char **, const char *, ..
> > __attribute__((__format__ (printf, 2, 3)))
> > __attribute__((__nonnull__ (2)));
> >
> > -#endif /* XMALLOC_H */
> > +#endif /* XMALLOC_H */
> > Index: usr.bin/ssh/xmalloc.c
> > ===================================================================
> > RCS file: /cvs/src/usr.bin/ssh/xmalloc.c,v
> > retrieving revision 1.32
> > diff -u -p -u -p -r1.32 xmalloc.c
> > --- usr.bin/ssh/xmalloc.c 24 Apr 2015 01:36:01 -0000 1.32
> > +++ usr.bin/ssh/xmalloc.c 5 Nov 2015 16:32:21 -0000
> > @@ -42,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 %zu bytes)",
> > - size * nmemb);
> > + fatal("xcalloc: out of memory (allocating %zu * %zu bytes)",
> > + nmemb, size);
> > return ptr;
> > }
> >
> > @@ -58,20 +56,18 @@ xreallocarray(void *ptr, size_t nmemb, s
> >
> > new_ptr = reallocarray(ptr, nmemb, size);
> > if (new_ptr == NULL)
> > - fatal("xreallocarray: out of memory (%zu elements of %zu
> > bytes)",
> > - nmemb, 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;
> > }
> >
> > @@ -88,5 +84,5 @@ xasprintf(char **ret, const char *fmt, .
> > if (i < 0 || *ret == NULL)
> > fatal("xasprintf: could not allocate memory");
> >
> > - return (i);
> > + return i;
> > }
> > Index: usr.bin/ssh/xmalloc.h
> > ===================================================================
> > RCS file: /cvs/src/usr.bin/ssh/xmalloc.h,v
> > retrieving revision 1.15
> > diff -u -p -u -p -r1.15 xmalloc.h
> > --- usr.bin/ssh/xmalloc.h 24 Apr 2015 01:36:01 -0000 1.15
> > +++ usr.bin/ssh/xmalloc.h 5 Nov 2015 16:32:21 -0000
> > @@ -16,6 +16,9 @@
> > * called by a name other than "ssh" or "Secure Shell".
> > */
> >
> > +#ifndef XMALLOC_H
> > +#define XMALLOC_H
> > +
> > void *xmalloc(size_t);
> > void *xcalloc(size_t, size_t);
> > void *xreallocarray(void *, size_t, size_t);
> > @@ -23,3 +26,5 @@ char *xstrdup(const char *);
> > int xasprintf(char **, const char *, ...)
> > __attribute__((__format__ (printf, 2, 3)))
> > __attribute__((__nonnull__ (2)));
> > +
> > +#endif /* XMALLOC_H */
> > Index: usr.bin/tmux/xmalloc.c
> > ===================================================================
> > RCS file: /cvs/src/usr.bin/tmux/xmalloc.c,v
> > retrieving revision 1.7
> > diff -u -p -u -p -r1.7 xmalloc.c
> > --- usr.bin/tmux/xmalloc.c 20 Oct 2014 23:57:14 -0000 1.7
> > +++ usr.bin/tmux/xmalloc.c 5 Nov 2015 16:32:21 -0000
> > @@ -25,16 +25,13 @@
> > #include "tmux.h"
> >
> > char *
> > -xstrdup(const char *s)
> > +xstrdup(const char *str)
> > {
> > - char *ptr;
> > - size_t len;
> > + char *cp;
> >
> > - len = strlen(s) + 1;
> > - ptr = xmalloc(len);
> > -
> > - strlcpy(ptr, s, len);
> > - return (ptr);
> > + if ((cp = strdup(str)) == NULL)
> > + fatalx("xstrdup: could not allocate memory");
> > + return (cp);
> > }
> >
> > void *
> > @@ -43,11 +40,10 @@ xcalloc(size_t nmemb, size_t size)
> > void *ptr;
> >
> > if (size == 0 || nmemb == 0)
> > - fatalx("zero size");
> > - if (SIZE_MAX / nmemb < size)
> > - fatalx("nmemb * size > SIZE_MAX");
> > + fatalx("xcalloc: zero size");
> > if ((ptr = calloc(nmemb, size)) == NULL)
> > - fatal("xcalloc failed");
> > + log_fatalx("xcalloc: out of memory (allocating %zu bytes)",
> > + size);
> >
> > return (ptr);
> > }
> > @@ -58,9 +54,10 @@ xmalloc(size_t size)
> > void *ptr;
> >
> > if (size == 0)
> > - fatalx("zero size");
> > + fatalx("xmalloc: zero size");
> > if ((ptr = malloc(size)) == NULL)
> > - fatal("xmalloc failed");
> > + log_fatalx("xmalloc: out of memory (allocating %zu bytes)",
> > + size);
> >
> > return (ptr);
> > }
> > @@ -71,9 +68,10 @@ xrealloc(void *oldptr, size_t newsize)
> > void *newptr;
> >
> > if (newsize == 0)
> > - fatalx("zero size");
> > + fatalx("xrealloc: zero size");
> > if ((newptr = realloc(oldptr, newsize)) == NULL)
> > - fatal("xrealloc failed");
> > + log_fatalx("xrealloc: out of memory (allocating %zu bytes)",
> > + newsize);
> >
> > return (newptr);
> > }
> > @@ -81,15 +79,13 @@ xrealloc(void *oldptr, size_t newsize)
> > void *
> > xreallocarray(void *oldptr, size_t nmemb, size_t size)
> > {
> > - size_t newsize = nmemb * size;
> > void *newptr;
> >
> > - if (newsize == 0)
> > - fatalx("zero size");
> > - if (SIZE_MAX / nmemb < size)
> > - fatalx("nmemb * size > SIZE_MAX");
> > - if ((newptr = realloc(oldptr, newsize)) == NULL)
> > - fatal("xreallocarray failed");
> > + if (nmemb == 0 || size == 0)
> > + fatalx("xreallocarray: zero size");
> > + if ((newptr = reallocarray(oldptr, nmemb, size)) == NULL)
> > + log_fatalx("xreallocarray: out of memory "
> > + "(allocating %zu * %zu bytes)", nmemb, size);
> >
> > return (newptr);
> > }
> > @@ -114,7 +110,7 @@ xvasprintf(char **ret, const char *fmt,
> >
> > i = vasprintf(ret, fmt, ap);
> > if (i < 0 || *ret == NULL)
> > - fatal("xvasprintf failed");
> > + fatalx("xvasprintf: could not allocate memory");
> >
> > return (i);
> > }
> > @@ -138,11 +134,11 @@ xvsnprintf(char *buf, size_t len, const
> > int i;
> >
> > if (len > INT_MAX)
> > - fatalx("len > INT_MAX");
> > + fatalx("xvsnprintf: len > INT_MAX");
> >
> > i = vsnprintf(buf, len, fmt, ap);
> > - if (i < 0)
> > - fatal("vsnprintf failed");
> > + if (i < 0 || i >= (int)len)
> > + fatalx("xvsnprintf: could not allocate memory");
> >
> > return (i);
> > }
>
