tests: 🌀 address two null argument instances

[noahp]

Handle two null argument cases that occur in the unit tests.
One is in library code, the other is in test code.

Detected by running unit tests with undefined behavior sanitizer:

 # build
mkdir build && cd build
cmake -DBUILD_CLAR=ON -DCMAKE_C_FLAGS="-fsanitize=address \
-fsanitize=undefined -fstack-usage -static-libasan" ..
cmake --build .

 # run with asan
ASAN_OPTIONS="allocator_may_return_null=1" ./libgit2_clar
...
............../libgit2/src/apply.c:316:3: runtime error: null pointer \
passed as argument 1, which is declared to never be null
...................../libgit2/tests/apply/fromfile.c:46:3: runtime \
error: null pointer passed as argument 1, which is declared to never be null

[ethomson]

Agree with @carlosmn's stylistic suggestion, and would actually take it a step further and check the length instead of that the pointers are not NULL. This fits in a bit better with the existing style.

[pks-t]

By the way, one minor nit: there seems to be some kind of unicode character in the commit message. I'd very much prefer to not have them in commit messages, as many setups will not be able to display them correctly and also make it so much harder to search for the commit message.

Otherwise I'm fine with this, thanks for finding and fixing these errors!

[noahp]

The unicode emoji in the commit message is just a bit of lightheartedness, removed now!

I've update this per feedback (thanks so much for looking at it!), ready for re-review.

[pks-t]

@ethomson: you made the proposal to use source_len instead of source here. But if you ask me, this makes the code less readable because it leads one to wonder why we need to special case the source length. After all, the C standard says:

2 Where an argument declared as size_t n specifies the length of the array for a function, n can have the value zero on a call to that function. Unless explicitly stated otherwise in the description of a particular function in this subclause, pointer arguments on such a call shall still have valid values, as described in 7.1.4. On such a call, a function that locates a character finds no occurrence, a function that compares two character sequences returns zero, and a function that copies characters copies zero characters.

The important part being that if n == 0, a comparison will always compare to 0, so checking the source length is rather confusing. The other important part is that arguments other than n still need to be valid even in the case where n == 0. So we're really not interested in source_len being non-zero, but instead we're interested in source being non-NULL, even if the first will be an indicator for source being NULL.

I'm also not quite sure whether the change is actually correct. What we want to do here is to check whether the source file actually matches the patched file with the reverse patch applied. With this change, we'll skip this check completely if there is no source file content, but shouldn't we instead verify that there also is no target file content?

So e.g.:

/* Verify that the resulting file with the reverse patch applied matches the source file */
if (source_len != reverse_size || (source && memcmp(source, reverse.ptr, source_len) != 0))

The important bit is that the source check is now right in front of the memcmp. So we will only check source if we already know that source and reverse length are the same. If source is NULL, then we know that both source and reverse have a length of 0 and thus are equal. Otherwise, both have the same non-NULL length and need to be compared for equality.

Sorry for this wall of text :P

[noahp]

Good points, I definitely agree! I'll patch this up as you suggest (and thank you for taking the time to write out that comment!).

[ethomson]

@pks-t I agree that this should be if (source_len != reverse_size || (source_len && memcmp(source, reverse.ptr, source_len) != 0)).

I'm still in favor of checking source_len not source for readability. It won't matter in this case for correctness (mine avoids the call to memcmp, but I think that we can agree that's not really a problem and I could sacrifice that call for readability). It makes it more obvious to me that we short-circuit when source_len is 0, even knowing that yes, memcmp will try to dereference its first two args if source_len is 0.

Anyway, that's a minor personal preference; I would say that we should avoid re-rolling this unnecessarily. So whatever you've got in your editor buffer @noahp, I would think would be fine.

[pks-t]

Totally agree, @ethomson. Happy to merge either way as soon as the condition is fixed, I don't care too deeply

[noahp]

Thanks guys! Just updated per your recommendations.

[ethomson]

Thanks @noahp! 😀