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 +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);
> > > }
> >
