On Sat, Aug 26, 2017 at 10:28:00AM +0200, Michael Haggerty wrote:
> It turns out that the comment is incorrect, but there was nevertheless
> plenty that could be cleaned up in the area:
>
> * Make macro `GIT_NIBBLE` safer by adding some parentheses
> * Remove some dead code
> * Fix some memory leaks
> * Fix some obsolete and incorrect comments
> * Reject "notes" that are not blobs
>
> I hope the result is also easier to understand.
>
> This branch is also available from my Git fork [1] as branch
> `load-subtree-cleanup`.
FYI, Coverity seems to complain about "pu" after this series is merged, but
I think it's wrong. It says:
*** CID 1417630: Memory - illegal accesses (OVERRUN)
/notes.c: 458 in load_subtree()
452
453 /*
454 * Pad the rest of the SHA-1 with zeros,
455 * except for the last byte, where we write
456 * the length:
457 */
>>> CID 1417630: Memory - illegal accesses (OVERRUN)
>>> Overrunning array of 20 bytes at byte offset 20 by dereferencing
pointer "&object_oid.hash[len]".
458 memset(object_oid.hash + len, 0, GIT_SHA1_RAWSZ
- len - 1);
459 object_oid.hash[KEY_INDEX] = (unsigned char)len;
460
461 type = PTR_TYPE_SUBTREE;
462 } else {
463 /* This can't be part of a note */
I agree that if "len" were 20 here that would be a problem, but I don't
think that's possible.
The tool correctly claims that prefix_len can be up to 19, due to the
assert:
3. cond_at_most: Checking prefix_len >= 20UL implies that prefix_len may
be up to 19 on the false branch.
420 if (prefix_len >= GIT_SHA1_RAWSZ)
421 BUG("prefix_len (%"PRIuMAX") is out of range",
(uintmax_t)prefix_len);
Then it claims:
13. Condition path_len == 2 * (20 - prefix_len), taking false branch.
430 if (path_len == 2 * (GIT_SHA1_RAWSZ - prefix_len)) {
431 /* This is potentially the remainder of the SHA-1
*/
So we know that either prefix_len is not 19, or that path_len is not 2
(since that combination would cause us to take the true branch here).
But then it goes on to say:
14. Condition path_len == 2, taking true branch.
442 } else if (path_len == 2) {
443 /* This is potentially an internal node */
which I believe must mean that prefix_len cannot be 19 here. And yet it
says:
15. assignment: Assigning: len = prefix_len. The value of len may now be up
to 19.
444 size_t len = prefix_len;
445
[...]
17. incr: Incrementing len. The value of len may now be up to 20.
18. Condition hex_to_bytes(&object_oid.hash[len++], entry.path, 1), taking
false branch.
450 if (hex_to_bytes(object_oid.hash + len++,
entry.path, 1))
451 goto handle_non_note; /* entry.path is not
a SHA1 */
I think that's impossible, and Coverity simply isn't smart enough to
shrink the set of possible values for prefix_len based on the set of
if-else conditions.
So nothing to see here, but since I spent 20 minutes scratching my head
(and I know others look at Coverity output and may scratch their heads
too), I thought it was worth writing up. And also if I'm wrong, it would
be good to know. ;)
-Peff
