On Fri, Jan 10, 2025 at 10:27:23AM +0100, Johannes Schindelin wrote: > 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. Oh. That's even an old-standing issue that wasn't caused by the refactoring, right? Your proposed fix to set `length = GIT_MAX_HEXSZ + 3` to account for the old behaviour wouldn't fix it either, as we could still end up overwriting two bytes. I'll send a new version with another commit on top to fix this. Patrick
