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

lib1537.c : check ptr against NULL before dereferencing it #6707

Closed
kokke opened this issue Mar 9, 2021 · 10 comments
Closed

lib1537.c : check ptr against NULL before dereferencing it #6707

kokke opened this issue Mar 9, 2021 · 10 comments
Labels

Comments

@kokke
Copy link
Contributor

@kokke kokke commented Mar 9, 2021

I did this

Hi guys,

I am testing a homemade static analysis tool on a few code bases.
I checked out your master-branch and ran my tool and fell over this curiosity, which I think is indicative of a bug.

From tests/libtest/lib1537.c : 49-51

49  ptr = curl_escape((char *)a, asize);
50  printf("%s\n", ptr);
           ^^^^^^^^^^^  'ptr' gets dereferenced here, before the NULL-check
51  if(!ptr) {
52    res = TEST_ERR_MAJOR_BAD;
53    goto test_cleanup;
54  }
@kokke kokke changed the title test1537.c : check ptr against NULL before dereferencing it lib1537.c : check ptr against NULL before dereferencing it Mar 9, 2021
@gvanem
Copy link
Contributor

@gvanem gvanem commented Mar 9, 2021

What is wrong with printing a (null)?

@kokke
Copy link
Contributor Author

@kokke kokke commented Mar 9, 2021

@gvanem AFAIK not all implementations of printf output (null) given printf("%s", 0).

As far as I am aware, it is undefined behavior (not even just implementation-defined). See e.g. https://trust-in-soft.com/blog/2019/07/09/printing-a-null-pointer-with-s-is-undefined-behavior/

EDIT:

From https://www.gnu.org/software/libc/manual/html_node/Other-Output-Conversions.html

If you accidentally pass a null pointer as the argument for a ‘%s’ conversion, the GNU C Library prints it as ‘(null)’. We think this is more useful than crashing. But it’s not good practice to pass a null argument intentionally.

That was my rationale. Some implementations of printf would crash during test instead of giving a nice error-message 🤷‍♂️

@bagder
Copy link
Member

@bagder bagder commented Mar 9, 2021

But

  1. we don't use those implementations
  2. we can even test this: ./runtests.pl -t 1537

@kokke
Copy link
Contributor Author

@kokke kokke commented Mar 9, 2021

Then it seem the issue is non-problematic for you. Thanks for quick response :)

@jay jay added the tests label Mar 10, 2021
@jay
Copy link
Member

@jay jay commented Mar 10, 2021

@bagder
Copy link
Member

@bagder bagder commented Mar 10, 2021

@jay what do you mean? Are you suggesting we build without using our own implementation at times?

@kokke
Copy link
Contributor Author

@kokke kokke commented Mar 10, 2021

@bagder is right that many printf implementations work around this problem (because it commonly occurs in code) but it is technically undefined behavior.

If you dont care much about UB in test code, that is understandable as well.

@bagder
Copy link
Member

@bagder bagder commented Mar 10, 2021

If we use our printf() implementation where we deal with this perfectly fine (to my knowledge). How can that be UB?

@kokke
Copy link
Contributor Author

@kokke kokke commented Mar 10, 2021

@bagder do you use your own printf implementation and not the built-in from cstdlib? If so there is no UB and the issue should be closed. The standard specifies UB for the printf-implementation from the standard library.

My tool thinks all calls to printf invoke the standard-implementation. That is often a sound assumption; but apologies for wasting time and energy if that is not the case here :)

@jay
Copy link
Member

@jay jay commented Mar 10, 2021

@jay what do you mean? Are you suggesting we build without using our own implementation at times?

I forgot about that, withdrawn!

@bagder bagder closed this in b2d9067 Mar 10, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
4 participants