Re: [PATCH 15/67] convert trivial sprintf / strcpy calls to xsnprintf

On Tue, Sep 15, 2015 at 11:36 AM, Jeff King wrote: > 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

Re: [PATCH 26/67] replace trivial malloc + sprintf /strcpy calls to xstrfmt

On Tue, Sep 15, 2015 at 11:45 AM, Jeff King wrote: > replace trivial malloc + sprintf /strcpy calls to xstrfmt s/to/with/ Also, do you want either to add a space after '/' or drop the one before it? > It's a common pattern to do: > > foo = xmalloc(strlen(one) + strlen(two) + 1

Re: [PATCH] strtoul_ui: actually report error in case of negative input

On Tue, Sep 15, 2015 at 08:50:03AM +0200, Matthieu Moy wrote: > I think it would be better to just return a long to avoid needless > limitations, but changing the argument to "long" would interfer with > in-flight topics. Not worth the trouble. Sure. > > One potential issue with your patch is

Re: [PATCH 29/67] use strip_suffix and xstrfmt to replace suffix

On Tue, Sep 15, 2015 at 11:47 AM, Jeff King wrote: > 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

Re: [PATCH 15/67] convert trivial sprintf / strcpy calls to xsnprintf

Ramsay Jones writes: > 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. Please do not

Re: [PATCH 08/67] add reentrant variants of sha1_to_hex and find_unique_abbrev

Jeff King writes: >> 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'

Re: [PATCH 07/67] strbuf: make strbuf_complete_line more generic

Eric Sunshine writes: >> +static inline void strbuf_complete(struct strbuf *sb, char term) >> +{ >> + if (sb->len && sb->buf[sb->len - 1] != term) >> + strbuf_addch(sb, term); >> +} > > Hmm, so this only adds 'term' if not already present *and* if

Re: [PATCH 0/67] war on sprintf, strcpy, etc

Jeff King writes: > 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. Thanks.

Re: [PATCH] strtoul_ui: actually report error in case of negative input

Matthieu Moy writes: > Not just the return type (which is the error status), but also the type > of the result argument indeed. It's not clear to me whether this is > intentional (09f2825 (git-grep: don't use sscanf, 2007-03-12) introduced > it, the commit message

Re: "Medium" log format: change proposal for author != committer

"Robin H. Johnson" writes: > Specifically, if the author is NOT the same as the committer, then > display both in the header. Otherwise continue to display only the > author. I too found myself wanting to see both of the names sometimes, and the "fuller" format was added

[PATCH v5] remote: add get-url subcommand

Expanding `insteadOf` is a part of ls-remote --url and there is no way to expand `pushInsteadOf` as well. Add a get-url subcommand to be able to query both as well as a way to get all configured urls. Signed-off-by: Ben Boeckel --- Documentation/git-remote.txt | 10

Re: [PATCH 07/67] strbuf: make strbuf_complete_line more generic

On Tue, Sep 15, 2015 at 11:25 AM, Jeff King wrote: > The strbuf_complete_line function make sure that a buffer s/make/makes/ > 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

Re: [PATCH 10/67] mailsplit: make PATH_MAX buffers dynamic

On Tue, Sep 15, 2015 at 11:28 AM, Jeff King wrote: > 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 >

Re: [PATCH 11/67] trace: use strbuf for quote_crnl output

On Tue, Sep 15, 2015 at 11:28 AM, Jeff King wrote: > 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

Re: [Feature Request] git blame showing only revisions from git rev-list --first-parent

Jeff King writes: > It seems like nobody is actually that interested in what "blame > --first-parent --reverse" does in the first place, though, and there's > no reason for its complexity to hold up vanilla --first-parent. So what > do you think of: I like the part that

"Medium" log format: change proposal for author != committer

Hi, I want to propose a change to the 'medium' log output format, to improve readability. Specifically, if the author is NOT the same as the committer, then display both in the header. Otherwise continue to display only the author. This would aid quick review of changes in git-log & git-show

Re: [PATCH v2 2/2] git-p4: handle "Translation of file content failed"

On 15/09/15 16:38, Lars Schneider wrote: On 15 Sep 2015, at 08:43, Luke Diamand wrote: Do we know the mechanism by which we end up in this state? Unfortunately no. I tried hard to reproduce the error with “conventional” methods. As you can see I ended up manipulating

RE: Git Deployment using existing multiple environments

