pkt-line: prepare buffer before handling ERR packets

Since 2d103c31c2 (pack-protocol.txt: accept error packets in any context, 2018-12-29), the pktline code will detect an ERR packet and die automatically, saving the caller from dealing with it. But we do so too early in the function, before we have terminated the buffer with a NUL. As a result, passing the ERR message to die() may result in us printing random cruft from a previous packet. This doesn't trigger memory tools like ASan because we reuse the same buffer over and over (so the contents are valid and initialized; they're just stale). We can see demonstrate this by tightening the regex we use to match the error message in t5516; without this patch, git-fetch will accidentally print the capabilities from the (much longer) initial packet we received. By moving the ERR code later in the function we get a few other benefits, too: - we'll now chomp any newline sent by the other side (which is what we want, since die() will add its own newline) - we'll now mention the ERR packet with GIT_TRACE_PACKET Signed-off-by: Jeff King <peff@peff.net> Signed-off-by: Junio C Hamano <gitster@pobox.com>

Jeff King committed Apr 13, 2019 at 01:54 UTC 533ddba47e8cc36a6ec2864f482a8e540465eb64
2 files changed +6 -5
pkt-line.c
+5 -4
@@ -350,16 +350,17 @@ enum packet_read_status packet_read_with_status(int fd, char **src_buffer,
350 return PACKET_READ_EOF;
351 }
352
353 - if ((options & PACKET_READ_DIE_ON_ERR_PACKET) &&
354 - starts_with(buffer, "ERR "))
355 - die(_("remote error: %s"), buffer + 4);
356 -
353 if ((options & PACKET_READ_CHOMP_NEWLINE) &&
354 len && buffer[len-1] == '\n')
355 len--;
356
357 buffer[len] = 0;
358 packet_trace(buffer, len, 0);
359 +
360 + if ((options & PACKET_READ_DIE_ON_ERR_PACKET) &&
361 + starts_with(buffer, "ERR "))
362 + die(_("remote error: %s"), buffer + 4);
363 +
364 *pktlen = len;
365 return PACKET_READ_NORMAL;
366 }
t/t5516-fetch-push.sh
+1 -1
@@ -1241,7 +1241,7 @@ do
1241 git fetch ../testrepo/.git $SHA1_2 &&
1242 git cat-file commit $SHA1_2 &&
1243 test_must_fail git fetch ../testrepo/.git $SHA1_3 2>err &&
1244 - test_i18ngrep "remote error:.*not our ref" err
1244 + test_i18ngrep "remote error:.*not our ref.*$SHA1_3\$" err
1245 )
1246 '
1247 done