| Message ID | c54233ce-ff72-ca29-68c2-1416169b8e42@web.de (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 39356747b4345f3170d6294f7259740a5e0f0c5c |
| Headers | show |
| Series | fetch-pack: disregard invalid pack lockfiles | expand |
On Mon, Nov 30, 2020 at 08:27:15PM +0100, René Scharfe wrote: > index_pack_lockfile() can return NULL if it doesn't like the contents it > reads from the file descriptor passed to it. unlink(2) is declared to > not accept NULL pointers (at least with glibc). Undefined Behavior > Sanitizer together with Address Sanitizer detects a case where a NULL > lockfile name is passed to unlink(2) by transport_unlock_pack() in t1060 > (make SANITIZE=address,undefined; cd t; ./t1060-object-corruption.sh). Which test in t1060? I tried to reproduce this myself, but couldn't seem to coax out a failure. (Initially I thought that my ccache wasn't letting me recompile with the SANITIZE options, but running 'ccache clear' and then trying again left the test still passing). > Reinstate the NULL check to avoid undefined behavior, but put it right > at the source, so that the number of items in the string_list reflects > the number of valid lockfiles. > > Signed-off-by: René Scharfe <l.s.r@web.de> > --- > fetch-pack.c | 5 +++-- > 1 file changed, 3 insertions(+), 2 deletions(-) > > diff --git a/fetch-pack.c b/fetch-pack.c > index b10c432315..4625926cf0 100644 > --- a/fetch-pack.c > +++ b/fetch-pack.c > @@ -915,8 +915,9 @@ static int get_pack(struct fetch_pack_args *args, > if (start_command(&cmd)) > die(_("fetch-pack: unable to fork off %s"), cmd_name); > if (do_keep && pack_lockfiles) { > - string_list_append_nodup(pack_lockfiles, > - index_pack_lockfile(cmd.out)); > + char *pack_lockfile = index_pack_lockfile(cmd.out); > + if (pack_lockfile) > + string_list_append_nodup(pack_lockfiles, pack_lockfile); Makes sense. Thanks, Taylor
Am 30.11.20 um 20:53 schrieb Taylor Blau: > On Mon, Nov 30, 2020 at 08:27:15PM +0100, René Scharfe wrote: >> index_pack_lockfile() can return NULL if it doesn't like the contents it >> reads from the file descriptor passed to it. unlink(2) is declared to >> not accept NULL pointers (at least with glibc). Undefined Behavior >> Sanitizer together with Address Sanitizer detects a case where a NULL >> lockfile name is passed to unlink(2) by transport_unlock_pack() in t1060 >> (make SANITIZE=address,undefined; cd t; ./t1060-object-corruption.sh). > > Which test in t1060? I tried to reproduce this myself, but couldn't seem > to coax out a failure. (Initially I thought that my ccache wasn't > letting me recompile with the SANITIZE options, but running 'ccache > clear' and then trying again left the test still passing). 15 - fetch into corrupted repo with index-pack $ cat trash\ directory.t1060-object-corruption/bit-error-cp/stderr error: inflate: data stream error (invalid distance too far back) error: unable to unpack d95f3ad14dee633a758d2e331151e950dd13e4ed header fatal: cannot read existing object info d95f3ad14dee633a758d2e331151e950dd13e4ed fatal: index-pack failed wrapper.c:568:52: runtime error: null pointer passed as argument 1, which is declared to never be null /usr/include/unistd.h:825:48: note: nonnull attribute specified here SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior wrapper.c:568:52 in Aborted Compiled with: Debian clang version 11.0.0-5+b1 Target: x86_64-pc-linux-gnu Thread model: posix InstalledDir: /usr/bin René
On Mon, Nov 30, 2020 at 09:15:47PM +0100, René Scharfe wrote: > Am 30.11.20 um 20:53 schrieb Taylor Blau: > > On Mon, Nov 30, 2020 at 08:27:15PM +0100, René Scharfe wrote: > >> index_pack_lockfile() can return NULL if it doesn't like the contents it > >> reads from the file descriptor passed to it. unlink(2) is declared to > >> not accept NULL pointers (at least with glibc). Undefined Behavior > >> Sanitizer together with Address Sanitizer detects a case where a NULL > >> lockfile name is passed to unlink(2) by transport_unlock_pack() in t1060 > >> (make SANITIZE=address,undefined; cd t; ./t1060-object-corruption.sh). > > > > Which test in t1060? I tried to reproduce this myself, but couldn't seem > > to coax out a failure. (Initially I thought that my ccache wasn't > > letting me recompile with the SANITIZE options, but running 'ccache > > clear' and then trying again left the test still passing). > > 15 - fetch into corrupted repo with index-pack > > $ cat trash\ directory.t1060-object-corruption/bit-error-cp/stderr > error: inflate: data stream error (invalid distance too far back) > error: unable to unpack d95f3ad14dee633a758d2e331151e950dd13e4ed header > fatal: cannot read existing object info d95f3ad14dee633a758d2e331151e950dd13e4ed > fatal: index-pack failed > wrapper.c:568:52: runtime error: null pointer passed as argument 1, which is declared to never be null > /usr/include/unistd.h:825:48: note: nonnull attribute specified here > SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior wrapper.c:568:52 in > Aborted > > Compiled with: > Debian clang version 11.0.0-5+b1 > Target: x86_64-pc-linux-gnu > Thread model: posix > InstalledDir: /usr/bin I see. I was compiling with: gcc 10.2.0, so setting CC=clang does reproduce the error for me. Reviewed-by: Taylor Blau <me@ttaylorr.com> Thanks, Taylor
On Mon, Nov 30, 2020 at 08:27:15PM +0100, René Scharfe wrote: > 9da69a6539 (fetch-pack: support more than one pack lockfile, 2020-06-10) > started to use a string_list for pack lockfile names instead of a single > string pointer. It removed a NULL check from transport_unlock_pack() as > well, which is the function that eventually deletes these lockfiles and > releases their name strings. > > index_pack_lockfile() can return NULL if it doesn't like the contents it > reads from the file descriptor passed to it. unlink(2) is declared to > not accept NULL pointers (at least with glibc). Undefined Behavior > Sanitizer together with Address Sanitizer detects a case where a NULL > lockfile name is passed to unlink(2) by transport_unlock_pack() in t1060 > (make SANITIZE=address,undefined; cd t; ./t1060-object-corruption.sh). > > Reinstate the NULL check to avoid undefined behavior, but put it right > at the source, so that the number of items in the string_list reflects > the number of valid lockfiles. It took me a minute to understand how 9da69a6539 made this worse, since in the hunk you're touching here, the original "if NULL, do nothing" check was checking the pointer-to-pointer to see if the caller was interested in the lockfile name. But your "but put it right at the source" pointed me in the right direction. The hunk from 9da69a6539 that matters is this one: - if (pack_lockfile) { - printf("lock %s\n", pack_lockfile); + if (pack_lockfiles.nr) { + int i; + + printf("lock %s\n", pack_lockfiles.items[0].string); fflush(stdout); + for (i = 1; i < pack_lockfiles.nr; i++) + warning(_("Lockfile created but not reported: %s"), + pack_lockfiles.items (not complaining about anything, just verbosely reviewing). > diff --git a/fetch-pack.c b/fetch-pack.c > index b10c432315..4625926cf0 100644 > --- a/fetch-pack.c > +++ b/fetch-pack.c > @@ -915,8 +915,9 @@ static int get_pack(struct fetch_pack_args *args, > if (start_command(&cmd)) > die(_("fetch-pack: unable to fork off %s"), cmd_name); > if (do_keep && pack_lockfiles) { > - string_list_append_nodup(pack_lockfiles, > - index_pack_lockfile(cmd.out)); > + char *pack_lockfile = index_pack_lockfile(cmd.out); > + if (pack_lockfile) > + string_list_append_nodup(pack_lockfiles, pack_lockfile); > close(cmd.out); > } So this is an obviously correct fix, but I have to wonder whether we ought to be (even before 9da69a6539) complaining about pack-objects not correctly reporting the pack name to us. I think in practice this happens when it dies early without reporting anything to us, in which case we'd notice its non-zero exit anyway. So it's probably not a big deal, but would amount to an assertion (and if we did want to do it, it should come on top of your fix, and not hold it up). -Peff
diff --git a/fetch-pack.c b/fetch-pack.c index b10c432315..4625926cf0 100644 --- a/fetch-pack.c +++ b/fetch-pack.c @@ -915,8 +915,9 @@ static int get_pack(struct fetch_pack_args *args, if (start_command(&cmd)) die(_("fetch-pack: unable to fork off %s"), cmd_name); if (do_keep && pack_lockfiles) { - string_list_append_nodup(pack_lockfiles, - index_pack_lockfile(cmd.out)); + char *pack_lockfile = index_pack_lockfile(cmd.out); + if (pack_lockfile) + string_list_append_nodup(pack_lockfiles, pack_lockfile); close(cmd.out); }
9da69a6539 (fetch-pack: support more than one pack lockfile, 2020-06-10) started to use a string_list for pack lockfile names instead of a single string pointer. It removed a NULL check from transport_unlock_pack() as well, which is the function that eventually deletes these lockfiles and releases their name strings. index_pack_lockfile() can return NULL if it doesn't like the contents it reads from the file descriptor passed to it. unlink(2) is declared to not accept NULL pointers (at least with glibc). Undefined Behavior Sanitizer together with Address Sanitizer detects a case where a NULL lockfile name is passed to unlink(2) by transport_unlock_pack() in t1060 (make SANITIZE=address,undefined; cd t; ./t1060-object-corruption.sh). Reinstate the NULL check to avoid undefined behavior, but put it right at the source, so that the number of items in the string_list reflects the number of valid lockfiles. Signed-off-by: René Scharfe <l.s.r@web.de> --- fetch-pack.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) -- 2.29.2