Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

check ptr against NULL before dereferencing it #6710

Closed
wants to merge 1 commit into from
Closed

Conversation

@kokke
Copy link
Contributor

@kokke kokke commented Mar 9, 2021

This is a proposed fix for #6709

There is a small issue of dereferencing a pointer before checking it against NULL.

From tests/libtest/lib1536.c : 72 - 76

72  if(memcmp(scheme, "HTTP", 5) != 0) {
              ^^^^^^ scheme is dereferenced here, but checked against NULL later
73    fprintf(stderr, "%s:%d scheme of http resource is incorrect; "
74            "expected 'HTTP' but is %s\n",
75            __FILE__, __LINE__,
76            (scheme == NULL ? "NULL" : "invalid"));
77    res = CURLE_HTTP_RETURNED_ERROR;
78    goto test_cleanup;
79  }

I propose to either:

  • add a test in the if-condition, to check against NULL to avoid the potential null-dereference
  • remove the check against NULL in (scheme == NULL ? "NULL" : "invalid") if scheme can never be NULL
@@ -69,7 +69,7 @@ int test(char *URL)
__FILE__, __LINE__, res, curl_easy_strerror(res));
goto test_cleanup;
}
if(memcmp(scheme, "HTTP", 5) != 0) {
if(scheme == NULL || memcmp(scheme, "HTTP", 5) != 0) {
Copy link
Member

@danielgustafsson danielgustafsson Mar 9, 2021

Agreed, makes sense.

@kokke
Copy link
Contributor Author

@kokke kokke commented Mar 9, 2021

Hi @danielgustafsson

Thanks for super fast approval of both my PRs.

I agree the problems "fixed" are benign. I would personally prefer a failed test-case with an error-message instead of a segfault, which is the main effect of this.

@bagder
Copy link
Member

@bagder bagder commented Mar 9, 2021

instead of a segfault, which is the main effect of this.

Is it? This uses the internal printf() code, right? It doesn't segfault on a NULL for %s. It outputs (nil).

@kokke
Copy link
Contributor Author

@kokke kokke commented Mar 9, 2021

Hi @bagder in this case the segfault would be caused by the memcmp at line 72 (as mentioned in the comments) which, to my knowledge, dereferences its arguments.

EDIT:

72  if(memcmp(scheme, "HTTP", 5) != 0) {
              ^^^^^^ scheme is dereferenced here, but checked against NULL later

EDIT2:
Or if it is simply impossible for scheme to be NULL, the check/ternary at line 76 is superfluous.

Copy link
Contributor

@emilengler emilengler left a comment

The C89 standard doesn't define anything for memcmp with a NULL pointer.
See: http://port70.net/~nsz/c/c89/c89-draft.html#4.11.4.1

@bagder
Copy link
Member

@bagder bagder commented Mar 9, 2021

I don't think scheme can be NULL at that point. How would that happen? And if it can, why hasn't that been detected by torture tests?

@jay
Copy link
Member

@jay jay commented Mar 10, 2021

I don't think scheme can be NULL at that point. How would that happen? And if it can, why hasn't that been detected by torture tests?

According to CURLINFO_SCHEME it can return NULL so technically he's correct however internally it may currently be an impossibility that it's NULL after the transfer.

@kokke
Copy link
Contributor Author

@kokke kokke commented Mar 10, 2021

A comment mostly FYI:
My tool searches for places where code is “logically inconsistent”. Here the inconcistency is that memcmp is passed a pointer (which it dereferences) that is later checked against NULL.

The dereference / memcmp-call suggests the programmer meant scheme could never be NULL at this point. So either the NULL-check happens “too late” or is superfluous.

That is my reasoning for the fix proposals

I propose to either:

  • add a test in the if-condition, to check against NULL to avoid the potential null-dereference
  • remove the check against NULL in (scheme == NULL ? "NULL" : "invalid") if scheme can never be NULL

@bagder
Copy link
Member

@bagder bagder commented Mar 10, 2021

@kokke sorry, I didn't mean to question your tool nor your fix, I was just trying to take a step back and think why the NULL check is there in the first place.

@jay: thanks for checking!

@kokke
Copy link
Contributor Author

@kokke kokke commented Mar 10, 2021

@bagder no offense taken at all. I understand if you guys see many questionable change proposals, so I just wanted to make my reasoning explicitly clear.

@bagder bagder closed this in 4088b25 Mar 10, 2021
@bagder
Copy link
Member

@bagder bagder commented Mar 10, 2021

Thanks!

@kokke
Copy link
Contributor Author

@kokke kokke commented Mar 10, 2021

And thank you for kindly accepting all my small change proposals 👍 Contributing has been a very pleasant experience

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

5 participants