Opened 6 months ago

Closed 4 months ago

Last modified 4 months ago

#26269 closed defect (fixed)

new compiler warning src/or/router.c:2034:36: warning: potential null pointer dereference

Reported by: toralf Owned by: nickm
Priority: Medium Milestone: Tor: 0.3.4.x-final
Component: Core Tor/Tor Version: Tor: 0.3.4.1-alpha
Severity: Normal Keywords: 034-must fast-fix
Cc: Actual Points:
Parent ID: Points:
Reviewer: asn Sponsor:

Description

CC       src/or/rephist.o
  CC       src/or/router.o
src/or/router.c: In function ‘router_my_exit_policy_is_reject_star’:
src/or/router.c:2034:36: warning: potential null pointer dereference [-Wnull-dereference]
   return router_get_my_routerinfo()->policy_is_reject_star;
          ~~~~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~
src/or/router.c:2034:36: warning: potential null pointer dereference [-Wnull-dereference]
src/or/router.c:2034:36: warning: potential null pointer dereference [-Wnull-dereference]
src/or/router.c: In function ‘check_descriptor_bandwidth_changed’:
src/or/router.c:2636:36: warning: potential null pointer dereference [-Wnull-dereference]
   prev = router_get_my_routerinfo()->bandwidthcapacity;
          ~~~~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~
src/or/router.c:2636:36: warning: potential null pointer dereference [-Wnull-dereference]
src/or/router.c:2636:36: warning: potential null pointer dereference [-Wnull-dereference]
src/or/router.c: In function ‘check_descriptor_ipaddress_changed’:
src/or/router.c:2696:8: warning: potential null pointer dereference [-Wnull-dereference]
   prev = router_get_my_routerinfo()->addr;
   ~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
src/or/router.c:2696:8: warning: potential null pointer dereference [-Wnull-dereference]
src/or/router.c:2696:8: warning: potential null pointer dereference [-Wnull-dereference]
  CC       src/or/routerlist.o
  CC       src/or/dirauth/shared_random.o

and

  CC       src/test/src_test_test-test_dos.o
  CC       src/test/src_test_test-test_entrynodes.o
src/test/test_entrynodes.c: In function ‘test_entry_guard_update_from_consensus_repair’:
src/test/test_entrynodes.c:1254:26: warning: potential null pointer dereference [-Wnull-dereference]
     n->is_possible_guard = 0;
     ~~~~~~~~~~~~~~~~~~~~~^~~
src/test/test_entrynodes.c:1254:26: warning: potential null pointer dereference [-Wnull-dereference]
src/test/test_entrynodes.c: In function ‘test_entry_guard_update_from_consensus_remove’:
src/test/test_entrynodes.c:1319:26: warning: potential null pointer dereference [-Wnull-dereference]
     n->is_possible_guard = 0;
     ~~~~~~~~~~~~~~~~~~~~~^~~
src/test/test_entrynodes.c:1319:26: warning: potential null pointer dereference [-Wnull-dereference]
src/test/test_entrynodes.c:1328:26: warning: potential null pointer dereference [-Wnull-dereference]
     n->is_possible_guard = 0;
     ~~~~~~~~~~~~~~~~~~~~~^~~
src/test/test_entrynodes.c:1328:26: warning: potential null pointer dereference [-Wnull-dereference]
src/test/test_entrynodes.c: In function ‘test_entry_guard_update_from_consensus_status’:
src/test/test_entrynodes.c:1153:26: warning: potential null pointer dereference [-Wnull-dereference]
     n->is_possible_guard = 0;
     ~~~~~~~~~~~~~~~~~~~~~^~~
src/test/test_entrynodes.c:1153:26: warning: potential null pointer dereference [-Wnull-dereference]
src/test/test_entrynodes.c:1191:26: warning: potential null pointer dereference [-Wnull-dereference]
     n->is_possible_guard = 1;
     ~~~~~~~~~~~~~~~~~~~~~^~~
src/test/test_entrynodes.c:1191:26: warning: potential null pointer dereference [-Wnull-dereference]
In file included from ./src/common/crypto.h:21:0,
                 from ./src/or/or.h:69,
                 from src/test/test_entrynodes.c:13:
src/test/test_entrynodes.c:85:13: warning: potential null pointer dereference [-Wnull-dereference]
   tor_free(n->md->onion_curve25519_pkey);
            ~^~~
./src/common/util.h:90:39: note: in definition of macro ‘tor_free’
     typeof(&(p)) tor_free__tmpvar = &(p);                      \
                                       ^
./src/common/util.h:118:21: warning: potential null pointer dereference [-Wnull-dereference]
 #define raw_free    free
                     ^
./src/common/util.h:91:5: note: in expansion of macro ‘raw_free’
     raw_free(*tor_free__tmpvar);                               \
     ^~~~~~~~
src/test/test_entrynodes.c:84:3: note: in expansion of macro ‘tor_free’
   tor_free(n->rs);
   ^~~~~~~~
