- 06 10月, 2015 24 次提交
-
-
由 Jeff King 提交于
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: NJeff King <peff@peff.net> Signed-off-by: NJunio C Hamano <gitster@pobox.com>
-
由 Jeff King 提交于
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: NJeff King <peff@peff.net> Signed-off-by: NJunio C Hamano <gitster@pobox.com>
-
由 Jeff King 提交于
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: NJeff King <peff@peff.net> Signed-off-by: NJunio C Hamano <gitster@pobox.com>
-
由 Jeff King 提交于
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: NJeff King <peff@peff.net> Signed-off-by: NJunio C Hamano <gitster@pobox.com>
-
由 Jeff King 提交于
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 fixed-size 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: NJeff King <peff@peff.net> Signed-off-by: NJunio C Hamano <gitster@pobox.com>
-
由 Jeff King 提交于
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: NJeff King <peff@peff.net> Signed-off-by: NJunio C Hamano <gitster@pobox.com>
-
由 Jeff King 提交于
This saves us some manual computation, and eliminates a call to strcpy. Signed-off-by: NJeff King <peff@peff.net> Signed-off-by: NJunio C Hamano <gitster@pobox.com>
-
由 Jeff King 提交于
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: NJeff King <peff@peff.net> Signed-off-by: NJunio C Hamano <gitster@pobox.com>
-
由 Jeff King 提交于
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. When this code was originally written, sha1_to_hex used a single buffer, and it was not safe to use it twice within a single expression. That changed as of dcb3450f (sha1_to_hex() usage cleanup, 2006-05-03), but this code was never updated. History-dug-by: NEric Sunshine <sunshine@sunshineco.com> Signed-off-by: NJeff King <peff@peff.net> Signed-off-by: NJunio C Hamano <gitster@pobox.com>
-
由 Jeff King 提交于
Before sha1_to_hex_r() existed, a simple way to get 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_r(), 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: NJeff King <peff@peff.net> Signed-off-by: NJunio C Hamano <gitster@pobox.com>
-
由 Jeff King 提交于
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: NJeff King <peff@peff.net> Signed-off-by: NJunio C Hamano <gitster@pobox.com>
-
由 Jeff King 提交于
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: NJeff King <peff@peff.net> Signed-off-by: NJunio C Hamano <gitster@pobox.com>
-
由 Jeff King 提交于
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: NJeff King <peff@peff.net> Signed-off-by: NJunio C Hamano <gitster@pobox.com>
-
由 Jeff King 提交于
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: NJeff King <peff@peff.net> Signed-off-by: NJunio C Hamano <gitster@pobox.com>
-
由 Jeff King 提交于
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: NJeff King <peff@peff.net> Signed-off-by: NJunio C Hamano <gitster@pobox.com>
-
由 Jeff King 提交于
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: NJeff King <peff@peff.net> Signed-off-by: NJunio C Hamano <gitster@pobox.com>
-
由 Jeff King 提交于
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: NJeff King <peff@peff.net> Signed-off-by: NJunio C Hamano <gitster@pobox.com>
-
由 Jeff King 提交于
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: NJeff King <peff@peff.net> Signed-off-by: NJunio C Hamano <gitster@pobox.com>
-
由 Jeff King 提交于
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: NJeff King <peff@peff.net> Signed-off-by: NJunio C Hamano <gitster@pobox.com>
-
由 Jeff King 提交于
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: NJeff King <peff@peff.net> Signed-off-by: NJunio C Hamano <gitster@pobox.com>
-
由 Jeff King 提交于
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: NJeff King <peff@peff.net> Signed-off-by: NJunio C Hamano <gitster@pobox.com>
-
由 Jeff King 提交于
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 in favor of 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 32d1776b (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: NJeff King <peff@peff.net> Signed-off-by: NJunio C Hamano <gitster@pobox.com>
-
由 Jeff King 提交于
When we are initializing a .git directory, we may call probe_utf8_pathname_composition to detect utf8 mangling. We pass in a path buffer for it to use, and it blindly strcpy()s into it, not knowing whether the buffer is large enough to hold the result or not. In practice this isn't a big deal, because the buffer we pass in already contains "$GIT_DIR/config", and we append only a few extra bytes to it. But we can easily do the right thing just by calling git_path_buf ourselves. Technically this results in a different pathname (before we appended our utf8 characters to the "config" path, and now they get their own files in $GIT_DIR), but that should not matter for our purposes. Signed-off-by: NJeff King <peff@peff.net> Signed-off-by: NJunio C Hamano <gitster@pobox.com>
-
由 Jeff King 提交于
The result of iconv is assigned to a variable, but we never use it (instead, we check errno and whether the function consumed all bytes). Let's drop the assignment, as it triggers gcc's -Wunused-but-set-variable. Signed-off-by: NJeff King <peff@peff.net> Signed-off-by: NJunio C Hamano <gitster@pobox.com>
-
- 26 9月, 2015 16 次提交
-
-
由 Jeff King 提交于
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: NJeff King <peff@peff.net> Signed-off-by: NJunio C Hamano <gitster@pobox.com>
-
由 Jeff King 提交于
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: NJeff King <peff@peff.net> Signed-off-by: NJunio C Hamano <gitster@pobox.com>
-
由 Jeff King 提交于
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: NJeff King <peff@peff.net> Signed-off-by: NJunio C Hamano <gitster@pobox.com>
-
由 Jeff King 提交于
We format a pkt-line into a heap buffer, which requires manual computation of the required size, and uses some bare sprintf calls. We could use a strbuf instead, which would take care of the computation for us. But it's even easier still to use packet_write(). Besides handling the formatting and writing for us, it fixes two things: 1. Our manual max-size check used 0xFFFF, while technically LARGE_PACKET_MAX is slightly smaller than this. 2. Our packet will now be output as part of GIT_TRACE_PACKET debugging. Unfortunately packet_write() does not let us build up the buffer progressively, so we do have to repeat ourselves a little depending on the "vhost" setting, but the end result is still far more readable than the original. Since there were no tests covering this feature at all, we'll add a few into t5802. Signed-off-by: NJeff King <peff@peff.net> Signed-off-by: NJunio C Hamano <gitster@pobox.com>
-
由 Jeff King 提交于
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: NJeff King <peff@peff.net> Signed-off-by: NJunio C Hamano <gitster@pobox.com>
-
由 Jeff King 提交于
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: NJeff King <peff@peff.net> Signed-off-by: NJunio C Hamano <gitster@pobox.com>
-
由 Jeff King 提交于
The main motivation for this cleanup is to switch our line-reading to a strbuf, which removes the use of a fixed-size buffer (which limited the size of remote URLs). Since we have the strbuf, we can make use of strbuf_rtrim(). While we're here, we can also simplify the parsing of each line. First, we can use skip_prefix() to avoid some magic numbers. But second, we can avoid splitting the parsing and actions for each line into two stages. Right now we figure out which type of line we have, set an int to a magic number, skip any intermediate whitespace, and then act on the resulting value based on the magic number. Instead, let's factor the whitespace skipping into a function. That lets us avoid the magic numbers and keep the actions close to the parsing. Signed-off-by: NJeff King <peff@peff.net> Signed-off-by: NJunio C Hamano <gitster@pobox.com>
-
由 Jeff King 提交于
This function does a lot of manual string handling, and has some unnecessary limits. This patch cleans up a number of things: 1. Drop the arbitrary 1000-byte limit on the size of the remote name (we do not have such a limit in any of the other remote-reading mechanisms). 2. Replace fgets into a fixed-size buffer with a strbuf, eliminating any limits on the length of the URL. 3. Replace manual whitespace handling with strbuf_trim (since we now have a strbuf). This also gets rid of a call to strcpy, and the confusing reuse of the "p" pointer for multiple purposes. 4. We currently build up the refspecs over multiple strbuf calls. We do this to handle the fact that the URL "frag" may not be present. But rather than have multiple conditionals, let's just default "frag" to "master". This lets us format the refspecs with a single xstrfmt. It's shorter, and easier to see what the final string looks like. We also update the misleading comment in this area (the local branch is named after the remote name, not after the branch name on the remote side). Signed-off-by: NJeff King <peff@peff.net> Signed-off-by: NJunio C Hamano <gitster@pobox.com>
-
由 Jeff King 提交于
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: NJeff King <peff@peff.net> Signed-off-by: NJunio C Hamano <gitster@pobox.com>
-
由 Jeff King 提交于
This function predates xstrfmt, and its functionality is a subset. Let's just use xstrfmt. Signed-off-by: NJeff King <peff@peff.net> Signed-off-by: NJunio C Hamano <gitster@pobox.com>
-
由 Jeff King 提交于
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: NJeff King <peff@peff.net> Signed-off-by: NJunio C Hamano <gitster@pobox.com>
-
由 Jeff King 提交于
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: NJeff King <peff@peff.net> Signed-off-by: NJunio C Hamano <gitster@pobox.com>
-
由 Jeff King 提交于
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: NJeff King <peff@peff.net> Signed-off-by: NJunio C Hamano <gitster@pobox.com>
-
由 Jeff King 提交于
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: NJeff King <peff@peff.net> Signed-off-by: NJunio C Hamano <gitster@pobox.com>
-
由 Jeff King 提交于
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: NJeff King <peff@peff.net> Signed-off-by: NJunio C Hamano <gitster@pobox.com>
-
由 Jeff King 提交于
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: NJeff King <peff@peff.net> Signed-off-by: NJunio C Hamano <gitster@pobox.com>
-