{"thread":{"id":"54660","subject":"[PATCH] gc: fix cast in compare_tasks_by_selection()","startedAt":"2020-11-17T22:00:11Z","lastAt":"2020-11-17T22:30:34Z","messageCount":2,"participants":["René Scharfe","Jeff King"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"410169","messageId":"d80423b7-c89d-99a2-ce49-8308822f7769@web.de","threadId":"54660","inReplyTo":null,"subject":"[PATCH] gc: fix cast in compare_tasks_by_selection()","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2020-11-17T21:59:49Z","receivedAt":"2020-11-17T22:00:11Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"compare_tasks_by_selection() is used with QSORT and gets passed pointers\nto the elements of \"static struct maintenance_task tasks[]\".  It casts\nthe *addresses* of these passed pointers to element pointers, though,\nand thus effectively compares some unrelated values from the stack.  Fix\nthe casts to actually compare array elements.\n\nDetected by USan (make SANITIZE=undefined test).\n\nSigned-off-by: René Scharfe <l.s.r@web.de>\n---\n builtin/gc.c | 6 ++----\n 1 file changed, 2 insertions(+), 4 deletions(-)\n\ndiff --git a/builtin/gc.c b/builtin/gc.c\nindex 5cd2a43f9f..986b760b30 100644\n--- a/builtin/gc.c\n+++ b/builtin/gc.c\n@@ -1213,10 +1213,8 @@ static struct maintenance_task tasks[] = {\n\n static int compare_tasks_by_selection(const void *a_, const void *b_)\n {\n-\tconst struct maintenance_task *a, *b;\n-\n-\ta = (const struct maintenance_task *)&a_;\n-\tb = (const struct maintenance_task *)&b_;\n+\tconst struct maintenance_task *a = a_;\n+\tconst struct maintenance_task *b = b_;\n\n \treturn b->selected_order - a->selected_order;\n }\n--\n2.29.2\n"},{"id":"410171","messageId":"20201117223011.GA642234@coredump.intra.peff.net","threadId":"54660","inReplyTo":"d80423b7-c89d-99a2-ce49-8308822f7769@web.de","subject":"Re: [PATCH] gc: fix cast in compare_tasks_by_selection()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-11-17T22:30:11Z","receivedAt":"2020-11-17T22:30:34Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Nov 17, 2020 at 10:59:49PM +0100, René Scharfe wrote:\n\n> compare_tasks_by_selection() is used with QSORT and gets passed pointers\n> to the elements of \"static struct maintenance_task tasks[]\".  It casts\n> the *addresses* of these passed pointers to element pointers, though,\n> and thus effectively compares some unrelated values from the stack.  Fix\n> the casts to actually compare array elements.\n> \n> Detected by USan (make SANITIZE=undefined test).\n\nI checked the caller here, and indeed, it's passing an array-of-struct\nso your patch is doing the right thing (not that I doubted it, but\nthat's what review is for).\n\nThis qsort void-pointer convention seems to create a lot of confusion\n(not just the lack of type-safety, but the fact that it's getting a\npointer to the element). I felt like we had a discussion about this a\nwhile ago, and indeed, I found:\n\n  https://lore.kernel.org/git/7b95417a-c8fb-4f1e-cb09-c36804a3a4d0@web.de/\n\nThe whole sub-thread is worth reading, but the macro you arrived at in:\n\n  https://lore.kernel.org/git/c141fb44-904f-e8b6-119f-7d2d6bcfd81a@web.de/\n\nseems pretty reasonable.\n\n-Peff\n"}]}