From: Jeff King <hidden> Date: 2016-06-15 23:06:33
The git code contains a lot of calls to sprintf, strcpy, and other
unchecked string functions. In many cases, these aren't actually
overflows, because some earlier part of the code implies that the copied
content is smaller than the destination buffer. But it's often hard to
tell, because the code enforcing that assumption is far away, or there's
a complicated expression to create a buffer. This makes it difficult to
audit git for buffer overflows, because you can spend a lot of time
chasing down false positives.
My goal with this series was to not only audit each of these sites for
overflows, but to convert them to more modern constructs (e.g.,
strbufs), so that it's easier to do more audits going forward.
There are quite a large number of changes. I've tried to group similar
changes together to make reviewing easier. Here's a rough breakdown:
[01/67]: show-branch: avoid segfault with --reflog of unborn branch
[02/67]: mailsplit: fix FILE* leak in split_maildir
[03/67]: archive-tar: fix minor indentation violation
[04/67]: fsck: don't fsck alternates for connectivity-only check
These are minor bugfixes that I found while digging into various
call-sites. There's no semantic dependency, but some of the later
patches depend on these textually.
[05/67]: add xsnprintf helper function
[06/67]: add git_path_buf helper function
[07/67]: strbuf: make strbuf_complete_line more generic
[08/67]: add reentrant variants of sha1_to_hex and find_unique_abbrev
These four patches introduce infrastructure that will help later
cleanups (alongside existing tools like strbuf, xstrfmt, etc).
[09/67]: fsck: use strbuf to generate alternate directories
[10/67]: mailsplit: make PATH_MAX buffers dynamic
[11/67]: trace: use strbuf for quote_crnl output
[12/67]: progress: store throughput display in a strbuf
[13/67]: test-dump-cache-tree: avoid overflow of cache-tree name
[14/67]: compat/inet_ntop: fix off-by-one in inet_ntop4
These cases are all things that _can_ overflow, given the right
input. But none of them is interesting security-wise because,
because their input is not typically attacker-controlled.
[15/67]: convert trivial sprintf / strcpy calls to xsnprintf
[16/67]: archive-tar: use xsnprintf for trivial formatting
[17/67]: use xsnprintf for generating git object headers
[18/67]: find_short_object_filename: convert sprintf to xsnprintf
[19/67]: stop_progress_msg: convert sprintf to xsnprintf
[20/67]: compat/hstrerror: convert sprintf to snprintf
[21/67]: grep: use xsnprintf to format failure message
[22/67]: entry.c: convert strcpy to xsnprintf
[23/67]: add_packed_git: convert strcpy into xsnprintf
[24/67]: http-push: replace strcat with xsnprintf
[25/67]: receive-pack: convert strncpy to xsnprintf
These cases can all be fixed by using the newly-added xsnprintf. The
trivial conversions are in patch 15, and the rest are cases that
needed a little more cleanup or explanation.
[26/67]: replace trivial malloc + sprintf /strcpy calls to xstrfmt
[27/67]: config: use xstrfmt in normalize_value
[28/67]: fetch: replace static buffer with xstrfmt
[29/67]: use strip_suffix and xstrfmt to replace suffix
[30/67]: ref-filter: drop sprintf and strcpy calls
[31/67]: help: drop prepend function in favor of xstrfmt
[32/67]: mailmap: replace strcpy with xstrdup
[33/67]: read_branches_file: replace strcpy with xstrdup
Ditto, but for xstrfmt/xstrdup.
[34/67]: resolve_ref: use strbufs for internal buffers
[35/67]: upload-archive: convert sprintf to strbuf
[36/67]: remote-ext: simplify git pkt-line generation
[37/67]: http-push: use strbuf instead of fwrite_buffer
[38/67]: http-walker: store url in a strbuf
[39/67]: sha1_get_pack_name: use a strbuf
[40/67]: init: use strbufs to store paths
[41/67]: apply: convert root string to strbuf
[42/67]: transport: use strbufs for status table "quickref" strings
[43/67]: merge-recursive: convert malloc / strcpy to strbuf
[44/67]: enter_repo: convert fixed-size buffers to strbufs
[45/67]: remove_leading_path: use a strbuf for internal storage
[46/67]: write_loose_object: convert to strbuf
[47/67]: diagnose_invalid_index_path: use strbuf to avoid strcpy/strcat
Ditto, but for strbufs. I generally used xstrfmt over a strbuf where
it was feasible, since the former is shorter. These cases typically
did something a little more complicated than xstrfmt could handle.
[48/67]: fetch-pack: use argv_array for index-pack / unpack-objects
[49/67]: http-push: use an argv_array for setup_revisions
[50/67]: stat_tracking_info: convert to argv_array
[51/67]: daemon: use cld->env_array when re-spawning
Ditto, but for argv_array. This helps regular overflows, because
argv_array_pushf uses a strbuf internally. But it also prevents
overflowing the array-of-pointers.
[52/67]: use sha1_to_hex_to() instead of strcpy
[53/67]: drop strcpy in favor of raw sha1_to_hex
Ditto, but for the new sha1-formatting helpers.
[54/67]: color: add overflow checks for parsing colors
[55/67]: use alloc_ref rather than hand-allocating "struct ref"
[56/67]: avoid sprintf and strcpy with flex arrays
[57/67]: receive-pack: simplify keep_arg computation
[58/67]: help: clean up kfmclient munging
[59/67]: prefer memcpy to strcpy
[60/67]: color: add color_set helper for copying raw colors
[61/67]: notes: document length of fanout path with a constant
[62/67]: convert strncpy to memcpy
These are ones that I couldn't fit it any other slot. :)
[63/67]: fsck: drop inode-sorting code
[64/67]: Makefile: drop D_INO_IN_DIRENT build knob
[65/67]: fsck: use for_each_loose_file_in_objdir
Another complicated case. The memory cleanups are in the third patch
here, but the other two are preparatory.
[66/67]: use strbuf_complete to conditionally append slash
[67/67]: name-rev: use strip_suffix to avoid magic numbers
And these are just minor code cleanups I ran into along the way.
Obviously this is not intended for v2.6.0. But all of the spots touched
here are relatively quiet right now, so I wanted to get it out onto the
list. There are a few minor conflicts against "pu", but they're all
just from touching nearby lines.
-Peff
From: Jeff King <hidden> Date: 2016-06-15 23:06:33
When no branch is given to the "--reflog" option, we resolve
HEAD to get the default branch. However, if HEAD points to
an unborn branch, resolve_ref returns NULL, and we later
segfault trying to access it.
Signed-off-by: Jeff King <redacted>
---
builtin/show-branch.c | 2 ++
1 file changed, 2 insertions(+)
@@ -743,6 +743,8 @@ int cmd_show_branch(int ac, const char **av, const char *prefix)fake_av[1]=NULL;av=fake_av;ac=1;+if(!*av)+die("no branches given, and HEAD is not valid");}if(ac!=1)die("--reflog option needs one branch name");
From: Jeff King <hidden> Date: 2016-06-15 23:06:33
This looks like a simple omission from 8539070 (archive-tar:
unindent write_tar_entry by one level, 2012-05-03).
Signed-off-by: Jeff King <redacted>
---
archive-tar.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
From: Jeff King <hidden> Date: 2016-06-15 23:06:33
If we encounter an error while splitting a maildir, we exit
the function early, leaking the open filehandle. This isn't
a big deal, since we exit the program soon after, but it's
easy enough to be careful.
Signed-off-by: Jeff King <redacted>
---
builtin/mailsplit.c | 5 ++++-
1 file changed, 4 insertions(+), 1 deletion(-)
From: Jeff King <hidden> Date: 2016-06-15 23:06:33
Commit 02976bf (fsck: introduce `git fsck --connectivity-only`,
2015-06-22) recently gave fsck an option to perform only a
subset of the checks, by skipping the fsck_object_dir()
call. However, it does so only for the local object
directory, and we still do expensive checks on any alternate
repos. We should skip them in this case, too.
Signed-off-by: Jeff King <redacted>
---
builtin/fsck.c | 17 +++++++++--------
1 file changed, 9 insertions(+), 8 deletions(-)
From: Jeff King <hidden> Date: 2016-06-15 23:06:33
There are a number of places in the code where we call
sprintf(), with the assumption that the output will fit into
the buffer. In many cases this is true (e.g., formatting a
number into a large buffer), but it is hard to tell
immediately from looking at the code. It would be nice if we
had some run-time check to make sure that our assumption is
correct (and to communicate to readers of the code that we
are not blindly calling sprintf, but have actually thought
about this case).
This patch introduces xsnprintf, which behaves just like
snprintf, except that it dies whenever the output is
truncated. This acts as a sort of assert() for these cases,
which can help find places where the assumption is violated
(as opposed to truncating and proceeding, which may just
silently give a wrong answer).
Signed-off-by: Jeff King <redacted>
---
git-compat-util.h | 3 +++
wrapper.c | 16 ++++++++++++++++
2 files changed, 19 insertions(+)
@@ -621,6 +621,22 @@ char *xgetcwd(void)returnstrbuf_detach(&sb,NULL);}+intxsnprintf(char*dst,size_tmax,constchar*fmt,...)+{+va_listap;+intlen;++va_start(ap,fmt);+len=vsnprintf(dst,max,fmt,ap);+va_end(ap);++if(len<0)+die("BUG: your snprintf is broken");+if(len>=max)+die("BUG: attempt to snprintf into too-small buffer");+returnlen;+}+staticintwrite_file_v(constchar*path,intfatal,constchar*fmt,va_listparams){
From: Jeff King <hidden> Date: 2016-06-15 23:06:33
If you have a function that uses git_path a lot, but would
prefer to avoid the static buffers, it's useful to keep a
single scratch buffer locally and reuse it for each call.
You used to be able to do this with git_snpath:
char buf[PATH_MAX];
foo(git_snpath(buf, sizeof(buf), "foo"));
bar(git_snpath(buf, sizeof(buf), "bar"));
but since 1a83c24, git_snpath has been replaced with
strbuf_git_path. This is good, because it removes the
arbitrary PATH_MAX limit. But using strbuf_git_path is more
awkward for two reasons:
1. It adds to the buffer, rather than replacing it. This
is consistent with other strbuf functions, but makes
reuse of a single buffer more tedious.
2. It doesn't return the buffer, so you can't format
as part of a function's arguments.
The new git_path_buf solves both of these, so you can use it
like:
struct strbuf buf = STRBUF_INIT;
foo(git_path_buf(&buf, "foo"));
bar(git_path_buf(&buf, "bar"));
strbuf_release(&buf);
Signed-off-by: Jeff King <redacted>
---
cache.h | 2 ++
path.c | 10 ++++++++++
2 files changed, 12 insertions(+)
From: Jeff King <hidden> Date: 2016-06-15 23:06:33
The strbuf_complete_line function make sure that a buffer
ends in a newline. But we may want to do this for any
character (e.g., "/" on the end of a path). Let's factor out
a generic version, and keep strbuf_complete_line as a thin
wrapper.
Signed-off-by: Jeff King <redacted>
---
strbuf.h | 16 ++++++++++++++--
1 file changed, 14 insertions(+), 2 deletions(-)
From: Jeff King <hidden> Date: 2016-06-15 23:06:33
The sha1_to_hex and find_unique_abbrev functions always
write into reusable static buffers. There are a few problems
with this:
- future calls overwrite our result. This is especially
annoying with find_unique_abbrev, which does not have a
ring of buffers, so you cannot even printf() a result
that has two abbreviated sha1s.
- if you want to put the result into another buffer, we
often strcpy, which looks suspicious when auditing for
overflows.
This patch introduces sha1_to_hex_to and find_unique_abbrev_to,
which write into a user-provided buffer. Of course this is
just punting on the overflow-auditing, as the buffer
obviously needs to be GIT_SHA1_HEXSZ + 1 bytes. But it is
much easier to audit, since that is a well-known size.
We retain the non-reentrant forms, which just become thin
wrappers around the reentrant ones. This patch also adds a
strbuf variant of find_unique_abbrev, which will be handy in
later patches.
Signed-off-by: Jeff King <redacted>
---
If we wanted to be really meticulous, these functions could
take a size for the output buffer, and complain if it is not
GIT_SHA1_HEXSZ+1 bytes. But that would bloat every call
like:
sha1_to_hex_to(buf, sizeof(buf), sha1);
cache.h | 27 ++++++++++++++++++++++++++-
hex.c | 13 +++++++++----
sha1_name.c | 16 +++++++++++-----
strbuf.c | 9 +++++++++
strbuf.h | 8 ++++++++
5 files changed, 63 insertions(+), 10 deletions(-)
From: Jeff King <hidden> Date: 2016-06-15 23:06:33
When fsck-ing alternates, we make a copy of the alternate
directory in a fixed PATH_MAX buffer. We memcpy directly,
without any check whether we are overflowing the buffer.
This is OK if PATH_MAX is a true representation of the
maximum path on the system, because any path here will have
already been vetted by the alternates subsystem. But that is
not true on every system, so we should be more careful.
Signed-off-by: Jeff King <redacted>
---
builtin/fsck.c | 11 ++++++-----
1 file changed, 6 insertions(+), 5 deletions(-)
From: Jeff King <hidden> Date: 2016-06-15 23:06:33
There are several static PATH_MAX-sized buffers in
mailsplit, along with some questionable uses of sprintf.
These are not really of security interest, as local
mailsplit pathnames are not typically under control of an
attacker. But it does not hurt to be careful, and as a
bonus we lift some limits for systems with too-small
PATH_MAX varibles.
Signed-off-by: Jeff King <redacted>
---
builtin/mailsplit.c | 46 +++++++++++++++++++++++++++++-----------------
1 file changed, 29 insertions(+), 17 deletions(-)
@@ -161,20 +167,25 @@ static int split_maildir(const char *maildir, const char *dir,gotoout;for(i=0;i<list.nr;i++){-snprintf(file,sizeof(file),"%s/%s",maildir,list.items[i].string);-f=fopen(file,"r");+char*name;++strbuf_reset(&file);+strbuf_addf(&file,"%s/%s",maildir,list.items[i].string);++f=fopen(file.buf,"r");if(!f){-error("cannot open mail %s (%s)",file,strerror(errno));+error("cannot open mail %s (%s)",file.buf,strerror(errno));gotoout;}if(strbuf_getwholeline(&buf,f,'\n')){-error("cannot read mail %s (%s)",file,strerror(errno));+error("cannot read mail %s (%s)",file.buf,strerror(errno));gotoout;}-sprintf(name,"%s/%0*d",dir,nr_prec,++skip);+name=xstrfmt("%s/%0*d",dir,nr_prec,++skip);split_one(f,name,1);+free(name);fclose(f);f=NULL;
From: Jeff King <hidden> Date: 2016-06-15 23:06:33
When we output GIT_TRACE_SETUP paths, we quote any
meta-characters. But our buffer to hold the result is only
PATH_MAX bytes, and we could double the size of the input
path (if every character needs quoted). We could use a
2*PATH_MAX buffer, if we assume the input will never be more
than PATH_MAX. But it's easier still to just switch to a
strbuf and not worry about whether the input can exceed
PATH_MAX or not.
Signed-off-by: Jeff King <redacted>
---
trace.c | 16 ++++++++--------
1 file changed, 8 insertions(+), 8 deletions(-)
@@ -277,25 +277,25 @@ void trace_performance_fl(const char *file, int line, uint64_t nanos,staticconstchar*quote_crnl(constchar*path){-staticcharnew_path[PATH_MAX];+staticstructstrbufnew_path=STRBUF_INIT;constchar*p2=path;-char*p1=new_path;if(!path)returnNULL;+strbuf_reset(&new_path);+while(*p2){switch(*p2){-case'\\':*p1++='\\';*p1++='\\';break;-case'\n':*p1++='\\';*p1++='n';break;-case'\r':*p1++='\\';*p1++='r';break;+case'\\':strbuf_addstr(&new_path,"\\\\");break;+case'\n':strbuf_addstr(&new_path,"\\n");break;+case'\r':strbuf_addstr(&new_path,"\\r");break;default:-*p1++=*p2;+strbuf_addch(&new_path,*p2);}p2++;}-*p1='\0';-returnnew_path;+returnnew_path.buf;}/* FIXME: move prefix to startup_info struct and get rid of this arg */
From: Jeff King <hidden> Date: 2016-06-15 23:06:33
Coverity noticed that we strncpy() into a fixed-size buffer
without making sure that it actually ended up
NUL-terminated. This is unlikely to be a bug in practice,
since throughput strings rarely hit 32 characters, but it
would be nice to clean it up.
The most obvious way to do so is to add a NUL-terminator.
But instead, this patch switches the fixed-size buffer out
for a strbuf. At first glance this seems much less
efficient, until we realize that filling in the fixed-size
buffer is done by writing into a strbuf and copying the
result!
By writing straight to the buffer, we actually end up more
efficient:
1. We avoid an extra copy of the bytes.
2. Rather than malloc/free each time progress is shown, we
can strbuf_reset and use the same buffer each time.
Signed-off-by: Jeff King <redacted>
---
I actually sent this one to the list in June:
http://thread.gmane.org/gmane.comp.version-control.git/271880
but it got overlooked. Good luck overlooking this 67-patch
monstrosity. :)
progress.c | 18 ++++++++----------
1 file changed, 8 insertions(+), 10 deletions(-)
From: Jeff King <hidden> Date: 2016-06-15 23:06:33
When dumping a cache-tree, we sprintf sub-tree names directly
into a fixed-size buffer, which can overflow. We can
trivially fix this by converting to xsnprintf to at least
notice and die.
This probably should handle arbitrary-sized names, but
there's not much point. It's used only by the test scripts,
so the trivial fix is enough.
Signed-off-by: Jeff King <redacted>
---
test-dump-cache-tree.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
From: Jeff King <hidden> Date: 2016-06-15 23:06:33
Our compat inet_ntop4 function writes to a temporary buffer
with snprintf, and then uses strcpy to put the result into
the final "dst" buffer. We check the return value of
snprintf against the size of "dst", but fail to account for
the NUL terminator. As a result, we may overflow "dst" with
a single NUL. In practice, this doesn't happen because the
output of inet_ntop is limited, and we provide buffers that
are way oversized.
We can fix the off-by-one check easily, but while we are
here let's also use strlcpy for increased safety, just in
case there are other bugs lurking.
As a side note, this compat code seems to be BSD-derived.
Searching for "vixie inet_ntop" turns up NetBSD's latest
version of the same code, which has an identical fix (and
switches to strlcpy, too!).
Signed-off-by: Jeff King <redacted>
---
compat/inet_ntop.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
@@ -53,11 +53,11 @@ inet_ntop4(const u_char *src, char *dst, size_t size)nprinted=snprintf(tmp,sizeof(tmp),fmt,src[0],src[1],src[2],src[3]);if(nprinted<0)return(NULL);/* we assume "errno" was set by "snprintf()" */-if((size_t)nprinted>size){+if((size_t)nprinted>=size){errno=ENOSPC;return(NULL);}-strcpy(dst,tmp);+strlcpy(dst,tmp,size);return(dst);}
From: Jeff King <hidden> Date: 2016-06-15 23:06:33
We sometimes sprintf into static buffers when we know that
the size of the buffer is large enough to fit the input
(either because it's a constant, or because it's numeric
input that is bounded in size). Likewise with strcpy of
constant strings.
However, these sites make it hard to audit sprintf and
strcpy calls for buffer overflows, as a reader has to
cross-reference the size of the array with the input. Let's
use xsnprintf instead, which communicates to a reader that
we don't expect this to overflow (and catches the mistake in
case we do).
Signed-off-by: Jeff King <redacted>
---
These are all pretty trivial; the obvious thing to get wrong is that
"sizeof(buf)" is not the correct length if "buf" is a pointer. I
considered a macro wrapper like:
#define xsnprintf_array(dst, fmt, ...) \
xsnprintf(dst, sizeof(dst) + BARF_UNLESS_AN_ARRAY(dst), \
fmt, __VA_ARGS__)
but obviously that requires variadic macro support.
archive-tar.c | 2 +-
builtin/gc.c | 2 +-
builtin/init-db.c | 11 ++++++-----
builtin/ls-tree.c | 9 +++++----
builtin/merge-index.c | 2 +-
builtin/merge-recursive.c | 2 +-
builtin/read-tree.c | 2 +-
builtin/unpack-file.c | 2 +-
compat/mingw.c | 8 +++++---
compat/winansi.c | 2 +-
connect.c | 2 +-
convert.c | 3 ++-
daemon.c | 4 ++--
diff.c | 12 ++++++------
http-push.c | 2 +-
http.c | 6 +++---
ll-merge.c | 12 ++++++------
refs.c | 8 ++++----
sideband.c | 4 ++--
strbuf.c | 4 ++--
20 files changed, 52 insertions(+), 47 deletions(-)
@@ -262,7 +262,8 @@ static int create_default_files(const char *template_path)}/* This forces creation of new config file */-sprintf(repo_version_string,"%d",GIT_REPO_VERSION);+xsnprintf(repo_version_string,sizeof(repo_version_string),+"%d",GIT_REPO_VERSION);git_config_set("core.repositoryformatversion",repo_version_string);path[len]=0;
@@ -414,13 +415,13 @@ int init_db(const char *template_dir, unsigned int flags)*/if(shared_repository<0)/* force to the mode value */-sprintf(buf,"0%o",-shared_repository);+xsnprintf(buf,sizeof(buf),"0%o",-shared_repository);elseif(shared_repository==PERM_GROUP)-sprintf(buf,"%d",OLD_PERM_GROUP);+xsnprintf(buf,sizeof(buf),"%d",OLD_PERM_GROUP);elseif(shared_repository==PERM_EVERYBODY)-sprintf(buf,"%d",OLD_PERM_EVERYBODY);+xsnprintf(buf,sizeof(buf),"%d",OLD_PERM_EVERYBODY);else-die("oops");+die("BUG: invalid value for shared_repository");git_config_set("core.sharedrepository",buf);git_config_set("receive.denyNonFastforwards","true");}
@@ -539,7 +539,7 @@ void winansi_init(void)return;/* create a named pipe to communicate with the console thread */-sprintf(name,"\\\\.\\pipe\\winansi%lu",GetCurrentProcessId());+xsnprintf(name,sizeof(name),"\\\\.\\pipe\\winansi%lu",GetCurrentProcessId());hwrite=CreateNamedPipe(name,PIPE_ACCESS_OUTBOUND,PIPE_TYPE_BYTE|PIPE_WAIT,1,BUFFER_SIZE,0,0,NULL);if(hwrite==INVALID_HANDLE_VALUE)
From: Jeff King <hidden> Date: 2016-06-15 23:06:33
When we generate tar headers, we sprintf() values directly
into a struct with the fixed-size header values. For the
most part this is fine, as we are formatting small values
(e.g., the octal format of "mode & 0x7777" is of fixed
length). But it's still a good idea to use xsnprintf here.
It communicates to readers what our expectation is, and it
provides a run-time check that we are not overflowing the
buffers.
The one exception here is the mtime, which comes from the
epoch time of the commit we are archiving. For sane values,
this fits into the 12-byte value allocated in the header.
But since git can handle 64-bit times, if I claim to be a
visitor from the year 10,000 AD, I can overflow the buffer.
This turns out to be harmless, as we simply overflow into
the chksum field, which is then overwritten.
This case is also best as an xsnprintf. It should never come
up, short of extremely malformed dates, and in that case we
are probably better off dying than silently truncating the
date value (and we cannot expand the size of the buffer,
since it is dictated by the ustar format). Our friends in
the year 5138 (when we legitimately flip to a 12-digit
epoch) can deal with that problem then.
Signed-off-by: Jeff King <redacted>
---
archive-tar.c | 26 +++++++++++++-------------
1 file changed, 13 insertions(+), 13 deletions(-)
From: Jeff King <hidden> Date: 2016-06-15 23:06:33
We generally use 32-byte buffers to format git's "type size"
header fields. These should not generally overflow unless
you can produce some truly gigantic objects (and our types
come from our internal array of constant strings). But it is
a good idea to use xsnprintf to make sure this is the case.
Note that we slightly modify the interface to
write_sha1_file_prepare, which nows uses "hdrlen" as an "in"
parameter as well as an "out" (on the way in it stores the
allocated size of the header, and on the way out it returns
the ultimate size of the header).
Signed-off-by: Jeff King <redacted>
---
builtin/index-pack.c | 2 +-
bulk-checkin.c | 4 ++--
fast-import.c | 4 ++--
http-push.c | 2 +-
sha1_file.c | 13 +++++++------
5 files changed, 13 insertions(+), 12 deletions(-)
@@ -200,8 +200,8 @@ static int deflate_to_pack(struct bulk_checkin_state *state,if(seekback==(off_t)-1)returnerror("cannot find the current offset");-header_len=sprintf((char*)obuf,"%s %"PRIuMAX,-typename(type),(uintmax_t)size)+1;+header_len=xsnprintf((char*)obuf,sizeof(obuf),"%s %"PRIuMAX,+typename(type),(uintmax_t)size)+1;git_SHA1_Init(&ctx);git_SHA1_Update(&ctx,obuf,header_len);
@@ -361,7 +361,7 @@ static void start_put(struct transfer_request *request)git_zstreamstream;unpacked=read_sha1_file(request->obj->sha1,&type,&len);-hdrlen=sprintf(hdr,"%s %lu",typename(type),len)+1;+hdrlen=xsnprintf(hdr,sizeof(hdr),"%s %lu",typename(type),len)+1;/* Set it up */git_deflate_init(&stream,zlib_compression_level);
@@ -1464,7 +1464,7 @@ int check_sha1_signature(const unsigned char *sha1, void *map,return-1;/* Generate the header */-hdrlen=sprintf(hdr,"%s %lu",typename(obj_type),size)+1;+hdrlen=xsnprintf(hdr,sizeof(hdr),"%s %lu",typename(obj_type),size)+1;/* Sha1.. */git_SHA1_Init(&c);
@@ -2930,7 +2930,7 @@ static void write_sha1_file_prepare(const void *buf, unsigned long len,git_SHA_CTXc;/* Generate the header */-*hdrlen=sprintf(hdr,"%s %lu",type,len)+1;+*hdrlen=xsnprintf(hdr,*hdrlen,"%s %lu",type,len)+1;/* Sha1.. */git_SHA1_Init(&c);
@@ -2993,7 +2993,7 @@ int hash_sha1_file(const void *buf, unsigned long len, const char *type,unsignedchar*sha1){charhdr[32];-inthdrlen;+inthdrlen=sizeof(hdr);write_sha1_file_prepare(buf,len,type,sha1,hdr,&hdrlen);return0;}
@@ -3139,7 +3139,7 @@ static int freshen_packed_object(const unsigned char *sha1)intwrite_sha1_file(constvoid*buf,unsignedlonglen,constchar*type,unsignedchar*sha1){charhdr[32];-inthdrlen;+inthdrlen=sizeof(hdr);/* Normally if we have it in the pack then we do not bother writing*itoutinto.git/objects/??/?{38}file.
@@ -3157,7 +3157,8 @@ int hash_sha1_file_literally(const void *buf, unsigned long len, const char *typinthdrlen,status=0;/* type string, SP, %lu of the length plus NUL must fit this */-header=xmalloc(strlen(type)+32);+hdrlen=strlen(type)+32;+header=xmalloc(hdrlen);write_sha1_file_prepare(buf,len,type,sha1,header,&hdrlen);if(!(flags&HASH_WRITE_OBJECT))
@@ -3185,7 +3186,7 @@ int force_object_loose(const unsigned char *sha1, time_t mtime)buf=read_packed_sha1(sha1,&type,&len);if(!buf)returnerror("cannot read sha1_file for %s",sha1_to_hex(sha1));-hdrlen=sprintf(hdr,"%s %lu",typename(type),len)+1;+hdrlen=xsnprintf(hdr,sizeof(hdr),"%s %lu",typename(type),len)+1;ret=write_loose_object(sha1,hdr,hdrlen,buf,len,mtime);free(buf);
From: Jeff King <hidden> Date: 2016-06-15 23:06:33
We use sprintf() to format some hex data into a buffer. The
buffer is clearly long enough, and using snprintf here is
not necessary. And in fact, it does not really make anything
easier to audit, as the size we feed to snprintf accounts
for the magic extra 42 bytes found in each alt->name field
of struct alternate_object_database (which is there exactly
to do this formatting).
Still, it is nice to remove an sprintf call and replace it
with an xsnprintf and explanatory comment, which makes it
easier to audit the code base for overflows.
Signed-off-by: Jeff King <redacted>
---
sha1_name.c | 8 ++++++--
1 file changed, 6 insertions(+), 2 deletions(-)
From: Jeff King <hidden> Date: 2016-06-15 23:06:33
The usual arguments for using xsnprintf over sprintf apply,
but this case is a little tricky. We print to a static
buffer if we have room, and otherwise to an allocated
buffer. So there should be no overflow here, but it is still
good to communicate our intention, as well as to check our
earlier math for how much space the string will need.
Signed-off-by: Jeff King <redacted>
---
progress.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
From: Jeff King <hidden> Date: 2016-06-15 23:06:33
This is a trivially correct use of sprintf, as our error
number should not be excessively long. But it's still nice
to drop an sprintf call.
Note that we cannot use xsnprintf here, because this is
compat code which does not load git-compat-util.h.
Signed-off-by: Jeff King <redacted>
---
compat/hstrerror.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
From: Jeff King <hidden> Date: 2016-06-15 23:06:33
This looks at first glance like the sprintf can overflow our
buffer, but it's actually fine; the p->origin string is
something constant and small, like "command line" or "-e
option".
Signed-off-by: Jeff King <redacted>
---
grep.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
From: Jeff King <hidden> Date: 2016-06-15 23:06:33
This particular conversion is non-obvious, because nobody
has passed our function the length of the destination
buffer. However, the interface to checkout_entry specifies
that the buffer must be at least TEMPORARY_FILENAME_LENGTH
bytes long, so we can check that (meaning the existing code
was not buggy, but merely worrisome to somebody reading it).
Signed-off-by: Jeff King <redacted>
---
entry.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
From: Jeff King <hidden> Date: 2016-06-15 23:06:33
We have the path "foo.idx", and we create a buffer big
enough to hold "foo.pack" and "foo.keep", and then strcpy
straight into it. This isn't a bug (we have enough space),
but it's very hard to tell from the strcpy that this is so.
Let's instead use strip_suffix to take off the ".idx",
record the size of our allocation, and use xsnprintf to make
sure we don't violate our assumptions.
Signed-off-by: Jeff King <redacted>
---
cache.h | 2 +-
sha1_file.c | 19 ++++++++++---------
2 files changed, 11 insertions(+), 10 deletions(-)
From: Jeff King <hidden> Date: 2016-06-15 23:06:33
We account for these strcats in our initial allocation, but
the code is confusing to follow and verify. Let's remember
our original allocation length, and then xsnprintf can
verify that we don't exceed it.
Note that we can't just use xstrfmt here (which would be
even cleaner) because the code tries to grow the buffer only
when necessary.
Signed-off-by: Jeff King <redacted>
---
It would probably be a good match for a strbuf, but it's
hard to over-emphasize how little interest I have in
refactoring the http-push webdav code.
http-push.c | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)
From: Jeff King <hidden> Date: 2016-06-15 23:06:33
This strncpy is pointless; we pass the strlen() of the src
string, meaning that it works just like a memcpy. Worse,
though, is that the size has no relation to the destination
buffer, meaning it is a potential overflow. In practice,
it's not. We pass only short constant strings like
"warning: " and "error: ", which are much smaller than the
destination buffer.
We can make this much simpler by just using xsnprintf, which
will check for overflow and return the size for our next
vsnprintf, without us having to run a separate strlen().
Signed-off-by: Jeff King <redacted>
---
builtin/receive-pack.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
From: Jeff King <hidden> Date: 2016-06-15 23:06:33
It's a common pattern to do:
foo = xmalloc(strlen(one) + strlen(two) + 1 + 1);
sprintf(foo, "%s %s", one, two);
(or possibly some variant with strcpy()s or a more
complicated length computation). We can switch these to use
xstrfmt, which is shorter, involves less error-prone manual
computation, and removes many sprintf and strcpy calls which
make it harder to audit the code for real buffer overflows.
Signed-off-by: Jeff King <redacted>
---
builtin/apply.c | 5 +----
builtin/ls-remote.c | 8 ++------
builtin/name-rev.c | 13 +++++--------
environment.c | 7 ++-----
imap-send.c | 5 ++---
reflog-walk.c | 7 +++----
remote.c | 7 +------
setup.c | 12 +++---------
unpack-trees.c | 4 +---
9 files changed, 20 insertions(+), 48 deletions(-)
From: Jeff King <hidden> Date: 2016-06-15 23:06:33
We xmalloc a fixed-size buffer and sprintf into it; this is
OK because the size of our formatting types is finite, but
that's not immediately clear to a reader auditing sprintf
calls. Let's switch to xstrfmt, which is shorter and
obviously correct.
Note that just dropping the common xmalloc here causes gcc
to complain with -Wmaybe-uninitialized. That's because if
"types" does not match any of our known types, we never
write anything into the "normalized" pointer. With the
current code, gcc doesn't notice because we always return a
valid pointer (just one which might point to uninitialized
data, but the compiler doesn't know that). In other words,
the current code is potentially buggy if new types are added
without updating this spot.
So let's take this opportunity to clean up the function a
bit more. We can drop the "normalized" pointer entirely, and
just return directly from each code path. And then add an
assertion at the end in case we haven't covered any cases.
Signed-off-by: Jeff King <redacted>
---
builtin/config.c | 34 +++++++++++++---------------------
1 file changed, 13 insertions(+), 21 deletions(-)
From: Jeff King <hidden> Date: 2016-06-15 23:06:33
We parse the INFINITE_DEPTH constant into a static,
fixed-size buffer using sprintf. This buffer is sufficiently
large for the current constant, but it's a suspicious
pattern, as the constant is defined far away, and it's not
immediately obvious that 12 bytes are large enough to hold
it.
We can just use xstrfmt here, which gets rid of any question
of the buffer size. It also removes any concerns with object
lifetime, which means we do not have to wonder why this
buffer deep within a conditional is marked "static" (we
never free our newly allocated result, of course, but that's
OK; it's global that lasts the lifetime of the whole program
anyway).
Signed-off-by: Jeff King <redacted>
---
builtin/fetch.c | 7 ++-----
1 file changed, 2 insertions(+), 5 deletions(-)
@@ -1156,11 +1156,8 @@ int cmd_fetch(int argc, const char **argv, const char *prefix)die(_("--depth and --unshallow cannot be used together"));elseif(!is_repository_shallow())die(_("--unshallow on a complete repository does not make sense"));-else{-staticcharinf_depth[12];-sprintf(inf_depth,"%d",INFINITE_DEPTH);-depth=inf_depth;-}+else+depth=xstrfmt("%d",INFINITE_DEPTH);}/* no need to be strict, transport_set_option() will validate it again */
From: Jeff King <hidden> Date: 2016-06-15 23:06:33
When we want to convert "foo.pack" to "foo.idx", we do it by
duplicating the original string and then munging the bytes
in place. Let's use strip_suffix and xstrfmt instead, which
has several advantages:
1. It's more clear what the intent is.
2. It does not implicitly rely on the fact that
strlen(".idx") <= strlen(".pack") to avoid an overflow.
3. We communicate the assumption that the input file ends
with ".pack" (and get a run-time check that this is so).
4. We drop calls to strcpy, which makes auditing the code
base easier.
Likewise, we can do this to convert ".pack" to ".bitmap",
avoiding some manual memory computation.
Signed-off-by: Jeff King <redacted>
---
http.c | 7 ++++---
pack-bitmap.c | 13 ++++---------
sha1_file.c | 6 ++++--
3 files changed, 12 insertions(+), 14 deletions(-)
@@ -1511,6 +1511,7 @@ int finish_http_pack_request(struct http_pack_request *preq)structpacked_git**lst;structpacked_git*p=preq->target;char*tmp_idx;+size_tlen;structchild_processip=CHILD_PROCESS_INIT;constchar*ip_argv[8];
@@ -1524,9 +1525,9 @@ int finish_http_pack_request(struct http_pack_request *preq)lst=&((*lst)->next);*lst=(*lst)->next;-tmp_idx=xstrdup(preq->tmpfile);-strcpy(tmp_idx+strlen(tmp_idx)-strlen(".pack.temp"),-".idx.temp");+if(!strip_suffix(preq->tmpfile,".pack.temp",&len))+die("BUG: pack tmpfile does not end in .pack.temp?");+tmp_idx=xstrfmt("%.*s.idx.temp",(int)len,preq->tmpfile);ip_argv[0]="index-pack";ip_argv[1]="-o";
@@ -252,16 +252,11 @@ static int load_bitmap_entries_v1(struct bitmap_index *index)staticchar*pack_bitmap_filename(structpacked_git*p){-char*idx_name;-intlen;--len=strlen(p->pack_name)-strlen(".pack");-idx_name=xmalloc(len+strlen(".bitmap")+1);--memcpy(idx_name,p->pack_name,len);-memcpy(idx_name+len,".bitmap",strlen(".bitmap")+1);+size_tlen;-returnidx_name;+if(!strip_suffix(p->pack_name,".pack",&len))+die("BUG: pack_name does not end in .pack");+returnxstrfmt("%.*s.bitmap",(int)len,p->pack_name);}staticintopen_pack_bitmap_1(structpacked_git*packfile)
@@ -671,13 +671,15 @@ static int check_packed_git_idx(const char *path, struct packed_git *p)intopen_pack_index(structpacked_git*p){char*idx_name;+size_tlen;intret;if(p->index_data)return0;-idx_name=xstrdup(p->pack_name);-strcpy(idx_name+strlen(idx_name)-strlen(".pack"),".idx");+if(!strip_suffix(p->pack_name,".pack",&len))+die("BUG: pack_name does not end in .pack");+idx_name=xstrfmt("%.*s.idx",(int)len,p->pack_name);ret=check_packed_git_idx(idx_name,p);free(idx_name);returnret;
From: Jeff King <hidden> Date: 2016-06-15 23:06:33
The ref-filter code comes from for-each-ref, and inherited a
number of raw sprintf and strcpy calls. These are generally
all safe, as we custom-size the buffers, or are formatting
numbers into sufficiently large buffers. But we can make the
resulting code even simpler and more obviously correct by
using some of our helper functions.
Signed-off-by: Jeff King <redacted>
---
ref-filter.c | 70 +++++++++++++++++++-----------------------------------------
1 file changed, 22 insertions(+), 48 deletions(-)
From: Jeff King <hidden> Date: 2016-06-15 23:06:33
This function predates xstrfmt, and its functionality is a
subset. Let's just use xstrfmt.
Signed-off-by: Jeff King <redacted>
---
builtin/help.c | 14 ++------------
1 file changed, 2 insertions(+), 12 deletions(-)
From: Jeff King <hidden> Date: 2016-06-15 23:06:33
This code is exactly replicating strdup, so let's just use
that. It's shorter, and eliminates some confusion (such as
whether "p - s" is really enough to hold the result; it is,
because we write NULs as we shrink "p").
Signed-off-by: Jeff King <redacted>
---
remote.c | 5 +----
1 file changed, 1 insertion(+), 4 deletions(-)
From: Jeff King <hidden> Date: 2016-06-15 23:06:33
resolve_ref already uses a strbuf internally when generating
pathnames, but it uses fixed-size buffers for storing the
refname and symbolic refs. This means that you cannot
actually point HEAD to a ref that is larger than 256 bytes.
We can lift this limit by using strbufs here, too. Like
sb_path, we pass the the buffers into our helper function,
so that we can easily clean up all output paths. We can also
drop the "unsafe" name from our helper function, as it no
longer uses a single static buffer (but of course
resolve_ref_unsafe is still unsafe, because the static
buffers moved there).
As a bonus, we also get to drop some strcpy calls between
the two fixed buffers (that cannot currently overflow
because the two buffers are sized identically).
Signed-off-by: Jeff King <redacted>
---
refs.c | 57 ++++++++++++++++++++++++++-----------------------
t/t1401-symbolic-ref.sh | 29 +++++++++++++++++++++++++
2 files changed, 59 insertions(+), 27 deletions(-)
@@ -1579,16 +1579,15 @@ static int resolve_missing_loose_ref(const char *refname,}/* This function needs to return a meaningful errno on failure */-staticconstchar*resolve_ref_unsafe_1(constchar*refname,-intresolve_flags,-unsignedchar*sha1,-int*flags,-structstrbuf*sb_path)+staticconstchar*resolve_ref_1(constchar*refname,+intresolve_flags,+unsignedchar*sha1,+int*flags,+structstrbuf*sb_refname,+structstrbuf*sb_path,+structstrbuf*sb_contents){intdepth=MAXDEPTH;-ssize_tlen;-charbuffer[256];-staticcharrefname_buffer[256];intbad_name=0;if(flags)
@@ -1654,19 +1653,18 @@ static const char *resolve_ref_unsafe_1(const char *refname,/* Follow "normalized" - ie "refs/.." symlinks by hand */if(S_ISLNK(st.st_mode)){-len=readlink(path,buffer,sizeof(buffer)-1);-if(len<0){+strbuf_reset(sb_contents);+if(strbuf_readlink(sb_contents,path,0)<0){if(errno==ENOENT||errno==EINVAL)/* inconsistent with lstat; retry */gotostat_ref;elsereturnNULL;}-buffer[len]=0;-if(starts_with(buffer,"refs/")&&-!check_refname_format(buffer,0)){-strcpy(refname_buffer,buffer);-refname=refname_buffer;+if(starts_with(sb_contents->buf,"refs/")&&+!check_refname_format(sb_contents->buf,0)){+strbuf_swap(sb_refname,sb_contents);+refname=sb_refname->buf;if(flags)*flags|=REF_ISSYMREF;if(resolve_flags&RESOLVE_REF_NO_RECURSE){
@@ -63,4 +63,33 @@ test_expect_success 'symbolic-ref fails to delete real ref' '' reset_to_sane+test_expect_success'create large ref name''+# make 256+ character ref; some systems may not handle that,+# so be gentle+long=0123456789abcdef&&+long=$long/$long/$long/$long&&+long=$long/$long/$long/$long&&+long_ref=refs/heads/$long&&+tree=$(gitwrite-tree)&&+commit=$(echofoo|gitcommit-tree$tree)&&+ifgitupdate-ref$long_ref$commit;then+test_set_prereqLONG_REF+else+echo>&2"long refs not supported"+fi+'++test_expect_successLONG_REF'symbolic-ref can point to large ref name''+gitsymbolic-refHEAD$long_ref&&+echo$long_ref>expect&&+gitsymbolic-refHEAD>actual&&+test_cmpexpectactual+'++test_expect_successLONG_REF'we can parse long symbolic ref''+echo$commit>expect&&+gitrev-parse--verifyHEAD>actual&&+test_cmpexpectactual+'+ test_done
From: Jeff King <hidden> Date: 2016-06-15 23:06:33
When we report an error to the client, we format it into a
fixed-size buffer using vsprintf(). This can't actually
overflow in practice, since we only format a very tame
subset of strings (mostly strerror() output). However, it's
hard to tell immediately, so let's just use a strbuf so
readers do not have to wonder.
We do add an allocation here, but the performance is not
important; the next step is to call die() anyway.
Signed-off-by: Jeff King <redacted>
---
builtin/upload-archive.c | 9 ++++-----
1 file changed, 4 insertions(+), 5 deletions(-)
@@ -49,15 +49,14 @@ int cmd_upload_archive_writer(int argc, const char **argv, const char *prefix)__attribute__((format(printf,1,2)))staticvoiderror_clnt(constchar*fmt,...){-charbuf[1024];+structstrbufbuf=STRBUF_INIT;va_listparams;-intlen;va_start(params,fmt);-len=vsprintf(buf,fmt,params);+strbuf_vaddf(&buf,fmt,params);va_end(params);-send_sideband(1,3,buf,len,LARGE_PACKET_MAX);-die("sent error to the client: %s",buf);+send_sideband(1,3,buf.buf,buf.len,LARGE_PACKET_MAX);+die("sent error to the client: %s",buf.buf);}staticssize_tprocess_input(intchild_fd,intband)
From: Jeff King <hidden> Date: 2016-06-15 23:06:33
We format a pkt-line into a heap buffer, which requires
manual computation of the required size. We can just use a
strbuf instead, which handles this for us, and lets us drop
some bare sprintf calls.
Note that we _could_ also use a fixed-size buffer here, as
we are limited by 16-bit pkt-line limit. But we'd still have
to worry about the length computation in that case, and the
allocation overhead is not important here.
Signed-off-by: Jeff King <redacted>
---
builtin/remote-ext.c | 32 +++++++++-----------------------
1 file changed, 9 insertions(+), 23 deletions(-)
@@ -142,36 +142,22 @@ static const char **parse_argv(const char *arg, const char *service)staticvoidsend_git_request(intstdin_fd,constchar*serv,constchar*repo,constchar*vhost){-size_tbufferspace;-size_twpos=0;-char*buffer;+structstrbufbuffer=STRBUF_INIT;-/*-*Requestneeds12bytesextraifthereisvhost(xxxx\0host=\0)and-*6bytesextra(xxxx\0)ifthereisnovhost.-*/+/* Generate packet with a dummy size header */+strbuf_addf(&buffer,"0000%s %s%c",serv,repo,0);if(vhost)-bufferspace=strlen(serv)+strlen(repo)+strlen(vhost)+12;-else-bufferspace=strlen(serv)+strlen(repo)+6;+strbuf_addf(&buffer,"host=%s%c",vhost,0);-if(bufferspace>0xFFFF)+/* Now go back and fill in the size */+if(buffer.len>0xFFFF)die("Request too large to send");-buffer=xmalloc(bufferspace);--/* Make the packet. */-wpos=sprintf(buffer,"%04x%s %s%c",(unsigned)bufferspace,-serv,repo,0);--/* Add vhost if any. */-if(vhost)-sprintf(buffer+wpos,"host=%s%c",vhost,0);+xsnprintf(buffer.buf,buffer.alloc,"%04x",(unsigned)buffer.len);-/* Send the request */-if(write_in_full(stdin_fd,buffer,bufferspace)<0)+if(write_in_full(stdin_fd,buffer.buf,buffer.len)<0)die_errno("Failed to send request");-free(buffer);+strbuf_release(&buffer);}staticintrun_child(constchar*arg,constchar*service)
From: Jeff King <hidden> Date: 2016-06-15 23:06:33
We do an unchecked sprintf directly into our url buffer.
This doesn't overflow because we know that it was sized for
"$base/objects/info/http-alternates", and we are writing
"$base/objects/info/alternates", which must be smaller. But
that is not immediately obvious to a reader who is looking
for buffer overflows. Let's switch to a strbuf, so that we
do not have to think about this issue at all.
Signed-off-by: Jeff King <redacted>
---
http-walker.c | 19 ++++++++++---------
1 file changed, 10 insertions(+), 9 deletions(-)
From: Jeff King <hidden> Date: 2016-06-15 23:06:33
We do some manual memory computation here, and there's no
check that our 60 is not overflowed by the raw sprintf (it
isn't, because the "which" parameter is never longer than
"pack"). We can simplify this greatly with a strbuf.
Technically the end result is not identical, as the original
took care not to rewrite the object directory on each call
for performance reasons. We could do that here, too (by
saving the baselen and resetting to it), but it's not worth
the complexity; this function is not called a lot (generally
once per packfile that we open).
Signed-off-by: Jeff King <redacted>
---
sha1_file.c | 39 ++++++++++-----------------------------
1 file changed, 10 insertions(+), 29 deletions(-)
From: Jeff King <hidden> Date: 2016-06-15 23:06:34
The http-push code defines an fwrite_buffer function for use
as a curl callback; it just writes to a strbuf. There's no
reason we need to use it ourselves, as we know we have a
strbuf. This lets us format directly into it, rather than
dealing with an extra temporary buffer (which required
manual length computation).
While we're here, let's also remove the literal tabs from
the source in favor of "\t", which is more visually obvious.
Signed-off-by: Jeff King <redacted>
---
http-push.c | 21 +++++----------------
1 file changed, 5 insertions(+), 16 deletions(-)
From: Jeff King <hidden> Date: 2016-06-15 23:06:34
The init code predates strbufs, and uses PATH_MAX-sized
buffers along with many manual checks on intermediate sizes
(some of which make magic assumptions, such as that init
will not create a path inside .git longer than 50
characters).
We can simplify this greatly by using strbufs, which drops
some hard-to-verify strcpy calls. Note that we need to
update probe_utf8_pathname_composition, too, as it assumes
we are passing a buffer large enough to append its probe
filenames (it now just takes a strbuf, which also gets rid
of the confusing "len" parameter, which was not the length of
"path" but rather the offset to start writing).
Some of the conversion makes new calls to git_path_buf.
While we're in the area, let's also convert existing calls
to git_path to the safer git_path_buf (our existing calls
were passed to pretty tame functions, and so were not a
problem, but it's easy to be consistent and safe here).
Note that we had an explicit test that "git init" rejects
long template directories. This comes from 32d1776 (init: Do
not segfault on big GIT_TEMPLATE_DIR environment variable,
2009-04-18). We can drop the test_must_fail here, as we now
accept this and need only confirm that we don't segfault,
which was the original point of the test.
Signed-off-by: Jeff King <redacted>
---
builtin/init-db.c | 174 ++++++++++++++++++++---------------------------
compat/precompose_utf8.c | 12 ++--
compat/precompose_utf8.h | 2 +-
git-compat-util.h | 2 +-
t/t0001-init.sh | 4 +-
5 files changed, 87 insertions(+), 107 deletions(-)
@@ -36,10 +36,11 @@ static void safe_create_dir(const char *dir, int share)die(_("Could not make %s writable by group"),dir);}-staticvoidcopy_templates_1(char*path,intbaselen,-char*template,inttemplate_baselen,+staticvoidcopy_templates_1(structstrbuf*path,structstrbuf*template,DIR*dir){+size_tpath_baselen=path->len;+size_ttemplate_baselen=template->len;structdirent*de;/* Note: if ".git/hooks" file exists in the repository being
@@ -49,77 +50,64 @@ static void copy_templates_1(char *path, int baselen,*withthewaythenamespaceunder.git/isorganized,should*bereallycarefullychosen.*/-safe_create_dir(path,1);+safe_create_dir(path->buf,1);while((de=readdir(dir))!=NULL){structstatst_git,st_template;-intnamelen;intexists=0;+strbuf_setlen(path,path_baselen);+strbuf_setlen(template,template_baselen);+if(de->d_name[0]=='.')continue;-namelen=strlen(de->d_name);-if((PATH_MAX<=baselen+namelen)||-(PATH_MAX<=template_baselen+namelen))-die(_("insanely long template name %s"),de->d_name);-memcpy(path+baselen,de->d_name,namelen+1);-memcpy(template+template_baselen,de->d_name,namelen+1);-if(lstat(path,&st_git)){+strbuf_addstr(path,de->d_name);+strbuf_addstr(template,de->d_name);+if(lstat(path->buf,&st_git)){if(errno!=ENOENT)-die_errno(_("cannot stat '%s'"),path);+die_errno(_("cannot stat '%s'"),path->buf);}elseexists=1;-if(lstat(template,&st_template))-die_errno(_("cannot stat template '%s'"),template);+if(lstat(template->buf,&st_template))+die_errno(_("cannot stat template '%s'"),template->buf);if(S_ISDIR(st_template.st_mode)){-DIR*subdir=opendir(template);-intbaselen_sub=baselen+namelen;-inttemplate_baselen_sub=template_baselen+namelen;+DIR*subdir=opendir(template->buf);if(!subdir)-die_errno(_("cannot opendir '%s'"),template);-path[baselen_sub++]=-template[template_baselen_sub++]='/';-path[baselen_sub]=-template[template_baselen_sub]=0;-copy_templates_1(path,baselen_sub,-template,template_baselen_sub,-subdir);+die_errno(_("cannot opendir '%s'"),template->buf);+strbuf_addch(path,'/');+strbuf_addch(template,'/');+copy_templates_1(path,template,subdir);closedir(subdir);}elseif(exists)continue;elseif(S_ISLNK(st_template.st_mode)){-charlnk[256];-intlen;-len=readlink(template,lnk,sizeof(lnk));-if(len<0)-die_errno(_("cannot readlink '%s'"),template);-if(sizeof(lnk)<=len)-die(_("insanely long symlink %s"),template);-lnk[len]=0;-if(symlink(lnk,path))-die_errno(_("cannot symlink '%s' '%s'"),lnk,path);+structstrbuflnk=STRBUF_INIT;+if(strbuf_readlink(&lnk,template->buf,0)<0)+die_errno(_("cannot readlink '%s'"),template->buf);+if(symlink(lnk.buf,path->buf))+die_errno(_("cannot symlink '%s' '%s'"),+lnk.buf,path->buf);+strbuf_release(&lnk);}elseif(S_ISREG(st_template.st_mode)){-if(copy_file(path,template,st_template.st_mode))-die_errno(_("cannot copy '%s' to '%s'"),template,-path);+if(copy_file(path->buf,template->buf,st_template.st_mode))+die_errno(_("cannot copy '%s' to '%s'"),+template->buf,path->buf);}else-error(_("ignoring template %s"),template);+error(_("ignoring template %s"),template->buf);}}staticvoidcopy_templates(constchar*template_dir){-charpath[PATH_MAX];-chartemplate_path[PATH_MAX];-inttemplate_len;+structstrbufpath=STRBUF_INIT;+structstrbuftemplate_path=STRBUF_INIT;+size_ttemplate_len;DIR*dir;-constchar*git_dir=get_git_dir();-intlen=strlen(git_dir);char*to_free=NULL;if(!template_dir)
@@ -132,26 +120,23 @@ static void copy_templates(const char *template_dir)free(to_free);return;}-template_len=strlen(template_dir);-if(PATH_MAX<=(template_len+strlen("/config")))-die(_("insanely long template path %s"),template_dir);-strcpy(template_path,template_dir);-if(template_path[template_len-1]!='/'){-template_path[template_len++]='/';-template_path[template_len]=0;-}-dir=opendir(template_path);++strbuf_addstr(&template_path,template_dir);+strbuf_complete(&template_path,'/');+template_len=template_path.len;++dir=opendir(template_path.buf);if(!dir){warning(_("templates not found %s"),template_dir);gotofree_return;}/* Make sure that template is from the correct vintage */-strcpy(template_path+template_len,"config");+strbuf_addstr(&template_path,"config");repository_format_version=0;git_config_from_file(check_repository_format_version,-template_path,NULL);-template_path[template_len]=0;+template_path.buf,NULL);+strbuf_setlen(&template_path,template_len);if(repository_format_version&&repository_format_version!=GIT_REPO_VERSION){
@@ -290,14 +263,13 @@ static int create_default_files(const char *template_path)/* allow template config file to override the default */if(log_all_ref_updates==-1)git_config_set("core.logallrefupdates","true");-if(needs_work_tree_config(git_dir,work_tree))+if(needs_work_tree_config(get_git_dir(),work_tree))git_config_set("core.worktree",work_tree);}if(!reinit){/* Check if symlink is supported in the work tree */-path[len]=0;-strcpy(path+len,"tXXXXXX");+path=git_path_buf(&buf,"tXXXXXX");if(!close(xmkstemp(path))&&!unlink(path)&&!symlink("testing",path)&&
@@ -308,31 +280,35 @@ static int create_default_files(const char *template_path)git_config_set("core.symlinks","false");/* Check if the filesystem is case-insensitive */-path[len]=0;-strcpy(path+len,"CoNfIg");+path=git_path_buf(&buf,"CoNfIg");if(!access(path,F_OK))git_config_set("core.ignorecase","true");-probe_utf8_pathname_composition(path,len);+probe_utf8_pathname_composition(path);}+strbuf_release(&buf);returnreinit;}staticvoidcreate_object_directory(void){-constchar*object_directory=get_object_directory();-intlen=strlen(object_directory);-char*path=xmalloc(len+40);+structstrbufpath=STRBUF_INIT;+size_tbaselen;++strbuf_addstr(&path,get_object_directory());+baselen=path.len;++safe_create_dir(path.buf,1);-memcpy(path,object_directory,len);+strbuf_setlen(&path,baselen);+strbuf_addstr(&path,"/pack");+safe_create_dir(path.buf,1);-safe_create_dir(object_directory,1);-strcpy(path+len,"/pack");-safe_create_dir(path,1);-strcpy(path+len,"/info");-safe_create_dir(path,1);+strbuf_setlen(&path,baselen);+strbuf_addstr(&path,"/info");+safe_create_dir(path.buf,1);-free(path);+strbuf_release(&path);}intset_git_dir_init(constchar*git_dir,constchar*real_git_dir,
@@ -36,24 +36,28 @@ static size_t has_non_ascii(const char *s, size_t maxlen, size_t *strlen_c)}-voidprobe_utf8_pathname_composition(char*path,intlen)+voidprobe_utf8_pathname_composition(structstrbuf*path){staticconstchar*auml_nfc="\xc3\xa4";staticconstchar*auml_nfd="\x61\xcc\x88";+size_tbaselen=path->len;intoutput_fd;if(precomposed_unicode!=-1)return;/* We found it defined in the global config, respect it */-strcpy(path+len,auml_nfc);+strbuf_addstr(path,auml_nfc);output_fd=open(path,O_CREAT|O_EXCL|O_RDWR,0600);if(output_fd>=0){close(output_fd);-strcpy(path+len,auml_nfd);+strbuf_setlen(path,baselen);+strbuf_addstr(path,auml_nfd);precomposed_unicode=access(path,R_OK)?0:1;git_config_set("core.precomposeunicode",precomposed_unicode?"true":"false");-strcpy(path+len,auml_nfc);+strbuf_setlen(path,baselen);+strbuf_addstr(path,auml_nfc);if(unlink(path))die_errno(_("failed to unlink '%s'"),path);}+strbuf_setlen(path,baselen);}
@@ -202,8 +202,8 @@ test_expect_success 'init honors global core.sharedRepository' 'x$(gitconfig-fshared-honor-global/.git/configcore.sharedRepository)'-test_expect_success'init rejects insanely long --template''-test_must_failgitinit--template=$(printf"x%09999dx"1)test+test_expect_success'init allows insanely long --template''+gitinit--template=$(printf"x%09999dx"1)test' test_expect_success'init creates a new directory''
From: Jeff King <hidden> Date: 2016-06-15 23:06:34
We want to make a copy of a string without any leading
whitespace. To do so, we allocate a buffer large enough to
hold the original, skip past the whitespace, then copy that.
It's much simpler to just allocate after we've skipped, in
which case we can just copy the remainder of the string,
leaving no question of whether "len" is large enough.
Signed-off-by: Jeff King <redacted>
---
mailmap.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
From: Jeff King <hidden> Date: 2016-06-15 23:06:34
We use manual computation and strcpy to allocate the "root"
variable. This would be much simpler using xstrfmt. But
since we store the length, too, we can just use a strbuf,
which handles that for us.
Note that we stop distinguishing between "no root" and
"empty root" in some cases, but that's OK; the results are
the same (e.g., inserting an empty string is a noop).
Signed-off-by: Jeff King <redacted>
---
builtin/apply.c | 26 ++++++++++----------------
1 file changed, 10 insertions(+), 16 deletions(-)
@@ -1274,8 +1273,8 @@ static int parse_git_header(const char *line, int len, unsigned int size, struct*thedefaultnamefromtheheader.*/patch->def_name=git_header_name(line,len);-if(patch->def_name&&root){-char*s=xstrfmt("%s%s",root,patch->def_name);+if(patch->def_name&&root.len){+char*s=xstrfmt("%s%s",root.buf,patch->def_name);free(patch->def_name);patch->def_name=s;}
@@ -4498,14 +4497,9 @@ static int option_parse_whitespace(const struct option *opt,staticintoption_parse_directory(conststructoption*opt,constchar*arg,intunset){-root_len=strlen(arg);-if(root_len&&arg[root_len-1]!='/'){-char*new_root;-root=new_root=xmalloc(root_len+2);-strcpy(new_root,arg);-strcpy(new_root+root_len++,"/");-}else-root=arg;+strbuf_reset(&root);+strbuf_addstr(&root,arg);+strbuf_complete(&root,'/');return0;}
From: Jeff King <hidden> Date: 2016-06-15 23:06:34
We generate range strings like "1234abcd...5678efab" for use
in the the fetch and push status tables. We use fixed-size
buffers along with strcat to do so. These aren't buggy, as
our manual size computation is correct, but there's nothing
checking that this is so. Let's switch them to strbufs
instead, which are obviously correct, and make it easier to
audit the code base for problematic calls to strcat().
Signed-off-by: Jeff King <redacted>
---
builtin/fetch.c | 22 ++++++++++++----------
transport.c | 13 +++++++------
2 files changed, 19 insertions(+), 16 deletions(-)
From: Jeff King <hidden> Date: 2016-06-15 23:06:34
This would be a fairly routine use of xstrfmt, except that
we need to remember the length of the result to pass to
cache_name_pos. So just use a strbuf, which makes this
simple.
As a bonus, this gets rid of confusing references to
"pathlen+1". The "1" is for the trailing slash we added, but
that is automatically accounted for in the strbuf's len
parameter.
Signed-off-by: Jeff King <redacted>
---
merge-recursive.c | 17 ++++++++---------
1 file changed, 8 insertions(+), 9 deletions(-)
From: Jeff King <hidden> Date: 2016-06-15 23:06:34
We use two PATH_MAX-sized buffers to represent the repo
path, and must make sure not to overflow them. We do take
care to check the lengths, but the logic is rather hard to
follow, as we use several magic numbers (e.g., "PATH_MAX -
10"). And in fact you _can_ overflow the buffer if you have
a ".git" file with an extremely long path in it.
By switching to strbufs, these problems all go away. We do,
however, retain the check that the initial input we get is
no larger than PATH_MAX. This function is an entry point for
untrusted repo names from the network, and it's a good idea
to keep a sanity check (both to avoid allocating arbitrary
amounts of memory, and also as a layer of defense against
any downstream users of the names).
Signed-off-by: Jeff King <redacted>
---
path.c | 57 +++++++++++++++++++++++++++++----------------------------
1 file changed, 29 insertions(+), 28 deletions(-)
From: Jeff King <hidden> Date: 2016-06-15 23:06:34
This function strcpy's directly into a PATH_MAX-sized
buffer. There's only one caller, which feeds the git_dir into
it, so it's not easy to trigger in practice (even if you fed
a large $GIT_DIR through the environment or .git file, it
would have to actually exist and be accessible on the
filesystem to get to this point). We can fix it by moving to
a strbuf.
Signed-off-by: Jeff King <redacted>
---
path.c | 10 ++++++----
1 file changed, 6 insertions(+), 4 deletions(-)
From: Jeff King <hidden> Date: 2016-06-15 23:06:34
When creating a loose object tempfile, we use a fixed
PATH_MAX-sized buffer, and strcpy directly into it. This
isn't buggy, because we do a rough check of the size, but
there's no verification that our guesstimate of the required
space is enough (in fact, it's several bytes too big for the
current naming scheme).
Let's switch to a strbuf, which makes this much easier to
verify. The allocation overhead should be negligible, since
we are replacing a static buffer with a static strbuf, and
we'll only need to allocate on the first call.
While we're here, we can also document a subtle interaction
with mkstemp that would be easy to overlook.
Signed-off-by: Jeff King <redacted>
---
sha1_file.c | 41 +++++++++++++++++++++--------------------
1 file changed, 21 insertions(+), 20 deletions(-)
@@ -3007,29 +3007,30 @@ static inline int directory_size(const char *filename)*Wewanttoavoidcross-directoryfilenamerenames,becausethose*canhaveproblemsonvariousfilesystems(FAT,NFS,Coda).*/-staticintcreate_tmpfile(char*buffer,size_tbufsiz,constchar*filename)+staticintcreate_tmpfile(structstrbuf*tmp,constchar*filename){intfd,dirlen=directory_size(filename);-if(dirlen+20>bufsiz){-errno=ENAMETOOLONG;-return-1;-}-memcpy(buffer,filename,dirlen);-strcpy(buffer+dirlen,"tmp_obj_XXXXXX");-fd=git_mkstemp_mode(buffer,0444);+strbuf_reset(tmp);+strbuf_add(tmp,filename,dirlen);+strbuf_addstr(tmp,"tmp_obj_XXXXXX");+fd=git_mkstemp_mode(tmp->buf,0444);if(fd<0&&dirlen&&errno==ENOENT){-/* Make sure the directory exists */-memcpy(buffer,filename,dirlen);-buffer[dirlen-1]=0;-if(mkdir(buffer,0777)&&errno!=EEXIST)+/*+*Makesurethedirectoryexists;notethatmkstempwillhave+*putaNULinourbuffer,sowehavetorewritethepath,+*ratherthanjustchompingthelength.+*/+strbuf_reset(tmp);+strbuf_add(tmp,filename,dirlen-1);+if(mkdir(tmp->buf,0777)&&errno!=EEXIST)return-1;-if(adjust_shared_perm(buffer))+if(adjust_shared_perm(tmp->buf))return-1;/* Try again */-strcpy(buffer+dirlen-1,"/tmp_obj_XXXXXX");-fd=git_mkstemp_mode(buffer,0444);+strbuf_addstr(tmp,"/tmp_obj_XXXXXX");+fd=git_mkstemp_mode(tmp->buf,0444);}returnfd;}
@@ -3042,10 +3043,10 @@ static int write_loose_object(const unsigned char *sha1, char *hdr, int hdrlen,git_zstreamstream;git_SHA_CTXc;unsignedcharparano_sha1[20];-staticchartmp_file[PATH_MAX];+staticstructstrbuftmp_file=STRBUF_INIT;constchar*filename=sha1_file_name(sha1);-fd=create_tmpfile(tmp_file,sizeof(tmp_file),filename);+fd=create_tmpfile(&tmp_file,filename);if(fd<0){if(errno==EACCES)returnerror("insufficient permission for adding an object to repository database %s",get_object_directory());
@@ -3094,12 +3095,12 @@ static int write_loose_object(const unsigned char *sha1, char *hdr, int hdrlen,structutimbufutb;utb.actime=mtime;utb.modtime=mtime;-if(utime(tmp_file,&utb)<0)+if(utime(tmp_file.buf,&utb)<0)warning("failed utime() on %s: %s",-tmp_file,strerror(errno));+tmp_file.buf,strerror(errno));}-returnfinalize_object_file(tmp_file,filename);+returnfinalize_object_file(tmp_file.buf,filename);}staticintfreshen_loose_object(constunsignedchar*sha1)
From: Jeff King <hidden> Date: 2016-06-15 23:06:34
We dynamically allocate a buffer and then strcpy and strcat
into it. This isn't buggy, but we'd prefer to avoid these
suspicious functions.
This would be a good candidate for converstion to xstrfmt,
but we need to record the length for dealing with index
entries. A strbuf handles that for us.
Signed-off-by: Jeff King <redacted>
---
sha1_name.c | 21 +++++++++------------
1 file changed, 9 insertions(+), 12 deletions(-)
@@ -1314,21 +1313,19 @@ static void diagnose_invalid_index_path(int stage,}/* Confusion between relative and absolute filenames? */-fullnamelen=namelen+strlen(prefix);-fullname=xmalloc(fullnamelen+1);-strcpy(fullname,prefix);-strcat(fullname,filename);-pos=cache_name_pos(fullname,fullnamelen);+strbuf_addstr(&fullname,prefix);+strbuf_addstr(&fullname,filename);+pos=cache_name_pos(fullname.buf,fullname.len);if(pos<0)pos=-pos-1;if(pos<active_nr){ce=active_cache[pos];-if(ce_namelen(ce)==fullnamelen&&-!memcmp(ce->name,fullname,fullnamelen))+if(ce_namelen(ce)==fullname.len&&+!memcmp(ce->name,fullname.buf,fullname.len))die("Path '%s' is in the index, but not '%s'.\n""Did you mean ':%d:%s' aka ':%d:./%s'?",-fullname,filename,-ce_stage(ce),fullname,+fullname.buf,filename,+ce_stage(ce),fullname.buf,ce_stage(ce),filename);}
@@ -1338,7 +1335,7 @@ static void diagnose_invalid_index_path(int stage,die("Path '%s' does not exist (neither on disk nor in the index).",filename);-free(fullname);+strbuf_release(&fullname);}
From: Jeff King <hidden> Date: 2016-06-15 23:06:34
This cleans up a magic number that must be kept in sync with
the rest of the code (the number of argv slots). It also
lets us drop some fixed buffers and an sprintf (since we
can now use argv_array_pushf).
We do still have to keep one fixed buffer for calling
gethostname, but at least now the size computations for it
are much simpler.
Signed-off-by: Jeff King <redacted>
---
fetch-pack.c | 56 +++++++++++++++++++++++++++-----------------------------
1 file changed, 27 insertions(+), 29 deletions(-)
@@ -681,11 +681,10 @@ static int get_pack(struct fetch_pack_args *args,intxd[2],char**pack_lockfile){structasyncdemux;-constchar*argv[22];-charkeep_arg[256];-charhdr_arg[256];-constchar**av,*cmd_name;intdo_keep=args->keep_pack;+constchar*cmd_name;+structpack_headerheader;+intpass_header=0;structchild_processcmd=CHILD_PROCESS_INIT;intret;
@@ -705,17 +704,11 @@ static int get_pack(struct fetch_pack_args *args,elsedemux.out=xd[0];-cmd.argv=argv;-av=argv;-*hdr_arg=0;if(!args->keep_pack&&unpack_limit){-structpack_headerheader;if(read_pack_header(demux.out,&header))die("protocol error: bad pack header");-snprintf(hdr_arg,sizeof(hdr_arg),-"--pack_header=%"PRIu32",%"PRIu32,-ntohl(header.hdr_version),ntohl(header.hdr_entries));+pass_header=1;if(ntohl(header.hdr_entries)<unpack_limit)do_keep=0;else
@@ -723,44 +716,49 @@ static int get_pack(struct fetch_pack_args *args,}if(alternate_shallow_file){-*av++="--shallow-file";-*av++=alternate_shallow_file;+argv_array_push(&cmd.args,"--shallow-file");+argv_array_push(&cmd.args,alternate_shallow_file);}if(do_keep){if(pack_lockfile)cmd.out=-1;-*av++=cmd_name="index-pack";-*av++="--stdin";+cmd_name="index-pack";+argv_array_push(&cmd.args,cmd_name);+argv_array_push(&cmd.args,"--stdin");if(!args->quiet&&!args->no_progress)-*av++="-v";+argv_array_push(&cmd.args,"-v");if(args->use_thin_pack)-*av++="--fix-thin";+argv_array_push(&cmd.args,"--fix-thin");if(args->lock_pack||unpack_limit){-ints=sprintf(keep_arg,-"--keep=fetch-pack %"PRIuMAX" on ",(uintmax_t)getpid());-if(gethostname(keep_arg+s,sizeof(keep_arg)-s))-strcpy(keep_arg+s,"localhost");-*av++=keep_arg;+charhostname[256];+if(gethostname(hostname,sizeof(hostname)))+xsnprintf(hostname,sizeof(hostname),"localhost");+argv_array_pushf(&cmd.args,+"--keep=fetch-pack %"PRIuMAX" on %s",+(uintmax_t)getpid(),hostname);}if(args->check_self_contained_and_connected)-*av++="--check-self-contained-and-connected";+argv_array_push(&cmd.args,"--check-self-contained-and-connected");}else{-*av++=cmd_name="unpack-objects";+cmd_name="unpack-objects";+argv_array_push(&cmd.args,cmd_name);if(args->quiet||args->no_progress)-*av++="-q";+argv_array_push(&cmd.args,"-q");args->check_self_contained_and_connected=0;}-if(*hdr_arg)-*av++=hdr_arg;++if(pass_header)+argv_array_pushf(&cmd.args,"--pack_header=%"PRIu32",%"PRIu32,+ntohl(header.hdr_version),+ntohl(header.hdr_entries));if(fetch_fsck_objects>=0?fetch_fsck_objects:transfer_fsck_objects>=0?transfer_fsck_objects:0)-*av++="--strict";-*av++=NULL;+argv_array_push(&cmd.args,"--strict");cmd.in=demux.out;cmd.git_cmd=1;
From: Jeff King <hidden> Date: 2016-06-15 23:06:34
This drops the magic number for the fixed-size argv arrays,
so we do not have to wonder if we are overflowing it. We can
also drop some confusing sha1_to_hex memory allocation
(which seems to predate the ring of buffers allowing
multiple calls), and get rid of an unchecked sprintf call.
Signed-off-by: Jeff King <redacted>
---
http-push.c | 32 ++++++++++----------------------
1 file changed, 10 insertions(+), 22 deletions(-)
@@ -1856,9 +1857,7 @@ int main(int argc, char **argv)new_refs=0;for(ref=remote_refs;ref;ref=ref->next){charold_hex[60],*new_hex;-constchar*commit_argv[5];-intcommit_argc;-char*new_sha1_hex,*old_sha1_hex;+structargv_arraycommit_argv=ARGV_ARRAY_INIT;if(!ref->peer_ref)continue;
@@ -1937,27 +1936,15 @@ int main(int argc, char **argv)}/* Set up revision info for this refspec */-commit_argc=3;-new_sha1_hex=xstrdup(sha1_to_hex(ref->new_sha1));-old_sha1_hex=NULL;-commit_argv[1]="--objects";-commit_argv[2]=new_sha1_hex;-if(!push_all&&!is_null_sha1(ref->old_sha1)){-old_sha1_hex=xmalloc(42);-sprintf(old_sha1_hex,"^%s",-sha1_to_hex(ref->old_sha1));-commit_argv[3]=old_sha1_hex;-commit_argc++;-}-commit_argv[commit_argc]=NULL;+argv_array_push(&commit_argv,"");/* ignored */+argv_array_push(&commit_argv,"--objects");+argv_array_push(&commit_argv,sha1_to_hex(ref->new_sha1));+if(!push_all&&!is_null_sha1(ref->old_sha1))+argv_array_pushf(&commit_argv,"^%s",+sha1_to_hex(ref->old_sha1));init_revisions(&revs,setup_git_directory());-setup_revisions(commit_argc,commit_argv,&revs,NULL);+setup_revisions(commit_argv.argc,commit_argv.argv,&revs,NULL);revs.edge_hint=0;/* just in case */-free(new_sha1_hex);-if(old_sha1_hex){-free(old_sha1_hex);-commit_argv[1]=NULL;-}/* Generate a list of objects that need to be pushed */pushing=0;
@@ -1986,6 +1973,7 @@ int main(int argc, char **argv)printf("%s %s\n",!rc?"ok":"error",ref->name);unlock_remote(ref_lock);check_locks();+argv_array_clear(&commit_argv);}/* Update remote server info if appropriate */
From: Jeff King <hidden> Date: 2016-06-15 23:06:34
In addition to dropping the magic number for the fixed-size
argv, we can also drop a fixed-length buffer and some
strcpy's into it.
Signed-off-by: Jeff King <redacted>
---
remote.c | 26 ++++++++++++--------------
1 file changed, 12 insertions(+), 14 deletions(-)
@@ -2027,10 +2028,9 @@ int stat_tracking_info(struct branch *branch, int *num_ours, int *num_theirs,{unsignedcharsha1[20];structcommit*ours,*theirs;-charsymmetric[84];structrev_inforevs;-constchar*rev_argv[10],*base;-intrev_argc;+constchar*base;+structargv_arrayargv=ARGV_ARRAY_INIT;/* Cannot stat unless we are marked to build on top of somebody else. */base=branch_get_upstream(branch,NULL);
@@ -2059,19 +2059,15 @@ int stat_tracking_info(struct branch *branch, int *num_ours, int *num_theirs,}/* Run "rev-list --left-right ours...theirs" internally... */-rev_argc=0;-rev_argv[rev_argc++]=NULL;-rev_argv[rev_argc++]="--left-right";-rev_argv[rev_argc++]=symmetric;-rev_argv[rev_argc++]="--";-rev_argv[rev_argc]=NULL;--strcpy(symmetric,sha1_to_hex(ours->object.sha1));-strcpy(symmetric+40,"...");-strcpy(symmetric+43,sha1_to_hex(theirs->object.sha1));+argv_array_push(&argv,"");/* ignored */+argv_array_push(&argv,"--left-right");+argv_array_pushf(&argv,"%s...%s",+sha1_to_hex(ours->object.sha1),+sha1_to_hex(theirs->object.sha1));+argv_array_push(&argv,"--");init_revisions(&revs,NULL);-setup_revisions(rev_argc,rev_argv,&revs,NULL);+setup_revisions(argv.argc,argv.argv,&revs,NULL);if(prepare_revision_walk(&revs))die("revision walk setup failed");
@@ -2091,6 +2087,8 @@ int stat_tracking_info(struct branch *branch, int *num_ours, int *num_theirs,/* clear object flags smudged by the above traversal */clear_commit_marks(ours,ALL_REV_FLAGS);clear_commit_marks(theirs,ALL_REV_FLAGS);++argv_array_clear(&argv);return0;}
From: Jeff King <hidden> Date: 2016-06-15 23:06:34
This avoids an ugly strcat into a fixed-size buffer. It's
not wrong (the buffer is plenty large enough for an IPv6
address plus some minor formatting), but it takes some
effort to verify that.
Unfortunately we are still stuck with some fixed-size
buffers to hold the output of inet_ntop. But at least we now
pass very easy-to-verify parameters, rather than doing a
manual computation to account for other data in the buffer.
As a side effect, this also fixes the case where we might
pass an uninitialized portbuf buffer through the
environment. This probably couldn't happen in practice, as
it would mean that addr->sa_family was neither AF_INET nor
AF_INET6 (and that is all we are listening on).
Signed-off-by: Jeff King <redacted>
---
daemon.c | 26 ++++++++++----------------
1 file changed, 10 insertions(+), 16 deletions(-)
From: Jeff King <hidden> Date: 2016-06-15 23:06:34
Before sha1_to_hex_to() existed, a simple way to get a hex
sha1 into a buffer was with:
strcpy(buf, sha1_to_hex(sha1));
This isn't wrong (assuming the buf is 41 characters), but it
makes auditing the code base for bad strcpy() calls harder,
as these become false positives.
Let's convert them to sha1_to_hex_to(), and likewise for
some calls to find_unique_abbrev(). While we're here, we'll
double-check that all of the buffers are correctly sized,
and use the more obvious GIT_SHA1_HEXSZ constant.
Signed-off-by: Jeff King <redacted>
---
builtin/blame.c | 8 ++++----
builtin/merge-index.c | 4 ++--
builtin/merge.c | 20 ++++++++++----------
builtin/receive-pack.c | 15 +++++++++------
builtin/rev-list.c | 4 ++--
diff.c | 9 ++++-----
6 files changed, 31 insertions(+), 29 deletions(-)
@@ -1415,15 +1415,15 @@ int cmd_merge(int argc, const char **argv, const char *prefix)/* Again the most common case of merging one remote. */structstrbufmsg=STRBUF_INIT;structcommit*commit;-charhex[41];-strcpy(hex,find_unique_abbrev(head_commit->object.sha1,DEFAULT_ABBREV));--if(verbosity>=0)-printf(_("Updating %s..%s\n"),-hex,-find_unique_abbrev(remoteheads->item->object.sha1,-DEFAULT_ABBREV));+if(verbosity>=0){+charfrom[GIT_SHA1_HEXSZ+1],to[GIT_SHA1_HEXSZ+1];+find_unique_abbrev_to(from,head_commit->object.sha1,+DEFAULT_ABBREV);+find_unique_abbrev_to(to,remoteheads->item->object.sha1,+DEFAULT_ABBREV);+printf(_("Updating %s..%s\n"),from,to);+}strbuf_addstr(&msg,"Fast-forward");if(have_message)strbuf_addstr(&msg,
@@ -242,7 +242,7 @@ static int show_bisect_vars(struct rev_list_info *info, int reaches, int all)cnt=reaches;if(revs->commits)-strcpy(hex,sha1_to_hex(revs->commits->item->object.sha1));+sha1_to_hex_to(hex,revs->commits->item->object.sha1);if(flags&BISECT_SHOW_ALL){traverse_commit_list(revs,show_commit,show_object,info);
@@ -2926,9 +2925,9 @@ static struct diff_tempfile *prepare_temp_file(const char *name,/* we can borrow from the file in the work tree */temp->name=name;if(!one->sha1_valid)-strcpy(temp->hex,sha1_to_hex(null_sha1));+sha1_to_hex_to(temp->hex,null_sha1);else-strcpy(temp->hex,sha1_to_hex(one->sha1));+sha1_to_hex_to(temp->hex,one->sha1);/* Even though we may sometimes borrow the*contentsfromtheworktree,wealwayswant*one->mode.modeistrustworthyevenwhen
From: Jeff King <hidden> Date: 2016-06-15 23:06:34
In some cases where we strcpy() the result of sha1_to_hex(),
there's no need; the result goes directly into a printf
statement, and we can simply pass the return value from
sha1_to_hex() directly.
Signed-off-by: Jeff King <redacted>
---
http-push.c | 6 ++----
walker.c | 5 ++---
2 files changed, 4 insertions(+), 7 deletions(-)
@@ -1856,7 +1856,6 @@ int main(int argc, char **argv)new_refs=0;for(ref=remote_refs;ref;ref=ref->next){-charold_hex[60],*new_hex;structargv_arraycommit_argv=ARGV_ARRAY_INIT;if(!ref->peer_ref)
@@ -1911,13 +1910,12 @@ int main(int argc, char **argv)}hashcpy(ref->new_sha1,ref->peer_ref->new_sha1);new_refs++;-strcpy(old_hex,sha1_to_hex(ref->old_sha1));-new_hex=sha1_to_hex(ref->new_sha1);fprintf(stderr,"updating '%s'",ref->name);if(strcmp(ref->name,ref->peer_ref->name))fprintf(stderr," using '%s'",ref->peer_ref->name);-fprintf(stderr,"\n from %s\n to %s\n",old_hex,new_hex);+fprintf(stderr,"\n from %s\n to %s\n",+sha1_to_hex(ref->old_sha1),sha1_to_hex(ref->new_sha1));if(dry_run){if(helper_status)printf("ok %s\n",ref->name);
From: Jeff King <hidden> Date: 2016-06-15 23:06:34
Our color parsing is designed to never exceed COLOR_MAXLEN
bytes. But the relationship between that hand-computed
number and the parsing code is not at all obvious, and we
merely hope that it has been computed correctly for all
cases.
Let's mark the expected "end" pointer for the destination
buffer and make sure that we do not exceed it.
Signed-off-by: Jeff King <redacted>
---
color.c | 39 ++++++++++++++++++++++++---------------
1 file changed, 24 insertions(+), 15 deletions(-)
@@ -150,22 +150,24 @@ int color_parse(const char *value, char *dst)*alreadyhavetheANSIescapecodeinit."out"shouldhaveenough*spaceinittofitanycolor.*/-staticchar*color_output(char*out,conststructcolor*c,chartype)+staticchar*color_output(char*out,intlen,conststructcolor*c,chartype){switch(c->type){caseCOLOR_UNSPECIFIED:caseCOLOR_NORMAL:break;caseCOLOR_ANSI:+if(len<2)+die("BUG: color parsing ran out of space");*out++=type;*out++='0'+c->value;break;caseCOLOR_256:-out+=sprintf(out,"%c8;5;%d",type,c->value);+out+=xsnprintf(out,len,"%c8;5;%d",type,c->value);break;caseCOLOR_RGB:-out+=sprintf(out,"%c8;2;%d;%d;%d",type,-c->red,c->green,c->blue);+out+=xsnprintf(out,len,"%c8;2;%d;%d;%d",type,+c->red,c->green,c->blue);break;}returnout;
@@ -180,12 +182,13 @@ int color_parse_mem(const char *value, int value_len, char *dst){constchar*ptr=value;intlen=value_len;+char*end=dst+COLOR_MAXLEN;unsignedintattr=0;structcolorfg={COLOR_UNSPECIFIED};structcolorbg={COLOR_UNSPECIFIED};if(!strncasecmp(value,"reset",len)){-strcpy(dst,GIT_COLOR_RESET);+xsnprintf(dst,end-dst,GIT_COLOR_RESET);return0;}
@@ -224,12 +227,18 @@ int color_parse_mem(const char *value, int value_len, char *dst)gotobad;}+#define OUT(x) do { \+if(dst==end)\+die("BUG: color parsing ran out of space");\+*dst++=(x);\+}while(0)+if(attr||!color_empty(&fg)||!color_empty(&bg)){intsep=0;inti;-*dst++='\033';-*dst++='[';+OUT('\033');+OUT('[');for(i=0;attr;i++){unsignedbit=(1<<i);
@@ -237,24 +246,24 @@ int color_parse_mem(const char *value, int value_len, char *dst)continue;attr&=~bit;if(sep++)-*dst++=';';-dst+=sprintf(dst,"%d",i);+OUT(';');+dst+=xsnprintf(dst,end-dst,"%d",i);}if(!color_empty(&fg)){if(sep++)-*dst++=';';+OUT(';');/* foreground colors are all in the 3x range */-dst=color_output(dst,&fg,'3');+dst=color_output(dst,end-dst,&fg,'3');}if(!color_empty(&bg)){if(sep++)-*dst++=';';+OUT(';');/* background colors are all in the 4x range */-dst=color_output(dst,&bg,'4');+dst=color_output(dst,end-dst,&bg,'4');}-*dst++='m';+OUT('m');}-*dst=0;+OUT(0);return0;bad:returnerror(_("invalid color value: %.*s"),value_len,value);
From: Jeff King <hidden> Date: 2016-06-15 23:06:34
This saves us some manual computation, and eliminates a call
to strcpy.
Signed-off-by: Jeff King <redacted>
---
builtin/fetch.c | 3 +--
remote-curl.c | 5 +----
2 files changed, 2 insertions(+), 6 deletions(-)
From: Jeff King <hidden> Date: 2016-06-15 23:06:34
When we are allocating a struct with a FLEX_ARRAY member, we
generally compute the size of the array and then sprintf or
strcpy into it. Normally we could improve a dynamic allocation
like this by using xstrfmt, but it doesn't work here; we
have to account for the size of the rest of the struct.
But we can improve things a bit by storing the length that
we use for the allocation, and then feeding it to xsnprintf
or memcpy, which makes it more obvious that we are not
writing more than the allocated number of bytes.
It would be nice if we had some kind of helper for
allocating generic flex arrays, but it doesn't work that
well:
- the call signature is a little bit unwieldy:
d = flex_struct(sizeof(*d), offsetof(d, path), fmt, ...);
You need offsetof here instead of just writing to the
end of the base size, because we don't know how the
struct is packed (partially this is because FLEX_ARRAY
might not be zero, though we can account for that; but
the size of the struct may actually be rounded up for
alignment, and we can't know that).
- some sites do clever things, like over-allocating because
they know they will write larger things into the buffer
later (e.g., struct packed_git here).
So we're better off to just write out each allocation (or
add type-specific helpers, though many of these are one-off
allocations anyway).
Signed-off-by: Jeff King <redacted>
---
Actually, I have a malloc-hardening series (which I'll post after this),
in which I _do_ break down and add a FLEX_ALLOC() macro. But we still
cannot use it for these cases anyway, because we don't assume all
platforms support variadic macros.
archive.c | 5 +++--
builtin/blame.c | 5 +++--
fast-import.c | 6 ++++--
refs.c | 8 ++++----
sha1_file.c | 5 +++--
submodule.c | 6 ++++--
6 files changed, 21 insertions(+), 14 deletions(-)
From: Jeff King <hidden> Date: 2016-06-15 23:06:34
To generate "--keep=receive-pack $pid on $host", we write
progressively into a single buffer, which requires keeping
track of how much we've written so far. But since the result
is destined to go into our argv array, we can simply use
argv_array_pushf.
Unfortunately we still have to have a static buffer for the
gethostname() call, but at least it now doesn't involve any
extra size computation. And as a bonus, we drop an sprintf
and a strcpy call.
Signed-off-by: Jeff King <redacted>
---
builtin/receive-pack.c | 17 ++++++++++-------
1 file changed, 10 insertions(+), 7 deletions(-)
From: Jeff King <hidden> Date: 2016-06-15 23:06:34
When we are going to launch "/path/to/konqueror", we instead
rewrite this into "/path/to/kfmclient" by duplicating the
original string and writing over the ending bits. This can
be done more obviously with strip_suffix and xstrfmt.
Note that we also fix a subtle bug with the "filename"
parameter, which is passed as argv[0] to the child. If the
user has configured a program name with no directory
component, we always pass the string "kfmclient", even if
your program is called something else. But if you give a
full path, we give the basename of that path. But more
bizarrely, if we rewrite "konqueror" to "kfmclient", we
still pass "konqueror".
The history of this function doesn't reveal anything
interesting, so it looks like just an oversight from
combining the suffix-munging with the basename-finding.
Let's just call basename on the munged path, which produces
consistent results (if you gave a program, whether a full
path or not, we pass its basename).
Probably this doesn't matter at all in practice, but it
makes the code slightly less confusing to read.
Signed-off-by: Jeff King <redacted>
---
builtin/help.c | 15 ++++-----------
1 file changed, 4 insertions(+), 11 deletions(-)
From: Jeff King <hidden> Date: 2016-06-15 23:06:34
When we already know the length of a string (e.g., because
we just malloc'd to fit it), it's nicer to use memcpy than
strcpy, as it makes it more obvious that we are not going to
overflow the buffer (because the size we pass matches the
size in the allocation).
This also eliminates calls to strcpy, which make auditing
the code base harder.
Signed-off-by: Jeff King <redacted>
---
compat/nedmalloc/nedmalloc.c | 5 +++--
fast-import.c | 5 +++--
revision.c | 2 +-
3 files changed, 7 insertions(+), 5 deletions(-)
From: Jeff King <hidden> Date: 2016-06-15 23:06:34
To set up default colors, we sometimes strcpy() from the
default string literals into our color buffers. This isn't a
bug (assuming the destination is COLOR_MAXLEN bytes), but
makes it harder to audit the code for problematic strcpy
calls.
Let's introduce a color_set which copies under the
assumption that there are COLOR_MAXLEN bytes in the
destination (of course you can call it on a smaller buffer,
so this isn't providing a huge amount of safety, but it's
more convenient than calling xsnprintf yourself).
Signed-off-by: Jeff King <redacted>
---
color.c | 5 +++++
color.h | 7 +++++++
grep.c | 32 ++++++++++++++++----------------
3 files changed, 28 insertions(+), 16 deletions(-)
From: Jeff King <hidden> Date: 2016-06-15 23:06:34
We know that a fanned-out sha1 in a notes tree cannot be
more than "aa/bb/cc/...", and we have an assert() to confirm
that. But let's factor out that length into a constant so we
can be sure it is used consistently. And even though we
assert() earlier, let's replace a strcpy with xsnprintf, so
it is clear to a reader that all cases are covered.
Signed-off-by: Jeff King <redacted>
---
notes.c | 9 ++++++---
1 file changed, 6 insertions(+), 3 deletions(-)
From: Jeff King <hidden> Date: 2016-06-15 23:06:34
strncpy is known to be a confusing function because of its
termination semantics. These calls are all correct, but it
takes some examination to see why. In particular, every one
of them expects to copy up to the length limit, and then
makes some arrangement for terminating the result.
We can just use memcpy, along with noting explicitly how the
result is terminated (if it is not already obvious). That
should make it more clear to a reader that we are doing the
right thing.
Signed-off-by: Jeff King <redacted>
---
builtin/help.c | 4 ++--
fast-import.c | 2 +-
tag.c | 2 +-
3 files changed, 4 insertions(+), 4 deletions(-)
@@ -82,7 +82,7 @@ int parse_tag_buffer(struct tag *item, const void *data, unsigned long size)nl=memchr(bufptr,'\n',tail-bufptr);if(!nl||sizeof(type)<=(nl-bufptr))return-1;-strncpy(type,bufptr,nl-bufptr);+memcpy(type,bufptr,nl-bufptr);type[nl-bufptr]='\0';bufptr=nl+1;
From: Jeff King <hidden> Date: 2016-06-15 23:06:34
Fsck tries to access loose objects in order of inode number,
with the hope that this would make cold cache access faster
on a spinning disk. This dates back to 7e8c174 (fsck-cache:
sort entries by inode number, 2005-05-02), which predates
the invention of packfiles.
These days, there's not much point in trying to optimize
cold cache for a large number of loose objects. You are much
better off to simply pack the objects, which will reduce the
disk footprint _and_ provide better locality of data access.
So while you can certainly construct pathological cases
where this code might help, it is not worth the trouble
anymore.
Signed-off-by: Jeff King <redacted>
---
builtin/fsck.c | 70 ++--------------------------------------------------------
1 file changed, 2 insertions(+), 68 deletions(-)
From: Jeff King <hidden> Date: 2016-06-15 23:06:34
Now that fsck has dropped its inode-sorting, there are no
longer any users of this knob, and it can go away.
Signed-off-by: Jeff King <redacted>
---
Makefile | 5 -----
config.mak.uname | 3 ---
configure.ac | 7 -------
3 files changed, 15 deletions(-)
@@ -74,8 +74,6 @@ all::# Define HAVE_PATHS_H if you have paths.h and want to use the default PATH# it specifies.#-# Define NO_D_INO_IN_DIRENT if you don't have d_ino in your struct dirent.-## Define NO_D_TYPE_IN_DIRENT if your platform defines DT_UNKNOWN but lacks# d_type in struct dirent (Cygwin 1.5, fixed in Cygwin 1.7).#
@@ -767,13 +767,6 @@ elif test x$ac_cv_member_struct_stat_st_mtim_tv_nsec != xyes; then GIT_CONF_SUBST([NO_NSEC]) fi #-# Define NO_D_INO_IN_DIRENT if you don't have d_ino in your struct dirent.-AC_CHECK_MEMBER(struct dirent.d_ino,-[NO_D_INO_IN_DIRENT=],-[NO_D_INO_IN_DIRENT=YesPlease],-[#include <dirent.h>])-GIT_CONF_SUBST([NO_D_INO_IN_DIRENT])-# # Define NO_D_TYPE_IN_DIRENT if your platform defines DT_UNKNOWN but lacks # d_type in struct dirent (latest Cygwin -- will be fixed soonish). AC_CHECK_MEMBER(struct dirent.d_type,
From: Jeff King <hidden> Date: 2016-06-15 23:06:34
Since 27e1e22 (prune: factor out loose-object directory
traversal, 2014-10-15), we now have a generic callback
system for iterating over the loose object directories. This
is used by prune, count-objects, etc.
We did not convert git-fsck at the time because it
implemented an inode-sorting scheme that was not part of the
generic code. Now that the inode-sorting code is gone, we
can reuse the generic code. The result is shorter,
hopefully more readable, and drops some unchecked sprintf
calls.
Signed-off-by: Jeff King <redacted>
---
builtin/fsck.c | 69 ++++++++++++++++++++--------------------------------------
1 file changed, 23 insertions(+), 46 deletions(-)
From: Jeff King <hidden> Date: 2016-06-15 23:06:34
When working with paths in strbufs, we frequently want to
ensure that a directory contains a trailing slash before
appending to it. We can shorten this code (and make the
intent more obvious) by calling strbuf_complete.
Most of these cases are trivially identical conversions, but
there are two things to note:
- in a few cases we did not check that the strbuf is
non-empty (which would lead to an out-of-bounds memory
access). These were generally not triggerable in
practice, either from earlier assertions, or typically
because we would have just fed the strbuf to opendir(),
which would choke on an empty path.
- in a few cases we indexed the buffer with "original_len"
or similar, rather than the current sb->len, and it is
not immediately obvious from the diff that they are the
same. In all of these cases, I manually verified that
the strbuf does not change between the assignment and
the strbuf_complete call.
This does not convert cases which look like:
if (sb->len && !is_dir_sep(sb->buf[sb->len - 1]))
strbuf_addch(sb, '/');
as those are obviously semantically different. Some of these
cases arguably should be doing that, but that is out of
scope for this change, which aims purely for cleanup with no
behavior change (and at least it will make such sites easier
to find and examine in the future, as we can grep for
strbuf_complete).
Signed-off-by: Jeff King <redacted>
---
builtin/clean.c | 6 ++----
builtin/log.c | 3 +--
diff-no-index.c | 6 ++----
dir.c | 6 ++----
imap-send.c | 3 +--
path.c | 3 +--
refs.c | 3 +--
url.c | 3 +--
8 files changed, 11 insertions(+), 22 deletions(-)
@@ -796,8 +796,7 @@ static int reopen_stdout(struct commit *commit, const char *subject,if(filename.len>=PATH_MAX-FORMAT_PATCH_NAME_MAX-suffix_len)returnerror(_("name of output directory is too long"));-if(filename.buf[filename.len-1]!='/')-strbuf_addch(&filename,'/');+strbuf_complete(&filename,'/');}if(rev->numbered_files)
@@ -1519,8 +1519,7 @@ static enum path_treatment treat_path_fast(struct dir_struct *dir,}strbuf_addstr(path,cdir->ucd->name);/* treat_one_path() does this before it calls treat_directory() */-if(path->buf[path->len-1]!='/')-strbuf_addch(path,'/');+strbuf_complete(path,'/');if(cdir->ucd->check_only)/**check_onlyissetasaresultoftreat_directory()getting
@@ -2126,8 +2125,7 @@ static int remove_dir_recurse(struct strbuf *path, int flag, int *kept_up)elsereturn-1;}-if(path->buf[original_len-1]!='/')-strbuf_addch(path,'/');+strbuf_complete(path,'/');len=path->len;while((e=readdir(dir))!=NULL){
@@ -2193,8 +2193,7 @@ int for_each_glob_ref_in(each_ref_fn fn, const char *pattern,if(!has_glob_specials(pattern)){/* Append implied '/' '*' if not present. */-if(real_pattern.buf[real_pattern.len-1]!='/')-strbuf_addch(&real_pattern,'/');+strbuf_complete(&real_pattern,'/');/* No need to check for '*', there is none. */strbuf_addch(&real_pattern,'*');}
From: Jeff King <hidden> Date: 2016-06-15 23:06:34
The manual size computations here are correct, but using
strip_suffix makes that obvious, and hopefully communicates
the intent of the code more clearly.
Signed-off-by: Jeff King <redacted>
---
builtin/name-rev.c | 9 ++++-----
1 file changed, 4 insertions(+), 5 deletions(-)
From: Ramsay Jones <hidden> Date: 2016-06-15 23:06:34
On 15/09/15 16:26, Jeff King wrote:
The sha1_to_hex and find_unique_abbrev functions always
write into reusable static buffers. There are a few problems
with this:
- future calls overwrite our result. This is especially
annoying with find_unique_abbrev, which does not have a
ring of buffers, so you cannot even printf() a result
that has two abbreviated sha1s.
- if you want to put the result into another buffer, we
often strcpy, which looks suspicious when auditing for
overflows.
This patch introduces sha1_to_hex_to and find_unique_abbrev_to,
which write into a user-provided buffer. Of course this is
just punting on the overflow-auditing, as the buffer
obviously needs to be GIT_SHA1_HEXSZ + 1 bytes. But it is
much easier to audit, since that is a well-known size.
Hmm, I haven't read any other patches yet (including those which use these
new '_to' functions), but I can't help feeling they should be named something
like 'sha1_to_hex_str()' and 'find_unique_abbrev_str()' instead. i.e. I don't get
the '_to' thing - not that I'm any good at naming things ...
ATB,
Ramsay Jones
quoted hunk
We retain the non-reentrant forms, which just become thin
wrappers around the reentrant ones. This patch also adds a
strbuf variant of find_unique_abbrev, which will be handy in
later patches.
Signed-off-by: Jeff King <redacted>
---
If we wanted to be really meticulous, these functions could
take a size for the output buffer, and complain if it is not
GIT_SHA1_HEXSZ+1 bytes. But that would bloat every call
like:
sha1_to_hex_to(buf, sizeof(buf), sha1);
cache.h | 27 ++++++++++++++++++++++++++-
hex.c | 13 +++++++++----
sha1_name.c | 16 +++++++++++-----
strbuf.c | 9 +++++++++
strbuf.h | 8 ++++++++
5 files changed, 63 insertions(+), 10 deletions(-)
From: Jeff King <hidden> Date: 2016-06-15 23:06:34
On Tue, Sep 15, 2015 at 05:55:55PM +0100, Ramsay Jones wrote:
On 15/09/15 16:26, Jeff King wrote:
quoted
The sha1_to_hex and find_unique_abbrev functions always
write into reusable static buffers. There are a few problems
with this:
- future calls overwrite our result. This is especially
annoying with find_unique_abbrev, which does not have a
ring of buffers, so you cannot even printf() a result
that has two abbreviated sha1s.
- if you want to put the result into another buffer, we
often strcpy, which looks suspicious when auditing for
overflows.
This patch introduces sha1_to_hex_to and find_unique_abbrev_to,
which write into a user-provided buffer. Of course this is
just punting on the overflow-auditing, as the buffer
obviously needs to be GIT_SHA1_HEXSZ + 1 bytes. But it is
much easier to audit, since that is a well-known size.
Hmm, I haven't read any other patches yet (including those which use these
new '_to' functions), but I can't help feeling they should be named something
like 'sha1_to_hex_str()' and 'find_unique_abbrev_str()' instead. i.e. I don't get
the '_to' thing - not that I'm any good at naming things ...
I meant it as a contrast with their original. sha1_to_hex() formats into
an internal buffer and returns it. But sha1_to_hex_to() formats "to" a
buffer of your choice.
I'm happy to switch the names to something else, but I don't think
_str() conveys the difference. If I were starting from scratch, I would
probably have just called my variant sha1_to_hex(), and called the
original sha1_to_hex_unsafe(). :)
-Peff
From: Johannes Schindelin <hidden> Date: 2016-06-15 23:06:34
Hi Peff,
On 2015-09-15 17:24, Jeff King wrote:
Commit 02976bf (fsck: introduce `git fsck --connectivity-only`,
2015-06-22) recently gave fsck an option to perform only a
subset of the checks, by skipping the fsck_object_dir()
call. However, it does so only for the local object
directory, and we still do expensive checks on any alternate
repos. We should skip them in this case, too.
Signed-off-by: Jeff King <redacted>
From: Ramsay Jones <hidden> Date: 2016-06-15 23:06:34
On 15/09/15 16:36, Jeff King wrote:
quoted hunk
We sometimes sprintf into static buffers when we know that
the size of the buffer is large enough to fit the input
(either because it's a constant, or because it's numeric
input that is bounded in size). Likewise with strcpy of
constant strings.
However, these sites make it hard to audit sprintf and
strcpy calls for buffer overflows, as a reader has to
cross-reference the size of the array with the input. Let's
use xsnprintf instead, which communicates to a reader that
we don't expect this to overflow (and catches the mistake in
case we do).
Signed-off-by: Jeff King <redacted>
---
These are all pretty trivial; the obvious thing to get wrong is that
"sizeof(buf)" is not the correct length if "buf" is a pointer. I
considered a macro wrapper like:
#define xsnprintf_array(dst, fmt, ...) \
xsnprintf(dst, sizeof(dst) + BARF_UNLESS_AN_ARRAY(dst), \
fmt, __VA_ARGS__)
but obviously that requires variadic macro support.
archive-tar.c | 2 +-
builtin/gc.c | 2 +-
builtin/init-db.c | 11 ++++++-----
builtin/ls-tree.c | 9 +++++----
builtin/merge-index.c | 2 +-
builtin/merge-recursive.c | 2 +-
builtin/read-tree.c | 2 +-
builtin/unpack-file.c | 2 +-
compat/mingw.c | 8 +++++---
compat/winansi.c | 2 +-
connect.c | 2 +-
convert.c | 3 ++-
daemon.c | 4 ++--
diff.c | 12 ++++++------
http-push.c | 2 +-
http.c | 6 +++---
ll-merge.c | 12 ++++++------
refs.c | 8 ++++----
sideband.c | 4 ++--
strbuf.c | 4 ++--
20 files changed, 52 insertions(+), 47 deletions(-)
@@ -301,7 +301,7 @@ static int write_global_extended_header(struct archiver_args *args)memset(&header,0,sizeof(header));*header.typeflag=TYPEFLAG_GLOBAL_HEADER;mode=0100666;-strcpy(header.name,"pax_global_header");+xsnprintf(header.name,sizeof(header.name),"pax_global_header");
How about using strlcpy() instead? Thus:
- strcpy(header.name, "pax_global_header");
+ strlcpy(header.name, "pax_global_header", sizeof(header.name));
Ditto for other similar (strcpy->xsnprintf) hunks below.
ATB,
Ramsay Jones
@@ -262,7 +262,8 @@ static int create_default_files(const char *template_path)}/* This forces creation of new config file */-sprintf(repo_version_string,"%d",GIT_REPO_VERSION);+xsnprintf(repo_version_string,sizeof(repo_version_string),+"%d",GIT_REPO_VERSION);git_config_set("core.repositoryformatversion",repo_version_string);path[len]=0;
@@ -414,13 +415,13 @@ int init_db(const char *template_dir, unsigned int flags)*/if(shared_repository<0)/* force to the mode value */-sprintf(buf,"0%o",-shared_repository);+xsnprintf(buf,sizeof(buf),"0%o",-shared_repository);elseif(shared_repository==PERM_GROUP)-sprintf(buf,"%d",OLD_PERM_GROUP);+xsnprintf(buf,sizeof(buf),"%d",OLD_PERM_GROUP);elseif(shared_repository==PERM_EVERYBODY)-sprintf(buf,"%d",OLD_PERM_EVERYBODY);+xsnprintf(buf,sizeof(buf),"%d",OLD_PERM_EVERYBODY);else-die("oops");+die("BUG: invalid value for shared_repository");git_config_set("core.sharedrepository",buf);git_config_set("receive.denyNonFastforwards","true");}
@@ -539,7 +539,7 @@ void winansi_init(void)return;/* create a named pipe to communicate with the console thread */-sprintf(name,"\\\\.\\pipe\\winansi%lu",GetCurrentProcessId());+xsnprintf(name,sizeof(name),"\\\\.\\pipe\\winansi%lu",GetCurrentProcessId());hwrite=CreateNamedPipe(name,PIPE_ACCESS_OUTBOUND,PIPE_TYPE_BYTE|PIPE_WAIT,1,BUFFER_SIZE,0,0,NULL);if(hwrite==INVALID_HANDLE_VALUE)
@@ -301,7 +301,7 @@ static int write_global_extended_header(struct archiver_args *args)memset(&header,0,sizeof(header));*header.typeflag=TYPEFLAG_GLOBAL_HEADER;mode=0100666;-strcpy(header.name,"pax_global_header");+xsnprintf(header.name,sizeof(header.name),"pax_global_header");
How about using strlcpy() instead? Thus:
- strcpy(header.name, "pax_global_header");
+ strlcpy(header.name, "pax_global_header", sizeof(header.name));
Ditto for other similar (strcpy->xsnprintf) hunks below.
That misses the "assert" behavior of xsnprintf. We are preventing
overflow here, but also truncation. What should happen if
"pax_global_header" does not fit in header.name? I think complaining
loudly and immediately is the most helpful thing, because it is surely a
programming error.
We could make xstrlcpy(), of course, but I don't see much point when
xsnprintf does the same thing (and more).
-Peff
From: Ramsay Jones <hidden> Date: 2016-06-15 23:06:34
On 15/09/15 16:40, Jeff King wrote:
quoted hunk
This particular conversion is non-obvious, because nobody
has passed our function the length of the destination
buffer. However, the interface to checkout_entry specifies
that the buffer must be at least TEMPORARY_FILENAME_LENGTH
bytes long, so we can check that (meaning the existing code
was not buggy, but merely worrisome to somebody reading it).
Signed-off-by: Jeff King <redacted>
---
entry.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
@@ -96,8 +96,8 @@ static int open_output_fd(char *path, const struct cache_entry *ce, int to_tempf{intsymlink=(ce->ce_mode&S_IFMT)!=S_IFREG;if(to_tempfile){-strcpy(path,symlink-?".merge_link_XXXXXX":".merge_file_XXXXXX");+xsnprintf(path,TEMPORARY_FILENAME_LENGTH,"%s",+symlink?".merge_link_XXXXXX":".merge_file_XXXXXX");returnmkstemp(path);}else{returncreate_file(path,!symlink?ce->ce_mode:0666);
Hmm, I was going to suggest strlcpy() again. However, if you expect an overflow to
occur, then xsnprintf() will at least bring it to your attention! Checking for overflow
with strlcpy() is not rocket science either, and I guess we could add xstrlcpy() ... :-D
dunno.
ATB,
Ramsay Jones
@@ -301,7 +301,7 @@ static int write_global_extended_header(struct archiver_args *args)memset(&header,0,sizeof(header));*header.typeflag=TYPEFLAG_GLOBAL_HEADER;mode=0100666;-strcpy(header.name,"pax_global_header");+xsnprintf(header.name,sizeof(header.name),"pax_global_header");
How about using strlcpy() instead? Thus:
- strcpy(header.name, "pax_global_header");
+ strlcpy(header.name, "pax_global_header", sizeof(header.name));
Ditto for other similar (strcpy->xsnprintf) hunks below.
That misses the "assert" behavior of xsnprintf. We are preventing
overflow here, but also truncation. What should happen if
"pax_global_header" does not fit in header.name? I think complaining
loudly and immediately is the most helpful thing, because it is surely a
programming error.
We could make xstrlcpy(), of course, but I don't see much point when
xsnprintf does the same thing (and more).
Heh, I just sent an email about patch 22/67 which says similar things. I don't feel
too strongly, either way, but I have a slight preference for the use of [x]strlcpy()
in these cases.
I have to stop at patch #22 for now.
ATB,
Ramsay Jones