Hi, please stop top-posting. It is quite irritating by now. Thank you. On 2015-09-15 08:50, Sukhwinder Singh wrote: > Now lets say we set up a repository at github which has the latest > code (all test code)., Now at each of our own servers we already have > existing code, that is Test, UAT and

Re: What's cooking in git.git (Sep 2015, #03; Mon, 14)

From: "Junio C Hamano" Sent: Monday, September 14, 2015 11:43 PM -- [New Topics] * po/doc-branch-desc (2015-09-14) 1 commit (merged to 'next' on 2015-09-14 at 4934a96) + doc: show usage of branch description The branch

Re: [PATCH v2 2/2] git-p4: handle "Translation of file content failed"

On 14/09/15 17:55, larsxschnei...@gmail.com wrote: From: Lars Schneider A P4 repository can get into a state where it contains a file with type UTF-16 that does not contain a valid UTF-16 BOM. If git-p4 Sorry - what's a BOM? I'm assuming it's not a Bill of

Re: [PATCH v5 1/7] git-p4: add optional type specifier to gitConfig reader

On 14/09/15 14:26, larsxschnei...@gmail.com wrote: From: Lars Schneider The functions “gitConfig” and “gitConfigBool” are almost identical. Make “gitConfig” more generic by adding an optional type specifier. Use the type specifier “—bool” with “gitConfig” to

Re: [PATCH v7] git-p4: improve path encoding verbose output

On 15/09/15 08:31, Luke Diamand wrote: On 14/09/15 18:10, larsxschnei...@gmail.com wrote: It would be better to query this once at startup. Otherwise we're potentially forking "git config" twice per file which on a large repo could become significant. Make it an instance variable perhaps?

Re: [PATCH v7] git-p4: improve path encoding verbose output

On 14/09/15 18:10, larsxschnei...@gmail.com wrote: From: Lars Schneider If a path with non-ASCII characters is detected then print always the s/print always/print/ encoding and the encoded string in verbose mode. Signed-off-by: Lars Schneider

RE: Git Deployment using existing multiple environments

Thank you very much for the detailed reply. There is going to be just one person who'll manage all the moving of the code. On approval he'll move code from one environment to other on our own servers (right now it is being done by manual merging). Development team is not that big right now.

Re: [PATCH] strtoul_ui: actually report error in case of negative input

[ Cc-ing Michael Haggerty who wrote the numparse module ] Max Kirillov writes: > On Mon, Sep 14, 2015 at 08:30:54AM +0200, Matthieu Moy wrote: >>> Fix it by changing the last check to trigger earlier, as soon as it >>> becomes bigger than INT_MAX. >> >> What if the value is

Re: [PATCH 15/67] convert trivial sprintf / strcpy calls to xsnprintf

On 15/09/15 16:36, Jeff King wrote: > 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. > >

Re: [PATCH 15/67] convert trivial sprintf / strcpy calls to xsnprintf

On Tue, Sep 15, 2015 at 07:32:29PM +0100, Ramsay Jones wrote: > > diff --git a/archive-tar.c b/archive-tar.c > > index b6b30bb..d543f93 100644 > > --- a/archive-tar.c > > +++ b/archive-tar.c > > @@ -301,7 +301,7 @@ static int write_global_extended_header(struct > > archiver_args *args) > >

Re: What's cooking in git.git (Sep 2015, #03; Mon, 14)

Oops.. From: "Philip Oakley" [New Topics] * po/doc-branch-desc (2015-09-14) 1 commit (merged to 'next' on 2015-09-14 at 4934a96) + doc: show usage of branch description The branch descriptions that are set with "git branch --edit-description" option were used in many

Re: [PATCH] doc: show usage of branch description

From: "Junio C Hamano" "Philip Oakley" writes: It still means that my patch is incomplete in its aim to bring out these possible broader usages. I haven't yet looked at the mail archives to see if there is more around the time of those introductions.

Re: [PATCH 04/67] fsck: don't fsck alternates for connectivity-only check

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 >

Re: [PATCH 22/67] entry.c: convert strcpy to xsnprintf

On 15/09/15 16:40, Jeff King wrote: > 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

Re: [PATCH] doc: show usage of branch description

From: "Junio C Hamano" "Philip Oakley" writes: It still means that my patch is incomplete in its aim to bring out these possible broader usages. I haven't yet looked at the mail archives to see if there is more around the time of those introductions.

Re: [PATCH 15/67] convert trivial sprintf / strcpy calls to xsnprintf

