From: Jeff King <hidden> Date: 2016-06-15 23:08:14
About 6 months or so ago, I did an audit of git's code base for uses of
strcpy and sprintf that could overflow, fixing any bugs and cleaning up
any suspect spots to make further audits simpler. This is a
continuation of that work, for size computations which can overflow and
cause us to allocate a too-small buffer. E.g., something like:
char *concat(const char *a, const char *b)
{
unsigned len_a = strlen(a);
unsigned len_b = strlen(b);
char *ret = xmalloc(len_a + len_b);
memcpy(ret, a, len_a);
memcpy(ret, b, len_b);
}
will behave badly if the sum of "a" and "b" overflows "unsigned". There
are other variants, too (we are also truncating the return value from
strlen, and we'd frequently use "int" here, so the lengths can actually
be negative!). It also varies based on platform. If the sites use size_t
instead of int, then 64-bit systems are typically hard to trigger in
practice (just because you'd need petabytes to store "a" and "b" in the
first place).
The only bug I have actually confirmed in practice here is fixed by
patch 2 (which is why it's at the front). There's another one in
path_name(), but that function is already dropped by the nearby
jk/lose-name-path topic.
The rest are cleanups of spots which _might_ be buggy, but I didn't dig
too far to find out. As with the earlier audit, I tried to refactor
using helpers that make the code clearer and less error-prone. So maybe
they're fixing bugs or not, but they certainly make it easier to audit
the result for problems.
[01/18]: add helpers for detecting size_t overflow
[02/18]: tree-diff: catch integer overflow in combine_diff_path allocation
[03/18]: harden REALLOC_ARRAY and xcalloc against size_t overflow
[04/18]: add helpers for allocating flex-array structs
[05/18]: convert trivial cases to ALLOC_ARRAY
[06/18]: use xmallocz to avoid size arithmetic
[07/18]: convert trivial cases to FLEX_ARRAY macros
[08/18]: use st_add and st_mult for allocation size computation
[09/18]: write_untracked_extension: use FLEX_ALLOC helper
[10/18]: fast-import: simplify allocation in start_packfile
[11/18]: fetch-pack: simplify add_sought_entry
[12/18]: test-path-utils: fix normalize_path_copy output buffer size
[13/18]: sequencer: simplify memory allocation of get_message
[14/18]: git-compat-util: drop mempcpy compat code
[15/18]: transport_anonymize_url: use xstrfmt
[16/18]: diff_populate_gitlink: use a strbuf
[17/18]: convert ewah/bitmap code to use xmalloc
[18/18]: ewah: convert to REALLOC_ARRAY, etc
-Peff
From: Jeff King <hidden> Date: 2016-06-15 23:08:14
Performing computations on size_t variables that we feed to
xmalloc and friends can be dangerous, as an integer overflow
can cause us to allocate a much smaller chunk than we
realized.
We already have unsigned_add_overflows(), but let's add
unsigned_mult_overflows() to that. Furthermore, rather than
have each site manually check and die on overflow, we can
provide some helpers that will:
- promote the arguments to size_t, so that we know we are
doing our computation in the same size of integer that
will ultimately be fed to xmalloc
- check and die on overflow
- return the result so that computations can be done in
the parameter list of xmalloc.
These functions are a lot uglier to use than normal
arithmetic operators (you have to do "st_add(foo, bar)"
instead of "foo + bar"). To at least limit the damage, we
also provide multi-valued versions. So rather than:
st_add(st_add(a, b), st_add(c, d));
you can write:
st_add4(a, b, c, d);
This isn't nearly as elegant as a varargs function, but it's
a lot harder to get it wrong. You don't have to remember to
add a sentinel value at the end, and the compiler will
complain if you get the number of arguments wrong. This
patch adds only the numbered variants required to convert
the current code base; we can easily add more later if
needed.
Signed-off-by: Jeff King <redacted>
---
The st_* names aren't amazing, but I think they mostly work. Suggestions
welcome, but please keep bikeshedding to a minimum. :)
I almost went with checked_add(), checked_mult(), etc. But these
inherently promote their arguments to size_t, so:
int x = checked_add(a, b);
is buggy; the user _should_ be reminded of the result type in each call.
These could also build on compiler intrinsics for extra speed. I'm happy
to do that as a follow-up, but I doubt it really matters in practice.
We're about to call malloc() in all cases, so an extra integer
computation is almost certainly irrelevant. So I went for simplicity to
start with.
git-compat-util.h | 34 ++++++++++++++++++++++++++++++++++
1 file changed, 34 insertions(+)
From: Jeff King <hidden> Date: 2016-06-15 23:08:14
A combine_diff_path struct has two "flex" members allocated
alongside the struct: a string to hold the pathname, and an
array of parent pointers. We use an "int" to compute this,
meaning we may easily overflow it if the pathname is
extremely long.
We can fix this by using size_t, and checking for overflow
with the st_add helper.
Signed-off-by: Jeff King <redacted>
---
diff.h | 4 ++--
tree-diff.c | 4 ++--
2 files changed, 4 insertions(+), 4 deletions(-)
@@ -124,8 +124,8 @@ static struct combine_diff_path *path_appendnew(struct combine_diff_path *last,unsignedmode,constunsignedchar*sha1){structcombine_diff_path*p;-intlen=base->len+pathlen;-intalloclen=combine_diff_path_size(nparent,len);+size_tlen=st_add(base->len,pathlen);+size_talloclen=combine_diff_path_size(nparent,len);/* if last->next is !NULL - it is a pre-allocated memory, we can reuse */p=last->next;
From: Jeff King <hidden> Date: 2016-06-15 23:08:14
REALLOC_ARRAY inherently involves a multiplication which can
overflow size_t, resulting in a much smaller buffer than we
think we've allocated. We can easily harden it by using
st_mult() to check for overflow. Likewise, we can add
ALLOC_ARRAY to do the same thing for xmalloc calls.
xcalloc() should already be fine, because it takes the two
factors separately, assuming the system calloc actually
checks for overflow. However, before we even hit the system
calloc(), we do our memory_limit_check, which involves a
multiplication. Let's check for overflow ourselves so that
this limit cannot be bypassed.
Signed-off-by: Jeff King <redacted>
---
git-compat-util.h | 3 ++-
wrapper.c | 3 +++
2 files changed, 5 insertions(+), 1 deletion(-)
@@ -152,6 +152,9 @@ void *xcalloc(size_t nmemb, size_t size){void*ret;+if(unsigned_mult_overflows(nmemb,size))+die("data too large to fit into virtual memory space");+memory_limit_check(size*nmemb,0);ret=calloc(nmemb,size);if(!ret&&(!nmemb||!size))
From: Jeff King <hidden> Date: 2016-06-15 23:08:14
Allocating a struct with a flex array is pretty simple in
practice: you over-allocate the struct, then copy some data
into the over-allocation. But it can be a slight pain to
make sure you're allocating and copying the right amounts.
This patch adds a few helpers to turn simple cases of into a
one-liner that properly checks for overflow. See the
embedded documentation for details.
Ideally we could provide a more flexible version that could
handle multiple strings, like:
FLEX_ALLOC_FMT(ref, name, "%s%s", prefix, name);
But we have to implement this as a macro (because of the
offset calculation of the flex member), which means we would
need all compilers to support variadic macros.
Signed-off-by: Jeff King <redacted>
---
git-compat-util.h | 62 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
1 file changed, 62 insertions(+)
@@ -939,7 +939,7 @@ static wchar_t *make_environment_block(char **deltaenv)i++;/* copy the environment, leaving space for changes */-tmpenv=xmalloc((size+i)*sizeof(char*));+ALLOC_ARRAY(tmpenv,size+i);memcpy(tmpenv,environ,size*sizeof(char*));/* merge supplied environment changes into the temporary environment */
@@ -1129,7 +1129,7 @@ static int try_shell_exec(const char *cmd, char *const *argv)intargc=0;constchar**argv2;while(argv[argc])argc++;-argv2=xmalloc(sizeof(*argv)*(argc+1));+ALLOC_ARRAY(argv2,argc+1);argv2[0]=(char*)cmd;/* full path to the script file */memcpy(&argv2[1],&argv[1],sizeof(*argv)*argc);pid=mingw_spawnv(prog,argv2,1);
@@ -117,7 +117,7 @@ static const double __ac_HASH_UPPER = 0.77;if(new_n_buckets<4)new_n_buckets=4;\if(h->size>=(khint_t)(new_n_buckets*__ac_HASH_UPPER+0.5))j=0;/* requested size is too small */\else{/* hash table size to be changed (shrink or expand); rehash */\-new_flags=(khint32_t*)xmalloc(__ac_fsize(new_n_buckets)*sizeof(khint32_t));\+ALLOC_ARRAY(new_flags,__ac_fsize(new_n_buckets));\if(!new_flags)return-1;\memset(new_flags,0xaa,__ac_fsize(new_n_buckets)*sizeof(khint32_t));\if(h->n_buckets<new_n_buckets){/* expand */\
@@ -89,7 +89,7 @@ static int verify_packfile(struct packed_git *p,*wedonotdoscan-streamingcheckonthepackfile.*/nr_objects=p->num_objects;-entries=xmalloc((nr_objects+1)*sizeof(*entries));+ALLOC_ARRAY(entries,nr_objects+1);entries[nr_objects].offset=pack_sig_ofs;/* first sort entries by pack offset, since unpacking them is more efficient that way */for(i=0;i<nr_objects;i++){
@@ -696,9 +696,10 @@ static int rpc_service(struct rpc_state *rpc, struct discovery *heads)staticintfetch_dumb(intnr_heads,structref**to_fetch){structwalker*walker;-char**targets=xmalloc(nr_heads*sizeof(char*));+char**targets;intret,i;+ALLOC_ARRAY(targets,nr_heads);if(options.depth)die("dumb http transport does not support --depth");for(i=0;i<nr_heads;i++)
@@ -168,7 +168,7 @@ static const char **prepare_shell_cmd(const char **argv)for(argc=0;argv[argc];argc++);/* just counting *//* +1 for NULL, +3 for "sh -c" plus extra $0 */-nargv=xmalloc(sizeof(*nargv)*(argc+1+3));+ALLOC_ARRAY(nargv,argc+1+3);if(argc<1)die("BUG: shell command is empty");
From: Jeff King <hidden> Date: 2016-06-15 23:08:14
We frequently allocate strings as xmalloc(len + 1), where
the extra 1 is for the NUL terminator. This can be done more
simply with xmallocz, which also checks for integer
overflow.
There's no case where switching xmalloc(n+1) to xmallocz(n)
is wrong; the result is the same length, and malloc made no
guarantees about what was in the buffer anyway. But in some
cases, we can stop manually placing NUL at the end of the
allocated buffer. But that's only safe if it's clear that
the contents will always fill the buffer.
In each case where this patch does so, I manually examined
the control flow, and I tried to err on the side of caution.
Signed-off-by: Jeff King <redacted>
---
builtin/check-ref-format.c | 2 +-
builtin/merge-tree.c | 2 +-
builtin/worktree.c | 2 +-
column.c | 3 +--
combine-diff.c | 4 +---
config.c | 4 +---
dir.c | 2 +-
entry.c | 2 +-
grep.c | 3 +--
imap-send.c | 5 ++---
ll-merge.c | 2 +-
progress.c | 2 +-
refs.c | 2 +-
setup.c | 5 ++---
strbuf.c | 2 +-
15 files changed, 17 insertions(+), 25 deletions(-)
@@ -1051,8 +1051,6 @@ static void show_patch_diff(struct combine_diff_path *elem, int num_parent,elseif(done<len)die("early EOF '%s'",elem->path);-result[len]=0;-/* If not a fake symlink, apply filters, e.g. autocrlf */if(is_file){structstrbufbuf=STRBUF_INIT;
@@ -2197,10 +2189,9 @@ static int pack_if_possible_fn(struct ref_entry *entry, void *cb_data)/* Schedule the loose reference for pruning if requested. */if((cb->flags&PACK_REFS_PRUNE)){-intnamelen=strlen(entry->name)+1;-structref_to_prune*n=xcalloc(1,sizeof(*n)+namelen);+structref_to_prune*n;+FLEX_ALLOC_STR(n,name,entry->name);hashcpy(n->sha1,entry->u.value.oid.hash);-memcpy(n->name,entry->name,namelen);/* includes NUL */n->next=cb->ref_to_prune;cb->ref_to_prune=n;}
@@ -2136,16 +2136,13 @@ static int one_local_ref(const char *refname, const struct object_id *oid,{structref***local_tail=cb_data;structref*ref;-intlen;/* we already know it starts with refs/ to get here */if(check_refname_format(refname+5,0))return0;-len=strlen(refname)+1;-ref=xcalloc(1,sizeof(*ref)+len);+ref=alloc_ref(refname);oidcpy(&ref->new_oid,oid);-memcpy(ref->name,refname,len);**local_tail=ref;*local_tail=&ref->next;return0;
@@ -939,7 +939,7 @@ static int setup_with_upstream(const char ***argv)if(!branch->merge_nr)die(_("No default upstream defined for the current branch."));-args=xcalloc(branch->merge_nr+1,sizeof(char*));+args=xcalloc(st_add(branch->merge_nr,1),sizeof(char*));for(i=0;i<branch->merge_nr;i++){if(!branch->merge[i]->dst)die(_("No remote-tracking branch for %s from %s"),
@@ -1111,7 +1111,7 @@ static void show_patch_diff(struct combine_diff_path *elem, int num_parent,if(result_size&&result[result_size-1]!='\n')cnt++;/* incomplete line */-sline=xcalloc(cnt+2,sizeof(*sline));+sline=xcalloc(st_add(cnt,2),sizeof(*sline));sline[0].bol=result;for(lno=0,cp=result;cp<result+result_size;cp++){if(*cp=='\n'){
@@ -1130,7 +1130,7 @@ static void show_patch_diff(struct combine_diff_path *elem, int num_parent,/* Even p_lno[cnt+1] is valid -- that is for the end line number*fordeletionhunkattheend.*/-sline[0].p_lno=xcalloc((cnt+2)*num_parent,sizeof(unsignedlong));+sline[0].p_lno=xcalloc(st_mult(st_add(cnt,2),num_parent),sizeof(unsignedlong));for(lno=0;lno<=cnt;lno++)sline[lno+1].p_lno=sline[lno].p_lno+num_parent;
From: Jeff King <hidden> Date: 2016-06-15 23:08:14
We perform unchecked additions when computing the size of a
"struct ondisk_untracked_cache". This is unlikely to have an
integer overflow in practice, but we'd like to avoid this
dangerous pattern to make further audits easier.
Note that there's one subtlety here, though. We protect
ourselves against a NULL exclude_per_dir entry in our
source, and avoid calling strlen() on it, keeping "len" at
0. But later, we unconditionally memcpy "len + 1" bytes to
get the trailing NUL byte. If we did have a NULL
exclude_per_dir, we would read from bogus memory.
As it turns out, though, we always create this field
pointing to a string literal, so there's no bug. We can just
get rid of the pointless extra conditional.
Signed-off-by: Jeff King <redacted>
---
dir.c | 9 ++++-----
1 file changed, 4 insertions(+), 5 deletions(-)
From: Jeff King <hidden> Date: 2016-06-15 23:08:14
This function allocates a packed_git flex-array, and adds a
mysterious 2 bytes to the length of the pack_name field. One
is for the trailing NUL, but the other has no purpose. This
is probably cargo-culted from add_packed_git, which gets the
".idx" path and needs to allocate enough space to hold the
matching ".pack" (though since 48bcc1c, we calculate the
size there differently).
This site, however, is using the raw path of a tempfile, and
does not need the extra byte. We can just replace the
allocation with FLEX_ALLOC_STR, which handles the allocation
and the NUL for us.
Signed-off-by: Jeff King <redacted>
---
fast-import.c | 5 +----
1 file changed, 1 insertion(+), 4 deletions(-)
From: Jeff King <hidden> Date: 2016-06-15 23:08:14
We have two variants of this function, one that takes a
string and one that takes a ptr/len combo. But we only call
the latter with the length of a NUL-terminated string, so
our first simplification is to drop it in favor of the
string variant.
Since we know we have a string, we can also replace the
manual memory computation with a call to alloc_ref().
Furthermore, we can rely on get_oid_hex() to complain if it
hits the end of the string. That means we can simplify the
check for "<sha1> <ref>" versus just "<ref>". Rather than
manage the ptr/len pair, we can just bump the start of our
string forward. The original code over-allocated based on
the original "namelen" (which wasn't _wrong_, but was simply
wasteful and confusing).
Signed-off-by: Jeff King <redacted>
---
builtin/fetch-pack.c | 27 +++++++++------------------
1 file changed, 9 insertions(+), 18 deletions(-)
From: Jeff King <hidden> Date: 2016-06-15 23:08:14
The normalize_path_copy function needs an output buffer that
is at least as long as its input (it may shrink the path,
but never expand it). However, this test program feeds it
static PATH_MAX-sized buffers, which have no relation to the
input size.
In the normalize_ceiling_entry case, we do at least check
the size against PATH_MAX and die(), but that case is even
more convoluted. We normalize into a fixed-size buffer, free
the original, and then replace it with a strdup'd copy of
the result. But normalize_path_copy explicitly allows
normalizing in-place, so we can simply do that.
Signed-off-by: Jeff King <redacted>
---
test-path-utils.c | 15 ++++-----------
1 file changed, 4 insertions(+), 11 deletions(-)
@@ -8,21 +8,14 @@*/staticintnormalize_ceiling_entry(structstring_list_item*item,void*unused){-constchar*ceil=item->string;-intlen=strlen(ceil);-charbuf[PATH_MAX+1];+char*ceil=item->string;-if(len==0)+if(!*ceil)die("Empty path is not supported");-if(len>PATH_MAX)-die("Path \"%s\" is too long",ceil);if(!is_absolute_path(ceil))die("Path \"%s\" is not absolute",ceil);-if(normalize_path_copy(buf,ceil)<0)+if(normalize_path_copy(ceil,ceil)<0)die("Path \"%s\" could not be normalized",ceil);-len=strlen(buf);-free(item->string);-item->string=xstrdup(buf);return1;}
From: Jeff King <hidden> Date: 2016-06-15 23:08:14
For a commit with has "1234abcd" and subject "foo", this
function produces a struct with three strings:
1. "foo"
2. "1234abcd... foo"
3. "parent of 1234abcd... foo"
It takes advantage of the fact that these strings are
subsets of each other, and allocates only _one_ string, with
pointers into the various parts. Unfortunately, this makes
the string allocation complicated and hard to follow.
Since we keep only one of these in memory at a time, we can
afford to simply allocate three strings. This lets us build
on tools like xstrfmt and avoid manual computation.
While we're here, we can also drop the ad-hoc
reimplementation of get_git_commit_encoding(), and simply
call that function.
Signed-off-by: Jeff King <redacted>
---
sequencer.c | 29 ++++++++++-------------------
1 file changed, 10 insertions(+), 19 deletions(-)
From: Jeff King <hidden> Date: 2016-06-15 23:08:14
There are no callers of this left, as the last one was
dropped in the previous patch. And there are no likely to be
new ones, as the function has been around since 2010 without
gaining any new callers.
Signed-off-by: Jeff King <redacted>
---
git-compat-util.h | 9 ---------
1 file changed, 9 deletions(-)
From: Jeff King <hidden> Date: 2016-06-15 23:08:14
This function uses xcalloc and two memcpy calls to
concatenate two strings. We can do this as an xstrfmt
one-liner, and then it is more clear that we are allocating
the correct amount of memory.
Signed-off-by: Jeff King <redacted>
---
transport.c | 8 +++-----
1 file changed, 3 insertions(+), 5 deletions(-)
@@ -1351,7 +1351,7 @@ int transport_disconnect(struct transport *transport)*/char*transport_anonymize_url(constchar*url){-char*anon_url,*scheme_prefix,*anon_part;+char*scheme_prefix,*anon_part;size_tanon_len,prefix_len=0;anon_part=strchr(url,'@');
From: Jeff King <hidden> Date: 2016-06-15 23:08:14
We allocate 100 bytes to hold the "Submodule commit ..."
text. This is enough, but it's not immediately obvious that
this is the case, and we have to repeat the magic 100 twice.
We could get away with xstrfmt here, but we want to know the
size, as well, so let's use a real strbuf. And while we're
here, we can clean up the logic around size_only. It
currently sets and clears the "data" field pointlessly, and
leaves the "should_free" flag on even after we have cleared
the data.
Signed-off-by: Jeff King <redacted>
---
diff.c | 16 ++++++++--------
1 file changed, 8 insertions(+), 8 deletions(-)
@@ -2704,21 +2704,21 @@ static int reuse_worktree_file(const char *name, const unsigned char *sha1, intstaticintdiff_populate_gitlink(structdiff_filespec*s,intsize_only){-intlen;-char*data=xmalloc(100),*dirty="";+structstrbufbuf=STRBUF_INIT;+char*dirty="";/* Are we looking at the work tree? */if(s->dirty_submodule)dirty="-dirty";-len=snprintf(data,100,-"Subproject commit %s%s\n",sha1_to_hex(s->sha1),dirty);-s->data=data;-s->size=len;-s->should_free=1;+strbuf_addf(&buf,"Subproject commit %s%s\n",sha1_to_hex(s->sha1),dirty);+s->size=buf.len;if(size_only){s->data=NULL;-free(data);+strbuf_release(&buf);+}else{+s->data=strbuf_detach(&buf,NULL);+s->should_free=1;}return0;}
From: Jeff King <hidden> Date: 2016-06-15 23:08:14
This code was originally written with the idea that it could
be spun off into its own ewah library, and uses the
overrideable ewah_malloc to do allocations.
We plug in xmalloc as our ewah_malloc, of course. But over
the years the ewah code itself has become more entangled
with git, and the return value of many ewah_malloc sites is
not checked.
Let's just drop the level of indirection and use xmalloc and
friends directly. This saves a few lines, and will let us
adapt these sites to our more advanced malloc helpers.
Signed-off-by: Jeff King <redacted>
---
ewah/bitmap.c | 12 ++++++------
ewah/ewah_bitmap.c | 9 +++------
ewah/ewah_io.c | 10 ++--------
ewah/ewok.h | 10 ----------
4 files changed, 11 insertions(+), 30 deletions(-)
@@ -180,12 +177,9 @@ int ewah_deserialize(struct ewah_bitmap *self, int fd)return-1;self->buffer_size=self->alloc_size=(size_t)ntohl(word_count);-self->buffer=ewah_realloc(self->buffer,+self->buffer=xrealloc(self->buffer,self->alloc_size*sizeof(eword_t));-if(!self->buffer)-return-1;-/** 64 bit x N -- compressed words */buffer=self->buffer;words_left=self->buffer_size;
From: Jeff King <hidden> Date: 2016-06-15 23:08:14
Now that we're built around xmalloc and friends, we can use
helpers like REALLOC_ARRAY, ALLOC_GROW, and so on to make
the code shorter and protect against integer overflow.
Signed-off-by: Jeff King <redacted>
---
ewah/bitmap.c | 16 ++++------------
ewah/ewah_bitmap.c | 5 ++---
ewah/ewah_io.c | 6 ++----
3 files changed, 8 insertions(+), 19 deletions(-)
@@ -177,8 +176,7 @@ int ewah_deserialize(struct ewah_bitmap *self, int fd)return-1;self->buffer_size=self->alloc_size=(size_t)ntohl(word_count);-self->buffer=xrealloc(self->buffer,-self->alloc_size*sizeof(eword_t));+REALLOC_ARRAY(self->buffer,self->alloc_size);/** 64 bit x N -- compressed words */buffer=self->buffer;
From: Jeff King <hidden> Date: 2016-06-15 23:08:14
On Mon, Feb 15, 2016 at 04:45:16PM -0500, Jeff King wrote:
The only bug I have actually confirmed in practice here is fixed by
patch 2 (which is why it's at the front). There's another one in
path_name(), but that function is already dropped by the nearby
jk/lose-name-path topic.
The rest are cleanups of spots which _might_ be buggy, but I didn't dig
too far to find out. As with the earlier audit, I tried to refactor
using helpers that make the code clearer and less error-prone. So maybe
they're fixing bugs or not, but they certainly make it easier to audit
the result for problems.
After this, looking for /[cm]alloc.*\+/ is pretty clean. I _didn't_ fix
any sites in xdiff, but those are already protected by dcd1742 (xdiff:
reject files larger than ~1GB, 2015-09-24).
That's not necessarily complete coverage, though, as you can always
screw up the computation outside of the xmalloc call, and pass in the
truncated version. E.g.:
int alloc = a + b;
char *foo = xmalloc(alloc);
However, this is only a big problem if you then copy "a" and "b"
separately. If you use "alloc" later as the limit, like:
xsnprintf(foo, alloc, "%s%s", a, b);
that's only a minor bug (we notice the overflow and complain, rather
than smashing the heap).
-Peff
From: Eric Sunshine <hidden> Date: 2016-06-15 23:08:15
On Mon, Feb 15, 2016 at 4:50 PM, Jeff King [off-list ref] wrote:
Allocating a struct with a flex array is pretty simple in
practice: you over-allocate the struct, then copy some data
into the over-allocation. But it can be a slight pain to
make sure you're allocating and copying the right amounts.
This patch adds a few helpers to turn simple cases of into a
Grammo: "cases of into"
quoted hunk
one-liner that properly checks for overflow. See the
embedded documentation for details.
[...]
Signed-off-by: Jeff King <redacted>
---
@@ -782,6 +782,68 @@ extern FILE *fopen_for_writing(const char *path);+ * struct foo *f;+ * FLEX_ALLOC_STR(f, name, src);+ *+ * and "name" will point to a block of memory after the struct, which will be+ * freed along with the struct (but the pointer can be repoined anywhere).
"repoined"?
+ * The *_STR variants accept a string parameter rather than a ptr/len
+ * combination.
From: Eric Sunshine <hidden> Date: 2016-06-15 23:08:15
On Mon, Feb 15, 2016 at 4:52 PM, Jeff King [off-list ref] wrote:
quoted hunk
Using FLEX_ARRAY macros reduces the amount of manual
computation size we have to do. It also ensures we don't
overflow size_t, and it makes sure we write the same number
of bytes that we allocated.
Signed-off-by: Jeff King <redacted>
---
Does the incoming 'len' already account for the NUL terminator, or was
the original code underallocating?
Answering my own question: Looking at reflog_expire_config() and
parse_config_key(), I gather that 'len' already accounts for the NUL,
thus the new code is overallocating (which should not be a problem).
From: Jeff King <hidden> Date: 2016-06-15 23:08:15
On Mon, Feb 15, 2016 at 08:47:30PM -0500, Eric Sunshine wrote:
quoted
This patch adds a few helpers to turn simple cases of into a
Grammo: "cases of into"
Oops. Cases of "flex-array struct allocation into...".
quoted
+ * and "name" will point to a block of memory after the struct, which will be
+ * freed along with the struct (but the pointer can be repoined anywhere).
"repoined"?
Repointed.
Fixed patch below.
-- >8 --
Subject: [PATCH] add helpers for allocating flex-array structs
Allocating a struct with a flex array is pretty simple in
practice: you over-allocate the struct, then copy some data
into the over-allocation. But it can be a slight pain to
make sure you're allocating and copying the right amounts.
This patch adds a few helpers to turn simple cases of
flex-array struct allocation into a one-liner that properly
checks for overflow. See the embedded documentation for
details.
Ideally we could provide a more flexible version that could
handle multiple strings, like:
FLEX_ALLOC_FMT(ref, name, "%s%s", prefix, name);
But we have to implement this as a macro (because of the
offset calculation of the flex member), which means we would
need all compilers to support variadic macros.
Signed-off-by: Jeff King <redacted>
---
git-compat-util.h | 62 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
1 file changed, 62 insertions(+)
Does the incoming 'len' already account for the NUL terminator, or was
the original code underallocating?
Answering my own question: Looking at reflog_expire_config() and
parse_config_key(), I gather that 'len' already accounts for the NUL,
thus the new code is overallocating (which should not be a problem).
Actually, I think the original underallocates. If we have
gc.foobar.reflogExpire, then "pattern" will poitn to "foobar" and "len"
will be 6. Meaning we allocate without a trailing NUL.
That _should_ be OK, because the struct has a "len" field, and readers
can be careful not to go past it. And indeed, in the loop above, we
check the length and use memcmp().
But later, in set_reflog_expiry_param(), we walk through the list and
hand ent->pattern directly to wildmatch, which assumes a NUL-terminated
string. In practice, it probably works out 7 out of 8 times, because
malloc will align the struct, and we're on a zeroed page, so unless the
string is exactly 8 characters, we'll get some extra NULs afterwards.
But I could demonstrate it by doing:
gdb --args git -c gc.foobar12.reflogexpire=never reflog expire --all
and breaking on wildmatch, which yields:
Breakpoint 1, wildmatch (pattern=0x85eb70 "foobar12Q", text=0x85e4d4
"refs/heads/master", flags=0, wo=0x0)
So this is in fact fixing a bug. I can't say I'm terribly surprised
nobody noticed it, as per-ref reflog expiration is pretty obscure.
I hope this increases confidence in my patch series. Even though I
didn't _know_ there was a bug here, I did know that malloc computations
are a potential source of errors. And the FLEX_ALLOC helpers are
designed to remove that work and have a simple interface. We don't know
whether the caller will want a NUL afterwards or not, but we err on the
side of over-allocating by a byte (just as we over-allocate
read_sha1_file() output by a byte), because safety is better than
squeezing out a single byte.
quoted
diff --git a/hashmap.c b/hashmap.c
@@ -256,10 +256,9 @@ const void *memintern(const void *data, size_t len) e = hashmap_get(&map, &key, data); if (!e) { /* not found: create it */- e = xmallocz(sizeof(struct pool_entry) + len);+ FLEX_ALLOC_MEM(e, data, data, len);
Ditto. I guess the new code is overallocating (which should be okay).
It is, but so was the original (it used xmallocz to get an extra NUL).
-Peff
From: Jeff King <hidden> Date: 2016-06-15 23:08:15
On Mon, Feb 15, 2016 at 10:15:54PM -0500, Jeff King wrote:
quoted
Answering my own question: Looking at reflog_expire_config() and
parse_config_key(), I gather that 'len' already accounts for the NUL,
thus the new code is overallocating (which should not be a problem).
Actually, I think the original underallocates. If we have
gc.foobar.reflogExpire, then "pattern" will poitn to "foobar" and "len"
will be 6. Meaning we allocate without a trailing NUL.
That _should_ be OK, because the struct has a "len" field, and readers
can be careful not to go past it. And indeed, in the loop above, we
check the length and use memcmp().
But later, in set_reflog_expiry_param(), we walk through the list and
hand ent->pattern directly to wildmatch, which assumes a NUL-terminated
string. In practice, it probably works out 7 out of 8 times, because
malloc will align the struct, and we're on a zeroed page, so unless the
string is exactly 8 characters, we'll get some extra NULs afterwards.
But I could demonstrate it by doing:
gdb --args git -c gc.foobar12.reflogexpire=never reflog expire --all
and breaking on wildmatch, which yields:
Breakpoint 1, wildmatch (pattern=0x85eb70 "foobar12Q", text=0x85e4d4
"refs/heads/master", flags=0, wo=0x0)
So this is in fact fixing a bug. I can't say I'm terribly surprised
nobody noticed it, as per-ref reflog expiration is pretty obscure.
We could do this on top of my series (I can also factor out the fix
separately to go at the beginning if we don't want to hold the bugfix
hostage).
-- >8 --
Subject: [PATCH] reflog_expire_cfg: drop misleading "len" parameter
You can tweak the reflog expiration for a particular subset
of refs by configuring gc.foo.reflogexpire. We keep a linked
list of reflog_expire_cfg structs, each of which holds the
pattern and a "len" field for the length of the pattern.
However, we feed the pattern directly to wildmatch(), which
means that it must be a NUL-terminated string. Before the
recent conversion to FLEX_ALLOC_MEM, we got this wrong, and
could feed extra garbage to wildmatch(). That's now fixed,
but the "len" parameter is simply misleading. The pattern is
a string, and we don't need to record its length.
To get rid of it, we do need to tweak the "do we have it
already?" search in find_cfg_ent(), but we can do so without
having a recorded length by just using strncmp.
Signed-off-by: Jeff King <redacted>
---
builtin/reflog.c | 6 ++----
1 file changed, 2 insertions(+), 4 deletions(-)
From: Jeff King <hidden> Date: 2016-06-15 23:08:15
On Mon, Feb 15, 2016 at 10:26:26PM -0500, Jeff King wrote:
We could do this on top of my series (I can also factor out the fix
separately to go at the beginning if we don't want to hold the bugfix
hostage).
-- >8 --
Subject: [PATCH] reflog_expire_cfg: drop misleading "len" parameter
Here it is as a separate fix. Applying my series on top would need a
minor and obvious tweak. I'll hold a re-roll for more comments, but will
otherwise plan to stick this at the front of the series.
-- >8 --
Subject: [PATCH] reflog_expire_cfg: NUL-terminate pattern field
You can tweak the reflog expiration for a particular subset
of refs by configuring gc.foo.reflogexpire. We keep a linked
list of reflog_expire_cfg structs, each of which holds the
pattern and a "len" field for the length of the pattern. The
pattern itself is _not_ NUL-terminated.
However, we feed the pattern directly to wildmatch(), which
expects a NUL-terminated string, meaning it may keep reading
random junk after our struct.
We can fix this by allocating an extra byte for the NUL
(which is already zero because we use xcalloc). Let's also
drop the misleading "len" field, which is no longer
necessary. The existing use of "len" can be converted to use
strncmp().
Signed-off-by: Jeff King <redacted>
---
builtin/reflog.c | 8 +++-----
1 file changed, 3 insertions(+), 5 deletions(-)
Does the incoming 'len' already account for the NUL terminator, or was
the original code underallocating?
Answering my own question: Looking at reflog_expire_config() and
parse_config_key(), I gather that 'len' already accounts for the NUL,
thus the new code is overallocating (which should not be a problem).
Actually, I think the original underallocates. If we have
gc.foobar.reflogExpire, then "pattern" will poitn to "foobar" and "len"
will be 6. Meaning we allocate without a trailing NUL.
Ugh, yeah, I misread the code.
That _should_ be OK, because the struct has a "len" field, and readers
can be careful not to go past it. And indeed, in the loop above, we
check the length and use memcmp().
But later, in set_reflog_expiry_param(), we walk through the list and
hand ent->pattern directly to wildmatch, which assumes a NUL-terminated
string. In practice, it probably works out 7 out of 8 times, because
malloc will align the struct, and we're on a zeroed page, so unless the
string is exactly 8 characters, we'll get some extra NULs afterwards.
But I could demonstrate it by doing:
gdb --args git -c gc.foobar12.reflogexpire=never reflog expire --all
and breaking on wildmatch, which yields:
Breakpoint 1, wildmatch (pattern=0x85eb70 "foobar12Q", text=0x85e4d4
"refs/heads/master", flags=0, wo=0x0)
From: Eric Sunshine <hidden> Date: 2016-06-15 23:08:15
On Mon, Feb 15, 2016 at 10:36 PM, Jeff King [off-list ref] wrote:
On Mon, Feb 15, 2016 at 10:26:26PM -0500, Jeff King wrote:
quoted
We could do this on top of my series (I can also factor out the fix
separately to go at the beginning if we don't want to hold the bugfix
hostage).
-- >8 --
Subject: [PATCH] reflog_expire_cfg: drop misleading "len" parameter
Here it is as a separate fix. Applying my series on top would need a
minor and obvious tweak. I'll hold a re-roll for more comments, but will
otherwise plan to stick this at the front of the series.
Yep, I prefer this version of the patch too, as it makes it explicit
that a bug is being fixed rather than it happening "by accident" via
the FLEX_ALLOC_MEM conversion, which is easily overlooked.
quoted hunk
-- >8 --
Subject: [PATCH] reflog_expire_cfg: NUL-terminate pattern field
You can tweak the reflog expiration for a particular subset
of refs by configuring gc.foo.reflogexpire. We keep a linked
list of reflog_expire_cfg structs, each of which holds the
pattern and a "len" field for the length of the pattern. The
pattern itself is _not_ NUL-terminated.
However, we feed the pattern directly to wildmatch(), which
expects a NUL-terminated string, meaning it may keep reading
random junk after our struct.
We can fix this by allocating an extra byte for the NUL
(which is already zero because we use xcalloc). Let's also
drop the misleading "len" field, which is no longer
necessary. The existing use of "len" can be converted to use
strncmp().
Signed-off-by: Jeff King <redacted>
---
diff --git a/builtin/reflog.c b/builtin/reflog.c
@@ -408,13 +407,12 @@ static struct reflog_expire_cfg *find_cfg_ent(const char *pattern, size_t len) reflog_expire_cfg_tail = &reflog_expire_cfg; for (ent = reflog_expire_cfg; ent; ent = ent->next)- if (ent->len == len &&- !memcmp(ent->pattern, pattern, len))+ if (!strncmp(ent->pattern, pattern, len) &&+ ent->pattern[len] == '\0')
If 'ent->pattern' is shorter than 'pattern' then the strncmp() will
fail, thus it will short-circuit before ent->pattern[len] has a chance
to access beyond the end of memory allocated for 'ent->pattern'. Okay,
makes sense.
From: Eric Sunshine <hidden> Date: 2016-06-15 23:08:15
On Mon, Feb 15, 2016 at 4:51 PM, Jeff King [off-list ref] wrote:
quoted hunk
Each of these cases can be converted to use ALLOC_ARRAY or
REALLOC_ARRAY, which has two advantages:
1. It automatically checks the array-size multiplication
for overflow.
2. It always uses sizeof(*array) for the element-size,
so that it can never go out of sync with the declared
type of the array.
Signed-off-by: Jeff King <redacted>
---
From: Jeff King <hidden> Date: 2016-06-15 23:08:15
On Mon, Feb 15, 2016 at 11:18:56PM -0500, Eric Sunshine wrote:
quoted
diff --git a/builtin/reflog.c b/builtin/reflog.c
@@ -408,13 +407,12 @@ static struct reflog_expire_cfg *find_cfg_ent(const char *pattern, size_t len) reflog_expire_cfg_tail = &reflog_expire_cfg; for (ent = reflog_expire_cfg; ent; ent = ent->next)- if (ent->len == len &&- !memcmp(ent->pattern, pattern, len))+ if (!strncmp(ent->pattern, pattern, len) &&+ ent->pattern[len] == '\0')
If 'ent->pattern' is shorter than 'pattern' then the strncmp() will
fail, thus it will short-circuit before ent->pattern[len] has a chance
to access beyond the end of memory allocated for 'ent->pattern'. Okay,
makes sense.
Yeah. It took me a minute to convince myself that this was correct. If
you have a shorter or more clear way of writing it, I'm open to it. The
best I could come up with is running an extra "strlen" and otherwise
keeping the loop as it is; the performance on that is not as good, but
if performance is a concern, maybe something besides a linear search
would be in order. :)
-Peff
From: Jeff King <hidden> Date: 2016-06-15 23:08:15
On Mon, Feb 15, 2016 at 11:22:12PM -0500, Eric Sunshine wrote:
On Mon, Feb 15, 2016 at 4:51 PM, Jeff King [off-list ref] wrote:
quoted
Each of these cases can be converted to use ALLOC_ARRAY or
REALLOC_ARRAY, which has two advantages:
1. It automatically checks the array-size multiplication
for overflow.
2. It always uses sizeof(*array) for the element-size,
so that it can never go out of sync with the declared
type of the array.
Signed-off-by: Jeff King <redacted>
---
Elsewhere in this patch, you've reformatted "x+c" as "x + c"; perhaps
do so here, as well.
Will do. I noticed while going over this before sending it out that it
may also be technically possible for "n+1" to overflow here (and I think
in a few other places in this patch). I don't know how paranoid we want
to be.
-Peff
Elsewhere in this patch, you've reformatted "x+c" as "x + c"; perhaps
do so here, as well.
Will do. I noticed while going over this before sending it out that it
may also be technically possible for "n+1" to overflow here (and I think
in a few other places in this patch). I don't know how paranoid we want
to be.
Yes, I also noticed those and considered mentioning it. There was also
some multiplication which might be of concern.
ALLOC_ARRAY(graph->mapping, 2 * graph->column_capacity);
It would be easy enough to manually call st_add() and st_mult() for
those cases, but I haven't examined them closely enough to determine
how likely they would be to overflow, nor do I know if the resulting
noisiness of code is desirable.
Elsewhere in this patch, you've reformatted "x+c" as "x + c"; perhaps
do so here, as well.
Will do. I noticed while going over this before sending it out that it
may also be technically possible for "n+1" to overflow here (and I think
in a few other places in this patch). I don't know how paranoid we want
to be.
Yes, I also noticed those and considered mentioning it. There was also
some multiplication which might be of concern.
ALLOC_ARRAY(graph->mapping, 2 * graph->column_capacity);
It would be easy enough to manually call st_add() and st_mult() for
those cases, but I haven't examined them closely enough to determine
how likely they would be to overflow, nor do I know if the resulting
noisiness of code is desirable.
Yeah, I'm quite sure that one is safe (we set column_capacity to a fixed
integer immediately beforehand). And many of the "+" ones are likely
safe, too. If "n" is close to wrapping, then allocating "n" structs
will probably fail beforehand (though not always, if you have a ton of
RAM and "n" is a signed int).
But part of the point of this series is that we shouldn't have to wonder
if things are safe. They should just be obviously so, and we should err
on the side of caution. So I think it probably _is_ worth sprinkling
st_add() calls in those places. I'll take a look for the re-roll.
-Peff
From: Eric Sunshine <hidden> Date: 2016-06-15 23:08:15
On Mon, Feb 15, 2016 at 4:53 PM, Jeff King [off-list ref] wrote:
quoted hunk
If our size computation overflows size_t, we may allocate a
much smaller buffer than we expected and overflow it. It's
probably impossible to trigger an overflow in most of these
sites in practice, but it is easy enough convert their
additions and multiplications into overflow-checking
variants. This may be fixing real bugs, and it makes
auditing the code easier.
Signed-off-by: Jeff King <redacted>
---
Phew, what a mouthful, and not easy to read compared to the original.
Fortunately, the remainder of the changes in this patch are
straightforward and often simple.
If we've gotten this far without die()ing due to overflow in st_add3()
when invoking xmalloc(), then we know that this fakeent->name
computation won't overflow. Okay.
From: Eric Sunshine <hidden> Date: 2016-06-15 23:08:15
On Mon, Feb 15, 2016 at 4:56 PM, Jeff King [off-list ref] wrote:
For a commit with has "1234abcd" and subject "foo", this
Did you mean s/with has/which has ID/ or something?
function produces a struct with three strings:
1. "foo"
2. "1234abcd... foo"
3. "parent of 1234abcd... foo"
It takes advantage of the fact that these strings are
subsets of each other, and allocates only _one_ string, with
pointers into the various parts. Unfortunately, this makes
the string allocation complicated and hard to follow.
Since we keep only one of these in memory at a time, we can
afford to simply allocate three strings. This lets us build
on tools like xstrfmt and avoid manual computation.
While we're here, we can also drop the ad-hoc
reimplementation of get_git_commit_encoding(), and simply
call that function.
Signed-off-by: Jeff King <redacted>
From: Jeff King <hidden> Date: 2016-06-15 23:08:19
Here's a re-roll of jk/tighten-alloc series from:
http://thread.gmane.org/gmane.comp.version-control.git/286253
This one fixes all of the minor typo/gramm-o problems from the first
round. As Eric noted, the change to reflog_expire_cfg is an actual
bug-fix. Rather than silently fixing it, I've bumped the fix out to its
own commit at the front of the series.
I also took a look at the raw computation in the ALLOC_ARRAY and
REALLOC_ARRAY lines, as well as the ones in ALLOC_GROW. In theory
something like this is dangerous:
ALLOC_GROW(foo, nr_foo + 1, alloc_foo);
foo[nr_foo++] = whatever;
if we overflow nr_foo, which is quite often an "int". In practice, I
think we're OK, for two reasons:
1. Overflowing a signed "int" here is going to make it go negative
(technically, it invokes undefined behavior, but let's be blindly
pragmatic for a minute and assume twos-complement wrapping). On a
system with a 64-bit size_t, that will try to allocate an enormous
amount of memory and fail. On a 32 bit system, it will be only
about 2GB. But...
2. We're talking about overflowing 2^31 counters here. And the counter
is multiplied by the size of each object we're storing in the
array. So even if we assume that foo is "char *", we know we've
allocated close to 2GB already. On a 32-bit system, the subsequent
2GB allocation is pretty much guaranteed to fail.
On a 64-bit system, I suspect it's possible to convince some of
these counters to wrap (e.g., storing an array of ints, we're
talking about only 8GB; that's a lot, but plenty of machines,
especially servers, can allocate that).
So I have a feeling we're mostly OK there, but the reasoning is
certainly hand-wavy and I'd like to do better. Just switching to:
ALLOC_GROW(foo, st_add(nr_foo, 1), alloc_foo);
foo[nr_foo++] = whatever;
doesn't quite cut it. We might succeed in the allocation, and it stays
big, which is good. But if nr_foo is an int, and we wrap to negative
values, we'll start writing to memory before "foo", corrupting the heap.
So I really think we need to look at each site (and there are a lot of
them) and start using size_t more consistently for these. Or
alternatively, have an int-sized version of st_add and use that, though
it's probably just as much work to convert it to a size_t, which IMHO is
more correct. I really wanted to make a type-agnostic version of
st_add(), but I don't think it's possible to do so portably. My best
attempts needed either typeof() or compiler intrinsics.
So I've punted on that for this series, because I'm not convinced there
are active problems, and it's quite a lot of work (and the patches will
be quite disruptive).
While pondering this, I also looked at what happens if an incoming
packfile claims to have 2^32 objects in its header. In index-pack we
actually read this into a signed "int". Which is kind of bad, but in
practice means we run into the "whoops, I can't allocate (size_t)-1
memory" problem and die. We could change this to a uint32_t (which is
what the actual incoming format supports), but I have a feeling that
makes things worse (if we actually manage to process that many objects,
we then start doing some other computations based on the number of
objects, all using ints; so at least as it is now, we bail early).
While peeking at some of these sites, though, I did realize that many of
the ones that became "ALLOC_ARRAY(foo + 1)" were doing so to make a
NULL-terminated argv list. So there are two new patches in this
iteration to switch them to argv_array (one to catch the mundane cases,
and one for a unique snowflake).
[01/21]: reflog_expire_cfg: NUL-terminate pattern field
[02/21]: add helpers for detecting size_t overflow
[03/21]: tree-diff: catch integer overflow in combine_diff_path allocation
[04/21]: harden REALLOC_ARRAY and xcalloc against size_t overflow
[05/21]: add helpers for allocating flex-array structs
[06/21]: convert manual allocations to argv_array
[07/21]: convert trivial cases to ALLOC_ARRAY
[08/21]: use xmallocz to avoid size arithmetic
[09/21]: convert trivial cases to FLEX_ARRAY macros
[10/21]: use st_add and st_mult for allocation size computation
[11/21]: prepare_{git,shell}_cmd: use argv_array
[12/21]: write_untracked_extension: use FLEX_ALLOC helper
[13/21]: fast-import: simplify allocation in start_packfile
[14/21]: fetch-pack: simplify add_sought_entry
[15/21]: test-path-utils: fix normalize_path_copy output buffer size
[16/21]: sequencer: simplify memory allocation of get_message
[17/21]: git-compat-util: drop mempcpy compat code
[18/21]: transport_anonymize_url: use xstrfmt
[19/21]: diff_populate_gitlink: use a strbuf
[20/21]: convert ewah/bitmap code to use xmalloc
[21/21]: ewah: convert to REALLOC_ARRAY, etc
-Peff
From: Jeff King <hidden> Date: 2016-06-15 23:08:19
Performing computations on size_t variables that we feed to
xmalloc and friends can be dangerous, as an integer overflow
can cause us to allocate a much smaller chunk than we
realized.
We already have unsigned_add_overflows(), but let's add
unsigned_mult_overflows() to that. Furthermore, rather than
have each site manually check and die on overflow, we can
provide some helpers that will:
- promote the arguments to size_t, so that we know we are
doing our computation in the same size of integer that
will ultimately be fed to xmalloc
- check and die on overflow
- return the result so that computations can be done in
the parameter list of xmalloc.
These functions are a lot uglier to use than normal
arithmetic operators (you have to do "st_add(foo, bar)"
instead of "foo + bar"). To at least limit the damage, we
also provide multi-valued versions. So rather than:
st_add(st_add(a, b), st_add(c, d));
you can write:
st_add4(a, b, c, d);
This isn't nearly as elegant as a varargs function, but it's
a lot harder to get it wrong. You don't have to remember to
add a sentinel value at the end, and the compiler will
complain if you get the number of arguments wrong. This
patch adds only the numbered variants required to convert
the current code base; we can easily add more later if
needed.
Signed-off-by: Jeff King <redacted>
---
git-compat-util.h | 34 ++++++++++++++++++++++++++++++++++
1 file changed, 34 insertions(+)
From: Jeff King <hidden> Date: 2016-06-15 23:08:19
You can tweak the reflog expiration for a particular subset
of refs by configuring gc.foo.reflogexpire. We keep a linked
list of reflog_expire_cfg structs, each of which holds the
pattern and a "len" field for the length of the pattern. The
pattern itself is _not_ NUL-terminated.
However, we feed the pattern directly to wildmatch(), which
expects a NUL-terminated string, meaning it may keep reading
random junk after our struct.
We can fix this by allocating an extra byte for the NUL
(which is already zero because we use xcalloc). Let's also
drop the misleading "len" field, which is no longer
necessary. The existing use of "len" can be converted to use
strncmp().
Signed-off-by: Jeff King <redacted>
---
builtin/reflog.c | 8 +++-----
1 file changed, 3 insertions(+), 5 deletions(-)
From: Jeff King <hidden> Date: 2016-06-15 23:08:19
A combine_diff_path struct has two "flex" members allocated
alongside the struct: a string to hold the pathname, and an
array of parent pointers. We use an "int" to compute this,
meaning we may easily overflow it if the pathname is
extremely long.
We can fix this by using size_t, and checking for overflow
with the st_add helper.
Signed-off-by: Jeff King <redacted>
---
diff.h | 4 ++--
tree-diff.c | 4 ++--
2 files changed, 4 insertions(+), 4 deletions(-)
@@ -124,8 +124,8 @@ static struct combine_diff_path *path_appendnew(struct combine_diff_path *last,unsignedmode,constunsignedchar*sha1){structcombine_diff_path*p;-intlen=base->len+pathlen;-intalloclen=combine_diff_path_size(nparent,len);+size_tlen=st_add(base->len,pathlen);+size_talloclen=combine_diff_path_size(nparent,len);/* if last->next is !NULL - it is a pre-allocated memory, we can reuse */p=last->next;
From: Jeff King <hidden> Date: 2016-06-15 23:08:19
REALLOC_ARRAY inherently involves a multiplication which can
overflow size_t, resulting in a much smaller buffer than we
think we've allocated. We can easily harden it by using
st_mult() to check for overflow. Likewise, we can add
ALLOC_ARRAY to do the same thing for xmalloc calls.
xcalloc() should already be fine, because it takes the two
factors separately, assuming the system calloc actually
checks for overflow. However, before we even hit the system
calloc(), we do our memory_limit_check, which involves a
multiplication. Let's check for overflow ourselves so that
this limit cannot be bypassed.
Signed-off-by: Jeff King <redacted>
---
git-compat-util.h | 3 ++-
wrapper.c | 3 +++
2 files changed, 5 insertions(+), 1 deletion(-)
@@ -152,6 +152,9 @@ void *xcalloc(size_t nmemb, size_t size){void*ret;+if(unsigned_mult_overflows(nmemb,size))+die("data too large to fit into virtual memory space");+memory_limit_check(size*nmemb,0);ret=calloc(nmemb,size);if(!ret&&(!nmemb||!size))
From: Jeff King <hidden> Date: 2016-06-15 23:08:19
Allocating a struct with a flex array is pretty simple in
practice: you over-allocate the struct, then copy some data
into the over-allocation. But it can be a slight pain to
make sure you're allocating and copying the right amounts.
This patch adds a few helpers to turn simple cases of
flex-array struct allocation into a one-liner that properly
checks for overflow. See the embedded documentation for
details.
Ideally we could provide a more flexible version that could
handle multiple strings, like:
FLEX_ALLOC_FMT(ref, name, "%s%s", prefix, name);
But we have to implement this as a macro (because of the
offset calculation of the flex member), which means we would
need all compilers to support variadic macros.
Signed-off-by: Jeff King <redacted>
---
git-compat-util.h | 62 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
1 file changed, 62 insertions(+)
From: Jeff King <hidden> Date: 2016-06-15 23:08:19
There are many manual argv allocations that predate the
argv_array API. Switching to that API brings a few
advantages:
1. We no longer have to manually compute the correct final
array size (so it's one less thing we can screw up).
2. In many cases we had to make a separate pass to count,
then allocate, then fill in the array. Now we can do it
in one pass, making the code shorter and easier to
follow.
3. argv_array handles memory ownership for us, making it
more obvious when things should be free()d and and when
not.
Most of these cases are pretty straightforward. In some, we
switch from "run_command_v" to "run_command" which lets us
directly use the argv_array embedded in "struct
child_process".
Signed-off-by: Jeff King <redacted>
---
builtin/grep.c | 10 +++++-----
builtin/receive-pack.c | 12 +++---------
builtin/remote-ext.c | 26 +++++---------------------
daemon.c | 12 +++++-------
git.c | 14 +++++---------
line-log.c | 26 ++++++++++----------------
remote-curl.c | 23 +++++++++++------------
7 files changed, 44 insertions(+), 79 deletions(-)
@@ -114,30 +114,14 @@ static char *strip_escapes(const char *str, const char *service,}}-/* Should be enough... */-#define MAXARGUMENTS 256--staticconstchar**parse_argv(constchar*arg,constchar*service)+staticvoidparse_argv(structargv_array*out,constchar*arg,constchar*service){-intarguments=0;-inti;-constchar**ret;-char*temparray[MAXARGUMENTS+1];-while(*arg){-char*expanded;-if(arguments==MAXARGUMENTS)-die("remote-ext command has too many arguments");-expanded=strip_escapes(arg,service,&arg);+char*expanded=strip_escapes(arg,service,&arg);if(expanded)-temparray[arguments++]=expanded;+argv_array_push(out,expanded);+free(expanded);}--ret=xmalloc((arguments+1)*sizeof(char*));-for(i=0;i<arguments;i++)-ret[i]=temparray[i];-ret[arguments]=NULL;-returnret;}staticvoidsend_git_request(intstdin_fd,constchar*serv,constchar*repo,
@@ -158,7 +142,7 @@ static int run_child(const char *arg, const char *service)child.in=-1;child.out=-1;child.err=0;-child.argv=parse_argv(arg,service);+parse_argv(&child.args,arg,service);if(start_command(&child)<0)die("Can't run specified command");
From: Jeff King <hidden> Date: 2016-06-15 23:08:19
We frequently allocate strings as xmalloc(len + 1), where
the extra 1 is for the NUL terminator. This can be done more
simply with xmallocz, which also checks for integer
overflow.
There's no case where switching xmalloc(n+1) to xmallocz(n)
is wrong; the result is the same length, and malloc made no
guarantees about what was in the buffer anyway. But in some
cases, we can stop manually placing NUL at the end of the
allocated buffer. But that's only safe if it's clear that
the contents will always fill the buffer.
In each case where this patch does so, I manually examined
the control flow, and I tried to err on the side of caution.
Signed-off-by: Jeff King <redacted>
---
builtin/check-ref-format.c | 2 +-
builtin/merge-tree.c | 2 +-
builtin/worktree.c | 2 +-
column.c | 3 +--
combine-diff.c | 4 +---
config.c | 4 +---
dir.c | 2 +-
entry.c | 2 +-
grep.c | 3 +--
imap-send.c | 5 ++---
ll-merge.c | 2 +-
progress.c | 2 +-
refs.c | 2 +-
setup.c | 5 ++---
strbuf.c | 2 +-
15 files changed, 17 insertions(+), 25 deletions(-)
@@ -1051,8 +1051,6 @@ static void show_patch_diff(struct combine_diff_path *elem, int num_parent,elseif(done<len)die("early EOF '%s'",elem->path);-result[len]=0;-/* If not a fake symlink, apply filters, e.g. autocrlf */if(is_file){structstrbufbuf=STRBUF_INIT;
@@ -978,7 +978,7 @@ static wchar_t *make_environment_block(char **deltaenv)i++;/* copy the environment, leaving space for changes */-tmpenv=xmalloc((size+i)*sizeof(char*));+ALLOC_ARRAY(tmpenv,size+i);memcpy(tmpenv,environ,size*sizeof(char*));/* merge supplied environment changes into the temporary environment */
@@ -1168,7 +1168,7 @@ static int try_shell_exec(const char *cmd, char *const *argv)intargc=0;constchar**argv2;while(argv[argc])argc++;-argv2=xmalloc(sizeof(*argv)*(argc+1));+ALLOC_ARRAY(argv2,argc+1);argv2[0]=(char*)cmd;/* full path to the script file */memcpy(&argv2[1],&argv[1],sizeof(*argv)*argc);pid=mingw_spawnv(prog,argv2,1);
@@ -117,7 +117,7 @@ static const double __ac_HASH_UPPER = 0.77;if(new_n_buckets<4)new_n_buckets=4;\if(h->size>=(khint_t)(new_n_buckets*__ac_HASH_UPPER+0.5))j=0;/* requested size is too small */\else{/* hash table size to be changed (shrink or expand); rehash */\-new_flags=(khint32_t*)xmalloc(__ac_fsize(new_n_buckets)*sizeof(khint32_t));\+ALLOC_ARRAY(new_flags,__ac_fsize(new_n_buckets));\if(!new_flags)return-1;\memset(new_flags,0xaa,__ac_fsize(new_n_buckets)*sizeof(khint32_t));\if(h->n_buckets<new_n_buckets){/* expand */\
@@ -89,7 +89,7 @@ static int verify_packfile(struct packed_git *p,*wedonotdoscan-streamingcheckonthepackfile.*/nr_objects=p->num_objects;-entries=xmalloc((nr_objects+1)*sizeof(*entries));+ALLOC_ARRAY(entries,nr_objects+1);entries[nr_objects].offset=pack_sig_ofs;/* first sort entries by pack offset, since unpacking them is more efficient that way */for(i=0;i<nr_objects;i++){
@@ -696,9 +696,10 @@ static int rpc_service(struct rpc_state *rpc, struct discovery *heads)staticintfetch_dumb(intnr_heads,structref**to_fetch){structwalker*walker;-char**targets=xmalloc(nr_heads*sizeof(char*));+char**targets;intret,i;+ALLOC_ARRAY(targets,nr_heads);if(options.depth)die("dumb http transport does not support --depth");for(i=0;i<nr_heads;i++)
@@ -939,7 +939,7 @@ static int setup_with_upstream(const char ***argv)if(!branch->merge_nr)die(_("No default upstream defined for the current branch."));-args=xcalloc(branch->merge_nr+1,sizeof(char*));+args=xcalloc(st_add(branch->merge_nr,1),sizeof(char*));for(i=0;i<branch->merge_nr;i++){if(!branch->merge[i]->dst)die(_("No remote-tracking branch for %s from %s"),
@@ -1111,7 +1111,7 @@ static void show_patch_diff(struct combine_diff_path *elem, int num_parent,if(result_size&&result[result_size-1]!='\n')cnt++;/* incomplete line */-sline=xcalloc(cnt+2,sizeof(*sline));+sline=xcalloc(st_add(cnt,2),sizeof(*sline));sline[0].bol=result;for(lno=0,cp=result;cp<result+result_size;cp++){if(*cp=='\n'){
@@ -1130,7 +1130,7 @@ static void show_patch_diff(struct combine_diff_path *elem, int num_parent,/* Even p_lno[cnt+1] is valid -- that is for the end line number*fordeletionhunkattheend.*/-sline[0].p_lno=xcalloc((cnt+2)*num_parent,sizeof(unsignedlong));+sline[0].p_lno=xcalloc(st_mult(st_add(cnt,2),num_parent),sizeof(unsignedlong));for(lno=0;lno<=cnt;lno++)sline[lno+1].p_lno=sline[lno].p_lno+num_parent;
@@ -2197,10 +2189,9 @@ static int pack_if_possible_fn(struct ref_entry *entry, void *cb_data)/* Schedule the loose reference for pruning if requested. */if((cb->flags&PACK_REFS_PRUNE)){-intnamelen=strlen(entry->name)+1;-structref_to_prune*n=xcalloc(1,sizeof(*n)+namelen);+structref_to_prune*n;+FLEX_ALLOC_STR(n,name,entry->name);hashcpy(n->sha1,entry->u.value.oid.hash);-memcpy(n->name,entry->name,namelen);/* includes NUL */n->next=cb->ref_to_prune;cb->ref_to_prune=n;}
@@ -2136,16 +2136,13 @@ static int one_local_ref(const char *refname, const struct object_id *oid,{structref***local_tail=cb_data;structref*ref;-intlen;/* we already know it starts with refs/ to get here */if(check_refname_format(refname+5,0))return0;-len=strlen(refname)+1;-ref=xcalloc(1,sizeof(*ref)+len);+ref=alloc_ref(refname);oidcpy(&ref->new_oid,oid);-memcpy(ref->name,refname,len);**local_tail=ref;*local_tail=&ref->next;return0;
From: Jeff King <hidden> Date: 2016-06-15 23:08:19
These functions transform an existing argv into one suitable
for exec-ing or spawning via git or a shell. We can use an
argv_array in each to avoid dealing with manual counting and
allocation.
This also makes the memory allocation more clear and fixes
some leaks. In prepare_shell_cmd, we would sometimes
allocate a new string with "$@" in it and sometimes not,
meaning the caller could not correctly free it. On the
non-Windows side, we are in a child process which will
exec() or exit() immediately, so the leak isn't a big deal.
On Windows, though, we use spawn() from the parent process,
and leak a string for each shell command we run. On top of
that, the Windows code did not free the allocated argv array
at all (but does for the prepare_git_cmd case!).
By switching both of these functions to write into an
argv_array, we can consistently free the result as
appropriate.
Signed-off-by: Jeff King <redacted>
---
Note that I had to touch the Windows run-command code here, but I don't
actually have a platform to test it on.
exec_cmd.c | 28 +++++++++++-----------------
exec_cmd.h | 4 +++-
run-command.c | 60 +++++++++++++++++++++++++----------------------------------
3 files changed, 39 insertions(+), 53 deletions(-)
@@ -107,32 +108,25 @@ void setup_path(void)strbuf_release(&new_path);}-constchar**prepare_git_cmd(constchar**argv)+constchar**prepare_git_cmd(structargv_array*out,constchar**argv){-intargc;-constchar**nargv;--for(argc=0;argv[argc];argc++)-;/* just counting */-nargv=xmalloc(sizeof(*nargv)*(argc+2));--nargv[0]="git";-for(argc=0;argv[argc];argc++)-nargv[argc+1]=argv[argc];-nargv[argc+1]=NULL;-returnnargv;+argv_array_push(out,"git");+argv_array_pushv(out,argv);+returnout->argv;}intexecv_git_cmd(constchar**argv){-constchar**nargv=prepare_git_cmd(argv);-trace_argv_printf(nargv,"trace: exec:");+structargv_arraynargv=ARGV_ARRAY_INIT;++prepare_git_cmd(&nargv,argv);+trace_argv_printf(nargv.argv,"trace: exec:");/* execvp() can only ever return if it fails */-sane_execvp("git",(char**)nargv);+sane_execvp("git",(char**)nargv.argv);trace_printf("trace: exec failed: %s\n",strerror(errno));-free(nargv);+argv_array_clear(&nargv);return-1;}
From: Jeff King <hidden> Date: 2016-06-15 23:08:19
We have two variants of this function, one that takes a
string and one that takes a ptr/len combo. But we only call
the latter with the length of a NUL-terminated string, so
our first simplification is to drop it in favor of the
string variant.
Since we know we have a string, we can also replace the
manual memory computation with a call to alloc_ref().
Furthermore, we can rely on get_oid_hex() to complain if it
hits the end of the string. That means we can simplify the
check for "<sha1> <ref>" versus just "<ref>". Rather than
manage the ptr/len pair, we can just bump the start of our
string forward. The original code over-allocated based on
the original "namelen" (which wasn't _wrong_, but was simply
wasteful and confusing).
Signed-off-by: Jeff King <redacted>
---
builtin/fetch-pack.c | 27 +++++++++------------------
1 file changed, 9 insertions(+), 18 deletions(-)
From: Jeff King <hidden> Date: 2016-06-15 23:08:19
This function allocate a packed_git flex-array, and adds a
mysterious 2 bytes to the length of the pack_name field. One
is for the trailing NUL, but the other has no purpose. This
is probably cargo-culted from add_packed_git, which gets the
".idx" path and needed to allocate enough space to hold the
matching ".pack" (though since 48bcc1c, we calculate the
size there differently).
This site, however, is using the raw path of a tempfile, and
does not need the extra byte. We can just replace the
allocation with FLEX_ALLOC_STR, which handles the allocation
and the NUL for us.
Signed-off-by: Jeff King <redacted>
---
fast-import.c | 5 +----
1 file changed, 1 insertion(+), 4 deletions(-)
From: Jeff King <hidden> Date: 2016-06-15 23:08:19
We perform unchecked additions when computing the size of a
"struct ondisk_untracked_cache". This is unlikely to have an
integer overflow in practice, but we'd like to avoid this
dangerous pattern to make further audits easier.
Note that there's one subtlety here, though. We protect
ourselves against a NULL exclude_per_dir entry in our
source, and avoid calling strlen() on it, keeping "len" at
0. But later, we unconditionally memcpy "len + 1" bytes to
get the trailing NUL byte. If we did have a NULL
exclude_per_dir, we would read from bogus memory.
As it turns out, though, we always create this field
pointing to a string literal, so there's no bug. We can just
get rid of the pointless extra conditional.
Signed-off-by: Jeff King <redacted>
---
dir.c | 9 ++++-----
1 file changed, 4 insertions(+), 5 deletions(-)
From: Jeff King <hidden> Date: 2016-06-15 23:08:19
The normalize_path_copy function needs an output buffer that
is at least as long as its input (it may shrink the path,
but never expand it). However, this test program feeds it
static PATH_MAX-sized buffers, which have no relation to the
input size.
In the normalize_ceiling_entry case, we do at least check
the size against PATH_MAX and die(), but that case is even
more convoluted. We normalize into a fixed-size buffer, free
the original, and then replace it with a strdup'd copy of
the result. But normalize_path_copy explicitly allows
normalizing in-place, so we can simply do that.
Signed-off-by: Jeff King <redacted>
---
test-path-utils.c | 15 ++++-----------
1 file changed, 4 insertions(+), 11 deletions(-)
@@ -8,21 +8,14 @@*/staticintnormalize_ceiling_entry(structstring_list_item*item,void*unused){-constchar*ceil=item->string;-intlen=strlen(ceil);-charbuf[PATH_MAX+1];+char*ceil=item->string;-if(len==0)+if(!*ceil)die("Empty path is not supported");-if(len>PATH_MAX)-die("Path \"%s\" is too long",ceil);if(!is_absolute_path(ceil))die("Path \"%s\" is not absolute",ceil);-if(normalize_path_copy(buf,ceil)<0)+if(normalize_path_copy(ceil,ceil)<0)die("Path \"%s\" could not be normalized",ceil);-len=strlen(buf);-free(item->string);-item->string=xstrdup(buf);return1;}
From: Jeff King <hidden> Date: 2016-06-15 23:08:19
For a commit with sha1 "1234abcd" and subject "foo", this
function produces a struct with three strings:
1. "foo"
2. "1234abcd... foo"
3. "parent of 1234abcd... foo"
It takes advantage of the fact that these strings are
subsets of each other, and allocates only _one_ string, with
pointers into the various parts. Unfortunately, this makes
the string allocation complicated and hard to follow.
Since we keep only one of these in memory at a time, we can
afford to simply allocate three strings. This lets us build
on tools like xstrfmt and avoid manual computation.
While we're here, we can also drop the ad-hoc
reimplementation of get_git_commit_encoding(), and simply
call that function.
Signed-off-by: Jeff King <redacted>
---
sequencer.c | 29 ++++++++++-------------------
1 file changed, 10 insertions(+), 19 deletions(-)
From: Jeff King <hidden> Date: 2016-06-15 23:08:19
There are no callers of this left, as the last one was
dropped in the previous patch. And there are not likely to
be new ones, as the function has been around since 2010
without gaining any new callers.
Signed-off-by: Jeff King <redacted>
---
git-compat-util.h | 9 ---------
1 file changed, 9 deletions(-)
From: Jeff King <hidden> Date: 2016-06-15 23:08:19
We allocate 100 bytes to hold the "Submodule commit ..."
text. This is enough, but it's not immediately obvious that
this is the case, and we have to repeat the magic 100 twice.
We could get away with xstrfmt here, but we want to know the
size, as well, so let's use a real strbuf. And while we're
here, we can clean up the logic around size_only. It
currently sets and clears the "data" field pointlessly, and
leaves the "should_free" flag on even after we have cleared
the data.
Signed-off-by: Jeff King <redacted>
---
diff.c | 16 ++++++++--------
1 file changed, 8 insertions(+), 8 deletions(-)
@@ -2704,21 +2704,21 @@ static int reuse_worktree_file(const char *name, const unsigned char *sha1, intstaticintdiff_populate_gitlink(structdiff_filespec*s,intsize_only){-intlen;-char*data=xmalloc(100),*dirty="";+structstrbufbuf=STRBUF_INIT;+char*dirty="";/* Are we looking at the work tree? */if(s->dirty_submodule)dirty="-dirty";-len=snprintf(data,100,-"Subproject commit %s%s\n",sha1_to_hex(s->sha1),dirty);-s->data=data;-s->size=len;-s->should_free=1;+strbuf_addf(&buf,"Subproject commit %s%s\n",sha1_to_hex(s->sha1),dirty);+s->size=buf.len;if(size_only){s->data=NULL;-free(data);+strbuf_release(&buf);+}else{+s->data=strbuf_detach(&buf,NULL);+s->should_free=1;}return0;}
From: Jeff King <hidden> Date: 2016-06-15 23:08:19
This function uses xcalloc and two memcpy calls to
concatenate two strings. We can do this as an xstrfmt
one-liner, and then it is more clear that we are allocating
the correct amount of memory.
Signed-off-by: Jeff King <redacted>
---
transport.c | 8 +++-----
1 file changed, 3 insertions(+), 5 deletions(-)
@@ -1021,7 +1021,7 @@ int transport_disconnect(struct transport *transport)*/char*transport_anonymize_url(constchar*url){-char*anon_url,*scheme_prefix,*anon_part;+char*scheme_prefix,*anon_part;size_tanon_len,prefix_len=0;anon_part=strchr(url,'@');
From: Jeff King <hidden> Date: 2016-06-15 23:08:19
This code was originally written with the idea that it could
be spun off into its own ewah library, and uses the
overrideable ewah_malloc to do allocations.
We plug in xmalloc as our ewah_malloc, of course. But over
the years the ewah code itself has become more entangled
with git, and the return value of many ewah_malloc sites is
not checked.
Let's just drop the level of indirection and use xmalloc and
friends directly. This saves a few lines, and will let us
adapt these sites to our more advanced malloc helpers.
Signed-off-by: Jeff King <redacted>
---
ewah/bitmap.c | 12 ++++++------
ewah/ewah_bitmap.c | 9 +++------
ewah/ewah_io.c | 10 ++--------
ewah/ewok.h | 10 ----------
4 files changed, 11 insertions(+), 30 deletions(-)
@@ -180,12 +177,9 @@ int ewah_deserialize(struct ewah_bitmap *self, int fd)return-1;self->buffer_size=self->alloc_size=(size_t)ntohl(word_count);-self->buffer=ewah_realloc(self->buffer,+self->buffer=xrealloc(self->buffer,self->alloc_size*sizeof(eword_t));-if(!self->buffer)-return-1;-/** 64 bit x N -- compressed words */buffer=self->buffer;words_left=self->buffer_size;
From: Jeff King <hidden> Date: 2016-06-15 23:08:19
Now that we're built around xmalloc and friends, we can use
helpers like REALLOC_ARRAY, ALLOC_GROW, and so on to make
the code shorter and protect against integer overflow.
Signed-off-by: Jeff King <redacted>
---
ewah/bitmap.c | 16 ++++------------
ewah/ewah_bitmap.c | 5 ++---
ewah/ewah_io.c | 6 ++----
3 files changed, 8 insertions(+), 19 deletions(-)
@@ -177,8 +176,7 @@ int ewah_deserialize(struct ewah_bitmap *self, int fd)return-1;self->buffer_size=self->alloc_size=(size_t)ntohl(word_count);-self->buffer=xrealloc(self->buffer,-self->alloc_size*sizeof(eword_t));+REALLOC_ARRAY(self->buffer,self->alloc_size);/** 64 bit x N -- compressed words */buffer=self->buffer;
From: Eric Sunshine <hidden> Date: 2016-06-15 23:08:19
On Fri, Feb 19, 2016 at 6:23 AM, Jeff King [off-list ref] wrote:
quoted hunk
There are many manual argv allocations that predate the
argv_array API. Switching to that API brings a few
advantages:
1. We no longer have to manually compute the correct final
array size (so it's one less thing we can screw up).
2. In many cases we had to make a separate pass to count,
then allocate, then fill in the array. Now we can do it
in one pass, making the code shorter and easier to
follow.
3. argv_array handles memory ownership for us, making it
more obvious when things should be free()d and and when
not.
Most of these cases are pretty straightforward. In some, we
switch from "run_command_v" to "run_command" which lets us
directly use the argv_array embedded in "struct
child_process".
Signed-off-by: Jeff King <redacted>
---
diff --git a/line-log.c b/line-log.c
@@ -746,23 +747,16 @@ void line_log_init(struct rev_info *rev, const char *prefix, struct string_list if (!rev->diffopt.detect_rename) {- int i, count = 0;- struct line_log_data *r = range;- const char **paths;- while (r) {- count++;- r = r->next;- }- paths = xmalloc((count+1)*sizeof(char *));- r = range;- for (i = 0; i < count; i++) {- paths[i] = xstrdup(r->path);- r = r->next;- }- paths[count] = NULL;+ struct line_log_data *r;+ struct argv_array paths = ARGV_ARRAY_INIT;++ for (r = range; r; r = r->next)+ argv_array_push(&paths, r->path); parse_pathspec(&rev->diffopt.pathspec, 0,- PATHSPEC_PREFER_FULL, "", paths);- free(paths);+ PATHSPEC_PREFER_FULL, "", paths.argv);+ /* argv strings are now owned by pathspec */+ paths.argc = 0;+ argv_array_clear(&paths);
This overly intimate knowledge of the internal implementation of
argv_array_clear() is rather ugly.
From: Jeff King <hidden> Date: 2016-06-15 23:08:19
On Sat, Feb 20, 2016 at 03:07:00AM -0500, Eric Sunshine wrote:
quoted
diff --git a/line-log.c b/line-log.c
@@ -746,23 +747,16 @@ void line_log_init(struct rev_info *rev, const char *prefix, struct string_list if (!rev->diffopt.detect_rename) {- int i, count = 0;- struct line_log_data *r = range;- const char **paths;- while (r) {- count++;- r = r->next;- }- paths = xmalloc((count+1)*sizeof(char *));- r = range;- for (i = 0; i < count; i++) {- paths[i] = xstrdup(r->path);- r = r->next;- }- paths[count] = NULL;+ struct line_log_data *r;+ struct argv_array paths = ARGV_ARRAY_INIT;++ for (r = range; r; r = r->next)+ argv_array_push(&paths, r->path); parse_pathspec(&rev->diffopt.pathspec, 0,- PATHSPEC_PREFER_FULL, "", paths);- free(paths);+ PATHSPEC_PREFER_FULL, "", paths.argv);+ /* argv strings are now owned by pathspec */+ paths.argc = 0;+ argv_array_clear(&paths);
This overly intimate knowledge of the internal implementation of
argv_array_clear() is rather ugly.
Yep, I agree. Suggestions?
We can just leak the array of "char *". This function is called only
once per program invocation, and that's unlikely to change.
I guess we can make an argv_array_detach_strings() function. Or maybe
even just argv_array_detach() would be less gross, and then this
function could manually free the array but not the strings themselves.
-Peff
From: Eric Sunshine <hidden> Date: 2016-06-15 23:08:19
On Sat, Feb 20, 2016 at 3:10 AM, Jeff King [off-list ref] wrote:
On Sat, Feb 20, 2016 at 03:07:00AM -0500, Eric Sunshine wrote:
quoted
quoted
+ /* argv strings are now owned by pathspec */
+ paths.argc = 0;
+ argv_array_clear(&paths);
This overly intimate knowledge of the internal implementation of
argv_array_clear() is rather ugly.
Yep, I agree. Suggestions?
We can just leak the array of "char *". This function is called only
once per program invocation, and that's unlikely to change.
I guess we can make an argv_array_detach_strings() function. Or maybe
even just argv_array_detach() would be less gross, and then this
function could manually free the array but not the strings themselves.
The latter is what I was thinking, and I agree that
argv_array_detach() would be less gross than
argv_array_detach_strings(), however, it also feels a bit wrong since
this sort of ownership transfer is kind of out of scope for
argv_array.
I wonder if a simple "dup'ing" string_list would be more suitable for
this case. You'd have to append the NULL item manually with
string_list_append_nodup(), and string_list_clear() would then be the
correct way to dispose of the list without intimate knowledge of its
implementation and no need for an API extension.
From: Jeff King <hidden> Date: 2016-06-15 23:08:19
On Sat, Feb 20, 2016 at 03:29:29AM -0500, Eric Sunshine wrote:
On Sat, Feb 20, 2016 at 3:10 AM, Jeff King [off-list ref] wrote:
quoted
On Sat, Feb 20, 2016 at 03:07:00AM -0500, Eric Sunshine wrote:
quoted
quoted
+ /* argv strings are now owned by pathspec */
+ paths.argc = 0;
+ argv_array_clear(&paths);
This overly intimate knowledge of the internal implementation of
argv_array_clear() is rather ugly.
Yep, I agree. Suggestions?
We can just leak the array of "char *". This function is called only
once per program invocation, and that's unlikely to change.
I guess we can make an argv_array_detach_strings() function. Or maybe
even just argv_array_detach() would be less gross, and then this
function could manually free the array but not the strings themselves.
The latter is what I was thinking, and I agree that
argv_array_detach() would be less gross than
argv_array_detach_strings(), however, it also feels a bit wrong since
this sort of ownership transfer is kind of out of scope for
argv_array.
I wonder if a simple "dup'ing" string_list would be more suitable for
this case. You'd have to append the NULL item manually with
string_list_append_nodup(), and string_list_clear() would then be the
correct way to dispose of the list without intimate knowledge of its
implementation and no need for an API extension.
A string_list doesn't just store pointers; it's a struct with a util
field. So you can't pass it to things expecting a "const char **".
I think argv_array_detach() is the least-bad thing here. It matches
strbuf_detach() to say "you now own the storage" (as opposed to just
peeking at argv.argv, which we should do only in a read-only way).
-Peff
From: Eric Sunshine <hidden> Date: 2016-06-15 23:08:19
On Sat, Feb 20, 2016 at 3:34 AM, Jeff King [off-list ref] wrote:
On Sat, Feb 20, 2016 at 03:29:29AM -0500, Eric Sunshine wrote:
quoted
On Sat, Feb 20, 2016 at 3:10 AM, Jeff King [off-list ref] wrote:
quoted
On Sat, Feb 20, 2016 at 03:07:00AM -0500, Eric Sunshine wrote:
quoted
quoted
+ /* argv strings are now owned by pathspec */
+ paths.argc = 0;
+ argv_array_clear(&paths);
This overly intimate knowledge of the internal implementation of
argv_array_clear() is rather ugly.
Yep, I agree. Suggestions?
[...]
I guess we can make an argv_array_detach_strings() function. Or maybe
even just argv_array_detach() would be less gross, and then this
function could manually free the array but not the strings themselves.
[...]
I wonder if a simple "dup'ing" string_list would be more suitable for
this case. You'd have to append the NULL item manually with
string_list_append_nodup(), and string_list_clear() would then be the
correct way to dispose of the list without intimate knowledge of its
implementation and no need for an API extension.
A string_list doesn't just store pointers; it's a struct with a util
field. So you can't pass it to things expecting a "const char **".
Yep, I knew that but wasn't thinking straight.
I think argv_array_detach() is the least-bad thing here. It matches
strbuf_detach() to say "you now own the storage" (as opposed to just
peeking at argv.argv, which we should do only in a read-only way).
I also had made the strbuf_detach() analogy in my response but deleted
it before sending; I do think it's a reasonable API template to mirror
via new argv_array_detach().
From: Jeff King <hidden> Date: 2016-06-15 23:08:20
On Sat, Feb 20, 2016 at 03:39:36AM -0500, Eric Sunshine wrote:
I also had made the strbuf_detach() analogy in my response but deleted
it before sending; I do think it's a reasonable API template to mirror
via new argv_array_detach().
That would look like this, which I think is not too bad (on top of my
series for now; I'd do the API function as a separate patch at the
beginning and then use it immediately).
@@ -748,15 +748,17 @@ void line_log_init(struct rev_info *rev, const char *prefix, struct string_listif(!rev->diffopt.detect_rename){structline_log_data*r;-structargv_arraypaths=ARGV_ARRAY_INIT;+structargv_arrayarray=ARGV_ARRAY_INIT;+constchar**paths;for(r=range;r;r=r->next)-argv_array_push(&paths,r->path);+argv_array_push(&array,r->path);+paths=argv_array_detach(&array);+parse_pathspec(&rev->diffopt.pathspec,0,-PATHSPEC_PREFER_FULL,"",paths.argv);-/* argv strings are now owned by pathspec */-paths.argc=0;-argv_array_clear(&paths);+PATHSPEC_PREFER_FULL,"",paths);+/* strings are now owned by pathspec */+free(paths);}}
From: Eric Sunshine <hidden> Date: 2016-06-15 23:08:20
On Sat, Feb 20, 2016 at 3:57 AM, Jeff King [off-list ref] wrote:
On Sat, Feb 20, 2016 at 03:39:36AM -0500, Eric Sunshine wrote:
quoted
I also had made the strbuf_detach() analogy in my response but deleted
it before sending; I do think it's a reasonable API template to mirror
via new argv_array_detach().
That would look like this, which I think is not too bad (on top of my
series for now; I'd do the API function as a separate patch at the
beginning and then use it immediately).
@@ -748,15 +748,17 @@ void line_log_init(struct rev_info *rev, const char *prefix, struct string_listif(!rev->diffopt.detect_rename){structline_log_data*r;-structargv_arraypaths=ARGV_ARRAY_INIT;+structargv_arrayarray=ARGV_ARRAY_INIT;+constchar**paths;for(r=range;r;r=r->next)-argv_array_push(&paths,r->path);+argv_array_push(&array,r->path);+paths=argv_array_detach(&array);+parse_pathspec(&rev->diffopt.pathspec,0,-PATHSPEC_PREFER_FULL,"",paths.argv);-/* argv strings are now owned by pathspec */-paths.argc=0;-argv_array_clear(&paths);+PATHSPEC_PREFER_FULL,"",paths);+/* strings are now owned by pathspec */+free(paths);}}
From: René Scharfe <hidden> Date: 2016-06-15 23:08:20
Am 19.02.2016 um 12:22 schrieb Jeff King:
REALLOC_ARRAY inherently involves a multiplication which can
overflow size_t, resulting in a much smaller buffer than we
think we've allocated. We can easily harden it by using
st_mult() to check for overflow. Likewise, we can add
ALLOC_ARRAY to do the same thing for xmalloc calls.
Good idea!
quoted hunk
xcalloc() should already be fine, because it takes the two
factors separately, assuming the system calloc actually
checks for overflow. However, before we even hit the system
calloc(), we do our memory_limit_check, which involves a
multiplication. Let's check for overflow ourselves so that
this limit cannot be bypassed.
Signed-off-by: Jeff King <redacted>
---
git-compat-util.h | 3 ++-
wrapper.c | 3 +++
2 files changed, 5 insertions(+), 1 deletion(-)
st_mult(x, y) calls unsigned_mult_overflows(x, y), which divides by x.
This division can be done at compile time if x is a constant. This can
be guaranteed for all users of the two macros above by reversing the
arguments of st_mult(), so that sizeof comes first. Probably not a big
win, but why not do it if it's that easy?
Or perhaps a macro like this could help here and in other places which
use st_mult with sizeof:
#define SIZEOF_MULT(x, n) st_mult(sizeof(x), (n))
(I'd call it ARRAY_SIZE, but that name is already taken. :)
René
st_mult(x, y) calls unsigned_mult_overflows(x, y), which divides by x. This
division can be done at compile time if x is a constant. This can be
guaranteed for all users of the two macros above by reversing the arguments
of st_mult(), so that sizeof comes first. Probably not a big win, but why
not do it if it's that easy?
I doubt it's even measurable, but as you say, it's easy enough to do, so
why not.
If we really care about optimizing, I suspect that something like:
would do a lot more. But it needs #ifdefs for compilers besides gcc and
clang.
Or perhaps a macro like this could help here and in other places which use
st_mult with sizeof:
#define SIZEOF_MULT(x, n) st_mult(sizeof(x), (n))
(I'd call it ARRAY_SIZE, but that name is already taken. :)
I don't think we need that; we're really only checking allocations,
which means ALLOC_ARRAY() and friends cover all the uses of sizeof (we
might still use st_mult() for _another_ part of the computation, but it
won't usually be a sizeof then).
We also may do follow-up multiplications, like:
ALLOC_ARRAY(foo, nr);
memset(foo, 0, nr * sizeof(*foo));
but I didn't bother doing overflow checks for those. We know that they
should be fine if the original allocation was successful (though of
course, just using xcalloc here would be better still).
-Peff
And here's v3. The changes this time (and philosophical rationalization
of changes I didn't make) are pretty small:
- flip the order of arguments to st_mult() in ALLOC_ARRAY, et al, to
get a probably-irrelevant-but-so-easy-why-not optimization
- mark new global var in daemon.c as static
- add argv_array_detach (new patch 6) to avoid gross memory management
when converting line-log to use argv_array in the subsequent patch
So I hope this one is ready for 'next'. Thanks René, Ramsay, and Eric
for reviewing.
[01/22]: reflog_expire_cfg: NUL-terminate pattern field
[02/22]: add helpers for detecting size_t overflow
[03/22]: tree-diff: catch integer overflow in combine_diff_path allocation
[04/22]: harden REALLOC_ARRAY and xcalloc against size_t overflow
[05/22]: add helpers for allocating flex-array structs
[06/22]: argv-array: add detach function
[07/22]: convert manual allocations to argv_array
[08/22]: convert trivial cases to ALLOC_ARRAY
[09/22]: use xmallocz to avoid size arithmetic
[10/22]: convert trivial cases to FLEX_ARRAY macros
[11/22]: use st_add and st_mult for allocation size computation
[12/22]: prepare_{git,shell}_cmd: use argv_array
[13/22]: write_untracked_extension: use FLEX_ALLOC helper
[14/22]: fast-import: simplify allocation in start_packfile
[15/22]: fetch-pack: simplify add_sought_entry
[16/22]: test-path-utils: fix normalize_path_copy output buffer size
[17/22]: sequencer: simplify memory allocation of get_message
[18/22]: git-compat-util: drop mempcpy compat code
[19/22]: transport_anonymize_url: use xstrfmt
[20/22]: diff_populate_gitlink: use a strbuf
[21/22]: convert ewah/bitmap code to use xmalloc
[22/22]: ewah: convert to REALLOC_ARRAY, etc
-Peff
From: Jeff King <hidden> Date: 2016-06-15 23:08:21
You can tweak the reflog expiration for a particular subset
of refs by configuring gc.foo.reflogexpire. We keep a linked
list of reflog_expire_cfg structs, each of which holds the
pattern and a "len" field for the length of the pattern. The
pattern itself is _not_ NUL-terminated.
However, we feed the pattern directly to wildmatch(), which
expects a NUL-terminated string, meaning it may keep reading
random junk after our struct.
We can fix this by allocating an extra byte for the NUL
(which is already zero because we use xcalloc). Let's also
drop the misleading "len" field, which is no longer
necessary. The existing use of "len" can be converted to use
strncmp().
Signed-off-by: Jeff King <redacted>
---
builtin/reflog.c | 8 +++-----
1 file changed, 3 insertions(+), 5 deletions(-)
From: Jeff King <hidden> Date: 2016-06-15 23:08:21
Performing computations on size_t variables that we feed to
xmalloc and friends can be dangerous, as an integer overflow
can cause us to allocate a much smaller chunk than we
realized.
We already have unsigned_add_overflows(), but let's add
unsigned_mult_overflows() to that. Furthermore, rather than
have each site manually check and die on overflow, we can
provide some helpers that will:
- promote the arguments to size_t, so that we know we are
doing our computation in the same size of integer that
will ultimately be fed to xmalloc
- check and die on overflow
- return the result so that computations can be done in
the parameter list of xmalloc.
These functions are a lot uglier to use than normal
arithmetic operators (you have to do "st_add(foo, bar)"
instead of "foo + bar"). To at least limit the damage, we
also provide multi-valued versions. So rather than:
st_add(st_add(a, b), st_add(c, d));
you can write:
st_add4(a, b, c, d);
This isn't nearly as elegant as a varargs function, but it's
a lot harder to get it wrong. You don't have to remember to
add a sentinel value at the end, and the compiler will
complain if you get the number of arguments wrong. This
patch adds only the numbered variants required to convert
the current code base; we can easily add more later if
needed.
Signed-off-by: Jeff King <redacted>
---
git-compat-util.h | 34 ++++++++++++++++++++++++++++++++++
1 file changed, 34 insertions(+)
From: Jeff King <hidden> Date: 2016-06-15 23:08:21
Allocating a struct with a flex array is pretty simple in
practice: you over-allocate the struct, then copy some data
into the over-allocation. But it can be a slight pain to
make sure you're allocating and copying the right amounts.
This patch adds a few helpers to turn simple cases of
flex-array struct allocation into a one-liner that properly
checks for overflow. See the embedded documentation for
details.
Ideally we could provide a more flexible version that could
handle multiple strings, like:
FLEX_ALLOC_FMT(ref, name, "%s%s", prefix, name);
But we have to implement this as a macro (because of the
offset calculation of the flex member), which means we would
need all compilers to support variadic macros.
Signed-off-by: Jeff King <redacted>
---
git-compat-util.h | 62 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
1 file changed, 62 insertions(+)
From: Jeff King <hidden> Date: 2016-06-15 23:08:22
REALLOC_ARRAY inherently involves a multiplication which can
overflow size_t, resulting in a much smaller buffer than we
think we've allocated. We can easily harden it by using
st_mult() to check for overflow. Likewise, we can add
ALLOC_ARRAY to do the same thing for xmalloc calls.
xcalloc() should already be fine, because it takes the two
factors separately, assuming the system calloc actually
checks for overflow. However, before we even hit the system
calloc(), we do our memory_limit_check, which involves a
multiplication. Let's check for overflow ourselves so that
this limit cannot be bypassed.
Signed-off-by: Jeff King <redacted>
---
git-compat-util.h | 3 ++-
wrapper.c | 3 +++
2 files changed, 5 insertions(+), 1 deletion(-)
@@ -152,6 +152,9 @@ void *xcalloc(size_t nmemb, size_t size){void*ret;+if(unsigned_mult_overflows(nmemb,size))+die("data too large to fit into virtual memory space");+memory_limit_check(size*nmemb,0);ret=calloc(nmemb,size);if(!ret&&(!nmemb||!size))
From: Jeff King <hidden> Date: 2016-06-15 23:08:22
A combine_diff_path struct has two "flex" members allocated
alongside the struct: a string to hold the pathname, and an
array of parent pointers. We use an "int" to compute this,
meaning we may easily overflow it if the pathname is
extremely long.
We can fix this by using size_t, and checking for overflow
with the st_add helper.
Signed-off-by: Jeff King <redacted>
---
diff.h | 4 ++--
tree-diff.c | 4 ++--
2 files changed, 4 insertions(+), 4 deletions(-)
@@ -124,8 +124,8 @@ static struct combine_diff_path *path_appendnew(struct combine_diff_path *last,unsignedmode,constunsignedchar*sha1){structcombine_diff_path*p;-intlen=base->len+pathlen;-intalloclen=combine_diff_path_size(nparent,len);+size_tlen=st_add(base->len,pathlen);+size_talloclen=combine_diff_path_size(nparent,len);/* if last->next is !NULL - it is a pre-allocated memory, we can reuse */p=last->next;
From: Jeff King <hidden> Date: 2016-06-15 23:08:22
The usual pattern for an argv array is to initialize it,
push in some strings, and then clear it when done. Very
occasionally, though, we must do other exotic things with
the memory, like freeing the list but keeping the strings.
Let's provide a detach function so that callers can make use
of our API to build up the array, and then take ownership of
it.
Signed-off-by: Jeff King <redacted>
---
Documentation/technical/api-argv-array.txt | 7 +++++++
argv-array.c | 11 +++++++++++
argv-array.h | 1 +
3 files changed, 19 insertions(+)
@@ -56,3 +56,10 @@ Functions `argv_array_clear`:: Free all memory associated with the array and return it to the initial, empty state.++`argv_array_detach`::+ Disconnect the `argv` member from the `argv_array` struct and+ return it. The caller is responsible for freeing the memory used+ by the array, and by the strings it references. After detaching,+ the `argv_array` is in a reinitialized state and can be pushed+ into again.
From: Jeff King <hidden> Date: 2016-06-15 23:08:22
There are many manual argv allocations that predate the
argv_array API. Switching to that API brings a few
advantages:
1. We no longer have to manually compute the correct final
array size (so it's one less thing we can screw up).
2. In many cases we had to make a separate pass to count,
then allocate, then fill in the array. Now we can do it
in one pass, making the code shorter and easier to
follow.
3. argv_array handles memory ownership for us, making it
more obvious when things should be free()d and and when
not.
Most of these cases are pretty straightforward. In some, we
switch from "run_command_v" to "run_command" which lets us
directly use the argv_array embedded in "struct
child_process".
Signed-off-by: Jeff King <redacted>
---
builtin/grep.c | 10 +++++-----
builtin/receive-pack.c | 12 +++---------
builtin/remote-ext.c | 26 +++++---------------------
daemon.c | 12 +++++-------
git.c | 14 +++++---------
line-log.c | 22 +++++++++-------------
remote-curl.c | 23 +++++++++++------------
7 files changed, 43 insertions(+), 76 deletions(-)
@@ -114,30 +114,14 @@ static char *strip_escapes(const char *str, const char *service,}}-/* Should be enough... */-#define MAXARGUMENTS 256--staticconstchar**parse_argv(constchar*arg,constchar*service)+staticvoidparse_argv(structargv_array*out,constchar*arg,constchar*service){-intarguments=0;-inti;-constchar**ret;-char*temparray[MAXARGUMENTS+1];-while(*arg){-char*expanded;-if(arguments==MAXARGUMENTS)-die("remote-ext command has too many arguments");-expanded=strip_escapes(arg,service,&arg);+char*expanded=strip_escapes(arg,service,&arg);if(expanded)-temparray[arguments++]=expanded;+argv_array_push(out,expanded);+free(expanded);}--ret=xmalloc((arguments+1)*sizeof(char*));-for(i=0;i<arguments;i++)-ret[i]=temparray[i];-ret[arguments]=NULL;-returnret;}staticvoidsend_git_request(intstdin_fd,constchar*serv,constchar*repo,
@@ -158,7 +142,7 @@ static int run_child(const char *arg, const char *service)child.in=-1;child.out=-1;child.err=0;-child.argv=parse_argv(arg,service);+parse_argv(&child.args,arg,service);if(start_command(&child)<0)die("Can't run specified command");