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.

Sign up for GitHub

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

Jump to bottom

check ptr against NULL before dereferencing it #6710

Closed
wants to merge 1 commit into from
Closed

check ptr against NULL before dereferencing it #6710

wants to merge 1 commit into from

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

danielgustafsson
danielgustafsson approved these changes Mar 9, 2021
View reviewed changes
tests/libtest/lib1536.c
@@ -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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed, makes sense.

@kokke
Copy link
Contributor Author

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 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 commented Mar 9, 2021 •
edited

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.

emilengler
emilengler approved these changes Mar 9, 2021
View reviewed changes
Copy link
Contributor

@emilengler emilengler left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 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 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 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 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 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 commented Mar 10, 2021

Thanks!

@kokke
Copy link
Contributor Author

kokke commented Mar 10, 2021

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

@jay jay mentioned this pull request Mar 11, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@danielgustafsson danielgustafsson danielgustafsson approved these changes

@emilengler emilengler emilengler approved these changes

Assignees
No one assigned
Labels
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@kokke @bagder @jay @danielgustafsson @emilengler