./src/common/util.h:118:21: warning: potential null pointer dereference [-Wnull-dereference]
 #define raw_free    free
                     ^
./src/common/util.h:91:5: note: in expansion of macro ‘raw_free’
     raw_free(*tor_free__tmpvar);                               \
     ^~~~~~~~
src/test/test_entrynodes.c:87:3: note: in expansion of macro ‘tor_free’
   tor_free(n->md);
   ^~~~~~~~
src/test/test_entrynodes.c:86:22: warning: potential null pointer dereference [-Wnull-dereference]
   short_policy_free(n->md->exit_policy);
                     ~^~~
./src/common/util.h:129:45: note: in definition of macro ‘FREE_AND_NULL’
     typename **tmp__free__ptr ## freefn = &(var);                       \
                                             ^~~
src/test/test_entrynodes.c:86:3: note: in expansion of macro ‘short_policy_free’
   short_policy_free(n->md->exit_policy);
   ^~~~~~~~~~~~~~~~~
./src/common/util.h:92:22: warning: potential null pointer dereference [-Wnull-dereference]
     *tor_free__tmpvar=NULL;                                    \
                      ^
src/test/test_entrynodes.c:84:3: note: in expansion of macro ‘tor_free’
   tor_free(n->rs);
   ^~~~~~~~
src/test/test_entrynodes.c:85:13: warning: potential null pointer dereference [-Wnull-dereference]
   tor_free(n->md->onion_curve25519_pkey);
            ~^~~
./src/common/util.h:90:39: note: in definition of macro ‘tor_free’
     typeof(&(p)) tor_free__tmpvar = &(p);                      \
                                       ^
./src/common/util.h:118:21: warning: potential null pointer dereference [-Wnull-dereference]
 #define raw_free    free
                     ^
./src/common/util.h:91:5: note: in expansion of macro ‘raw_free’
     raw_free(*tor_free__tmpvar);                               \
     ^~~~~~~~
src/test/test_entrynodes.c:84:3: note: in expansion of macro ‘tor_free’
   tor_free(n->rs);
   ^~~~~~~~
./src/common/util.h:118:21: warning: potential null pointer dereference [-Wnull-dereference]
 #define raw_free    free
                     ^
./src/common/util.h:91:5: note: in expansion of macro ‘raw_free’
     raw_free(*tor_free__tmpvar);                               \
     ^~~~~~~~
src/test/test_entrynodes.c:87:3: note: in expansion of macro ‘tor_free’
   tor_free(n->md);
   ^~~~~~~~
src/test/test_entrynodes.c:86:22: warning: potential null pointer dereference [-Wnull-dereference]
   short_policy_free(n->md->exit_policy);
                     ~^~~
./src/common/util.h:129:45: note: in definition of macro ‘FREE_AND_NULL’
     typename **tmp__free__ptr ## freefn = &(var);                       \
                                             ^~~
src/test/test_entrynodes.c:86:3: note: in expansion of macro ‘short_policy_free’
   short_policy_free(n->md->exit_policy);
   ^~~~~~~~~~~~~~~~~
./src/common/util.h:92:22: warning: potential null pointer dereference [-Wnull-dereference]
     *tor_free__tmpvar=NULL;                                    \
                      ^
src/test/test_entrynodes.c:84:3: note: in expansion of macro ‘tor_free’
   tor_free(n->rs);
   ^~~~~~~~
  CC       src/test/src_test_test-test_extorport.o
  CC       src/test/src_test_test-test_hs_common.o
  CC       src/test/src_test_test-test_hs_cell.o
  CC       src/test/src_test_test-test_hs_service.o

at a stable Gentoo hardened Linux with app-forensics/afl 2.52b and gcc (Gentoo Hardened 6.4.0-r1 p1.3) 6.4.0

Child Tickets

Change History (10)

comment:1 Changed 6 months ago by toralf

FWIW tor-0.3.4.1-alpha-65-g9f884a38e

comment:2 Changed 6 months ago by teor

Keywords: 034-must added
Milestone: Tor: 0.3.4.x-final
Version: Tor: unspecifiedTor: 0.3.4.1-alpha

comment:3 in reply to:  description ; Changed 6 months ago by sysrqb

Replying to toralf:

CC       src/or/rephist.o
  CC       src/or/router.o
src/or/router.c: In function ‘router_my_exit_policy_is_reject_star’:
src/or/router.c:2034:36: warning: potential null pointer dereference [-Wnull-dereference]
   return router_get_my_routerinfo()->policy_is_reject_star;
          ~~~~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~
src/or/router.c:2034:36: warning: potential null pointer dereference [-Wnull-dereference]
src/or/router.c:2034:36: warning: potential null pointer dereference [-Wnull-dereference]
src/or/router.c: In function ‘check_descriptor_bandwidth_changed’:
src/or/router.c:2636:36: warning: potential null pointer dereference [-Wnull-dereference]
   prev = router_get_my_routerinfo()->bandwidthcapacity;
          ~~~~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~
