Home page logo

nmap-dev logo Nmap Development mailing list archives

Memory management strategies in Nmap?
From: Daniel Miller <bonsaiviking () gmail com>
Date: Tue, 26 Nov 2013 14:27:04 -0600

Fellow devs,

I've nearly tracked down all the "bugs" that Clang's AddressSanitizer[1] has shown in Nmap, but the fix to this last one has raised a question regarding memory management. I'm confident of this fix, but I would like to know more on the history of the various safe_*alloc functions, and why they might be preferable to C++ operator new.

So the bug was not really a bug, but could have started to cause problems. When an object of a derived type is allocated, it has a pointer called the vptr which is used to find inherited methods. It's not needed for data members, since those are all just offsets from the base pointer to the object. Clang was complaining:

osscan.cc:564:8: runtime error: member access within address 0xb1730d00 which does not point to an object of type 'FingerPrintResults'
0xb1730d00: note: object has invalid vptr
59 00 80 67 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
              invalid vptr

This turns out to be because the FingerPrintResultsIPv4 object (derived from FingerPrintResults) was allocated using safe_zalloc, so no vptr was set. All the appropriate data members would be accessible (and zero), but if we tried to use any virtual methods of the base class, we would segfault. Also, any constructors or destructors must be called directly (which they are not, though they do exist).

The fix, as I see it, is to use operator new[] instead of safe_zalloc, and the appropriate delete[] call instead of free:

diff --git a/osscan2.cc b/osscan2.cc
index eed70db..eb3b762 100644
--- a/osscan2.cc
+++ b/osscan2.cc
@@ -3381,7 +3381,7 @@ HostOsScanInfo::HostOsScanInfo(Target *t, OsScanInfo *OsSI) {
   OSI = OsSI;

FPs = (FingerPrint **) safe_zalloc(o.maxOSTries() * sizeof(FingerPrint *)); - FP_matches = (FingerPrintResultsIPv4 *) safe_zalloc(o.maxOSTries() * sizeof(FingerPrintResultsIPv4));
+  FP_matches = new FingerPrintResultsIPv4[o.maxOSTries()];
   timedOut = false;
   isCompleted = false;

@@ -3398,7 +3398,7 @@ HostOsScanInfo::HostOsScanInfo(Target *t, OsScanInfo *OsSI) {
 HostOsScanInfo::~HostOsScanInfo() {
   delete hss;
-  free(FP_matches);
+  delete[] FP_matches;

This may be a slight performance hit (since it necessarily involves the constructors and destructors for FingerPrintResults and FingerPrintResultsIPv4), but it seems the most correct way to follow the principle of least astonishment. Someone wrote the constructors and destructors for a purpose, and though this array may have been only used for quick data storage (I haven't dug that far yet), anyone hoping to use these objects in more depth would be stymied.

I'm holding off on patching this right away, since I would like to hear discussion before doing so, and nothing is truly broken at the moment.

Sent through the dev mailing list
Archived at http://seclists.org/nmap-dev/

  By Date           By Thread  

Current thread:
[ Nmap | Sec Tools | Mailing Lists | Site News | About/Contact | Advertising | Privacy ]