On 15/09/15 19:42, Jeff King wrote: > On Tue, Sep 15, 2015 at 07:32:29PM +0100, Ramsay Jones wrote: > >>> diff --git a/archive-tar.c b/archive-tar.c >>> index b6b30bb..d543f93 100644 >>> --- a/archive-tar.c >>> +++ b/archive-tar.c >>> @@ -301,7 +301,7 @@ static int

RE: Git Deployment using existing multiple environments

--- > Date: Tue, 15 Sep 2015 09:48:34 +0200 > From: johannes.schinde...@gmx.de > To: php_programmer_in...@hotmail.com > CC: jacob.kel...@gmail.com; git@vger.kernel.org > Subject: RE: Git Deployment using existing multiple environments > > Hi, > > please stop

Re: [bug] .git/hooks/commit-msg.sample test is reversed

Federico Fissore writes: > Hello everyone > > In file commit-msg.sample, grep test is reversed. It greps > '^Signed-off-by: ' > while it should grep > 'Signed-off-by: ' > > If you run the sample against attached file, it won't complain. Remove > the ^ and it will work as

Re: [Feature Request] git blame showing only revisions from git rev-list --first-parent

On Sun, Sep 13, 2015 at 10:19:33PM -0700, Junio C Hamano wrote: > For that matter, I am not sure how "blame A..B" with first-parent & > reverse should behave when A is not an ancestor on the first-parent > chain. Wouldn't we try to find a cut-point on the first-parent chain by > traversing

Re: Developing- Where to Start

Breanna Devore-McDonald writes: > Hello all, Hello, > I'm a third year Computer Science student at the University of Notre > Dame, and for the final project of my Data Structures class, my group > and I have to find a way to contribute our (hopefully)

[bug] .git/hooks/commit-msg.sample test is reversed

Hello everyone In file commit-msg.sample, grep test is reversed. It greps '^Signed-off-by: ' while it should grep 'Signed-off-by: ' If you run the sample against attached file, it won't complain. Remove the ^ and it will work as expected Regards Federico Commit message

[PATCH 19/67] stop_progress_msg: convert sprintf to xsnprintf

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

Re: [PATCH v2 2/2] git-p4: handle "Translation of file content failed"

On 15 Sep 2015, at 08:43, Luke Diamand wrote: > On 14/09/15 17:55, larsxschnei...@gmail.com wrote: >> From: Lars Schneider >> >> A P4 repository can get into a state where it contains a file with >> type UTF-16 that does not contain a valid UTF-16

[PATCH 18/67] find_short_object_filename: convert sprintf to xsnprintf

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

[PATCH 20/67] compat/hstrerror: convert sprintf to snprintf

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 ---

[PATCH 13/67] test-dump-cache-tree: avoid overflow of cache-tree name

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

[PATCH 21/67] grep: use xsnprintf to format failure message

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 --- grep.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)

[PATCH 41/67] apply: convert root string to strbuf

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

[PATCH 42/67] transport: use strbufs for status table "quickref" strings

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

[PATCH 48/67] fetch-pack: use argv_array for index-pack / unpack-objects

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

[PATCH 50/67] stat_tracking_info: convert to argv_array

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 --- remote.c | 26 -- 1 file changed, 12 insertions(+), 14 deletions(-) diff --git a/remote.c

[PATCH 44/67] enter_repo: convert fixed-size buffers to strbufs

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

[PATCH 46/67] write_loose_object: convert to strbuf

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

[PATCH 49/67] http-push: use an argv_array for setup_revisions

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.

[PATCH 45/67] remove_leading_path: use a strbuf for internal storage

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

[PATCH 43/67] merge-recursive: convert malloc / strcpy to strbuf

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

Re: [PATCH v7] git-p4: improve path encoding verbose output

On 15 Sep 2015, at 09:31, Luke Diamand wrote: > On 14/09/15 18:10, larsxschnei...@gmail.com wrote: >> From: Lars Schneider >> >> If a path with non-ASCII characters is detected then print always the > > s/print always/print/ I will fix it. > >

[PATCH 47/67] diagnose_invalid_index_path: use strbuf to avoid strcpy/strcat

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.

[PATCH 60/67] color: add color_set helper for copying raw colors

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

[PATCH 01/67] show-branch: avoid segfault with --reflog of unborn branch

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 --- builtin/show-branch.c | 2 ++ 1 file

[PATCH 10/67] mailsplit: make PATH_MAX buffers dynamic

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

[PATCH 09/67] fsck: use strbuf to generate alternate directories

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

[PATCH 11/67] trace: use strbuf for quote_crnl output

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.

