{"thread":{"id":"61039","subject":"[PATCH 0/3] Auditing use of xwrite()","startedAt":"2024-03-02T19:03:53Z","lastAt":"2024-03-04T16:43:08Z","messageCount":7,"participants":["Junio C Hamano","Patrick Steinhardt"],"isPatch":true,"patchVersion":1,"patchTotal":3},"messages":[{"id":"489787","messageId":"20240302190348.3946569-1-gitster@pobox.com","threadId":"61039","inReplyTo":null,"subject":"[PATCH 0/3] Auditing use of xwrite()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-03-02T19:03:45Z","receivedAt":"2024-03-02T19:03:53Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"The xwrite() helper relieves us from having to worry about an\ninterrupted write(2) system call that returns an EINTR error without\nwriting any, in which case we need to retry the system call.  It,\njust as the underlying write(2) system call, however can return\nwithout writing the bytes requested to be written fully.  A code\nthat uses xwrite() and wants to write the full contents of the\nbuffer needs to take care of such a \"short write\" situation,\ntypically by calling xwrite() in a loop.\n\nWe recently audited all the calls to xwrite(), and have found two\nclasses of potential \"problems\".  This miniseries is about fixing them.\n\n * There are such loops around xwrite() as described above, which\n   were written long time ago.  The write_in_full() helper does\n   implement such a loop in a reusable way, and we should just call\n   it instead.\n\n * There are xwrite() calls whose return value is ignored.  We would\n   not be able to detect (and complete) \"short write\" in such callers,\n   of course, but more importantly, we would not notice I/O failures.\n   Again, use write_in_full() to deal with short writes, and check\n   the return value from it to detect errors.\n\nJunio C Hamano (3):\n  unpack: rewrite hand-rolled loop of xwrite()s with write_in_full()\n  sideband: avoid short write(2)\n  repack: check error writing to pack-objects subprocess\n\n builtin/index-pack.c     | 17 +++--------------\n builtin/repack.c         |  5 +++--\n builtin/unpack-objects.c |  8 +-------\n sideband.c               |  4 ++--\n 4 files changed, 9 insertions(+), 25 deletions(-)\n\n-- \n2.44.0-84-gb387623c12\n\n"},{"id":"489788","messageId":"20240302190348.3946569-2-gitster@pobox.com","threadId":"61039","inReplyTo":"20240302190348.3946569-1-gitster@pobox.com","subject":"[PATCH 1/3] unpack: replace xwrite() loop with write_in_full()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-03-02T19:03:46Z","receivedAt":"2024-03-02T19:03:57Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"We have two packfile stream consumers, index-pack and\nunpack-objects, that allow excess payload after the packfile stream\ndata. Their code to relay excess data hasn't changed significantly\nsince their original implementation that appeared in 67e5a5ec\n(git-unpack-objects: re-write to read from stdin, 2005-06-28) and\n9bee2478 (mimic unpack-objects when --stdin is used with index-pack,\n2006-10-25).\n\nThese code blocks contain hand-rolled loops using xwrite(), written\nbefore our write_in_full() helper existed. This helper now provides\nthe same functionality.\n\nReplace these loops with write_in_full() for shorter, clearer\ncode. Update related variables accordingly.\n\nSigned-off-by: Junio C Hamano <gitster@pobox.com>\n---\n builtin/index-pack.c     | 17 +++--------------\n builtin/unpack-objects.c |  8 +-------\n 2 files changed, 4 insertions(+), 21 deletions(-)\n\ndiff --git a/builtin/index-pack.c b/builtin/index-pack.c\nindex a3a37bd215..856428fef9 100644\n--- a/builtin/index-pack.c\n+++ b/builtin/index-pack.c\n@@ -1524,14 +1524,12 @@ static void final(const char *final_pack_name, const char *curr_pack_name,\n \tstruct strbuf pack_name = STRBUF_INIT;\n \tstruct strbuf index_name = STRBUF_INIT;\n \tstruct strbuf rev_index_name = STRBUF_INIT;\n-\tint err;\n \n \tif (!from_stdin) {\n \t\tclose(input_fd);\n \t} else {\n \t\tfsync_component_or_die(FSYNC_COMPONENT_PACK, output_fd, curr_pack_name);\n-\t\terr = close(output_fd);\n-\t\tif (err)\n+\t\tif (close(output_fd))\n \t\t\tdie_errno(_(\"error while closing pack file\"));\n \t}\n \n@@ -1566,17 +1564,8 @@ static void final(const char *final_pack_name, const char *curr_pack_name,\n \t\twrite_or_die(1, buf.buf, buf.len);\n \t\tstrbuf_release(&buf);\n \n-\t\t/*\n-\t\t * Let's just mimic git-unpack-objects here and write\n-\t\t * the last part of the input buffer to stdout.\n-\t\t */\n-\t\twhile (input_len) {\n-\t\t\terr = xwrite(1, input_buffer + input_offset, input_len);\n-\t\t\tif (err <= 0)\n-\t\t\t\tbreak;\n-\t\t\tinput_len -= err;\n-\t\t\tinput_offset += err;\n-\t\t}\n+\t\t/* Write the last part of the buffer to stdout */\n+\t\twrite_in_full(1, input_buffer + input_offset, input_len);\n \t}\n \n \tstrbuf_release(&rev_index_name);\ndiff --git a/builtin/unpack-objects.c b/builtin/unpack-objects.c\nindex e0a701f2b3..f1c85a00ae 100644\n--- a/builtin/unpack-objects.c\n+++ b/builtin/unpack-objects.c\n@@ -679,13 +679,7 @@ int cmd_unpack_objects(int argc, const char **argv, const char *prefix UNUSED)\n \tuse(the_hash_algo->rawsz);\n \n \t/* Write the last part of the buffer to stdout */\n-\twhile (len) {\n-\t\tint ret = xwrite(1, buffer + offset, len);\n-\t\tif (ret <= 0)\n-\t\t\tbreak;\n-\t\tlen -= ret;\n-\t\toffset += ret;\n-\t}\n+\twrite_in_full(1, buffer + offset, len);\n \n \t/* All done */\n \treturn has_errors;\n-- \n2.44.0-84-gb387623c12\n\n"},{"id":"489789","messageId":"20240302190348.3946569-4-gitster@pobox.com","threadId":"61039","inReplyTo":"20240302190348.3946569-1-gitster@pobox.com","subject":"[PATCH 3/3] repack: check error writing to pack-objects subprocess","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-03-02T19:03:48Z","receivedAt":"2024-03-02T19:03:59Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"When \"git repack\" repacks promisor objects, it starts a pack-objects\nsubprocess and uses xwrite() to send object names over the pipe to\nit, but without any error checking.  An I/O error or short write\n(even though a short write is unlikely for such a small amount of\ndata) can result in a packfile that lacks certain objects we wanted\nto put in there, leading to a silent repository corruption.\n\nUse write_in_full(), instead of xwrite(), to mitigate short write\nrisks, check errors from it, and abort if we see a failure.\n\nSigned-off-by: Junio C Hamano <gitster@pobox.com>\n---\n builtin/repack.c | 5 +++--\n 1 file changed, 3 insertions(+), 2 deletions(-)\n\ndiff --git a/builtin/repack.c b/builtin/repack.c\nindex ede36328a3..15e4cccc45 100644\n--- a/builtin/repack.c\n+++ b/builtin/repack.c\n@@ -314,8 +314,9 @@ static int write_oid(const struct object_id *oid,\n \t\t\tdie(_(\"could not start pack-objects to repack promisor objects\"));\n \t}\n \n-\txwrite(cmd->in, oid_to_hex(oid), the_hash_algo->hexsz);\n-\txwrite(cmd->in, \"\\n\", 1);\n+\tif (write_in_full(cmd->in, oid_to_hex(oid), the_hash_algo->hexsz) < 0 ||\n+\t    write_in_full(cmd->in, \"\\n\", 1) < 0)\n+\t\tdie(_(\"failed to feed promisor objects to pack-objects\"));\n \treturn 0;\n }\n \n-- \n2.44.0-84-gb387623c12\n\n"},{"id":"489790","messageId":"20240302190348.3946569-3-gitster@pobox.com","threadId":"61039","inReplyTo":"20240302190348.3946569-1-gitster@pobox.com","subject":"[PATCH 2/3] sideband: avoid short write(2)","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-03-02T19:03:47Z","receivedAt":"2024-03-02T19:04:04Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"The sideband demultiplexor writes the data it receives on sideband\nwith xwrite().  We can lose data if the underlying write(2) results\nin a short write.\n\nIf they are limited to unimportant bytes like eye-candy progress\nmeter, it may be OK to lose them, but lets be careful and ensure\nthat we use write_in_full() instead.  Note that the original does\nnot check for errors, and this rewrite does not check for one.  At\nleast not yet.\n\nSigned-off-by: Junio C Hamano <gitster@pobox.com>\n---\n sideband.c | 4 ++--\n 1 file changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/sideband.c b/sideband.c\nindex 266a67342b..5d8907151f 100644\n--- a/sideband.c\n+++ b/sideband.c\n@@ -220,7 +220,7 @@ int demultiplex_sideband(const char *me, int status,\n \t\t\t}\n \n \t\t\tstrbuf_addch(scratch, *brk);\n-\t\t\txwrite(2, scratch->buf, scratch->len);\n+\t\t\twrite_in_full(2, scratch->buf, scratch->len);\n \t\t\tstrbuf_reset(scratch);\n \n \t\t\tb = brk + 1;\n@@ -247,7 +247,7 @@ int demultiplex_sideband(const char *me, int status,\n \t\tdie(\"%s\", scratch->buf);\n \tif (scratch->len) {\n \t\tstrbuf_addch(scratch, '\\n');\n-\t\txwrite(2, scratch->buf, scratch->len);\n+\t\twrite_in_full(2, scratch->buf, scratch->len);\n \t}\n \tstrbuf_release(scratch);\n \treturn 1;\n-- \n2.44.0-84-gb387623c12\n\n"},{"id":"489838","messageId":"ZeVxA1KvN18Y85J_@tanuki","threadId":"61039","inReplyTo":"20240302190348.3946569-2-gitster@pobox.com","subject":"Re: [PATCH 1/3] unpack: replace xwrite() loop with write_in_full()","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-03-04T06:58:11Z","receivedAt":"2024-03-04T06:58:17Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Sat, Mar 02, 2024 at 11:03:46AM -0800, Junio C Hamano wrote:\n> We have two packfile stream consumers, index-pack and\n> unpack-objects, that allow excess payload after the packfile stream\n> data. Their code to relay excess data hasn't changed significantly\n> since their original implementation that appeared in 67e5a5ec\n> (git-unpack-objects: re-write to read from stdin, 2005-06-28) and\n> 9bee2478 (mimic unpack-objects when --stdin is used with index-pack,\n> 2006-10-25).\n> \n> These code blocks contain hand-rolled loops using xwrite(), written\n> before our write_in_full() helper existed. This helper now provides\n> the same functionality.\n> \n> Replace these loops with write_in_full() for shorter, clearer\n> code. Update related variables accordingly.\n> \n> Signed-off-by: Junio C Hamano <gitster@pobox.com>\n> ---\n>  builtin/index-pack.c     | 17 +++--------------\n>  builtin/unpack-objects.c |  8 +-------\n>  2 files changed, 4 insertions(+), 21 deletions(-)\n> \n> diff --git a/builtin/index-pack.c b/builtin/index-pack.c\n> index a3a37bd215..856428fef9 100644\n> --- a/builtin/index-pack.c\n> +++ b/builtin/index-pack.c\n> @@ -1524,14 +1524,12 @@ static void final(const char *final_pack_name, const char *curr_pack_name,\n>  \tstruct strbuf pack_name = STRBUF_INIT;\n>  \tstruct strbuf index_name = STRBUF_INIT;\n>  \tstruct strbuf rev_index_name = STRBUF_INIT;\n> -\tint err;\n>  \n>  \tif (!from_stdin) {\n>  \t\tclose(input_fd);\n>  \t} else {\n>  \t\tfsync_component_or_die(FSYNC_COMPONENT_PACK, output_fd, curr_pack_name);\n> -\t\terr = close(output_fd);\n> -\t\tif (err)\n> +\t\tif (close(output_fd))\n>  \t\t\tdie_errno(_(\"error while closing pack file\"));\n>  \t}\n>  \n> @@ -1566,17 +1564,8 @@ static void final(const char *final_pack_name, const char *curr_pack_name,\n>  \t\twrite_or_die(1, buf.buf, buf.len);\n>  \t\tstrbuf_release(&buf);\n>  \n> -\t\t/*\n> -\t\t * Let's just mimic git-unpack-objects here and write\n> -\t\t * the last part of the input buffer to stdout.\n> -\t\t */\n> -\t\twhile (input_len) {\n> -\t\t\terr = xwrite(1, input_buffer + input_offset, input_len);\n> -\t\t\tif (err <= 0)\n> -\t\t\t\tbreak;\n> -\t\t\tinput_len -= err;\n> -\t\t\tinput_offset += err;\n> -\t\t}\n> +\t\t/* Write the last part of the buffer to stdout */\n> +\t\twrite_in_full(1, input_buffer + input_offset, input_len);\n\nWith this change we stop updating `input_len` and `input_offset`, both\nof which are global variables. Assuming that tests pass this must be\nokay right now given that this is the final part of what we are writing.\nBut I wonder whether we shouldn't update those regardless just so that\nthese remain consistent?\n\n>  \t}\n>  \n>  \tstrbuf_release(&rev_index_name);\n> diff --git a/builtin/unpack-objects.c b/builtin/unpack-objects.c\n> index e0a701f2b3..f1c85a00ae 100644\n> --- a/builtin/unpack-objects.c\n> +++ b/builtin/unpack-objects.c\n> @@ -679,13 +679,7 @@ int cmd_unpack_objects(int argc, const char **argv, const char *prefix UNUSED)\n>  \tuse(the_hash_algo->rawsz);\n>  \n>  \t/* Write the last part of the buffer to stdout */\n> -\twhile (len) {\n> -\t\tint ret = xwrite(1, buffer + offset, len);\n> -\t\tif (ret <= 0)\n> -\t\t\tbreak;\n> -\t\tlen -= ret;\n> -\t\toffset += ret;\n> -\t}\n> +\twrite_in_full(1, buffer + offset, len);\n\nSame here.\n\nPatrick\n\n>  \t/* All done */\n>  \treturn has_errors;\n> -- \n> 2.44.0-84-gb387623c12\n> \n> \n"},{"id":"489839","messageId":"xmqqfrx6sbdf.fsf@gitster.g","threadId":"61039","inReplyTo":"ZeVxA1KvN18Y85J_@tanuki","subject":"Re: [PATCH 1/3] unpack: replace xwrite() loop with write_in_full()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-03-04T07:29:48Z","receivedAt":"2024-03-04T07:29:56Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Patrick Steinhardt <ps@pks.im> writes:\n\n>> -\t\twhile (input_len) {\n>> -\t\t\terr = xwrite(1, input_buffer + input_offset, input_len);\n>> -\t\t\tif (err <= 0)\n>> -\t\t\t\tbreak;\n>> -\t\t\tinput_len -= err;\n>> -\t\t\tinput_offset += err;\n>> -\t\t}\n>> +\t\t/* Write the last part of the buffer to stdout */\n>> +\t\twrite_in_full(1, input_buffer + input_offset, input_len);\n>\n> With this change we stop updating `input_len` and `input_offset`, both\n> of which are global variables. Assuming that tests pass this must be\n> okay right now given that this is the final part of what we are writing.\n> But I wonder whether we shouldn't update those regardless just so that\n> these remain consistent?\n\nIt is probably a good hygiene, even though it may not matter at all\nfor the correctness in the current code.\n\nThanks for your sharp eyes.\n\n>> diff --git a/builtin/unpack-objects.c b/builtin/unpack-objects.c\n>> index e0a701f2b3..f1c85a00ae 100644\n>> --- a/builtin/unpack-objects.c\n>> +++ b/builtin/unpack-objects.c\n>> @@ -679,13 +679,7 @@ int cmd_unpack_objects(int argc, const char **argv, const char *prefix UNUSED)\n>>  \tuse(the_hash_algo->rawsz);\n>>  \n>>  \t/* Write the last part of the buffer to stdout */\n>> -\twhile (len) {\n>> -\t\tint ret = xwrite(1, buffer + offset, len);\n>> -\t\tif (ret <= 0)\n>> -\t\t\tbreak;\n>> -\t\tlen -= ret;\n>> -\t\toffset += ret;\n>> -\t}\n>> +\twrite_in_full(1, buffer + offset, len);\n>\n> Same here.\n>\n> Patrick\n>\n>>  \t/* All done */\n>>  \treturn has_errors;\n>> -- \n>> 2.44.0-84-gb387623c12\n>> \n>> \n"},{"id":"489900","messageId":"xmqqzfveq76w.fsf@gitster.g","threadId":"61039","inReplyTo":"xmqqfrx6sbdf.fsf@gitster.g","subject":"Re: [PATCH 1/3] unpack: replace xwrite() loop with write_in_full()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-03-04T16:43:03Z","receivedAt":"2024-03-04T16:43:08Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n> Patrick Steinhardt <ps@pks.im> writes:\n>\n>>> -\t\twhile (input_len) {\n>>> -\t\t\terr = xwrite(1, input_buffer + input_offset, input_len);\n>>> -\t\t\tif (err <= 0)\n>>> -\t\t\t\tbreak;\n>>> -\t\t\tinput_len -= err;\n>>> -\t\t\tinput_offset += err;\n>>> -\t\t}\n>>> +\t\t/* Write the last part of the buffer to stdout */\n>>> +\t\twrite_in_full(1, input_buffer + input_offset, input_len);\n>>\n>> With this change we stop updating `input_len` and `input_offset`, both\n>> of which are global variables. Assuming that tests pass this must be\n>> okay right now given that this is the final part of what we are writing.\n>> But I wonder whether we shouldn't update those regardless just so that\n>> these remain consistent?\n>\n> It is probably a good hygiene, even though it may not matter at all\n> for the correctness in the current code.\n>\n> Thanks for your sharp eyes.\n\nActually, I changed my mind.  As you said, this is flushing the very\nend of the data in the input_buffer[] and nobody will fill() the\ninput_buffer[] after the call to this function happens.\n\n>>> -\twhile (len) {\n>>> ...\n>>> -\t\tlen -= ret;\n>>> -\t\toffset += ret;\n>>> -\t}\n>>> +\twrite_in_full(1, buffer + offset, len);\n>>\n>> Same here.\n\nDitto.  We are about to pass the control back to the caller that\nwill exit using the \"has_errors\" we return from here.\n\n>>\n>> Patrick\n>>\n>>>  \t/* All done */\n>>>  \treturn has_errors;\n>>> -- \n>>> 2.44.0-84-gb387623c12\n>>> \n>>> \n"}]}