gustavonihei commented on pull request #695: URL: https://github.com/apache/incubator-nuttx-apps/pull/695#issuecomment-840591927
> > Well, I was referring to local variables that are about to go out-of-scope, that's why I mentioned the extended lifetime for the other cases where the value does matter. > > If a local variable goes out-of-scope (i.e. its lifetime is over), it cannot be referenced anymore, so its pointer value is harmless. But I won't bother if you still want to nullify it. > > buggy software can happily access the value by reading an uninitialized variable. We are not dealing with "uninitialized variables" here. And no, the compiler will happily emit an error if the software tries to reference a variable that went out-of-scope. > anyway, i'm not a fan of nullify. > You are completely free to have your personal preferences, but for employing them on an open-source project you are subject to a review process. And so far, on my judgement, your argumentation is not reasonable. > > > > > i pushed a counter-proposal to the defensive style. [219356c](https://github.com/apache/incubator-nuttx-apps/commit/219356ce7d1b8a26140c98d303d493c6f04b19e9) > > > > > > > > > > > > Good, this commit may be a proof that this code is not **currently** buggy. But that's not the issue I raised. > > > > Now, here it is a simple unit test for making this potential bug evident: > > > > https://godbolt.org/z/f4dzW8M44 > > > > > > > > > the unit test is just broken. > > > it doesn't seem to support your argument. > > > > > > Guess why it is broken? > > ``` > > Program stderr > > free(): double free detected in tcache 2 > > ``` > > > > > > > > > > > > > > > > > > > > > > > > > > I rest my case. > > it's something like having a unit test like the following and claiming that free() has a problem. > it doesn't make much sense. > > ``` > p = malloc(1); > free(p); > free(p); > ``` It's completely different. If your comparison is based on the fact that I've called the `webclient_abort` twice, feel free to create another test that calls `webclient_perform` after a single `webclient_abort`. The check here https://github.com/apache/incubator-nuttx-apps/blob/02bf1583d55cc8d0161866ee40c86cee9b97af46/netutils/webclient/webclient.c#L696 will not pass because you did not nullify `ctx->ws` and from https://github.com/apache/incubator-nuttx-apps/blob/02bf1583d55cc8d0161866ee40c86cee9b97af46/netutils/webclient/webclient.c#L723 onwards is undefined behavior because `ctx->ws` points to an already freed memory region. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: [email protected]