[PATCH 64/67] Makefile: drop D_INO_IN_DIRENT build knob

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 --- Makefile | 5 - config.mak.uname | 3 --- configure.ac | 7 --- 3 files changed, 15 deletions(-) diff --git a/Makefile

[PATCH 65/67] fsck: use for_each_loose_file_in_objdir

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

[PATCH 63/67] fsck: drop inode-sorting code

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

[PATCH 07/67] strbuf: make strbuf_complete_line more generic

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 --- strbuf.h |

[PATCH 04/67] fsck: don't fsck alternates for connectivity-only check

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.

[PATCH 06/67] add git_path_buf helper function

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"));

[PATCH 05/67] add xsnprintf helper function

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

[PATCH 59/67] prefer memcpy to strcpy

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

[PATCH 67/67] name-rev: use strip_suffix to avoid magic numbers

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 --- builtin/name-rev.c | 9 - 1 file changed, 4 insertions(+), 5 deletions(-) diff --git

[PATCH 03/67] archive-tar: fix minor indentation violation

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 --- archive-tar.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/archive-tar.c b/archive-tar.c index 0d1e6bd..b6b30bb

[PATCH 02/67] mailsplit: fix FILE* leak in split_maildir

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 --- builtin/mailsplit.c | 5 - 1 file

[PATCH 22/67] entry.c: convert strcpy to xsnprintf

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

[PATCH 54/67] color: add overflow checks for parsing colors

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

[PATCH 55/67] use alloc_ref rather than hand-allocating "struct ref"

This saves us some manual computation, and eliminates a call to strcpy. Signed-off-by: Jeff King --- builtin/fetch.c | 3 +-- remote-curl.c | 5 + 2 files changed, 2 insertions(+), 6 deletions(-) diff --git a/builtin/fetch.c b/builtin/fetch.c index 841880e..ed84963 100644

[PATCH 57/67] receive-pack: simplify keep_arg computation

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

[PATCH 58/67] help: clean up kfmclient munging

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,

[PATCH 0/67] war on sprintf, strcpy, etc

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

[PATCH 12/67] progress: store throughput display in a strbuf

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

[PATCH 24/67] http-push: replace strcat with xsnprintf

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

[PATCH 25/67] receive-pack: convert strncpy to xsnprintf

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: "

[PATCH 23/67] add_packed_git: convert strcpy into xsnprintf

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

[PATCH 52/67] use sha1_to_hex_to() instead of strcpy

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

[PATCH 51/67] daemon: use cld->env_array when re-spawning

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

[PATCH 61/67] notes: document length of fanout path with a constant

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

[PATCH 08/67] add reentrant variants of sha1_to_hex and find_unique_abbrev

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

[PATCH 14/67] compat/inet_ntop: fix off-by-one in inet_ntop4

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

[PATCH 15/67] convert trivial sprintf / strcpy calls to xsnprintf

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

[PATCH 26/67] replace trivial malloc + sprintf /strcpy calls to xstrfmt

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

[PATCH 30/67] ref-filter: drop sprintf and strcpy calls

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

[PATCH 28/67] fetch: replace static buffer with xstrfmt

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

[PATCH 31/67] help: drop prepend function in favor of xstrfmt

This function predates xstrfmt, and its functionality is a subset. Let's just use xstrfmt. Signed-off-by: Jeff King --- builtin/help.c | 14 ++ 1 file changed, 2 insertions(+), 12 deletions(-) diff --git a/builtin/help.c b/builtin/help.c index 3422e73..fba8c01 100644

[PATCH 29/67] use strip_suffix and xstrfmt to replace suffix

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

[PATCH 27/67] config: use xstrfmt in normalize_value

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

[PATCH 33/67] read_branches_file: replace strcpy with xstrdup

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 --- remote.c | 5 + 1 file

[PATCH 53/67] drop strcpy in favor of raw sha1_to_hex

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 --- http-push.c | 6 ++ walker.c| 5 ++--- 2 files

[PATCH 56/67] avoid sprintf and strcpy with flex arrays

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.

[PATCH 62/67] convert strncpy to memcpy

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

Re: [PATCH 08/67] add reentrant variants of sha1_to_hex and find_unique_abbrev

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 >

Re: [PATCH 08/67] add reentrant variants of sha1_to_hex and find_unique_abbrev

On Tue, Sep 15, 2015 at 05:55:55PM +0100, Ramsay Jones wrote: > 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

[PATCH 17/67] use xsnprintf for generating git object headers

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.

  1   2   >