[PATCH v2 1/1] nscd: replace alloca with malloc in aicache, hstcache [BZ #34624]

[Adhemerval Zanella Netto]

On 23/09/26 21:55, DJ Delorie wrote:
> This changes the alloca fallback to a malloc fallback
> in the addrinfo and host cache code.
> ---
> 
> v2: Florian suggested storing the malloc'd pointer and using *that* as
>     the "flag", which simplified the code.
> 
>  nscd/aicache.c  | 26 +++++++++++++++++++-------
>  nscd/hstcache.c | 16 +++++++++++-----
>  2 files changed, 30 insertions(+), 12 deletions(-)
> 
> diff --git a/nscd/aicache.c b/nscd/aicache.c
> index ef15c373f5..b08723a251 100644
> --- a/nscd/aicache.c
> +++ b/nscd/aicache.c
> @@ -102,7 +102,7 @@ addhstaiX (struct database_dyn *db, int fd,
> request_header *req,
>    int32_t ttl = INT32_MAX;
>    ssize_t total = 0;
>    char *key_copy = NULL;
> -  bool alloca_used = false;
> +  void *dataset_alloc = NULL;
>    time_t timeout = MAX_TIMEOUT_VALUE;
> 
>    while (!no_more)
> @@ -173,10 +173,12 @@ addhstaiX (struct database_dyn *db, int fd,
> request_header *req,
>        /* We cannot permanently add the result in the moment.  But
>   we can provide the result as is.  Store the data in some
>   temporary memory.  */
> -      dataset = (struct dataset *) alloca (total + req->key_len);
> +      dataset = (struct dataset *) malloc (total + req->key_len);
> +      if (dataset == NULL)
> + goto next_nip;
> 

I think this makes the client see a negative answer when the failure was memory
exhaustion, because status[1] == NSS_STATUS_SUCCESS at this point.  I think it
should go to 'out', similar to scratch_buffer_grow failure.



>        /* We cannot add this record to the permanent database.  */
> -      alloca_used = true;
> +      dataset_alloc = dataset;
>      }
> 
>    /* Fill in the address and address families.  */
> @@ -345,10 +347,12 @@ addhstaiX (struct database_dyn *db, int fd,
> request_header *req,
>        /* We cannot permanently add the result in the moment.  But
>   we can provide the result as is.  Store the data in some
>   temporary memory.  */
> -      dataset = (struct dataset *) alloca (total + req->key_len);
> +      dataset = (struct dataset *) malloc (total + req->key_len);
> +      if (dataset == NULL)
> + goto next_nip;
> 
>        /* We cannot add this record to the permanent database.  */
> -      alloca_used = true;
> +      dataset_alloc = dataset;
>      }
> 
>    /* Fill in the address and address families.  */
> @@ -419,7 +423,9 @@ addhstaiX (struct database_dyn *db, int fd,
> request_header *req,
>    key_copy = (char *) newp + (key_copy - (char *) dataset);
> 
>    dataset = memcpy (newp, dataset, total + req->key_len);
> -  alloca_used = false;
> +
> +  free (dataset_alloc);
> +  dataset_alloc = NULL;
>   }
> 
>        /* Mark the old record as obsolete.  */
> @@ -439,6 +445,9 @@ addhstaiX (struct database_dyn *db, int fd,
> request_header *req,
>        goto out;
> 
>  next_nip:
> +      free (dataset_alloc);
> +      dataset_alloc = NULL;
> +

And with the 'goto out' fix above this is not required because this is
no failure that will fall here.

>        if (nss_next_action (nip, status[1]) == NSS_ACTION_RETURN)
>   break;
> 
> @@ -502,7 +511,7 @@ next_nip:
>   out:
>    __resolv_context_put (ctx);
> 
> -  if (dataset != NULL && !alloca_used)
> +  if (dataset != NULL && dataset_alloc == NULL)
>      {
>        /* If necessary, we also propagate the data to disk.  */
>        if (db->persistent)
> @@ -528,6 +537,9 @@ next_nip:
>    scratch_buffer_free (&tmpbuf4);
>    scratch_buffer_free (&canonbuf);
> 
> +  free (dataset_alloc);
> +  dataset_alloc = NULL;

The NULL is not strictly required here.

> +
>    return timeout;
>  }
> 
> diff --git a/nscd/hstcache.c b/nscd/hstcache.c
> index fe91818640..bb91fd7a1a 100644
> --- a/nscd/hstcache.c
> +++ b/nscd/hstcache.c
> @@ -228,7 +228,7 @@ cache_addhst (struct database_dyn *db, int fd,
> request_header *req,
>   change.  Allocate memory on the cache since it is likely
>   discarded anyway.  If it turns out to be necessary to have a
>   new record we can still allocate real memory.  */
> -      bool alloca_used = false;
> +      void *dataset_alloc = false;

I think using 'false' is confusing here and out code guidelines as for NULL
(-std=gnu23 rejects it with "incompatible types ...'".

>        dataset = NULL;
> 
>        /* If the record contains more than one IP address (used for
> @@ -245,10 +245,12 @@ cache_addhst (struct database_dyn *db, int fd,
> request_header *req,
>    /* We cannot permanently add the result in the moment.  But
>       we can provide the result as is.  Store the data in some
>       temporary memory.  */
> -  dataset = (struct dataset *) alloca (total + req->key_len);
> +  dataset = (struct dataset *) malloc (total + req->key_len);
> +  if (dataset == NULL)
> +    return timeout;
> 
>    /* We cannot add this record to the permanent database.  */
> -  alloca_used = true;
> +  dataset_alloc = dataset;
>   }
> 
>        timeout = datahead_init_pos (&dataset->head, total + req->key_len,
> @@ -335,7 +337,9 @@ cache_addhst (struct database_dyn *db, int fd,
> request_header *req,
>        key_copy = (char *) newp + (key_copy - (char *) dataset);
> 
>        dataset = memcpy (newp, dataset, total + req->key_len);
> -      alloca_used = false;
> +
> +      free (dataset_alloc);
> +      dataset_alloc = NULL;
>      }
>   }
> 
> @@ -363,7 +367,7 @@ cache_addhst (struct database_dyn *db, int fd,
> request_header *req,
>   the current cache handling cannot handle and it is more than
>   questionable whether it is worthwhile complicating the cache
>   handling just for handling such a special case. */
> -      if (! alloca_used)
> +      if (dataset_alloc == NULL)
>   {
>    /* If necessary, we also propagate the data to disk.  */
>    if (db->persistent)
> @@ -394,6 +398,8 @@ cache_addhst (struct database_dyn *db, int fd,
> request_header *req,
> 
>    pthread_rwlock_unlock (&db->lock);
>   }
> +      free (dataset_alloc);
> +      dataset_alloc = NULL;
>      }
> 
>    if (__builtin_expect (!all_written, 0) && debug_level > 0)

There are still an alloca on htscache after this fix:

  h_aliases_len = (uint32_t *) alloca (h_aliases_cnt * sizeof (uint32_t));

Maybe also convert this one as well or add a comment on why it is not required.