{"thread":{"id":"61137","subject":"[PATCH 1/2] t-prio-queue: shorten array index message","startedAt":"2024-03-16T20:58:56Z","lastAt":"2024-03-18T16:25:54Z","messageCount":3,"participants":["René Scharfe","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"490801","messageId":"197f5ac9-7257-4caa-aa9e-041016c787f7@web.de","threadId":"61137","inReplyTo":null,"subject":"[PATCH 1/2] t-prio-queue: shorten array index message","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2024-03-16T20:45:51Z","receivedAt":"2024-03-16T20:58:56Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"If we get an unexpected result, the prio-queue unit test reports it like\nthis:\n\n # check \"result[j++] == show(get)\" failed at t/unit-tests/t-prio-queue.c:43\n #    left: 5\n #   right: 1\n # failed at result[] index 0\n\nThat last line repeats \"failed\" and \"result\" from the first line.\nShorten it to resemble a similar one in t-ctype and also remove the\nincrementation from the first line to avoid possible distractions from\nthe message of which comparison went wrong where:\n\n # check \"result[j] == show(get)\" failed at t/unit-tests/t-prio-queue.c:43\n #    left: 5\n #   right: 1\n #       j: 0\n\nSigned-off-by: René Scharfe <l.s.r@web.de>\n---\n t/unit-tests/t-prio-queue.c | 10 ++++++----\n 1 file changed, 6 insertions(+), 4 deletions(-)\n\ndiff --git a/t/unit-tests/t-prio-queue.c b/t/unit-tests/t-prio-queue.c\nindex d78b002f9e..616d0fc86f 100644\n--- a/t/unit-tests/t-prio-queue.c\n+++ b/t/unit-tests/t-prio-queue.c\n@@ -31,16 +31,18 @@ static void test_prio_queue(int *input, int *result, size_t input_size)\n \t\t\tget = prio_queue_get(&pq);\n \t\t\tif (!check(peek == get))\n \t\t\t\treturn;\n-\t\t\tif(!check_int(result[j++], ==, show(get)))\n-\t\t\t\ttest_msg(\"failed at result[] index %d\", j-1);\n+\t\t\tif (!check_int(result[j], ==, show(get)))\n+\t\t\t\ttest_msg(\"      j: %d\", j);\n+\t\t\tj++;\n \t\t\tbreak;\n \t\tcase DUMP:\n \t\t\twhile ((peek = prio_queue_peek(&pq))) {\n \t\t\t\tget = prio_queue_get(&pq);\n \t\t\t\tif (!check(peek == get))\n \t\t\t\t\treturn;\n-\t\t\t\tif(!check_int(result[j++], ==, show(get)))\n-\t\t\t\t\ttest_msg(\"failed at result[] index %d\", j-1);\n+\t\t\t\tif (!check_int(result[j], ==, show(get)))\n+\t\t\t\t\ttest_msg(\"      j: %d\", j);\n+\t\t\t\tj++;\n \t\t\t}\n \t\t\tbreak;\n \t\tcase STACK:\n--\n2.44.0\n"},{"id":"490802","messageId":"9bf36cc8-ff27-44df-b2fb-9f959c781269@web.de","threadId":"61137","inReplyTo":"197f5ac9-7257-4caa-aa9e-041016c787f7@web.de","subject":"[PATCH 2/2] t-prio-queue: check result array bounds","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2024-03-16T21:09:47Z","receivedAt":"2024-03-16T21:09:49Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Avoid reading past the end of the \"result\" array, which could otherwise\nhappen if the prio-queue were to yield more items than were put into it\ndue to an implementation bug, or if the array has not enough entries due\nto a test bug.\n\nAlso check at the end whether all \"result\" entries were consumed, which\nwould not be the case if the prio-queue forgot some entries or the test\ndefinition contained too many.\n\nSigned-off-by: René Scharfe <l.s.r@web.de>\n---\n t/unit-tests/t-prio-queue.c | 14 +++++++++++---\n 1 file changed, 11 insertions(+), 3 deletions(-)\n\ndiff --git a/t/unit-tests/t-prio-queue.c b/t/unit-tests/t-prio-queue.c\nindex 616d0fc86f..5358346361 100644\n--- a/t/unit-tests/t-prio-queue.c\n+++ b/t/unit-tests/t-prio-queue.c\n@@ -19,11 +19,13 @@ static int show(int *v)\n \treturn v ? *v : MISSING;\n }\n\n-static void test_prio_queue(int *input, int *result, size_t input_size)\n+static void test_prio_queue(int *input, size_t input_size,\n+\t\t\t    int *result, size_t result_size)\n {\n \tstruct prio_queue pq = { intcmp };\n+\tint j = 0;\n\n-\tfor (int i = 0, j = 0; i < input_size; i++) {\n+\tfor (int i = 0; i < input_size; i++) {\n \t\tvoid *peek, *get;\n \t\tswitch(input[i]) {\n \t\tcase GET:\n@@ -31,6 +33,8 @@ static void test_prio_queue(int *input, int *result, size_t input_size)\n \t\t\tget = prio_queue_get(&pq);\n \t\t\tif (!check(peek == get))\n \t\t\t\treturn;\n+\t\t\tif (!check_uint(j, <, result_size))\n+\t\t\t\tbreak;\n \t\t\tif (!check_int(result[j], ==, show(get)))\n \t\t\t\ttest_msg(\"      j: %d\", j);\n \t\t\tj++;\n@@ -40,6 +44,8 @@ static void test_prio_queue(int *input, int *result, size_t input_size)\n \t\t\t\tget = prio_queue_get(&pq);\n \t\t\t\tif (!check(peek == get))\n \t\t\t\t\treturn;\n+\t\t\t\tif (!check_uint(j, <, result_size))\n+\t\t\t\t\tbreak;\n \t\t\t\tif (!check_int(result[j], ==, show(get)))\n \t\t\t\t\ttest_msg(\"      j: %d\", j);\n \t\t\t\tj++;\n@@ -56,6 +62,7 @@ static void test_prio_queue(int *input, int *result, size_t input_size)\n \t\t\tbreak;\n \t\t}\n \t}\n+\tcheck_uint(j, ==, result_size);\n \tclear_prio_queue(&pq);\n }\n\n@@ -79,7 +86,8 @@ static void test_prio_queue(int *input, int *result, size_t input_size)\n {\t\t\t\t\t\t\t\t\\\n \tint input[] = {INPUT};\t\t\t\t\t\\\n \tint result[] = {RESULT};\t\t\t\t\\\n-\ttest_prio_queue(input, result, ARRAY_SIZE(input));\t\\\n+\ttest_prio_queue(input, ARRAY_SIZE(input),\t\t\\\n+\t\t\tresult, ARRAY_SIZE(result));\t\t\\\n }\n\n TEST_INPUT(BASIC_INPUT, BASIC_RESULT, basic)\n--\n2.44.0\n"},{"id":"490880","messageId":"xmqqa5mvjylv.fsf@gitster.g","threadId":"61137","inReplyTo":"197f5ac9-7257-4caa-aa9e-041016c787f7@web.de","subject":"Re: [PATCH 1/2] t-prio-queue: shorten array index message","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-03-18T16:25:48Z","receivedAt":"2024-03-18T16:25:54Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"René Scharfe <l.s.r@web.de> writes:\n\n> If we get an unexpected result, the prio-queue unit test reports it like\n> this:\n>\n>  # check \"result[j++] == show(get)\" failed at t/unit-tests/t-prio-queue.c:43\n>  #    left: 5\n>  #   right: 1\n>  # failed at result[] index 0\n>\n> That last line repeats \"failed\" and \"result\" from the first line.\n> Shorten it to resemble a similar one in t-ctype and also remove the\n> incrementation from the first line to avoid possible distractions from\n> the message of which comparison went wrong where:\n>\n>  # check \"result[j] == show(get)\" failed at t/unit-tests/t-prio-queue.c:43\n>  #    left: 5\n>  #   right: 1\n>  #       j: 0\n\nVery nice touch, especially the removal of ++ from the message ;-).\n\n> Signed-off-by: René Scharfe <l.s.r@web.de>\n> ---\n>  t/unit-tests/t-prio-queue.c | 10 ++++++----\n>  1 file changed, 6 insertions(+), 4 deletions(-)\n>\n> diff --git a/t/unit-tests/t-prio-queue.c b/t/unit-tests/t-prio-queue.c\n> index d78b002f9e..616d0fc86f 100644\n> --- a/t/unit-tests/t-prio-queue.c\n> +++ b/t/unit-tests/t-prio-queue.c\n> @@ -31,16 +31,18 @@ static void test_prio_queue(int *input, int *result, size_t input_size)\n>  \t\t\tget = prio_queue_get(&pq);\n>  \t\t\tif (!check(peek == get))\n>  \t\t\t\treturn;\n> -\t\t\tif(!check_int(result[j++], ==, show(get)))\n> -\t\t\t\ttest_msg(\"failed at result[] index %d\", j-1);\n> +\t\t\tif (!check_int(result[j], ==, show(get)))\n> +\t\t\t\ttest_msg(\"      j: %d\", j);\n> +\t\t\tj++;\n>  \t\t\tbreak;\n>  \t\tcase DUMP:\n>  \t\t\twhile ((peek = prio_queue_peek(&pq))) {\n>  \t\t\t\tget = prio_queue_get(&pq);\n>  \t\t\t\tif (!check(peek == get))\n>  \t\t\t\t\treturn;\n> -\t\t\t\tif(!check_int(result[j++], ==, show(get)))\n> -\t\t\t\t\ttest_msg(\"failed at result[] index %d\", j-1);\n> +\t\t\t\tif (!check_int(result[j], ==, show(get)))\n> +\t\t\t\t\ttest_msg(\"      j: %d\", j);\n> +\t\t\t\tj++;\n>  \t\t\t}\n>  \t\t\tbreak;\n>  \t\tcase STACK:\n> --\n> 2.44.0\n"}]}