Created attachment 120271 [details] [review] null dereference patch fix for toggle_server_watch() In toggle_server_watch(): Return value of function which returns null is dereferenced without checking. If the function server_get_context() actually returns a null value, a null pointer dereference will occurat below line: _dbus_loop_toggle_watch (context->loop, watch) In this contribution, check for NULL is added to avoid the case of NULL pointer dereference. The test suite works fine after applying the proposed patch. Summary is as below: ============================================================================ Testsuite summary for dbus 1.10.99 ============================================================================ # TOTAL: 18 # PASS: 18 # SKIP: 0 # XFAIL: 0 # FAIL: 0 # XPASS: 0 # ERROR: 0 ============================================================================
Comment on attachment 120271 [details] [review] null dereference patch fix for toggle_server_watch() Review of attachment 120271 [details] [review]: ----------------------------------------------------------------- Is this purely theoretical (found with a static analyzer like clang or Coverity) or is this something you have actually seen happen in real life? server_get_context() can only return NULL if dbus_server_allocate_data_slot() runs out of memory, or if the DBusServer has not been correctly set up with a BusServerData. The former cannot happen in practice if the latter does not, because the BusContext allocates the data slot during dbus-daemon startup. The latter cannot happen in practice, because the dbus-daemon sets up a BusServerData on every DBusServer that it uses. I think the correct solution would be something more like this: * in server_get_context(), assert that server_data_slot != -1, then just use it, without the allocate/free pair * in server_get_context(), assert that bd != NULL to teach static analyzers that this is a "can't happen" situation.
Yes, I found it using static analyzer tool. I am currently attaching my previous patch in the git formatted version (0003-Null-Dereference-fix.patch). Below are the full testsuite run results with it: ============================================================================ Testsuite summary for dbus 1.10.99 ============================================================================ # TOTAL: 99 # PASS: 99 # SKIP: 0 # XFAIL: 0 # FAIL: 0 # XPASS: 0 # ERROR: 0 ============================================================================ Test logs attached: dbustestsuite_null_deref_003.log Putting assert for bd != NULL and server_data_slot >=0 also seems to be ok. Though for slot_id there is already assert check in _dbus_data_slot_allocator_alloc(). So it can only return false due to lack of memory or _dbus_data_slot_list_get() failure. Also, assert will not be fired with DBUS_DISABLE_ASSERT case and hence Null dereference can still occur in failure cases.
Created attachment 120299 [details] [review] Fix for Null dereference check in toggle_server_watch() Fix for Null dereference check in toggle_server_watch()
Created attachment 120300 [details] Testsuite run log file
(In reply to Deepika Aggarwal from comment #3) > Fix for Null dereference check in toggle_server_watch() No, silently failing to do what the function is documented to do is not a valid solution. Please do not propose this particular change again. If the bad situation cannot actually happen, because other code in the process has ensured that it can't, then we can teach the static analyzer about that (by adding assertions) and move on. If the bad situation *can* happen, a design change would be required. This is difficult, because it affects public APIs, but it does not appear to be required in this case. (In reply to Deepika Aggarwal from comment #2) > Also, assert will not be fired with DBUS_DISABLE_ASSERT case and hence Null > dereference can still occur in failure cases. Static analyzers sometimes have false positives; this is just a fact of life. Adding assertions is one way to add "executable documentation" that the case the static analyzer's heuristic is concerned about cannot actually happen. A NULL dereference is not actually any worse than an assertion failure: either way, the program crashes if it is reached (and it is up to the developer to be sure that it can't happen in practice). If our reasoning about why the bad situation cannot happen is incorrect, I would prefer to have the program crash (which is detectable) than to have it silently do the wrong thing.
Yes, I too agree this. Its better to add assertions in this case and we may remove NULL returns incase of fail cases in server_get_context() to teach static analyzers of this "can't happen" situation. Updated assertion patch also attached.
Created attachment 120333 [details] [review] assertion patch fix Patch using assert in server_get_context()
Comment on attachment 120333 [details] [review] assertion patch fix Review of attachment 120333 [details] [review]: ----------------------------------------------------------------- ::: bus/bus.c @@ +90,4 @@ > BusContext *context; > BusServerData *bd; > > + dbus_server_allocate_data_slot (&server_data_slot); Why can't this fail with an out-of-memory error? The answer is: because we already allocated server_data_slot, in bus_context_new(), which is required to have been called before we started creating and configuring DBusServer objects; so this call to dbus_server_allocate_data_slot() is just increasing its reference count, it isn't actually doing new allocation, and therefore it can't fail. So, instead of dbus_server_allocate_data_slot (&server_data_slot); /* ... do things with server_data_slot */ dbus_server_free_data_slot (&server_data_slot); we should just do /* this data slot was allocated by the BusContext */ _dbus_assert (server_data_slot >= 0); /* ... do things with server_data_slot */ The comment is important: it tells us why the assertion is going to be true. @@ +95,3 @@ > > bd = BUS_SERVER_DATA (server); > + _dbus_assert (bd != NULL); Similarly, this deserves a comment, something like this: bd = BUS_SERVER_DATA (server); /* every DBusServer in the dbus-daemon has gone through setup_server() */ _dbus_assert (bd != NULL);
Comment on attachment 120333 [details] [review] assertion patch fix Review of attachment 120333 [details] [review]: ----------------------------------------------------------------- > Null derefrence fixed using dbus_assert This is not "fixing" anything. If the null dereference could happen in real life, then the _dbus_assert() would be invalid, because it would sometimes fail. What you are actually doing here is: * teaching the static analyzer that the scenario it is warning you about cannot actually happen, because our analysis of the code flow indicates that some things that appear to be failable are in fact not failable; * introducing an assertion so that if our analysis was wrong, it will be caught (as a crash) So the commit message would make more sense as something like > Subject: bus: reassure static analysis tools that server slot allocation can't fail > > This prevents a static analysis tool from reporting a possible NULL-dereference here. > The NULL-dereference is not actually possible in this case, because we know that > the allocation and setup were done previously.
(In reply to Simon McVittie from comment #9) > > This prevents a static analysis tool from reporting a possible > > NULL-dereference here. Of course if you are able to say "This prevents Coverity from reporting ..." or "This prevents clang from reporting ..." or whatever the name of the tool actually is, so much the better.
Updated patch and test results attached as per the comments. ============================================================================ Testsuite summary for dbus 1.10.99 ============================================================================ # TOTAL: 99 # PASS: 99 # SKIP: 0 # XFAIL: 0 # FAIL: 0 # XPASS: 0 # ERROR: 0 ============================================================================
Created attachment 120386 [details] [review] bus: reassure static analysis tool that server slot allocation can't fail. [Patch]bus: reassure static analysis tool that server slot allocation can't fail.
Created attachment 120387 [details] Full testsuite logs Full test suite log file
Fixed in git for 1.11.4, thanks. This doesn't seem to be a user-observable bug, so I'm not backporting it to the 1.10.x stable branch.
Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.