Hi Patrick,
On Fri, 6 Dec 2024, Patrick Steinhardt wrote:
> The `length` variable is used to store how many bytes we wish to emit
> from an object ID. This value will either be the full hash algorithm's
> length, or the abbreviated hash that can be set via `--abbrev` or the
> "core.abbrev" option. The former is of type `size_t`, whereas the latter
> is of type `int`, which causes a warning with "-Wsign-compare".
>
> The reason why `abbrev` is using a signed type is mostly that it is
> initialized with `-1` to indicate that we have to compute the minimum
> abbreviation length. This length is computed via `find_alignment()`,
> which always gets called before `emit_other()`, and thus we can assume
> that the value would never be negative in `emit_other()`.
>
> In fact, we can even assume that the value will always be at least
> `MINIMUM_ABBREV`, which is enforced by both `git_default_core_config()`
> and `parse_opt_abbrev_cb()`. We implicitly rely on this by subtracting
> up to 3 without checking for whether the value becomes negative. We then
> pass the value to printf(3p) to print the prefix of our object's ID, so
> if that assumption was violated we may end up with undefined behaviour.
>
> Squelch the warning by asserting this invariant and casting the value of
> `abbrev` to `size_t`. This allows us to store the whole length as an
> unsigned integer, which we can then pass to `fwrite()`.
>
> Signed-off-by: Patrick Steinhardt <ps@xxxxxx>
> ---
> builtin/blame.c | 10 +++++++---
> 1 file changed, 7 insertions(+), 3 deletions(-)
>
> diff --git a/builtin/blame.c b/builtin/blame.c
> index b33b44c89a431d45e05d9863f69c049ba5eec08c..867032e4c16878ffd56df8a73162b89ca4bd2694 100644
> --- a/builtin/blame.c
> +++ b/builtin/blame.c
> @@ -6,7 +6,6 @@
> */
>
> #define USE_THE_REPOSITORY_VARIABLE
> -#define DISABLE_SIGN_COMPARE_WARNINGS
>
> #include "builtin.h"
> #include "config.h"
> @@ -468,9 +467,14 @@ static void emit_other(struct blame_scoreboard *sb, struct blame_entry *ent, int
> reset = GIT_COLOR_RESET;
> }
>
> + if (abbrev < MINIMUM_ABBREV)
> + BUG("abbreviation is smaller than minimum length: %d < %d",
> + abbrev, MINIMUM_ABBREV);
> +
> for (cnt = 0; cnt < ent->num_lines; cnt++) {
> char ch;
> - int length = (opt & OUTPUT_LONG_OBJECT_NAME) ? the_hash_algo->hexsz : abbrev;
> + size_t length = (opt & OUTPUT_LONG_OBJECT_NAME) ?
> + the_hash_algo->hexsz : (size_t) abbrev;
>
> if (opt & OUTPUT_COLOR_LINE) {
> if (cnt > 0) {
> @@ -501,7 +505,7 @@ static void emit_other(struct blame_scoreboard *sb, struct blame_entry *ent, int
> length--;
> putchar('?');
> }
> - printf("%.*s", length, hex);
> + fwrite(hex, 1, length, stdout);
I just noticed this, and would like to point out a difference of behavior.
Try this at home:
git blame --abbrev=99999 git.c
The difference relative to the previous behavior that I am observing is
that the `fwrite()` call does not stop at the NUL character and hence
happily continues out-of-bounds. The `printf()` call would have stopped at
the NUL character.
Ciao,
Johannes
> if (opt & OUTPUT_ANNOTATE_COMPAT) {
> const char *name;
> if (opt & OUTPUT_SHOW_EMAIL)
>
> --
> 2.47.0.366.g5daf58cba8.dirty
>
>
>
