git/wrapper.c, branch v2.32.2

add open_nofollow() helper

2021-02-16T17:41:32Z

Some callers of open() would like to use O_NOFOLLOW, but it is not available on all platforms. Let's abstract this into a helper function so we can provide system-specific implementations. Some light web-searching reveals that we might be able to get something similar on Windows using FILE_FLAG_OPEN_REPARSE_POINT. I didn't dig into this further. For other systems without O_NOFOLLOW or any equivalent, we have two options for fallback: - we can just open anyway, following symlinks; this may have security implications (e.g., following untrusted in-tree symlinks) - we can determine whether the path is a symlink with lstat(). This is slower (two syscalls instead of one), but that may be acceptable for infrequent uses like looking up .gitattributes files (especially because we can get away with a single syscall for the common case of ENOENT). It's also racy, but should be sufficient for our needs (we are worried about in-tree symlinks that we ourselves would have previously created). We could make it non-racy at the cost of making it even slower, by doing an fstat() on the opened descriptor and comparing the dev/ino fields to the original lstat(). This patch implements the lstat() option in its slightly-faster racy form. Signed-off-by: Jeff King Signed-off-by: Junio C Hamano

xrealloc: do not reuse pointer freed by zero-length realloc()

2020-09-02T19:18:14Z

This patch fixes a bug where xrealloc(ptr, 0) can double-free and corrupt the heap on some platforms (including at least glibc). The C99 standard says of malloc (section 7.20.3): If the size of the space requested is zero, the behavior is implementation-defined: either a null pointer is returned, or the behavior is as if the size were some nonzero value, except that the returned pointer shall not be used to access an object. So we might get NULL back, or we might get an actual pointer (but we're not allowed to look at its contents). To simplify our code, our xmalloc() handles a NULL return by converting it into a single-byte allocation. That way callers get consistent behavior. This was done way back in 4e7a2eccc2 (?alloc: do not return NULL when asked for zero bytes, 2005-12-29). We also gave xcalloc() and xrealloc() the same treatment. And according to C99, that is fine; the text above is in a paragraph that applies to all three. But what happens to the memory we passed to realloc() in such a case? I.e., if we do: ret = realloc(ptr, 0); and "ptr" is non-NULL, but we get NULL back, is "ptr" still valid? C99 doesn't cover this case specifically, but says (section 7.20.3.4): The realloc function deallocates the old object pointed to by ptr and returns a pointer to a new object that has the size specified by size. So "ptr" is now deallocated, and we must only look at "ret". And since "ret" is NULL, that means we have no allocated object at all. But that's not quite the whole story. It also says: If memory for the new object cannot be allocated, the old object is not deallocated and its value is unchanged. [...] The realloc function returns a pointer to the new object (which may have the same value as a pointer to the old object), or a null pointer if the new object could not be allocated. So if we see a NULL return with a non-zero size, we can expect that the original object _is_ still valid. But with a non-zero size, it's ambiguous. The NULL return might mean a failure (in which case the object is valid), or it might mean that we successfully allocated nothing, and used NULL to represent that. The glibc manpage for realloc() explicitly says: [...]if size is equal to zero, and ptr is not NULL, then the call is equivalent to free(ptr). Likewise, this StackOverflow answer: https://stackoverflow.com/a/2135302 claims that C89 gave similar guidance (but I don't have a copy to verify it). A comment on this answer: https://stackoverflow.com/a/2022410 claims that Microsoft's CRT behaves the same. But our current "retry with 1 byte" code passes the original pointer again. So on glibc, we effectively free() the pointer and then try to realloc() it again, which is undefined behavior. The simplest fix here is to just pass "ret" (which we know to be NULL) to the follow-up realloc(). But that means that a system which _doesn't_ free the original pointer would leak it. It's not clear if any such systems exist, and that interpretation of the standard seems unlikely (I'd expect a system that doesn't deallocate to simply return the original pointer in this case). But it's easy enough to err on the safe side, and just never pass a zero size to realloc() at all. Signed-off-by: Jeff King Signed-off-by: Junio C Hamano

wrapper: add function to compare strings with different NUL termination

2020-05-27T17:07:06Z

When parsing capabilities for the pack protocol, there are times we'll want to compare the value of a capability to a NUL-terminated string. Since the data we're reading will be space-terminated, not NUL-terminated, we need a function that compares the two strings, but also checks that they're the same length. Otherwise, if we used strncmp to compare these strings, we might accidentally accept a parameter that was a prefix of the expected value. Add a function, xstrncmpz, that takes a NUL-terminated string and a non-NUL-terminated string, plus a length, and compares them, ensuring that they are the same length. Signed-off-by: brian m. carlson Signed-off-by: Junio C Hamano

Merge branch 'dl/wrapper-fix-indentation'

2020-04-22T20:42:47Z

Coding style fix. * dl/wrapper-fix-indentation: wrapper: indent with tabs

wrapper: indent with tabs

2020-03-29T01:06:51Z

The codebase uses tabs for indentation. Convert an erroneous space indent into a tab indent. Signed-off-by: Denton Liu Signed-off-by: Junio C Hamano

Merge branch 'ah/cleanups'

2019-10-09T05:01:00Z

Miscellaneous code clean-ups. * ah/cleanups: git_mkstemps_mode(): replace magic numbers with computed value wrapper: use a loop instead of repetitive statements diffcore-break: use a goto instead of a redundant if statement commit-graph: remove a duplicate assignment

git_mkstemps_mode(): replace magic numbers with computed value

2019-10-03T00:58:25Z

The magic number "6" appears several times in the function, and is related to the size of the "XXXXXX" string we expect to find in the template. Let's pull that "XXXXXX" into a constant array, whose size we can get at compile time with ARRAY_SIZE(). Note that we probably can't just change this value, since callers will be feeding us a certain number of X's, but it hopefully makes the function itself easier to follow. While we're here, let's do the same with the "letters" array (which we _could_ modify if we wanted to include more characters). Signed-off-by: Jeff King Signed-off-by: Junio C Hamano

wrapper: use a loop instead of repetitive statements

2019-10-02T06:04:23Z

A check into the history of this code revealed no particular reason for the code to be written in this way. All popular compilers are capable of unrolling loops if it benefits performance, and once this code is replaced with a loop, the magic number 6 used in multiple places in this function can be replaced with a named constant. Reviewed-by: Derrick Stolee Reviewed-by: Johannes Schindelin Reviewed-by: Jeff King Signed-off-by: Alex Henrie Signed-off-by: Junio C Hamano

packfile: drop release_pack_memory()

2019-08-13T19:21:33Z

Long ago, in 97bfeb34df (Release pack windows before reporting out of memory., 2006-12-24), we taught xmalloc() and friends to try unmapping pack windows when malloc() failed. It's unlikely that his helps a lot in practice, and it has some downsides. First, the downsides: 1. It makes xmalloc() not thread-safe. We've worked around this in pack-objects.c, which installs its own locking version of the try_to_free_routine(). But other threaded code doesn't. 2. It makes the system as a whole harder to reason about. Functions which allocate heap memory under the hood may have farther-reaching effects than expected. That might be worth the tradeoff if there's a benefit. But in practice, it seems unlikely. We're generally dealing with mmap'd files, so the OS is going to do a much better job at responding to memory pressure by dropping individual pages (the exception is systems with NO_MMAP, but even there the OS can probably respond just as well with swapping). So the only thing we're really freeing is address space. On 64-bit systems, we have plenty of that to go around. On 32-bit systems, it could possibly help. But around the same time we made two other changes: 77ccc5bbd1 (Introduce new config option for mmap limit., 2006-12-23) and 60bb8b1453 (Fully activate the sliding window pack access., 2006-12-23). Together that means that a 32-bit system should have no more than 256MB total of packed-git mmaps at one time, split between a few 32MB windows. It's unlikely we have any address space problems since then, but we don't have any data since the features were all added at the same time. Likewise, xmmap() will try to free memory. At first glance, it seems like we'd need this (when we try to mmap a new window, we might need to close an old one to save address space on a 32-bit system). But we're saved again by core.packedGitLimit: if we're going to exceed our 256MB limit, we'll close an existing window before we even call mmap(). So it seems unlikely that this feature is actually doing anything useful. And while we don't have reports of it harming anything (probably because it rarely if ever kicks in), it would be nice to simplify the system overall. This patch drops the whole try_to_free system from xmalloc(), as well as the manual pack memory release in xmmap(). Signed-off-by: Jeff King Signed-off-by: Junio C Hamano

wrapper: avoid undefined behaviour in macOS

2019-06-19T14:41:31Z

0620b39b3b ("compat: add a mkstemps() compatibility function", 2009-05-31) included a function based on code from libiberty which would result in undefined behaviour in platforms where timeval's tv_usec is a 32-bit signed type as shown by: wrapper.c:505:31: runtime error: left shift of 594546 by 16 places cannot be represented in type '__darwin_suseconds_t' (aka 'int') interestingly the version of this code from gcc never had this bug and the code had a cast that would had prevented the issue (at least in 64-bit platforms) but was misapplied. change the cast to uint64_t so it also works in 32-bit platforms. Signed-off-by: Carlo Marcelo Arenas Belón Signed-off-by: Junio C Hamano