Hi Patrick, On Thu, 9 Jan 2025, Johannes Schindelin wrote: > On Thu, 9 Jan 2025, Patrick Steinhardt wrote: > > > On Thu, Jan 09, 2025 at 11:49:43AM +0100, Johannes Schindelin wrote: > > > > diff --git a/builtin/blame.c b/builtin/blame.c > > > > index 867032e4c16878ffd56df8a73162b89ca4bd2694..ad91fe9e97f90625dd2708fbd44bf2dd24a337a6 100644 > > > > --- a/builtin/blame.c > > > > +++ b/builtin/blame.c > > > > @@ -475,6 +475,8 @@ static void emit_other(struct blame_scoreboard *sb, struct blame_entry *ent, int > > > > char ch; > > > > size_t length = (opt & OUTPUT_LONG_OBJECT_NAME) ? > > > > the_hash_algo->hexsz : (size_t) abbrev; > > > > + if (length > GIT_MAX_HEXSZ) > > > > + length = GIT_MAX_HEXSZ; > > > > > > This causes a subtle change of behavior because there are a couple of > > > conditional code blocks between this change and the `printf()` call > > > decrease `length`, i.e. specifying values larger than the maximal hex size > > > causes potentially-desirable, different behavior (and think about > > > https://www.hyrumslaw.com/). > > > > Alternatively we can move this until after we have done the > > subtractions. Then we don't have to do weird gymnastics. > > Or we can even avoid assiging a maximum altogether: > > if (length < GIT_MAX_HEXSZ) > printf("%.*s", (int)length, hex); > else > printf("%s", hex); > > Or be more consistent with Git's source code style which often prefers > ternaries, favoring succinctness over readability: > > printf("%.*s", (int)(length < GIT_MAX_HEXSZ ? length : GIT_MAX_HEXSZ), hex); Coverity noticed a problem with this approach, looking at https://github.com/git/git/blob/v2.48.0-rc2/builtin/blame.c#L493: memset(hex, ' ', length); If the `GIT_MAX_HEXSZ` guard is moved after this statement, then we can easily overrun the `hex` buffer. Ciao, Johannes
