{"thread":{"id":"60996","subject":"[PATCH] upload-pack: don't send null character in abort message to the client","startedAt":"2024-02-25T18:34:57Z","lastAt":"2024-02-26T17:49:07Z","messageCount":2,"participants":["SZEDER Gábor","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"489323","messageId":"20240225183452.1939334-1-szeder.dev@gmail.com","threadId":"60996","inReplyTo":null,"subject":"[PATCH] upload-pack: don't send null character in abort message to the client","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2024-02-25T18:34:52Z","receivedAt":"2024-02-25T18:34:57Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"Since 583b7ea31b (upload-pack/fetch-pack: support side-band\ncommunication, 2006-06-21) the abort message sent by upload-pack in\ncase of possible repository corruption ends with a null character.\nThis can be seen in several test cases in 't5530-upload-pack-error.sh'\nwhere 'grep <pattern> output.err' often reports \"Binary file\noutput.err matches\" because of that null character.\n\nThe reason for this is that the abort message is defined as a string\nliteral, and we pass its size to the send function as\nsizeof(abort_msg), which also counts the terminating null character.\n\nUse strlen() instead to avoid sending that terminating null character.\n\nSigned-off-by: SZEDER Gábor <szeder.dev@gmail.com>\n---\n upload-pack.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/upload-pack.c b/upload-pack.c\nindex 2537affa90..6e0d441ef5 100644\n--- a/upload-pack.c\n+++ b/upload-pack.c\n@@ -463,7 +463,7 @@ static void create_pack_file(struct upload_pack_data *pack_data,\n \n  fail:\n \tfree(output_state);\n-\tsend_client_data(3, abort_msg, sizeof(abort_msg),\n+\tsend_client_data(3, abort_msg, strlen(abort_msg),\n \t\t\t pack_data->use_sideband);\n \tdie(\"git upload-pack: %s\", abort_msg);\n }\n-- \n2.44.0.rc1.366.g26e5fbbdb0\n\n"},{"id":"489397","messageId":"xmqqttlvnmk1.fsf@gitster.g","threadId":"60996","inReplyTo":"20240225183452.1939334-1-szeder.dev@gmail.com","subject":"Re: [PATCH] upload-pack: don't send null character in abort message to the client","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-02-26T17:49:02Z","receivedAt":"2024-02-26T17:49:07Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"SZEDER Gábor <szeder.dev@gmail.com> writes:\n\n> Since 583b7ea31b (upload-pack/fetch-pack: support side-band\n> communication, 2006-06-21) the abort message sent by upload-pack in\n> case of possible repository corruption ends with a null character.\n\nIt is so so old that makes me wonder if it is safe to \"fix\" it, but\nI cannot think of a sensible way to write a third-party client that\nmay have been working fine and would break when this fix is made.\n\n> This can be seen in several test cases in 't5530-upload-pack-error.sh'\n> where 'grep <pattern> output.err' often reports \"Binary file\n> output.err matches\" because of that null character.\n>\n> The reason for this is that the abort message is defined as a string\n> literal, and we pass its size to the send function as\n> sizeof(abort_msg), which also counts the terminating null character.\n>\n> Use strlen() instead to avoid sending that terminating null character.\n>\n> Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com>\n> ---\n>  upload-pack.c | 2 +-\n>  1 file changed, 1 insertion(+), 1 deletion(-)\n>\n> diff --git a/upload-pack.c b/upload-pack.c\n> index 2537affa90..6e0d441ef5 100644\n> --- a/upload-pack.c\n> +++ b/upload-pack.c\n> @@ -463,7 +463,7 @@ static void create_pack_file(struct upload_pack_data *pack_data,\n>  \n>   fail:\n>  \tfree(output_state);\n> -\tsend_client_data(3, abort_msg, sizeof(abort_msg),\n> +\tsend_client_data(3, abort_msg, strlen(abort_msg),\n>  \t\t\t pack_data->use_sideband);\n>  \tdie(\"git upload-pack: %s\", abort_msg);\n>  }\n"}]}