Add assertions before potential null deferences #11238

pull meshcollider wants to merge 1 commits into bitcoin:master from meshcollider:201708_fbinfer_fixes changing 4 files +8 −0

meshcollider commented at 9:39 AM on September 5, 2017: contributor

Picked up by the static analyzer Facebook Infer which I was playing around with for another research project. Just adding some asserts before dereferencing potentially null pointers.
Fix potential null dereferences c001992440
fanquake added the label Refactoring on Sep 5, 2017
JeremyRubin commented at 11:38 PM on September 5, 2017: contributor

Seems reasonable, utAck.

in src/addrdb.cpp:53 in 3e9f9218e6 outdated

  48 | @@ -49,7 +49,10 @@ bool SerializeFileDB(const std::string& prefix, const fs::path& path, const Data
  49 |          return error("%s: Failed to open file %s", __func__, pathTmp.string());
  50 |  
  51 |      // Serialize
  52 | -    if (!SerializeDB(fileout, data)) return false;
  53 | +    if (!SerializeDB(fileout, data)) {
  54 | +        fileout.fclose();

laanwj commented at 12:01 AM on September 6, 2017:

This is an autofile - it closes automatically on release. So this is redundant. The only reason for closing it explicitly below is because of the RenameOver after.

promag commented at 1:17 AM on September 6, 2017:

Maybe remove explicit close and move fileout to an inner scope?

practicalswift commented at 8:41 AM on September 6, 2017: contributor

utACK modulo the autofile fix which is redundant.

Generally I think it is a good idea to add assert(…):s where it helps static analyzers understand the code better. It makes implicit assumptions explicit which helps static analyzers and humans alike to understand and reason about the code.

My experience is that if a code path makes a static analyzer confused it is very likely to be confusing for humans too. This holds even in a lot of cases where it could be argued that the static analyzer should be "smarter" and in some way misunderstands the code. Confusion leads to bugs, so better avoid confusion by liberal use of assert:s and other mechanisms to make tacit assumptions explicit.
meshcollider commented at 12:26 PM on September 6, 2017: contributor

Removed redundant file close as requested, thanks for the reviews :)
practicalswift commented at 12:29 PM on September 6, 2017: contributor

utACK c0019924406e1ce8368465c768de11019ad5eeed
promag commented at 1:03 PM on September 6, 2017: member

utACK c001992.
laanwj commented at 7:01 PM on September 6, 2017: member

Can you please change the title to Add nullptr assertions or such? The current title is incorrect, as adding assertions does not fix anything.
meshcollider renamed this:
~~Fix potential null deference and resource leaks~~
Add assertions before potential null deferences
on Sep 6, 2017
laanwj commented at 9:50 PM on September 6, 2017: member

utACK c00199244
laanwj merged this on Sep 6, 2017
laanwj closed this on Sep 6, 2017
laanwj referenced this in commit 6acdb1fab7 on Sep 6, 2017
meshcollider deleted the branch on Sep 7, 2017
PastaPastaPasta referenced this in commit d90d0fee0d on Sep 23, 2019
PastaPastaPasta referenced this in commit 53a3fd13da on Sep 24, 2019
PastaPastaPasta referenced this in commit 228672baec on Dec 22, 2019
PastaPastaPasta referenced this in commit 21d0df40d1 on Jan 2, 2020
PastaPastaPasta referenced this in commit d22aeb3547 on Jan 4, 2020
PastaPastaPasta referenced this in commit 2168ba9d2e on Jan 4, 2020
PastaPastaPasta referenced this in commit 1d509e3bd7 on Jan 10, 2020
PastaPastaPasta referenced this in commit ca4a749a18 on Jan 10, 2020
PastaPastaPasta referenced this in commit ec496efdb9 on Jan 10, 2020
PastaPastaPasta referenced this in commit 2c5ced3c67 on Jan 12, 2020
ckti referenced this in commit 5bc1c0eb3b on Mar 28, 2021
furszy referenced this in commit 60d36292bc on Jul 26, 2021
DrahtBot locked this on Sep 8, 2021

Contributors

Labels