Skip to content

Commit

Permalink
Make the tests run cleanly under UndefinedBehaviorSanitizer
Browse files Browse the repository at this point in the history
This change makes the tests run cleanly under
`-fsanitize=undefined,nullability` and comprises of:

* Avoids some arithmetic with NULL pointers (which UBSan does not like).
* Avoids an overflow in a shift, due to an uint8_t being implicitly
  converted to a signed 32-bit signed integer after being shifted by a
  32-bit signed integer.
* Avoids a unaligned read in libgit2.
* Ignores unaligned reads in the SHA1 library, since it only happens on
  Intel processors, where it is _still_ undefined behavior, but the
  semantics are moderately well-understood.
* Makes an empty `write_buf` (in the sense that `len` is zero) in the
  NTLM client a no-op, which prevents calling `memcpy` with a
  potentially `NULL` argument. Some versions of `string.h` mark that
  both `dest` and `src` arguments should be non-null.

Of notable omission is `-fsanitize=integer`, since there are lots of
warnings in zlib and the SHA1 library which probably don't make sense to
fix and I could not figure out how to silence easily. libgit2 itself
also has ~100s of warnings which are mostly innocuous (e.g. use of enum
constants that only fit on an `uint32_t`, but there is no way to do that
in a simple fashion because the data type chosen for enumerated types is
implementation-defined), and investigating whether there are worrying
warnings would need reducing the noise significantly.
  • Loading branch information
lhchavez committed Jun 29, 2020
1 parent d6c6285 commit 95f9f12
Show file tree
Hide file tree
Showing 5 changed files with 34 additions and 20 deletions.
3 changes: 3 additions & 0 deletions deps/ntlmclient/ntlm.c
Expand Up @@ -260,6 +260,9 @@ static inline bool write_buf(
const unsigned char *buf,
size_t len)
{
if (!len)
return true;

if (out->len - out->pos < len) {
ntlm_client_set_errmsg(ntlm, "out of buffer space");
return false;
Expand Down
4 changes: 4 additions & 0 deletions script/sanitizers.supp
@@ -0,0 +1,4 @@
[undefined]
# This library allows unaligned access on Intel-like processors. Prevent UBSan
# from complaining about that.
fun:sha1_compression_states
24 changes: 13 additions & 11 deletions src/apply.c
Expand Up @@ -66,22 +66,24 @@ static int patch_image_init_fromstr(
if (git_pool_init(&out->pool, sizeof(git_diff_line)) < 0)
return -1;

for (start = in; start < in + in_len; start = end) {
end = memchr(start, '\n', in_len - (start - in));
if (in_len) {
for (start = in; start < in + in_len; start = end) {
end = memchr(start, '\n', in_len - (start - in));

if (end == NULL)
end = in + in_len;
if (end == NULL)
end = in + in_len;

else if (end < in + in_len)
end++;
else if (end < in + in_len)
end++;

line = git_pool_mallocz(&out->pool, 1);
GIT_ERROR_CHECK_ALLOC(line);
line = git_pool_mallocz(&out->pool, 1);
GIT_ERROR_CHECK_ALLOC(line);

if (git_vector_insert(&out->lines, line) < 0)
return -1;
if (git_vector_insert(&out->lines, line) < 0)
return -1;

patch_line_init(line, start, (end - start), (start - in));
patch_line_init(line, start, (end - start), (start - in));
}
}

return 0;
Expand Down
7 changes: 4 additions & 3 deletions src/buffer.c
Expand Up @@ -365,7 +365,7 @@ int git_buf_encode_base85(git_buf *buf, const char *data, size_t len)

for (i = 24; i >= 0; i -= 8) {
uint8_t ch = *data++;
acc |= ch << i;
acc |= (uint32_t)ch << i;

if (--len == 0)
break;
Expand Down Expand Up @@ -759,7 +759,8 @@ int git_buf_join(
ssize_t offset_a = -1;

/* not safe to have str_b point internally to the buffer */
assert(str_b < buf->ptr || str_b >= buf->ptr + buf->size);
if (buf->size)
assert(str_b < buf->ptr || str_b >= buf->ptr + buf->size);

/* figure out if we need to insert a separator */
if (separator && strlen_a) {
Expand All @@ -769,7 +770,7 @@ int git_buf_join(
}

/* str_a could be part of the buffer */
if (str_a >= buf->ptr && str_a < buf->ptr + buf->size)
if (buf->size && str_a >= buf->ptr && str_a < buf->ptr + buf->size)
offset_a = str_a - buf->ptr;

GIT_ERROR_CHECK_ALLOC_ADD(&alloc_len, strlen_a, strlen_b);
Expand Down
16 changes: 10 additions & 6 deletions src/index.c
Expand Up @@ -2781,17 +2781,21 @@ static int write_disk_entry(git_filebuf *file, git_index_entry *entry, const cha
ondisk.flags = htons(entry->flags);

if (entry->flags & GIT_INDEX_ENTRY_EXTENDED) {
const size_t path_offset = offsetof(struct entry_long, path);
struct entry_long ondisk_ext;
memcpy(&ondisk_ext, &ondisk, sizeof(struct entry_short));
ondisk_ext.flags_extended = htons(entry->flags_extended &
GIT_INDEX_ENTRY_EXTENDED_FLAGS);
memcpy(mem, &ondisk_ext, offsetof(struct entry_long, path));
path = ((struct entry_long*)mem)->path;
disk_size -= offsetof(struct entry_long, path);
memcpy(mem, &ondisk_ext, path_offset);
path = mem;
path += path_offset;
disk_size -= path_offset;
} else {
memcpy(mem, &ondisk, offsetof(struct entry_short, path));
path = ((struct entry_short*)mem)->path;
disk_size -= offsetof(struct entry_short, path);
const size_t path_offset = offsetof(struct entry_short, path);
memcpy(mem, &ondisk, path_offset);
path = mem;
path += path_offset;
disk_size -= path_offset;
}

if (last) {
Expand Down

0 comments on commit 95f9f12

Please sign in to comment.