Re: unify xmalloc (was Re: [patch] cvs: retire xfree())
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())
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 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(&s->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 = &f->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 > retrievi
Re: unify xmalloc (was Re: [patch] cvs: retire xfree())
> > 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(&s->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 = &f->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(stru
Re: unify xmalloc (was Re: [patch] cvs: retire xfree())
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())
> 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())
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(&s->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(&s->buf); > - if (s->resampbuf) > - xfree(s->resampbuf); > - if (s->convbuf) > - xfree(s->convbuf); > + free(s->resampbuf); > + free(s->convbuf); > } > for (ps = &slot_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())
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(&s->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(&s->buf); - if (s->resampbuf) - xfree(s->resampbuf); - if (s->convbuf) - xfree(s->convbuf); + free(s->resampbuf); + free(s->convbuf); } for (ps = &slot_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())
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())
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())
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())
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
Re: unify xmalloc (was Re: [patch] cvs: retire xfree())
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())
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())
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(&s->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(&s->buf); > - if (s->resampbuf) > - xfree(s->resampbuf); > - if (s->convbuf) > - xfree(s->convbuf); > + free(s->resampbuf); > + free(s->convbuf); > } > for (ps = &slot_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())
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(&s->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(&s->buf); - if (s->resampbuf) - xfree(s->resampbuf); - if (s->convbuf) - xfree(s->convbuf); + free(s->resampbuf); + free(s->convbuf); } for (ps = &slot_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())
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())
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 new
Re: unify xmalloc (was Re: [patch] cvs: retire xfree())
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())
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())
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())
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 @@ xmalloc(size_
Re: unify xmalloc (was Re: [patch] cvs: retire xfree())
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) > > > - fatal("xsnpri
Re: unify xmalloc (was Re: [patch] cvs: retire xfree())
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", nmem
Re: unify xmalloc (was Re: [patch] cvs: retire xfree())
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())
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) > > pt
unify xmalloc (was Re: [patch] cvs: retire xfree())
On Thu, Nov 05, 2015 at 09:50:48AM +, Nicholas Marriott wrote: > I don't know why cvs and rcs xmalloc.c has ended up so different. It's not just about cvs and rcs: /usr/src/usr.bin/cvs/xmalloc.c /usr/src/usr.bin/diff/xmalloc.c /usr/src/usr.bin/file/xmalloc.c /usr/src/usr.bin/rcs/xmalloc.c /usr/src/usr.bin/ssh/xmalloc.c /usr/src/usr.bin/tmux/xmalloc.c (probably not same origin) All of them share code parts that almost look identical. Some of them skip tests, do additional tests, test for other return values, or have typos in their error messages (or call err instead of errx, duplicating their messages). This diff would unify them, taking into account that still different style guides apply (tmux) and some use fatal() or errx() with even different return values (diff). Ugh... Index: usr.bin/cvs/xmalloc.c === RCS file: /cvs/src/usr.bin/cvs/xmalloc.c,v retrieving revision 1.12 diff -u -p -u -p -r1.12 xmalloc.c --- usr.bin/cvs/xmalloc.c 5 Nov 2015 09:48:21 - 1.12 +++ usr.bin/cvs/xmalloc.c 5 Nov 2015 14:42:09 - @@ -13,6 +13,7 @@ * called by a name other than "ssh" or "Secure Shell". */ +#include #include #include #include @@ -30,7 +31,7 @@ xmalloc(size_t size) fatal("xmalloc: zero size"); ptr = malloc(size); if (ptr == NULL) - fatal("xmalloc: out of memory (allocating %lu bytes)", (u_long) size); + fatal("xmalloc: out of memory (allocating %zu bytes)", size); return ptr; } @@ -41,12 +42,10 @@ xcalloc(size_t nmemb, size_t size) if (size == 0 || nmemb == 0) fatal("xcalloc: zero size"); - if (SIZE_MAX / nmemb < size) - fatal("xcalloc: nmemb * size > SIZE_MAX"); ptr = calloc(nmemb, size); if (ptr == NULL) - fatal("xcalloc: out of memory (allocating %lu bytes)", - (u_long)(size * nmemb)); + fatal("xcalloc: out of memory (allocating %zu * %zu bytes)", + nmemb, size); return ptr; } @@ -54,28 +53,23 @@ void * xreallocarray(void *ptr, size_t nmemb, size_t size) { void *new_ptr; - size_t new_size = nmemb * size; - if (new_size == 0) - fatal("xrealloc: zero size"); - if (SIZE_MAX / nmemb < size) - fatal("xrealloc: nmemb * size > SIZE_MAX"); - new_ptr = realloc(ptr, new_size); + if (nmemb == 0 || size == 0) + fatal("xreallocarray: zero size"); + new_ptr = reallocarray(ptr, nmemb, size); if (new_ptr == NULL) - fatal("xrealloc: out of memory (new_size %lu bytes)", - (u_long) new_size); + fatal("xreallocarray: out of memory " + "(allocating %zu * %zu bytes)", nmemb, size); return new_ptr; } char * xstrdup(const char *str) { - size_t len; char *cp; - len = strlen(str) + 1; - cp = xmalloc(len); - strlcpy(cp, str, len); + if ((cp = strdup(str)) == NULL) + fatal("xstrdup: could not allocate memory"); return cp; } @@ -96,17 +90,20 @@ xasprintf(char **ret, const char *fmt, . } int -xsnprintf(char *str, size_t size, const char *fmt, ...) +xsnprintf(char *str, size_t len, const char *fmt, ...) { va_list ap; int i; + if (len > INT_MAX) + fatal("xsnprintf: len > INT_MAX"); + va_start(ap, fmt); - i = vsnprintf(str, size, fmt, ap); + i = vsnprintf(str, len, fmt, ap); va_end(ap); - if (i == -1 || i >= (int)size) - fatal("xsnprintf: overflow"); + if (i < 0 || i >= (int)len) + fatal("xsnprintf: could not allocate memory"); return (i); } Index: usr.bin/diff/xmalloc.c === RCS file: /cvs/src/usr.bin/diff/xmalloc.c,v retrieving revision 1.8 diff -u -p -u -p -r1.8 xmalloc.c --- usr.bin/diff/xmalloc.c 25 Sep 2015 16:16:26 - 1.8 +++ usr.bin/diff/xmalloc.c 5 Nov 2015 14:42:09 - @@ -27,9 +27,11 @@ xmalloc(size_t size) { void *ptr; + if (size == 0) + errx(2, "xmalloc: zero size"); ptr = malloc(size); if (ptr == NULL) - err(2, "xmalloc %zu", size); + errx(2, "xmalloc: out of memory (allocating %zu bytes)", size); return ptr; } @@ -40,7 +42,7 @@ xcalloc(size_t nmemb, size_t size) ptr = calloc(nmemb, size); if (ptr == NULL) - err(2, "xcalloc: out of memory (allocating %zu*%zu bytes)", + errx(2, "xcalloc: out of memory (allocating %zu * %zu bytes)", nmemb, size); return ptr; } @@ -52,7 +54,8 @@ xreallocarray(void *ptr, size_t nmemb, s new_ptr = reallocarray(ptr, nmemb, size); if (new_ptr == NULL) - err(2, "
Re: [patch] cvs: retire xfree()
Applied, thanks. I don't know why cvs and rcs xmalloc.c has ended up so different. On Thu, Nov 05, 2015 at 11:50:51AM +0800, Michael W. Bombardieri wrote: > Hi tech@, > > Function xfree() was previously removed from rcs, so drop it from > opencvs too... > > http://cvsweb.openbsd.org/cgi-bin/cvsweb/src/usr.bin/rcs/xmalloc.c?f=h#rev1.9 > > Footnote: > I noticed that rcsnum_free() is just free() so maybe that could be > removed also (not included in this patch). > > - Michael > > > Index: add.c > === > RCS file: /cvs/src/usr.bin/cvs/add.c,v > retrieving revision 1.111 > diff -u -p -u -r1.111 add.c > --- add.c 16 Jan 2015 06:40:06 - 1.111 > +++ add.c 5 Nov 2015 02:49:20 - > @@ -20,6 +20,7 @@ > > #include > #include > +#include > #include > #include > > @@ -146,7 +147,7 @@ cvs_add_entry(struct cvs_file *cf) > entlist = cvs_ent_open(cf->file_wd); > cvs_ent_add(entlist, entry); > > - xfree(entry); > + free(entry); > } else { > add_entry(cf); > } > @@ -252,7 +253,7 @@ cvs_add_tobranch(struct cvs_file *cf, ch > (void)xsnprintf(attic, PATH_MAX, "%s/%s/%s%s", repo, > CVS_PATH_ATTIC, cf->file_name, RCS_FILE_EXT); > > - xfree(cf->file_rpath); > + free(cf->file_rpath); > cf->file_rpath = xstrdup(attic); > > cf->repo_fd = open(cf->file_rpath, O_CREAT|O_RDONLY); > @@ -277,7 +278,7 @@ cvs_add_tobranch(struct cvs_file *cf, ch > if (rcs_rev_add(cf->file_rcs, RCS_HEAD_REV, msg, -1, NULL) == -1) > fatal("cvs_add_tobranch: failed to create first branch " > "revision"); > - xfree(msg); > + free(msg); > > if (rcs_findrev(cf->file_rcs, cf->file_rcs->rf_head) == NULL) > fatal("cvs_add_tobranch: cannot find newly added revision"); > @@ -359,7 +360,7 @@ add_directory(struct cvs_file *cf) > > entlist = cvs_ent_open(cf->file_wd); > cvs_ent_add(entlist, p); > - xfree(p); > + free(p); > } > } > > @@ -381,10 +382,8 @@ add_directory(struct cvs_file *cf) > } > cvs_printf("%s\n", msg); > > - if (tag != NULL) > - xfree(tag); > - if (date != NULL) > - xfree(date); > + free(tag); > + free(date); > > cvs_get_repository_name(cf->file_path, repo, PATH_MAX); > line_list = cvs_trigger_getlines(CVS_PATH_LOGINFO, repo); > @@ -400,8 +399,7 @@ add_directory(struct cvs_file *cf) > > cvs_trigger_freeinfo(&files_info); > cvs_trigger_freelist(line_list); > - if (loginfo != NULL) > - xfree(loginfo); > + free(loginfo); > } > } > > @@ -564,5 +562,5 @@ add_entry(struct cvs_file *cf) > entlist = cvs_ent_open(cf->file_wd); > cvs_ent_add(entlist, entry); > } > - xfree(entry); > + free(entry); > } > Index: admin.c > === > RCS file: /cvs/src/usr.bin/cvs/admin.c,v > retrieving revision 1.65 > diff -u -p -u -r1.65 admin.c > --- admin.c 16 Jan 2015 06:40:06 - 1.65 > +++ admin.c 5 Nov 2015 02:49:20 - > @@ -23,6 +23,7 @@ > #include > #include > #include > +#include > #include > #include > > @@ -304,8 +305,8 @@ cvs_admin_local(struct cvs_file *cf) > while (!TAILQ_EMPTY(&(cf->file_rcs->rf_access))) { > rap = TAILQ_FIRST(&(cf->file_rcs->rf_access)); > TAILQ_REMOVE(&(cf->file_rcs->rf_access), rap, ra_list); > - xfree(rap->ra_name); > - xfree(rap); > + free(rap->ra_name); > + free(rap); > } > /* no synced anymore */ > cf->file_rcs->rf_flags &= ~RCS_SYNCED; > Index: annotate.c > === > RCS file: /cvs/src/usr.bin/cvs/annotate.c,v > retrieving revision 1.64 > diff -u -p -u -r1.64 annotate.c > --- annotate.c16 Jan 2015 06:40:06 - 1.64 > +++ annotate.c5 Nov 2015 02:49:20 - > @@ -235,7 +235,7 @@ cvs_annotate_local(struct cvs_file *cf) > p[line->l_len] = '\0'; > > if (line->l_needsfree) > - xfree(line->l_line); > + free(line->l_line); > line->l_line = p; > line->l_len++; > line->l_needsfree = 1; > @@ -244,9 +244,9 @@ cvs_annotate_local(struct cvs_file *cf) > line->l_delta->rd_author, date, line->l_line); > > i
Re: [patch] cvs: retire xfree()
> Good catch, thanks. > > My eyes glazed over a little while reviewing this, but tentative ok > mmcc@ > > Below is what I consider a better refactoring of buf_free(). It makes it > NULL-safe and has more classic free function logic. I didn't include buf_free() in my patch but that looks good. > > Footnote: > > I noticed that rcsnum_free() is just free() so maybe that could be > > removed also (not included in this patch). > > I'll have to look, but this sounds like a good idea to me. In rcs(1) rcsnum_free() is more complicated since rn_id is dynamically allocated. > Index: buf.c > === > RCS file: /cvs/src/usr.bin/cvs/buf.c,v > retrieving revision 1.82 > diff -u -p -r1.82 buf.c > --- buf.c 5 Feb 2015 12:59:57 - 1.82 > +++ buf.c 5 Nov 2015 05:08:57 - > @@ -119,15 +119,15 @@ buf_load_fd(int fd) > void > buf_free(BUF *b) > { > - if (b->cb_buf != NULL) > - xfree(b->cb_buf); > - xfree(b); > + if (b != NULL) { > + free(b->cb_buf); > + free(b); > + } > } > > /* > * Free the buffer 's structural information but do not free the contents > - * of the buffer. Instead, they are returned and should be freed later using > - * xfree(). > + * of the buffer. Instead, they are returned and should be freed later. > */ > void * > buf_release(BUF *b) > @@ -135,7 +135,7 @@ buf_release(BUF *b) > void *tmp; > > tmp = b->cb_buf; > - xfree(b); > + free(b); > return (tmp); > } >
Re: [patch] cvs: retire xfree()
Michael W. Bombardieri wrote: > Hi tech@, > > Function xfree() was previously removed from rcs, so drop it from > opencvs too... > > http://cvsweb.openbsd.org/cgi-bin/cvsweb/src/usr.bin/rcs/xmalloc.c?f=h#rev1.9 Good catch, thanks. My eyes glazed over a little while reviewing this, but tentative ok mmcc@ Below is what I consider a better refactoring of buf_free(). It makes it NULL-safe and has more classic free function logic. > Footnote: > I noticed that rcsnum_free() is just free() so maybe that could be > removed also (not included in this patch). I'll have to look, but this sounds like a good idea to me. Index: buf.c === RCS file: /cvs/src/usr.bin/cvs/buf.c,v retrieving revision 1.82 diff -u -p -r1.82 buf.c --- buf.c 5 Feb 2015 12:59:57 - 1.82 +++ buf.c 5 Nov 2015 05:08:57 - @@ -119,15 +119,15 @@ buf_load_fd(int fd) void buf_free(BUF *b) { - if (b->cb_buf != NULL) - xfree(b->cb_buf); - xfree(b); + if (b != NULL) { + free(b->cb_buf); + free(b); + } } /* * Free the buffer 's structural information but do not free the contents - * of the buffer. Instead, they are returned and should be freed later using - * xfree(). + * of the buffer. Instead, they are returned and should be freed later. */ void * buf_release(BUF *b) @@ -135,7 +135,7 @@ buf_release(BUF *b) void *tmp; tmp = b->cb_buf; - xfree(b); + free(b); return (tmp); }
[patch] cvs: retire xfree()
Hi tech@, Function xfree() was previously removed from rcs, so drop it from opencvs too... http://cvsweb.openbsd.org/cgi-bin/cvsweb/src/usr.bin/rcs/xmalloc.c?f=h#rev1.9 Footnote: I noticed that rcsnum_free() is just free() so maybe that could be removed also (not included in this patch). - Michael Index: add.c === RCS file: /cvs/src/usr.bin/cvs/add.c,v retrieving revision 1.111 diff -u -p -u -r1.111 add.c --- add.c 16 Jan 2015 06:40:06 - 1.111 +++ add.c 5 Nov 2015 02:49:20 - @@ -20,6 +20,7 @@ #include #include +#include #include #include @@ -146,7 +147,7 @@ cvs_add_entry(struct cvs_file *cf) entlist = cvs_ent_open(cf->file_wd); cvs_ent_add(entlist, entry); - xfree(entry); + free(entry); } else { add_entry(cf); } @@ -252,7 +253,7 @@ cvs_add_tobranch(struct cvs_file *cf, ch (void)xsnprintf(attic, PATH_MAX, "%s/%s/%s%s", repo, CVS_PATH_ATTIC, cf->file_name, RCS_FILE_EXT); - xfree(cf->file_rpath); + free(cf->file_rpath); cf->file_rpath = xstrdup(attic); cf->repo_fd = open(cf->file_rpath, O_CREAT|O_RDONLY); @@ -277,7 +278,7 @@ cvs_add_tobranch(struct cvs_file *cf, ch if (rcs_rev_add(cf->file_rcs, RCS_HEAD_REV, msg, -1, NULL) == -1) fatal("cvs_add_tobranch: failed to create first branch " "revision"); - xfree(msg); + free(msg); if (rcs_findrev(cf->file_rcs, cf->file_rcs->rf_head) == NULL) fatal("cvs_add_tobranch: cannot find newly added revision"); @@ -359,7 +360,7 @@ add_directory(struct cvs_file *cf) entlist = cvs_ent_open(cf->file_wd); cvs_ent_add(entlist, p); - xfree(p); + free(p); } } @@ -381,10 +382,8 @@ add_directory(struct cvs_file *cf) } cvs_printf("%s\n", msg); - if (tag != NULL) - xfree(tag); - if (date != NULL) - xfree(date); + free(tag); + free(date); cvs_get_repository_name(cf->file_path, repo, PATH_MAX); line_list = cvs_trigger_getlines(CVS_PATH_LOGINFO, repo); @@ -400,8 +399,7 @@ add_directory(struct cvs_file *cf) cvs_trigger_freeinfo(&files_info); cvs_trigger_freelist(line_list); - if (loginfo != NULL) - xfree(loginfo); + free(loginfo); } } @@ -564,5 +562,5 @@ add_entry(struct cvs_file *cf) entlist = cvs_ent_open(cf->file_wd); cvs_ent_add(entlist, entry); } - xfree(entry); + free(entry); } Index: admin.c === RCS file: /cvs/src/usr.bin/cvs/admin.c,v retrieving revision 1.65 diff -u -p -u -r1.65 admin.c --- admin.c 16 Jan 2015 06:40:06 - 1.65 +++ admin.c 5 Nov 2015 02:49:20 - @@ -23,6 +23,7 @@ #include #include #include +#include #include #include @@ -304,8 +305,8 @@ cvs_admin_local(struct cvs_file *cf) while (!TAILQ_EMPTY(&(cf->file_rcs->rf_access))) { rap = TAILQ_FIRST(&(cf->file_rcs->rf_access)); TAILQ_REMOVE(&(cf->file_rcs->rf_access), rap, ra_list); - xfree(rap->ra_name); - xfree(rap); + free(rap->ra_name); + free(rap); } /* no synced anymore */ cf->file_rcs->rf_flags &= ~RCS_SYNCED; Index: annotate.c === RCS file: /cvs/src/usr.bin/cvs/annotate.c,v retrieving revision 1.64 diff -u -p -u -r1.64 annotate.c --- annotate.c 16 Jan 2015 06:40:06 - 1.64 +++ annotate.c 5 Nov 2015 02:49:20 - @@ -235,7 +235,7 @@ cvs_annotate_local(struct cvs_file *cf) p[line->l_len] = '\0'; if (line->l_needsfree) - xfree(line->l_line); + free(line->l_line); line->l_line = p; line->l_len++; line->l_needsfree = 1; @@ -244,9 +244,9 @@ cvs_annotate_local(struct cvs_file *cf) line->l_delta->rd_author, date, line->l_line); if (line->l_needsfree) - xfree(line->l_line); - xfree(line); + free(line->l_line); + free(line); } - xfree(alines); + free(alines); } Index: buf.c === R