Index | Thread | Search

From:
"Theo de Raadt" <deraadt@openbsd.org>
Subject:
Re: dhcpd(8): don't avoid free()
To:
David Gwynne <david@gwynne.id.au>
Cc:
tech@openbsd.org
Date:
Fri, 30 May 2025 06:15:27 -0600

Download raw body.

Thread
  • Otto Moerbeek:

    dhcpd(8): don't avoid free()

  • Theo de Raadt:

    dhcpd(8): don't avoid free()

  • Maybe they are trying to persist the benefits of use-after-free bugs?
    Something is culturally wrong when people write code like this.
    
    ok deraadt
    
    > dhcpd seems to keep it's own free lists for a couple of data
    > structures.  my best guess for why it would do this is to avoid
    > malloc overhead.  however, responding to dhcp packets is not what
    > i would consider a webscale problem^W^W^Wperformance critical, so
    > this optimisation is unecessary.
    > 
    > ok?
    > 
    > Index: alloc.c
    > ===================================================================
    > RCS file: /cvs/src/usr.sbin/dhcpd/alloc.c,v
    > diff -u -p -r1.15 alloc.c
    > --- alloc.c	13 Feb 2017 19:13:14 -0000	1.15
    > +++ alloc.c	30 May 2025 05:26:08 -0000
    > @@ -55,30 +55,22 @@
    >  #include "dhcpd.h"
    >  #include "log.h"
    >  
    > -struct lease_state *free_lease_states;
    > -struct tree_cache *free_tree_caches;
    > -
    >  struct tree_cache *
    >  new_tree_cache(char *name)
    >  {
    >  	struct tree_cache *rval;
    >  
    > -	if (free_tree_caches) {
    > -		rval = free_tree_caches;
    > -		free_tree_caches = (struct tree_cache *)(rval->value);
    > -	} else {
    > -		rval = calloc(1, sizeof(struct tree_cache));
    > -		if (!rval)
    > -			fatalx("unable to allocate tree cache for %s.", name);
    > -	}
    > +	rval = calloc(1, sizeof(*rval));
    > +	if (rval == NULL)
    > +		fatalx("unable to allocate tree cache for %s.", name);
    > +
    >  	return (rval);
    >  }
    >  
    >  void
    >  free_tree_cache(struct tree_cache *ptr)
    >  {
    > -	ptr->value = (unsigned char *)free_tree_caches;
    > -	free_tree_caches = ptr;
    > +	free(ptr);
    >  }
    >  
    >  struct lease_state *
    > @@ -86,14 +78,9 @@ new_lease_state(char *name)
    >  {
    >  	struct lease_state *rval;
    >  
    > -	if (free_lease_states) {
    > -		rval = free_lease_states;
    > -		free_lease_states = free_lease_states->next;
    > -	} else {
    > -		rval = calloc(1, sizeof(struct lease_state));
    > -		if (!rval)
    > -			fatalx("unable to allocate lease state for %s.", name);
    > -	}
    > +	rval = calloc(1, sizeof(*rval));
    > +	if (rval == NULL)
    > +		fatalx("unable to allocate lease state for %s.", name);
    >  
    >  	return (rval);
    >  }
    > @@ -102,6 +89,5 @@ void
    >  free_lease_state(struct lease_state *ptr, char *name)
    >  {
    >  	free(ptr->prl);
    > -	ptr->next = free_lease_states;
    > -	free_lease_states = ptr;
    > +	free(ptr);
    >  }
    > 
    
    
  • Otto Moerbeek:

    dhcpd(8): don't avoid free()

  • Theo de Raadt:

    dhcpd(8): don't avoid free()