src/or/router.c:2636:36: warning: potential null pointer dereference [-Wnull-dereference]
src/or/router.c:2636:36: warning: potential null pointer dereference [-Wnull-dereference]
src/or/router.c: In function ‘check_descriptor_ipaddress_changed’:
src/or/router.c:2696:8: warning: potential null pointer dereference [-Wnull-dereference]
   prev = router_get_my_routerinfo()->addr;
   ~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
src/or/router.c:2696:8: warning: potential null pointer dereference [-Wnull-dereference]
src/or/router.c:2696:8: warning: potential null pointer dereference [-Wnull-dereference]

These are actually (most likely) okay because there are null-checks preceding each dereference.

CC src/or/routerlist.o
CC src/or/dirauth/shared_random.o

}}}
and

  CC       src/test/src_test_test-test_dos.o
  CC       src/test/src_test_test-test_entrynodes.o
src/test/test_entrynodes.c: In function ‘test_entry_guard_update_from_consensus_repair’:
src/test/test_entrynodes.c:1254:26: warning: potential null pointer dereference [-Wnull-dereference]
     n->is_possible_guard = 0;
     ~~~~~~~~~~~~~~~~~~~~~^~~

Each of these should be an easy fix, other locations in this test module call tt_assert before using the returned ref from bfn_mock_node_get_by_id.

}}}
at a stable Gentoo hardened Linux with app-forensics/afl 2.52b and gcc (Gentoo Hardened 6.4.0-r1 p1.3) 6.4.0

This is only triggered at higher optimization level on amd64, as far as i can tell. Specifically, I trigger it with -O3 (but not -O1 or -O2): ./configure --enable-fatal-warnings --disable-asciidoc CFLAGS="-O3"

I'm not sure exactly which optimization(s) cause these warnings. The gcc man page for -Wnull-dereference says "This option is only active when -fdelete-null-pointer-checks is active", but there are additional optimizations required for triggering this - I'm just not sure which ones.

With regard to the router.c warnings above, I'm concerned the preceding null checks are being optimized away at -O3, but I haven't confirmed this.

comment:4 in reply to:  3 Changed 5 months ago by sysrqb

Replying to sysrqb:

Replying to toralf:

src/or/router.c: In function ‘check_descriptor_ipaddress_changed’:
src/or/router.c:2696:8: warning: potential null pointer dereference [-Wnull-dereference]
   prev = router_get_my_routerinfo()->addr;
   ~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
src/or/router.c:2696:8: warning: potential null pointer dereference [-Wnull-dereference]
src/or/router.c:2696:8: warning: potential null pointer dereference [-Wnull-dereference]

These are actually (most likely) okay because there are null-checks preceding each dereference.

Okay, I looked at the asm and it looks okay (basically router_get_my_routerinfo_with_err gets inlined into each router_get_my_routerinfo() call). I believe this warning is generated because this is a time-of-check-time-of-use violation. Given that tor is single threaded in this logic, this should not be triggerable - but it may be better to adjust the null-checks so this isn't theoretically possible.

comment:5 Changed 5 months ago by nickm

Owner: set to nickm
Status: newaccepted

comment:6 Changed 5 months ago by nickm

Keywords: fast-fix added
Status: acceptedneeds_review

I'm fairly sure that there's no underlying bug here, but we should try to build without warnings in any case.

The router.c code here was introduced in 0.2.8.2-alpha, and the test_entrynodes.c code in 0.3.0.1-alpha.

So there's need for one fix in 0.2.9: Branch bug26269_029 with PR at https://github.com/torproject/tor/pull/147

And for one fix in 0.3.1: Branch bug26269_031 with PR at https://github.com/torproject/tor/pull/148

I can't reproduce the compiler errors here, so I'm only _fairly_ sure that I fixed all the cases. Testing and review would be welcome!

comment:7 Changed 5 months ago by dgoulet

Reviewer: isis

comment:8 Changed 4 months ago by asn

Reviewer: isisasn
Status: needs_reviewmerge_ready

I also didn't manage to repro with 0.3.4.1 and afl-gcc on Debian. Could not find g9f884a38e it might be some Gentoo-specific patch.

In any case, the branches in the PRs both look good to me and should solve the warnings.

comment:9 Changed 4 months ago by nickm

Resolution: fixed
Status: merge_readyclosed

Thanks; merged to the appropriate branches (0.2.9 and 0.3.2) and forward.

sysrqb, please reopen if you still see any of these warnings?

comment:10 in reply to:  8 Changed 4 months ago by toralf

Replying to asn:

Could not find g9f884a38e it might be some Gentoo-specific patch.

pls just remove the "g" - that character comes from git --describe (sha1 has only 0.9 and a-f)
so this works:

git show 9f884a38e

Note: See TracTickets for help on using tickets.