{"thread":{"id":"54014","subject":"[PATCH] progress: don't dereference before checking for NULL","startedAt":"2020-08-10T19:48:11Z","lastAt":"2020-08-11T15:41:34Z","messageCount":5,"participants":["Martin Ågren","Eric Sunshine","Junio C Hamano","Taylor Blau"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"403281","messageId":"20200810194748.1483784-1-martin.agren@gmail.com","threadId":"54014","inReplyTo":null,"subject":"[PATCH] progress: don't dereference before checking for NULL","fromName":"Martin Ågren","fromEmail":"martin.agren@gmail.com","sentAt":"2020-08-10T19:47:48Z","receivedAt":"2020-08-10T19:48:11Z","isPatch":true,"sender":{"key":"martin.agren@gmail.com","avatar":null},"body":"In `stop_progress()`, we're careful to check that `p_progress` is\nnon-NULL before we dereference it, but by then we have already\ndereferenced it when calling `finish_if_sparse(*p_progress)`. And, for\nwhat it's worth, we'll go on to blindly dereference it again inside\n`stop_progress_msg()`.\n\nWe could return early if we get a NULL-pointer, but let's go one step\nfurther and BUG instead. The progress API handles NULL just fine, but\nthat's the NULL-ness of `*p_progress`, e.g., when running with\n`--no-progress`. If `p_progress` is NULL, chances are that's a mistake.\nFor symmetry, let's do the same check in `stop_progress_msg()`, too.\n\nSigned-off-by: Martin Ågren \n---\n progress.c | 12 ++++++++++--\n 1 file changed, 10 insertions(+), 2 deletions(-)\n\ndiff --git a/progress.c b/progress.c\nindex 3eda914518..31014e6fca 100644\n--- a/progress.c\n+++ b/progress.c\n@@ -319,9 +319,12 @@ static void finish_if_sparse(struct progress *progress)\n \n void stop_progress(struct progress **p_progress)\n {\n+\tif (!p_progress)\n+\t\tBUG(\"don't provide NULL to stop_progress\");\n+\n \tfinish_if_sparse(*p_progress);\n \n-\tif (p_progress && *p_progress) {\n+\tif (*p_progress) {\n \t\ttrace2_data_intmax(\"progress\", the_repository, \"total_objects\",\n \t\t\t\t (*p_progress)->total);\n \n@@ -338,7 +341,12 @@ void stop_progress(struct progress **p_progress)\n \n void stop_progress_msg(struct progress **p_progress, const char *msg)\n {\n-\tstruct progress *progress = *p_progress;\n+\tstruct progress *progress;\n+\n+\tif (!p_progress)\n+\t\tBUG(\"don't provide NULL to stop_progress_msg\");\n+\n+\tprogress = *p_progress;\n \tif (!progress)\n \t\treturn;\n \t*p_progress = NULL;\n-- \n2.28.0.220.ged08abb693\n\n"},{"id":"403282","messageId":"CAPig+cTsxAW=s1iXcK_-Kn+xiSNCm_u_o_Q2xm-0+a_v4qc5_w@mail.gmail.com","threadId":"54014","inReplyTo":"20200810194748.1483784-1-martin.agren@gmail.com","subject":"Re: [PATCH] progress: don't dereference before checking for NULL","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2020-08-10T21:26:59Z","receivedAt":"2020-08-10T21:27:13Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Mon, Aug 10, 2020 at 3:48 PM Martin Ågren wrote:\n> In `stop_progress()`, we're careful to check that `p_progress` is\n> non-NULL before we dereference it, but by then we have already\n> dereferenced it when calling `finish_if_sparse(*p_progress)`. And, for\n> what it's worth, we'll go on to blindly dereference it again inside\n> `stop_progress_msg()`.\n>\n> We could return early if we get a NULL-pointer, but let's go one step\n> further and BUG instead. The progress API handles NULL just fine, but\n> that's the NULL-ness of `*p_progress`, e.g., when running with\n> `--no-progress`. If `p_progress` is NULL, chances are that's a mistake.\n> For symmetry, let's do the same check in `stop_progress_msg()`, too.\n>\n> Signed-off-by: Martin Ågren \n> ---\n> diff --git a/progress.c b/progress.c\n> @@ -319,9 +319,12 @@ static void finish_if_sparse(struct progress *progress)\n> void stop_progress(struct progress **p_progress)\n> {\n> + if (!p_progress)\n> + BUG(\"don't provide NULL to stop_progress\");\n> +\n> finish_if_sparse(*p_progress);\n\nI'm wondering what this really buys us over simply crashing due to the\nNULL dereference (aside from the slightly more informative diagnostic\nmessage). Either way, it's going to crash, as it should because\npassing NULL is indeed a programmer error for these two functions. I'm\npretty sure that it is more common in this project simply to allow a\nprogrammer error like this simply to crash on its own rather than\nadding code to make it crash explicitly.\n\n> - if (p_progress && *p_progress) {\n> + if (*p_progress) {\n\nIn other words, I think the entire patch can be reduced to just this\nchange here (and a simplified commit message).\n"},{"id":"403306","messageId":"xmqqo8nib1ui.fsf@gitster.c.googlers.com","threadId":"54014","inReplyTo":"20200810194748.1483784-1-martin.agren@gmail.com","subject":"Re: [PATCH] progress: don't dereference before checking for NULL","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-08-10T21:57:57Z","receivedAt":"2020-08-10T21:58:03Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Martin Ågren writes:\n\n> The progress API handles NULL just fine, but\n> that's the NULL-ness of `*p_progress`, e.g., when running with\n> `--no-progress`. If `p_progress` is NULL, chances are that's a mistake.\n\nTrue, true.\n\nThanks.\n"},{"id":"403328","messageId":"CAN0heSotdXH50ssd01b7fFuqEuOs+X0f1Zpj+nhxuO=TRNStUg@mail.gmail.com","threadId":"54014","inReplyTo":"CAPig+cTsxAW=s1iXcK_-Kn+xiSNCm_u_o_Q2xm-0+a_v4qc5_w@mail.gmail.com","subject":"Re: [PATCH] progress: don't dereference before checking for NULL","fromName":"Martin Ågren","fromEmail":"martin.agren@gmail.com","sentAt":"2020-08-11T04:28:30Z","receivedAt":"2020-08-11T04:30:06Z","isPatch":true,"sender":{"key":"martin.agren@gmail.com","avatar":null},"body":"On Mon, 10 Aug 2020 at 23:27, Eric Sunshine wrote:\n>\n> On Mon, Aug 10, 2020 at 3:48 PM Martin Ågren wrote:\n> > void stop_progress(struct progress **p_progress)\n> > {\n> > + if (!p_progress)\n> > + BUG(\"don't provide NULL to stop_progress\");\n> > +\n> > finish_if_sparse(*p_progress);\n>\n> I'm wondering what this really buys us over simply crashing due to the\n> NULL dereference (aside from the slightly more informative diagnostic\n> message). Either way, it's going to crash, as it should because\n> passing NULL is indeed a programmer error for these two functions. I'm\n> pretty sure that it is more common in this project simply to allow a\n> programmer error like this simply to crash on its own rather than\n> adding code to make it crash explicitly.\n\nI'm not a big fan of undefined behavior. In general, I don't buy the\n\"but in practice it will do what we want\" argumentation.\n\nBefore 98a1364740 (\"trace2: log progress time and throughput\",\n2020-05-12), we didn't check for NULL in this function. Then that commit\ntried to do so. It would feel wrong for me to say \"that commit didn't\nget it quite right -- rip out the check\". Then, to be honest, I'd much\nrather just leave it in place. At least that way, someone else might\nspot it a year from now.\n\nI could add an early return (instead of an early BUG). That would\ngracefully handle NULL. Grepping around suggests there are other `if (!p)\nBUG();`. Even Documentation/CodingGuidelines BUGs on a NULL-pointer,\nalthough in the context of checking for NULL (as opposed to \"how to\nBUG\").\n\n> > - if (p_progress && *p_progress) {\n> > + if (*p_progress) {\n>\n> In other words, I think the entire patch can be reduced to just this\n> change here (and a simplified commit message).\n\nI started with this, but then I felt terrible for just sweeping the\nwhole thing under the rug.\n\nMartin\n"},{"id":"403361","messageId":"20200811154128.GE19871@syl.lan","threadId":"54014","inReplyTo":"CAN0heSotdXH50ssd01b7fFuqEuOs+X0f1Zpj+nhxuO=TRNStUg@mail.gmail.com","subject":"Re: [PATCH] progress: don't dereference before checking for NULL","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2020-08-11T15:41:28Z","receivedAt":"2020-08-11T15:41:34Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Tue, Aug 11, 2020 at 06:28:30AM +0200, Martin Ågren wrote:\n> On Mon, 10 Aug 2020 at 23:27, Eric Sunshine wrote:\n> >\n> > On Mon, Aug 10, 2020 at 3:48 PM Martin Ågren wrote:\n> > > void stop_progress(struct progress **p_progress)\n> > > {\n> > > + if (!p_progress)\n> > > + BUG(\"don't provide NULL to stop_progress\");\n> > > +\n> > > finish_if_sparse(*p_progress);\n> >\n> > I'm wondering what this really buys us over simply crashing due to the\n> > NULL dereference (aside from the slightly more informative diagnostic\n> > message). Either way, it's going to crash, as it should because\n> > passing NULL is indeed a programmer error for these two functions. I'm\n> > pretty sure that it is more common in this project simply to allow a\n> > programmer error like this simply to crash on its own rather than\n> > adding code to make it crash explicitly.\n>\n> I'm not a big fan of undefined behavior. In general, I don't buy the\n> \"but in practice it will do what we want\" argumentation.\n\nI think that this is good reasoning; having the guard around\n'p_progress' being non-NULL makes the implementation have no undefined\nbehavior, which is worth a lot.\n\n> Before 98a1364740 (\"trace2: log progress time and throughput\",\n> 2020-05-12), we didn't check for NULL in this function. Then that commit\n> tried to do so. It would feel wrong for me to say \"that commit didn't\n> get it quite right -- rip out the check\". Then, to be honest, I'd much\n> rather just leave it in place. At least that way, someone else might\n> spot it a year from now.\n>\n> I could add an early return (instead of an early BUG). That would\n> gracefully handle NULL. Grepping around suggests there are other `if (!p)\n> BUG();`. Even Documentation/CodingGuidelines BUGs on a NULL-pointer,\n> although in the context of checking for NULL (as opposed to \"how to\n> BUG\").\n>\n> > > - if (p_progress && *p_progress) {\n> > > + if (*p_progress) {\n> >\n> > In other words, I think the entire patch can be reduced to just this\n> > change here (and a simplified commit message).\n>\n> I started with this, but then I felt terrible for just sweeping the\n> whole thing under the rug.\n\nYep, I'm a fan of the direction you ended up taking. Thanks.\n\n Reviewed-by: Taylor Blau \n\n> Martin\n\nThanks,\nTaylor\n"}]}