Re: [ping][PATCH][BZ #16366] Fix return code from getent netgroup when the netgroup is not found

["Carlos O'Donell" <carlos at redhat dot com>]

On 12/31/2013 01:46 AM, Siddhesh Poyarekar wrote:
> Ping!
> 
> On Mon, Dec 23, 2013 at 05:22:22PM +0530, Siddhesh Poyarekar wrote:
>> Hi,
>>
>> nscd incorrectly returns a success even when the netgroup in question
>> is not found and adds a positive result in the cache.  this patch
>> fixes this behaviour by adding a negative lookup entry to cache and
>> returning an error when the netgroup is not found.
>>
>> Verified that the patch fixes the problem on Fedora rawhide.  OK to
>> commit?
>>
>> Siddhesh
>>
>> 	[BZ #16366]
>> 	* nscd/netgroupcache.c (do_notfound): New function.
>> 	(addgetnetgrentX): Use it.
>>
>> diff --git a/nscd/netgroupcache.c b/nscd/netgroupcache.c
>> index f555892..3fad8c9 100644
>> --- a/nscd/netgroupcache.c
>> +++ b/nscd/netgroupcache.c
>> @@ -65,6 +65,55 @@ struct dataset
>>    char strdata[0];
>>  };
>>  
>> +/* Sends a notfound message and prepares a notfound dataset to write to the
>> +   cache.  Returns true if there was enough memory to allocate the dataset and
>> +   returns the dataset in DATASETP, total bytes to write in TOTALP and the
>> +   timeout in TIMEOUTP.  KEY_COPY is set to point to the copy of the key in the
>> +   dataset. */

OK.

>> +static bool
>> +do_notfound (struct database_dyn *db, int fd, request_header *req,
>> +	       const char *key, struct dataset **datasetp, ssize_t *totalp,
>> +	       time_t *timeoutp, char **key_copy)
>> +{
>> +  struct dataset *dataset;
>> +  ssize_t total;
>> +  time_t timeout;
>> +  bool cacheable = false;
>> +
>> +  total = sizeof (notfound);
>> +  timeout = time (NULL) + db->negtimeout;
>> +
>> +  if (fd != -1)
>> +    TEMP_FAILURE_RETRY (send (fd, &notfound, total, MSG_NOSIGNAL));
>> +
>> +  dataset = mempool_alloc (db, sizeof (struct dataset) + req->key_len, 1);
>> +  /* If we cannot permanently store the result, so be it.  */
>> +  if (dataset != NULL)
>> +    {
>> +      dataset->head.allocsize = sizeof (struct dataset) + req->key_len;
>> +      dataset->head.recsize = total;
>> +      dataset->head.notfound = true;
>> +      dataset->head.nreloads = 0;
>> +      dataset->head.usable = true;
>> +
>> +      /* Compute the timeout time.  */
>> +      timeout = dataset->head.timeout = time (NULL) + db->negtimeout;
>> +      dataset->head.ttl = db->negtimeout;
>> +
>> +      /* This is the reply.  */
>> +      memcpy (&dataset->resp, &notfound, total);
>> +
>> +      /* Copy the key data.  */
>> +      memcpy (dataset->strdata, key, req->key_len);
>> +      *key_copy = dataset->strdata;
>> +
>> +      cacheable = true;
>> +    }
>> +  *timeoutp = timeout;
>> +  *totalp = total;
>> +  *datasetp = dataset;
>> +  return cacheable;
>> +}

This is a refactoring and it looks accurate to me.

>>  
>>  static time_t
>>  addgetnetgrentX (struct database_dyn *db, int fd, request_header *req,
>> @@ -84,6 +133,7 @@ addgetnetgrentX (struct database_dyn *db, int fd, request_header *req,
>>    struct dataset *dataset;
>>    bool cacheable = false;
>>    ssize_t total;
>> +  bool found = false;
>>  
>>    char *key_copy = NULL;
>>    struct __netgrent data;
>> @@ -103,35 +153,8 @@ addgetnetgrentX (struct database_dyn *db, int fd, request_header *req,
>>        && __nss_database_lookup ("netgroup", NULL, NULL, &netgroup_database))
>>      {
>>        /* No such service.  */
>> -      total = sizeof (notfound);
>> -      timeout = time (NULL) + db->negtimeout;
>> -
>> -      if (fd != -1)
>> -	TEMP_FAILURE_RETRY (send (fd, &notfound, total, MSG_NOSIGNAL));
>> -
>> -      dataset = mempool_alloc (db, sizeof (struct dataset) + req->key_len, 1);
>> -      /* If we cannot permanently store the result, so be it.  */
>> -      if (dataset != NULL)
>> -	{
>> -	  dataset->head.allocsize = sizeof (struct dataset) + req->key_len;
>> -	  dataset->head.recsize = total;
>> -	  dataset->head.notfound = true;
>> -	  dataset->head.nreloads = 0;
>> -	  dataset->head.usable = true;
>> -
>> -	  /* Compute the timeout time.  */
>> -	  timeout = dataset->head.timeout = time (NULL) + db->negtimeout;
>> -	  dataset->head.ttl = db->negtimeout;
>> -
>> -	  /* This is the reply.  */
>> -	  memcpy (&dataset->resp, &notfound, total);
>> -
>> -	  /* Copy the key data.  */
>> -	  memcpy (dataset->strdata, key, req->key_len);
>> -
>> -	  cacheable = true;
>> -	}
>> -
>> +      cacheable = do_notfound (db, fd, req, key, &dataset, &total, &timeout,
>> +			       &key_copy);

OK.

>>        goto writeout;
>>      }
>>  
>> @@ -167,6 +190,7 @@ addgetnetgrentX (struct database_dyn *db, int fd, request_header *req,
>>  
>>  	  if (status == NSS_STATUS_SUCCESS)
>>  	    {
>> +	      found = true;

OK. This is the one place where we register via NSS_STATUS_SUCCESS that there was a positive result.

>>  	      union
>>  	      {
>>  		enum nss_status (*f) (struct __netgrent *, char *, size_t,
>> @@ -326,6 +350,15 @@ addgetnetgrentX (struct database_dyn *db, int fd, request_header *req,
>>  	}
>>      }
>>  
>> +  /* No results.  Return a failure and write out a notfound record in the
>> +     cache.  */
>> +  if (!found)
>> +    {
>> +      cacheable = do_notfound (db, fd, req, key, &dataset, &total, &timeout,
>> +			       &key_copy);

OK.

>> +      goto writeout;
>> +    }
>> +
>>    total = buffilled;
>>  
>>    /* Fill in the dataset.  */

Looks OK to me.

Cheers,
Carlos.