It'd be really nice if gcc warned about NULL pointer dereferences. e.g. if (foo == NULL && foo->bar) See the attached test case for more examples.
Created attachment 6681 [details] Dereferencing null examples
Created attachment 6682 [details] A bad solution for this bug.
Confirmed.
better patch: http://gcc.gnu.org/ml/gcc-patches/2004-07/msg00423.html
dnovillo reviewed this over irc and said it should be done in fold_stmt. So far, implementing this in fold_stmt has caused false positives.
*** Bug 18854 has been marked as a duplicate of this bug. ***
Bug 18854 deals with *potential* NULL pointer dereferences, while this one mentions only *confirmed* NULL pointer dereferences, i.e: 1 #include <stdlib.h> 2 int main(int argc, char *argv[]) { 3 volatile char *monkey = (char*)malloc(1234); 4 monkey[0] = '\0'; 5 return 0; 6 } Since malloc() on line 3 can *potentially* return NULL, I'd like a warning on line 4. Phython, would you say potential NULL dereferences are within the scope of this bug, or should bug 18854 be re-opened for those cases?
I'd probably leave them as the same bug for now. Neither are being fixed for 4.0 so I don't think it's a problem having one bug.
*** Bug 30368 has been marked as a duplicate of this bug. ***
I wonder if this is ever going to be fixed and if not, if there's any point in being open.
(In reply to comment #10) > I wonder if this is ever going to be fixed and if not, if there's any point in > being open. Like most things in GCC, it will get fixed when someone motivated enough decides to spent the time and effort required to get it fixed. A bug being open means that GCC maintainers are not in principle against the feature, but it doesn't mean that they are ever going to work on it. There are much older bugs than this one that are still open and some of them even get fixed from time to time.
gimple-ssa-isolate-paths.c has the necessary logic to catch a lot of this kind of stuff now. From what I can tell, it would catch everything properly in the attached testcase.
Just adding a quick warning_at at the points where we optimize erroneous uses of NULL I get: j.c: In function 'test1': j.c:10:9: warning: Erroneous NULL pointer use (explicit) [enabled by default] s->bar = 1; /* { dg-warning "null" } */ ^ j.c: In function 'test2': j.c:15:20: warning: Erroneous NULL pointer use (explicit) [enabled by default] if (s == NULL && s->bar > 2) /* { dg-warning "null" } */ ^ j.c: In function 'test3': j.c:23:20: warning: Erroneous NULL pointer use (explicit) [enabled by default] if (s != NULL || s->bar > 2) /* { dg-warning "null" } */ ^ j.c:26:9: warning: Erroneous NULL pointer use (explicit) [enabled by default] s->bar = 3; /* { dg-warning "null" } */ ^ Which is exactly what I would expect.
(In reply to Jeffrey A. Law from comment #13) > Just adding a quick warning_at at the points where we optimize erroneous > uses of NULL I get: Very nice to see progress on this! Do you think this could be in-cooperated in a future version of gcc?
I expect it will be in GCC 4.9. It's a wrap-up item for -fisolate-erroneous-paths.
Created attachment 31928 [details] Work-In-Progress patch
*** Bug 50606 has been marked as a duplicate of this bug. ***
(In reply to Jeffrey A. Law from comment #16) > Created attachment 31928 [details] > Work-In-Progress patch Jeff, what happened with this?
Nobody ever reviewed the changes :(
(In reply to Jeffrey A. Law from comment #19) > Nobody ever reviewed the changes :( If precisely you cannot get someone to review your patches, the lack of manpower in GCC is becoming truly desperate, including the middle-end. In any case, you are a middle-end maintainer. Give a period for last minute comments and commit it? If something breaks, you can always revert it. This is not some major architectural work, it seems quite independent piece.
It's annoying, but I suspect others see this as so low priority as not to care. As for just committing my patch, I could make an argument that I ought to be able to do that, but we (the project) make a conscious decision to avoid that kind of behaviour, except for well defined areas. I'd consider this on the borderline and I believe it's particularly important that I adhere to the policies the project has put in place over time.
There were some comments by Florian: https://gcc.gnu.org/ml/gcc-patches/2014-02/msg00149.html I don't think the patch was ever pinged during stage 1, worth another try...
*PLEASE* put this on higher priority. In 25 years of C development I saw thousands of crashes due to NULL pointer dereferences. Many of them could have been avoided by simply printing a warning. You really save lifes ! And millions of working hours searching for obvious bugs. The requested warning is an absolutely must-have (enabled by default). Seeing this bug open since 2004 is... well ... I have no words. (Don't get me wrong: I make a deep kneefall for gcc and it's developers)
(In reply to Tim Ruehsen from comment #23) > The requested warning is an absolutely must-have (enabled by default). > Seeing this bug open since 2004 is... well ... I have no words. GCC needs lots of more developers. Not "experts" or "hackers", but simply people with some patience and curiosity and basic knowledge about C/C++ and compiling and debugging code in GNU/Linux. We need people that can focus on things like this for a couple of hours every week for months and get it done from start to finish. Paid developers are paid to work on other things (targets, optimizations, extensions) and rarely have the time and focus necessary to get projects like this finished. Volunteers are few and we already have our hands very full with our own little projects. I can give you many examples of old "must-have" bugs that are "easy" to fix, but simply there is no one with enough time and motivation to get them done.
(In reply to Manuel López-Ibáñez from comment #24) > I can give you many examples of old "must-have" bugs that are "easy" to fix, > but simply there is no one with enough time and motivation to get them done. It would be interesting to have that list. Or just those on the top of your head. I might not necessarily look at them now but I know I will have some time in the near future to work on a few of these, so it would be interesting to have this list so I can look at them. Thanks.
(In reply to pmatos from comment #25) > (In reply to Manuel López-Ibáñez from comment #24) > > I can give you many examples of old "must-have" bugs that are "easy" to fix, > > but simply there is no one with enough time and motivation to get them done. > > It would be interesting to have that list. Or just those on the top of your > head. I might not necessarily look at them now but I know I will have some > time in the near future to work on a few of these, so it would be > interesting to have this list so I can look at them. A good place to start is https://gcc.gnu.org/bugzilla/buglist.cgi?keywords=easyhack&list_id=116934&order=bug_id&query_format=advanced In particular, PR19808 should be fairly easy and there is some work already done. It is also a prerequisite for PR2972. PR7652 has an initial patch. PR23087 is very straightforward. PR17896 has a patch already. PR99 is the oldest bug in my list and it is still a mystery. It needs proper debugging to understand what goes wrong. Of course, there are also quite a few very old bugs where the fix is not so straight-forward but the impact would be very high: PR19430 (do the warning in the FE?) and PR18501 (the most common Wuninitialized bug).
(In reply to Manuel López-Ibáñez from comment #26) > A good place to start is > https://gcc.gnu.org/bugzilla/buglist. > cgi?keywords=easyhack&list_id=116934&order=bug_id&query_format=advanced > Thanks for the input. I will take a look.
I far as I can read, not a patch is missing. A review + commit is missing. How can you ask for more developers (=patches) when the work is ignored ? Don't get me wrong, I just try to understand how this should work.
(In reply to Tim Ruehsen from comment #28) > I far as I can read, not a patch is missing. A review + commit is missing. > How can you ask for more developers (=patches) when the work is ignored ? > Don't get me wrong, I just try to understand how this should work. I'm actually not asking for more patches. As you can see, many of those PRs already have patches. People send patches all the time. Sometimes very large ones, and many times they never get committed, because they do not ping them, or they do not address the reviewers' comments, or simply they get bored and move to something else. What we need is people that are "patient and motivated" to get their code committed to the upstream repository. Unfortunately, getting reviews is hard because there are few developers and they are busy, and new developers often fail to insist because they feel their work is being ignored or they do not have time to follow-up and do the changes asked by the reviewers. It is a vicious circle that I do not know how to solve. As a volunteer, I know a patch of mine may take weeks/months to get reviewed. I simply keep pinging (see https://gcc.gnu.org/wiki/Community for tips on how to do this effectively). If I have free time, I work on something else; if not, well, then it doesn't matter that it did not get reviewed yet. I am not in a hurry. I don't have a deadline or a boss to present results. It is better that it gets fixed in a year or two than never. For example, it took many years to get something as simple as coloured output for diagnostics, but it finally happened. This was not because the core GCC devs had a meeting and said "Oh, Clang is kicking our ass with their colors, let's make this a priority". No, it took one volunteer insisting during many months, revisions of patches and compromises. It did not actually take a lot of coding time in total, but it was a process spread across many months.
Hello I offer a bounty on getting this change incorporated into GCC. 150 USD. Email me at for a confirmation. This is the kind of feature I would love to see in GCC :-) Jon
Author: manu Date: Wed Aug 5 17:36:29 2015 New Revision: 226640 URL: https://gcc.gnu.org/viewcvs?rev=226640&root=gcc&view=rev Log: gcc/ChangeLog: 2015-08-05 Manuel López-Ibáñez <manu@gcc.gnu.org> Jeff Law <law@redhat.com> PR c/16351 * doc/invoke.texi (Wnull-dereference): New. * tree-vrp.c (infer_value_range): Update call to infer_nonnull_range. * gimple-ssa-isolate-paths.c (find_implicit_erroneous_behaviour): Warn for potential NULL dereferences. (find_explicit_erroneous_behaviour): Warn for NULL dereferences. * ubsan.c (instrument_nonnull_arg): Call infer_nonnull_range_by_attribute. (instrument_nonnull_return): Likewise. * common.opt (Wnull-dereference); New. * gimple.c (infer_nonnull_range): Remove bool arguments. (infer_nonnull_range_by_dereference): New. (infer_nonnull_range_by_attribute): New. * gimple.h: Update declarations. gcc/testsuite/ChangeLog: 2015-08-05 Manuel López-Ibáñez <manu@gcc.gnu.org> Jeff Law <law@redhat.com> PR c/16351 * gcc.dg/tree-ssa/isolate-2.c: Close comment. * gcc.dg/tree-ssa/isolate-4.c: Likewise. * gcc.dg/tree-ssa/wnull-dereference.c: New test. * gcc.dg/tree-ssa/isolate-1.c: Test warnings with -Wnull-dereference. * gcc.dg/tree-ssa/isolate-3.c: Likewise. * gcc.dg/tree-ssa/isolate-5.c: Likewise. Added: trunk/gcc/testsuite/gcc.dg/tree-ssa/wnull-dereference.c Modified: trunk/gcc/ChangeLog trunk/gcc/common.opt trunk/gcc/doc/invoke.texi trunk/gcc/gimple-ssa-isolate-paths.c trunk/gcc/gimple.c trunk/gcc/gimple.h trunk/gcc/testsuite/ChangeLog trunk/gcc/testsuite/gcc.dg/tree-ssa/isolate-1.c trunk/gcc/testsuite/gcc.dg/tree-ssa/isolate-2.c trunk/gcc/testsuite/gcc.dg/tree-ssa/isolate-3.c trunk/gcc/testsuite/gcc.dg/tree-ssa/isolate-4.c trunk/gcc/testsuite/gcc.dg/tree-ssa/isolate-5.c trunk/gcc/tree-vrp.c trunk/gcc/ubsan.c
The NULL dereference warnings originally requested should be working in GCC 6 (unless we find some issue and the patch gets reverted). However, the original patch by Jeff also used the nonnull attribute to give even more warnings, thus I'm leaving this open for now.
I found that using -g -O2 -Wall didn't enable this warning. Some of the documentation says it does. I can see that this new warning isn't ready for prime time yet.
(In reply to David Binderman from comment #33) > I found that using -g -O2 -Wall didn't enable this warning. > Some of the documentation says it does. Ops, you are right. I used the wrong EnabledBy() instead of LangEnabledBy(). I see now that are other options suffering from the same problem. I'll produce a patch.
(In reply to Manuel López-Ibáñez from comment #34) > (In reply to David Binderman from comment #33) > > I found that using -g -O2 -Wall didn't enable this warning. > > Some of the documentation says it does. Enabling this option with -Wall or -Wextra triggers errors in gcc bootstrap: /home/manuel/test1/226719M/build/./prev-gcc/xg++ -B/home/manuel/test1/226719M/build/./prev-gcc/ -B/home/manuel/test1/./226719M/install/x86_64-pc-linux-gnu/bin/ -nostdinc++ -B/home/manuel/test1/226719M/build/prev-x86_64-pc-linux-gnu/libstdc++-v3/src/.libs -B/home/manuel/test1/226719M/build/prev-x86_64-pc-linux-gnu/libstdc++-v3/libsupc++/.libs -I/home/manuel/test1/226719M/build/prev-x86_64-pc-linux-gnu/libstdc++-v3/include/x86_64-pc-linux-gnu -I/home/manuel/test1/226719M/build/prev-x86_64-pc-linux-gnu/libstdc++-v3/include -I/home/manuel/test1/src/libstdc++-v3/libsupc++ -L/home/manuel/test1/226719M/build/prev-x86_64-pc-linux-gnu/libstdc++-v3/src/.libs -L/home/manuel/test1/226719M/build/prev-x86_64-pc-linux-gnu/libstdc++-v3/libsupc++/.libs -I/home/manuel/test1/src/libcpp -I. -I/home/manuel/test1/src/libcpp/../include -I/home/manuel/test1/src/libcpp/include -g -O2 -gtoggle -W -Wall -Wno-narrowing -Wwrite-strings -Wmissing-format-attribute -pedantic -Wno-long-long -Werror -fno-exceptions -fno-rtti -I/home/manuel/test1/src/libcpp -I. -I/home/manuel/test1/src/libcpp/../include -I/home/manuel/test1/src/libcpp/include -c -o line-map.o -MT line-map.o -MMD -MP -MF .deps/line-map.Tpo /home/manuel/test1/src/libcpp/line-map.c -fdump-tree-isolate-paths-all-lineno In file included from /home/manuel/test1/src/libcpp/line-map.c:24:0: /home/manuel/test1/src/libcpp/include/line-map.h: In function ‘const line_map* linemap_add(line_maps*, lc_reason, unsigned int, const char*, linenum_type)’: /home/manuel/test1/src/libcpp/include/line-map.h:927:60: error: potential null pointer dereference [-Werror=null-dereference] : LINEMAPS_ORDINARY_MAP_AT (set, ord_map->included_from)); ^ /home/manuel/test1/src/libcpp/line-map.c: In function ‘source_location linemap_line_start(line_maps*, linenum_type, unsigned int)’: /home/manuel/test1/src/libcpp/line-map.c:591:18: error: potential null pointer dereference [-Werror=null-dereference] (linemap_add (set, LC_RENAME, ^ cc1plus: error: potential null pointer dereference [-Werror=null-dereference] /home/manuel/test1/src/libcpp/line-map.c:595:37: error: potential null pointer dereference [-Werror=null-dereference] map->column_bits = column_bits; ^ cc1plus: all warnings being treated as errors The reason is code like: map = linemap_check_ordinary (const_cast <line_map *> (linemap_add (set, LC_RENAME, ORDINARY_MAP_IN_SYSTEM_HEADER_P (map), ORDINARY_MAP_FILE_NAME (map), to_line))); map->column_bits = column_bits; where linemap_check_ordinary is inlined as: inline line_map_ordinary * linemap_check_ordinary (struct line_map *map) { linemap_assert (!map || map->reason != LC_ENTER_MACRO); return (line_map_ordinary *)map; } which means that there is a path through which a null pointer could be potentially dereferenced. However, this actually cannot happen because linemap_add will not return NULL in this case. I fear this case might be quite common and lead to many false positives that are then quite hard to understand due to the poor location info in the middle-end.
But those are *precisely* the ones that need deep investigation. That investigation may find a real bug, it may find a relatively simple missed optimization, complex missed optimizations or poor APIs. It's no different maybe uninitialized warnings. Note that in this case, the compiler should be determining that MAP can't be NULL and thus linemap_check_ordinary can't return NULL and propagating that information. So if we're still getting a warning, then something is wrong somewhere.
(In reply to Jeffrey A. Law from comment #36) > It's no different maybe uninitialized warnings. Well, one can silence uninitialized warnings by simply initializing the variable. I don't see how this code can be refactored to silence the warning (and the way I understood the warning is by looking at the GCC dumps, which people will not do). > Note that in this case, the compiler should be determining that MAP can't be > NULL and thus linemap_check_ordinary can't return NULL and propagating that > information. So if we're still getting a warning, then something is wrong > somewhere. But how it can do such a thing if the definition of linemap_add were not available? (In this case it is, but GCC's IPA is not smart enough to figure it that it cannot return NULL. The code injected by -f-isolate-paths survives until the end). In any case, I don't see how to keep this in -Wall or -Wextra if it breaks GCC bootstrap. And options that are not enabled by either of those are rarely used and tend to bit-rot or receive little attention. Hum, perhaps, we do really need some kind of -Weverything that includes not every warning, but those that are potentially useful but too noisy to not break GCC bootstrap.
(In reply to Manuel López-Ibáñez from comment #35) > I fear this case might be quite common and lead to many false positives that > are then quite hard to understand due to the poor location info in the > middle-end. I tried out the new warning flag by compiling the current Linux kernel and see something similar. No definately NULL pointers found, which is good news, but also about 60 messages about potentially NULL pointers. I checked a few of the 60 and couldn't see anything wrong. I didn't bother checking the rest. Suggest don't bother emitting warnings about potentially NULL pointers and keep the new code about definately NULL pointers. It could even be the case that the "potential NULL" code gets put into some backwater flag away from -Wall, and, after a suitable period of more testing, the "definately NULL" code gets to go into prime time of -Wall. Meanwhile, I'll be ignoring the warnings about potentially NULL.
(In reply to David Binderman from comment #38) > It could even be the case that the "potential NULL" code gets > put into some backwater flag away from -Wall, and, after a suitable period > of more testing, the "definately NULL" code gets to go into > prime time of -Wall. We could split the flag into -Wnull-dereference and -Wmaybe-null-dereference.
(In reply to Manuel López-Ibáñez from comment #35) <snip> > which means that there is a path through which a null pointer could be > potentially dereferenced. However, this actually cannot happen because > linemap_add will not return NULL in this case. > > I fear this case might be quite common and lead to many false positives that > are then quite hard to understand due to the poor location info in the > middle-end. I am sure this is very common.. we use static analysis, and there will be many potentially dereferences.. the thing is, the implementation of linemap_add may be changed.. so those calling linemap_add should always check result is non-NULL before using right? (then the calling code can handle gracefully)
Actually I think we want the concept of never returns NULL, both as an attribute and as a property the compiler can discover by analysis. Given that property on the return value, it can be propagated into call sites.
(In reply to Jeffrey A. Law from comment #41) > Actually I think we want the concept of never returns NULL, both as an > attribute and as a property the compiler can discover by analysis. Given > that property on the return value, it can be propagated into call sites. ++ That sounds like very useful feature.
(In reply to Tim Ruehsen from comment #42) > (In reply to Jeffrey A. Law from comment #41) > > Actually I think we want the concept of never returns NULL, both as an > > attribute and as a property the compiler can discover by analysis. Given > > that property on the return value, it can be propagated into call sites. > > ++ That sounds like very useful feature. We have returns_nonnull already.
Duh, I should have remembered that we had returns_nonnull because I wrote a dataflow pass to find any NULLs that flow to such statements :-) The pieces that are missing are tagging functions with that attribute when we can prove they never return NULL. We'll be relying a lot of IPA to present functions in the right order, but it'd be a good improvement.
(In reply to Manuel López-Ibáñez from comment #39) > (In reply to David Binderman from comment #38) > > It could even be the case that the "potential NULL" code gets > > put into some backwater flag away from -Wall, and, after a suitable period > > of more testing, the "definately NULL" code gets to go into > > prime time of -Wall. > > We could split the flag into -Wnull-dereference and -Wmaybe-null-dereference. Sounds good. I'm keen to see it in :)
*** Bug 78914 has been marked as a duplicate of this bug. ***
I'm happy to pay a 400 USD bug bounty to contributors who introduce this feature in GCC. Let's get this feature in! :)
Send a check to Manuel ;) The feature is available under the -Wnull-dereference option (see comment 31). Unfortunately, due to false positives, the option was removed from -Wall in a subsequent commit (r226751) and has to be enabled explicitly. The similar -Wnonnull option that was just recently enhanced to detect related problems is in -Wall, and it seems to me that it would make sense to treat both of these options the same: i.e., enable both with -Wall. To do that, the former option might need to be tweaked to reduce its false positive rate. At the same time, the -Wnonnull option still has a high rate of false negatives and so it could stand to be enhanced to do a better job. The challenge is striking the right balance between the two rates that is acceptable to everyone.
(In reply to Martin Sebor from comment #48) > Send a check to Manuel ;) The feature is available under the > -Wnull-dereference option (see comment 31). Unfortunately, due to false > positives, the option was removed from -Wall in a subsequent commit > (r226751) and has to be enabled explicitly. > > The similar -Wnonnull option that was just recently enhanced to detect > related problems is in -Wall, and it seems to me that it would make sense to > treat both of these options the same: i.e., enable both with -Wall. To do > that, the former option might need to be tweaked to reduce its false > positive rate. At the same time, the -Wnonnull option still has a high rate > of false negatives and so it could stand to be enhanced to do a better job. > The challenge is striking the right balance between the two rates that is > acceptable to everyone. Thank you for your reply. -Werror=null-dereference doesn't seem to be in my compiler: gcc (Ubuntu 5.4.0-6ubuntu1~16.04.4) 5.4.0 20160609 Is it just in GCC6 branch? -Wnonnull is present though. Re the feature, I think it can't be that hard to check per function can it...?
Yes, -Wnull-dereference is only in GCC 6 and later. -Wnonnull is in 5 and prior but it does only a superficial job of checking (it only detects null pointer constants). in GCC 7, -Wnonnull does a better job (but it's still far from perfect). Null pointer checking (and everything else) is always done per function. The challenge isn't in implementing it but rather in striking the right balance between the stages of compilation at which the checking is done. When done early early we may miss instances that could be exposed by later stages (optimizations). When done late we start flagging instances introduced by the earlier transformations that didn't appear in the original source code (and that may never be executed). It's a delicate balance between false negatives and false positives.
(In reply to Martin Sebor from comment #50) > Yes, -Wnull-dereference is only in GCC 6 and later. -Wnonnull is in 5 and > prior but it does only a superficial job of checking (it only detects null > pointer constants). in GCC 7, -Wnonnull does a better job (but it's still > far from perfect). This option is not for C ? gcc 6.3.0 with options '-Q --help=warnings,C' doesn't print it out. But '-Q --help=warnings' does. Correct or missing (and if missing what else is missing with C !?).
-Wnull-dereference is a language-independent option (it works for C, C++, and should work for all other languages). But this bug isn't the right forum to discuss general GCC usage questions (such as why -Q --help=warnings,C doesn't print some C language options). Please either raise those on the gcc-help list or open new bugs for suspected problems.
Carrying over the test case from PR 79572: #include <iostream> void f(const int &iref) { if (&iref) std::cout << iref << std::endl; else std::cout << "iref is NULL" << std::endl; } int main () { f(*((int*) NULL)); return 0; } This dereferences a NULL pointer in a very explicit fashion, but is not being caught by -Wnull-dereference, neither with 6.2 nor with current 7-trunk.
(In reply to janus from comment #53) Unfortunately, it isn't. The warning depends on actually dereferencing the null pointer (i.e., trying to access the object it points to) and passing the argument in the call to f(*((int*) NULL)) doesn't do that. The pointer is derefernced later, in f, but the -Wnull-dereference checker doesn't see it because the call to f() isn't inlined by the time it runs (or ever). To issue the warning in this case the check also needs to happen when binding to references.
Related: bug 71157, bug 84315, and bug 84316 (should this be a meta-bug for all issues with -Wnull-dereference? In which case I should be putting those under "Depends on" instead of "See Also"...)
I'd say any warning option with more than just a handful of pr's against it would benefit from having a meta-bug.
(In reply to Martin Sebor from comment #56) > I'd say any warning option with more than just a handful of pr's against it > would benefit from having a meta-bug. On second thought I think actually a meta-bug for -Wnull-dereference should be a new, separate bug. There's a large cc-list for this bug and making it a meta-bug would result in everyone on it getting way more emails than they signed up for. So, with a new, separate meta-bug for remaining issues with -Wnull-dereference, would it be okay to close this one?
It's fine with me, just as long as we don't lose track of any outstanding bugs.
(In reply to Martin Sebor from comment #58) > It's fine with me, just as long as we don't lose track of any outstanding > bugs. The meta-bug is now bug 86172; closing this one.