Memory management strategies in Nmap?

[Daniel Miller <bonsaiviking () gmail com>]

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(FPs);
> -  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.

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