{"thread":{"id":"51658","subject":"[PATCH 0/4] fix pax extended header length calculation","startedAt":"2019-08-17T16:19:44Z","lastAt":"2019-08-17T18:05:34Z","messageCount":7,"participants":["René Scharfe","brian m. carlson","Eric Sunshine"],"isPatch":true,"patchVersion":1,"patchTotal":4},"messages":[{"id":"380579","messageId":"c77a0081-93ab-7b97-e322-9616f6e7f86c@web.de","threadId":"51658","inReplyTo":null,"subject":"[PATCH 0/4] fix pax extended header length calculation","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2019-08-17T16:19:29Z","receivedAt":"2019-08-17T16:19:44Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"The code for building pax extended headers has been miscalculating\nlengths slightly shorter than powers of 10  since I wrote it in 2006.\nThat affects entries for paths with a length of 990, 991, 9989, 9990,\n9991, 99988 etc. and link targets 4 characters shorter.  Here's a\nseries for fixing it.\n\n  archive-tar: report wrong pax extended header length\n  archive-tar: fix pax extended header length calculation\n  archive-tar: use size_t in strbuf_append_ext_header()\n  archive-tar: turn length miscalculation warning into BUG\n\n archive-tar.c                   | 14 ++++++++++----\n t/t5004-archive-corner-cases.sh | 19 +++++++++++++++++++\n 2 files changed, 29 insertions(+), 4 deletions(-)\n\n--\n2.23.0\n"},{"id":"380580","messageId":"c9915187-cb18-2619-f50b-12fe93c2578c@web.de","threadId":"51658","inReplyTo":"c77a0081-93ab-7b97-e322-9616f6e7f86c@web.de","subject":"[PATCH 1/4] archive-tar: report wrong pax extended header length","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2019-08-17T16:23:52Z","receivedAt":"2019-08-17T16:24:01Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Extended header entries contain a length value that is a bit tricky to\ncalculate because it includes its own length (number of decimal digits)\nas well.  We get it wrong in corner cases.  Add a check, report wrong\nresults as a warning and add a test for exercising it.\n\nSigned-off-by: René Scharfe <l.s.r@web.de>\n---\n archive-tar.c                   |  6 ++++++\n t/t5004-archive-corner-cases.sh | 20 ++++++++++++++++++++\n 2 files changed, 26 insertions(+)\n\ndiff --git a/archive-tar.c b/archive-tar.c\nindex 3e53aac1e6..355c8142c6 100644\n--- a/archive-tar.c\n+++ b/archive-tar.c\n@@ -144,6 +144,7 @@ static int stream_blocked(const struct object_id *oid)\n static void strbuf_append_ext_header(struct strbuf *sb, const char *keyword,\n \t\t\t\t     const char *value, unsigned int valuelen)\n {\n+\tsize_t orig_len = sb->len;\n \tint len, tmp;\n\n \t/* \"%u %s=%s\\n\" */\n@@ -155,6 +156,11 @@ static void strbuf_append_ext_header(struct strbuf *sb, const char *keyword,\n \tstrbuf_addf(sb, \"%u %s=\", len, keyword);\n \tstrbuf_add(sb, value, valuelen);\n \tstrbuf_addch(sb, '\\n');\n+\n+\tif (len != sb->len - orig_len)\n+\t\twarning(\"pax extended header length miscalculated as %d\"\n+\t\t\t\", should be %\"PRIuMAX,\n+\t\t\tlen, (uintmax_t)(sb->len - orig_len));\n }\n\n /*\ndiff --git a/t/t5004-archive-corner-cases.sh b/t/t5004-archive-corner-cases.sh\nindex 271eb5a1fd..2f15d1899d 100755\n--- a/t/t5004-archive-corner-cases.sh\n+++ b/t/t5004-archive-corner-cases.sh\n@@ -204,4 +204,24 @@ test_expect_success EXPENSIVE,LONG_IS_64BIT,UNZIP,UNZIP_ZIP64_SUPPORT,ZIPINFO \\\n \tgrep $size big.lst\n '\n\n+build_tree() {\n+\tperl -e '\n+\t\tmy $hash = $ARGV[0];\n+\t\tforeach my $order (2..6) {\n+\t\t\t$first = 10 ** $order;\n+\t\t\tforeach my $i (-13..-9) {\n+\t\t\t\tmy $name = \"a\" x ($first + $i);\n+\t\t\t\tprint \"100644 blob $hash\\t$name\\n\"\n+\t\t\t}\n+\t\t}\n+\t' \"$1\"\n+}\n+\n+test_expect_failure 'tar archive with long paths' '\n+\tblob=$(echo foo | git hash-object -w --stdin) &&\n+\ttree=$(build_tree $blob | git mktree) &&\n+\tgit archive -o long_paths.tar $tree 2>stderr &&\n+\ttest_must_be_empty stderr\n+'\n+\n test_done\n--\n2.23.0\n"},{"id":"380581","messageId":"4b04cf3f-966a-3aa9-6a30-7ac3a234e1ed@web.de","threadId":"51658","inReplyTo":"c77a0081-93ab-7b97-e322-9616f6e7f86c@web.de","subject":"[PATCH 2/4] archive-tar: fix pax extended header length calculation","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2019-08-17T16:24:01Z","receivedAt":"2019-08-17T16:24:07Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"A pax extended header records starts with a decimal number.  Its value\nis the length of the whole record, including its own length.\n\nThe calculation of that number if strbuf_append_ext_header() is off by\none in case the length of the rest is close to a higher order of\nmagnitude.  This affects paths and link targets a bit shorter than 1000,\n10000, 100000 etc. characters -- paths with a length of up to 100 fit\ninto the tar header and don't need a pax extended header.\n\nThe mistake has been present since the function was added by ae64bbc18c\n(\"tar-tree: Introduce write_entry()\", 2006-03-25).\n\nAccount for digits added to len during the loop and keep incrementing\nuntil we have enough space for len and the rest.  The crucial change is\nto check against the current value of len before each iteration, instead\nof against its value before the loop.\n\nSigned-off-by: René Scharfe <l.s.r@web.de>\n---\n archive-tar.c                   | 2 +-\n t/t5004-archive-corner-cases.sh | 2 +-\n 2 files changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/archive-tar.c b/archive-tar.c\nindex 355c8142c6..4395a29ffb 100644\n--- a/archive-tar.c\n+++ b/archive-tar.c\n@@ -149,7 +149,7 @@ static void strbuf_append_ext_header(struct strbuf *sb, const char *keyword,\n\n \t/* \"%u %s=%s\\n\" */\n \tlen = 1 + 1 + strlen(keyword) + 1 + valuelen + 1;\n-\tfor (tmp = len; tmp > 9; tmp /= 10)\n+\tfor (tmp = 1; len / 10 >= tmp; tmp *= 10)\n \t\tlen++;\n\n \tstrbuf_grow(sb, len);\ndiff --git a/t/t5004-archive-corner-cases.sh b/t/t5004-archive-corner-cases.sh\nindex 2f15d1899d..4966a74b4d 100755\n--- a/t/t5004-archive-corner-cases.sh\n+++ b/t/t5004-archive-corner-cases.sh\n@@ -217,7 +217,7 @@ build_tree() {\n \t' \"$1\"\n }\n\n-test_expect_failure 'tar archive with long paths' '\n+test_expect_success 'tar archive with long paths' '\n \tblob=$(echo foo | git hash-object -w --stdin) &&\n \ttree=$(build_tree $blob | git mktree) &&\n \tgit archive -o long_paths.tar $tree 2>stderr &&\n--\n2.23.0\n"},{"id":"380582","messageId":"1933ad5e-612f-b5ce-065b-f435338c3c9c@web.de","threadId":"51658","inReplyTo":"c77a0081-93ab-7b97-e322-9616f6e7f86c@web.de","subject":"[PATCH 3/4] archive-tar: use size_t in strbuf_append_ext_header()","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2019-08-17T16:24:13Z","receivedAt":"2019-08-17T16:24:17Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"One of its callers already passes in a size_t value.  Use it\nconsistently in this function.\n\nSigned-off-by: René Scharfe <l.s.r@web.de>\n---\n archive-tar.c | 10 +++++-----\n 1 file changed, 5 insertions(+), 5 deletions(-)\n\ndiff --git a/archive-tar.c b/archive-tar.c\nindex 4395a29ffb..9d09edd547 100644\n--- a/archive-tar.c\n+++ b/archive-tar.c\n@@ -142,10 +142,10 @@ static int stream_blocked(const struct object_id *oid)\n  * string and appends it to a struct strbuf.\n  */\n static void strbuf_append_ext_header(struct strbuf *sb, const char *keyword,\n-\t\t\t\t     const char *value, unsigned int valuelen)\n+\t\t\t\t     const char *value, size_t valuelen)\n {\n \tsize_t orig_len = sb->len;\n-\tint len, tmp;\n+\tsize_t len, tmp;\n\n \t/* \"%u %s=%s\\n\" */\n \tlen = 1 + 1 + strlen(keyword) + 1 + valuelen + 1;\n@@ -153,14 +153,14 @@ static void strbuf_append_ext_header(struct strbuf *sb, const char *keyword,\n \t\tlen++;\n\n \tstrbuf_grow(sb, len);\n-\tstrbuf_addf(sb, \"%u %s=\", len, keyword);\n+\tstrbuf_addf(sb, \"%\"PRIuMAX\" %s=\", (uintmax_t)len, keyword);\n \tstrbuf_add(sb, value, valuelen);\n \tstrbuf_addch(sb, '\\n');\n\n \tif (len != sb->len - orig_len)\n-\t\twarning(\"pax extended header length miscalculated as %d\"\n+\t\twarning(\"pax extended header length miscalculated as %\"PRIuMAX\n \t\t\t\", should be %\"PRIuMAX,\n-\t\t\tlen, (uintmax_t)(sb->len - orig_len));\n+\t\t\t(uintmax_t)len, (uintmax_t)(sb->len - orig_len));\n }\n\n /*\n--\n2.23.0\n"},{"id":"380583","messageId":"f5ad09f8-9da5-9567-afe0-f1379bd9192b@web.de","threadId":"51658","inReplyTo":"c77a0081-93ab-7b97-e322-9616f6e7f86c@web.de","subject":"[PATCH 4/4] archive-tar: turn length miscalculation warning into BUG","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2019-08-17T16:24:23Z","receivedAt":"2019-08-17T16:24:28Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Now that we're confident our pax extended header calculation is correct,\nturn the criticality of the assertion up to the maximum, from warning\nright up to BUG.  Simplify the test, as the stderr comparison step would\nnot be reached in case the BUG message is triggered.\n\nSigned-off-by: René Scharfe <l.s.r@web.de>\n---\n archive-tar.c                   | 6 +++---\n t/t5004-archive-corner-cases.sh | 3 +--\n 2 files changed, 4 insertions(+), 5 deletions(-)\n\ndiff --git a/archive-tar.c b/archive-tar.c\nindex 9d09edd547..e16d3f756d 100644\n--- a/archive-tar.c\n+++ b/archive-tar.c\n@@ -158,9 +158,9 @@ static void strbuf_append_ext_header(struct strbuf *sb, const char *keyword,\n \tstrbuf_addch(sb, '\\n');\n\n \tif (len != sb->len - orig_len)\n-\t\twarning(\"pax extended header length miscalculated as %\"PRIuMAX\n-\t\t\t\", should be %\"PRIuMAX,\n-\t\t\t(uintmax_t)len, (uintmax_t)(sb->len - orig_len));\n+\t\tBUG(\"pax extended header length miscalculated as %\"PRIuMAX\n+\t\t    \", should be %\"PRIuMAX,\n+\t\t    (uintmax_t)len, (uintmax_t)(sb->len - orig_len));\n }\n\n /*\ndiff --git a/t/t5004-archive-corner-cases.sh b/t/t5004-archive-corner-cases.sh\nindex 4966a74b4d..3e7b23cb32 100755\n--- a/t/t5004-archive-corner-cases.sh\n+++ b/t/t5004-archive-corner-cases.sh\n@@ -220,8 +220,7 @@ build_tree() {\n test_expect_success 'tar archive with long paths' '\n \tblob=$(echo foo | git hash-object -w --stdin) &&\n \ttree=$(build_tree $blob | git mktree) &&\n-\tgit archive -o long_paths.tar $tree 2>stderr &&\n-\ttest_must_be_empty stderr\n+\tgit archive -o long_paths.tar $tree\n '\n\n test_done\n--\n2.23.0\n"},{"id":"380584","messageId":"20190817164014.GE365197@genre.crustytoothpaste.net","threadId":"51658","inReplyTo":"c77a0081-93ab-7b97-e322-9616f6e7f86c@web.de","subject":"Re: [PATCH 0/4] fix pax extended header length calculation","fromName":"brian m. carlson","fromEmail":"sandals@crustytoothpaste.net","sentAt":"2019-08-17T16:40:36Z","receivedAt":"2019-08-17T16:40:43Z","isPatch":true,"sender":{"key":"sandals@crustytoothpaste.net","avatar":"https://avatars.githubusercontent.com/u/497054?v=4"},"body":"On 2019-08-17 at 16:19:29, René Scharfe wrote:\n> The code for building pax extended headers has been miscalculating\n> lengths slightly shorter than powers of 10  since I wrote it in 2006.\n> That affects entries for paths with a length of 990, 991, 9989, 9990,\n> 9991, 99988 etc. and link targets 4 characters shorter.  Here's a\n> series for fixing it.\n\nThis series looked good to me. I'm don't completely understand our\ntechnique for computing the length, but the additional tests build my\nconfidence and your explanation makes sense.\n\nAs a side note, I have personally found computing the length of pax\nheaders to be enormously difficult due to this very edge case, so I'm\nnot surprised this bug crept in.\n-- \nbrian m. carlson: Houston, Texas, US\nOpenPGP: https://keybase.io/bk2204\n"},{"id":"380585","messageId":"CAPig+cRi5uUKHGNB+DezAMaQ6R27U-5Xyy3bUE2c=W0CoyPkVQ@mail.gmail.com","threadId":"51658","inReplyTo":"4b04cf3f-966a-3aa9-6a30-7ac3a234e1ed@web.de","subject":"Re: [PATCH 2/4] archive-tar: fix pax extended header length calculation","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2019-08-17T18:03:14Z","receivedAt":"2019-08-17T18:05:34Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Sat, Aug 17, 2019 at 12:24 PM René Scharfe <l.s.r@web.de> wrote:\n> A pax extended header records starts with a decimal number.  Its value\n\ns/records/record/\n\n> is the length of the whole record, including its own length.\n>\n> The calculation of that number if strbuf_append_ext_header() is off by\n\ns/if/in/\n\n> one in case the length of the rest is close to a higher order of\n> magnitude.  This affects paths and link targets a bit shorter than 1000,\n> 10000, 100000 etc. characters -- paths with a length of up to 100 fit\n> into the tar header and don't need a pax extended header.\n>\n> The mistake has been present since the function was added by ae64bbc18c\n> (\"tar-tree: Introduce write_entry()\", 2006-03-25).\n>\n> Account for digits added to len during the loop and keep incrementing\n> until we have enough space for len and the rest.  The crucial change is\n> to check against the current value of len before each iteration, instead\n> of against its value before the loop.\n>\n> Signed-off-by: René Scharfe <l.s.r@web.de>\n"}]}