Re: [PATCH] bisect: save heap memory. allocate only the required amount

From: Jeff King <peff@peff.net>
Date: Aug 25, 2014, 13:07 UTC
Message-ID: <20140825130732.GD17288@peff.net>
In-Reply-To: <CAPc5daWheSH8E-PycSUq2Coqp19t_+_6TuBEOKhK4QwsEtzkkA@mail.gmail.com>

On Sun, Aug 24, 2014 at 04:39:37PM -0700, Junio C Hamano wrote:

Show 20 quoted lines

> On Sun, Aug 24, 2014 at 8:10 AM, Stefan Beller <stefanbeller@gmail.com> wrote:
> >>       for (p = list, i = 0; i < cnt; i++) {
> >> -             struct name_decoration *r = xmalloc(sizeof(*r) + 100);
> >> +             char name[100];
> >
> > Would it make sense to convert the 'name' into a git strbuf?
> > Please have a look at Documentation/technical/api-strbuf.txt
> 
> Why not go one step further and format it twice, once only
> to measure the necessary size to allocate, allocate and
> then format into it for real? Then you do not need to print
> into a temporary strbuf, copy the result and free the strbuf,
> no?
> 
> BUT.
> 
> The string will always be "dist=" followed by decimal representation of
> a count that fits in "int" anyway, so I actually think use of strbuf is way
> overkill (and formatting it twice also is); the patch as posted should be
> just fine.

I think you are right, and the patch is the right direction (assuming we want to do this; I question whether there are enough elements in the list for us to care about the size, and if there are, we are probably better off storing the int and formatting the strings on the fly).

I wonder if there is a way we could get rid of the magic "100" here, though. Its meaning is "enough to hold 'dist=' and any integer". But you have to read carefully to see that this call to sprintf is not a buffer overflow. A strbuf is one way to get rid of it, though it is awkward because we then have to copy the result into a flex-array structure.

It would be nice if there was some way to abstract the idea of formatting a buffer directly into a flex-array. That would involve the double-format you mention, but we could use it in lots of places to make the code nicer. Maybe like:

  void *fmt_flex_array(size_t base, const char *fmt, ...)
  {
          va_list ap;
	  size_t flex;
	  unsigned char *ret;

	  va_start(ap, fmt);
	  flex = vsnprintf(NULL, 0, fmt, ap);
	  va_end(ap);

	  ret = xmalloc(base + flex + 1);
	  va_start(ap, fmt);
	  /* Eek, see below */
	  vsnprintf(ret + flex, flex + 1, fmt, ap);

	  return ret;
  }

and you'd call it like:

  struct name_decoration *r = fmt_flex_array(sizeof(*r), "dist=%d", x);

Except that I don't think we are guaranteed that offsetof(mystruct, flex_member) is equal to sizeof(mystruct). If FLEX_ARRAY is 0, it should be, but some platforms use FLEX_ARRAY=1. So you'd have to pass in the offset like:

  struct name_decoration *r = fmt_flex_array(sizeof(*r),
                                             offsetof(*r, name),
					     "dist=%d", x);

which is a little less nice. You could make it nicer with a macro, but we don't assume variadic macros. <sigh>

-Peff