From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from black.elm.relay.mailchannels.net (black.elm.relay.mailchannels.net [23.83.212.19]) by sourceware.org (Postfix) with ESMTPS id EAB29385840D for ; Wed, 24 Apr 2024 16:48:21 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org EAB29385840D Authentication-Results: sourceware.org; dmarc=none (p=none dis=none) header.from=gotplt.org Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=gotplt.org ARC-Filter: OpenARC Filter v1.0.0 sourceware.org EAB29385840D Authentication-Results: server2.sourceware.org; arc=pass smtp.remote-ip=23.83.212.19 ARC-Seal: i=2; a=rsa-sha256; d=sourceware.org; s=key; t=1713977305; cv=pass; b=SLmiPL902IKig96pG2pN36XYB/li3C2Demu5Ed9cZP9h09TdeOrr2PX/I446HpdGHJak1DclEHRBTA/5cAFoOkugk+BXxNyQBZF7zwmGSeycM4wxrd9KC+a0LdLAScZdCZdvs7pHKjc2Ne2fcjkAsN+akG61exbWIgfz/7bfskk= ARC-Message-Signature: i=2; a=rsa-sha256; d=sourceware.org; s=key; t=1713977305; c=relaxed/simple; bh=TPyfYKOuBV+Wxl3jx+DDbKhyzjo0EjXEopfYPf5Ei2k=; h=DKIM-Signature:Message-ID:Date:MIME-Version:Subject:To:From; b=fbifmHeG+SpueREli0YTM70nxVvn18UDKugrX83UXbE+rf3D0R/O6hrOfETmRmUW60/i9tA0CqGP4gTBEZPCp4b7b3ZtANUTKECu24vBCzs3TFosbS8QOAo6LaT1pgqA9mEl9ekP/Ob+iFkVLMOdo/UnMgm6FBgkA97zKb9a2+Q= ARC-Authentication-Results: i=2; server2.sourceware.org X-Sender-Id: dreamhost|x-authsender|siddhesh@gotplt.org Received: from relay.mailchannels.net (localhost [127.0.0.1]) by relay.mailchannels.net (Postfix) with ESMTP id 90DB482696; Wed, 24 Apr 2024 16:48:14 +0000 (UTC) Received: from pdx1-sub0-mail-a231.dreamhost.com (unknown [127.0.0.6]) (Authenticated sender: dreamhost) by relay.mailchannels.net (Postfix) with ESMTPA id 2BC2F82335; Wed, 24 Apr 2024 16:48:14 +0000 (UTC) ARC-Seal: i=1; s=arc-2022; d=mailchannels.net; t=1713977294; a=rsa-sha256; cv=none; b=dIgmSzpQe7o2v8BCmRTRJ81rgEmEHrEFKIuAkxsDoJoUdk9sGBNlY3E6Ne7BW3KdGkqvI5 G43XVXQO6rxVagxb2QMRg1PFoC9BE2B05Evfbq35AWJ1n341M/Pub5tV7ryLAHnrk7978W 2OS9mgoyFj5w288XkXjtLVi9igyuStpjir4/4zzlr5ciXbXRo27ENQ2tdkcKKzmo0WK8xX iMxvUXkhtWKfTFFimAFGsur9NX6JAgUVr0xlK/rqhkm53CwSMQhbRvePymFBlfhLNua29Z FNYDRYPmPl2BVmLcbigigtHlwvupiFd4PRCPHr9Aorwn1uVJ4EEn9ZtAbuQT/Q== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=mailchannels.net; s=arc-2022; t=1713977294; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references:dkim-signature; bh=gXthYk64KPagleBJrDjcCyNAb9uVlpbZCby19qeHYlc=; b=sbw/mJHeZV16klpgPXlPX/AFU0fDRo2SzM0N1wEdG56/Q0AhD84voaIZdQQyMgBiqNg9Sn tvviLgWMVtPxeInk3mZZW3WgZ4GQ9Xi5v52ep4tIJUQZqFAf8tzVQG+T1nsegE0TexkNh9 MqThUei2GloPUBGvkQvEnQlK9xJFKDhSsHbEHTC1VyMKInuiYrQFK+Qgb4XIjhVg4Hh9ui 5FldXUf+gNs+RKOFys8BCt7Rp0lAvXiTCHoG4VgTTDlV3/1FIt5eljpofvTfBthveZpS0k rR6bya4aYXlZhrWt3pKdLAL39ivfmrIlHz6D+HsBkI3wJNudZ8D3wiPh0hu8/g== ARC-Authentication-Results: i=1; rspamd-6f55c6f9dc-cbrxc; auth=pass smtp.auth=dreamhost smtp.mailfrom=siddhesh@gotplt.org X-Sender-Id: dreamhost|x-authsender|siddhesh@gotplt.org X-MC-Relay: Neutral X-MailChannels-SenderId: dreamhost|x-authsender|siddhesh@gotplt.org X-MailChannels-Auth-Id: dreamhost X-Eight-Wipe: 2af9d32d7473b20a_1713977294427_1097741081 X-MC-Loop-Signature: 1713977294427:1232595557 X-MC-Ingress-Time: 1713977294427 Received: from pdx1-sub0-mail-a231.dreamhost.com (pop.dreamhost.com [64.90.62.162]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384) by 100.117.243.76 (trex/6.9.2); Wed, 24 Apr 2024 16:48:14 +0000 Received: from [192.168.0.182] (bras-base-toroon4859w-grc-68-174-95-141-199.dsl.bell.ca [174.95.141.199]) (using TLSv1.3 with cipher TLS_AES_128_GCM_SHA256 (128/128 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) (Authenticated sender: siddhesh@gotplt.org) by pdx1-sub0-mail-a231.dreamhost.com (Postfix) with ESMTPSA id 4VPlKn5ZJRz1T; Wed, 24 Apr 2024 09:48:13 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gotplt.org; s=dreamhost; t=1713977294; bh=gXthYk64KPagleBJrDjcCyNAb9uVlpbZCby19qeHYlc=; h=Date:Subject:To:From:Content-Type:Content-Transfer-Encoding; b=AeHJe7iu6kPZGKBqDCJazMbxKrUNnXyO6J4lWwbrokJO+9a+P+Rj/TuNBT7ssCXKS psji6e5jAtodS6KSln1yOLl8j9Ue4bBMiINQt95NEYPffk5gontt6ikLbmkwGf1WK7 P3LwWSi+wTzwptEtaUyJgn8XdDqbNZ9FHIbboPWfqH+HtM+ZENAIeDb44a1viGnw+3 Mux8p6B0eZ++IAYRtsXe5jjacGy81Ql7lBSSa/VD25/dMjyonwEtDZ+iOWcF0Dgraq hx/+8kUkQxiU/03AuKlUtGzTiFkmSa9/SJw4b6I02owOmUtFd9ZEAjSAEbDnvEmqKN yuURe/sl2NFBw== Message-ID: <7618bd79-68ba-4bc0-a942-3bf4c1652299@gotplt.org> Date: Wed, 24 Apr 2024 12:48:11 -0400 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [PATCH 4/4] nscd: netgroup: Use two buffers in addgetnetgrentX (bug 31680) To: Florian Weimer , libc-alpha@sourceware.org References: <9abb85706e6a6876c55daed36e56c4e59e05b039.1713974801.git.fweimer@redhat.com> Content-Language: en-US From: Siddhesh Poyarekar In-Reply-To: <9abb85706e6a6876c55daed36e56c4e59e05b039.1713974801.git.fweimer@redhat.com> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit X-Spam-Status: No, score=-3036.7 required=5.0 tests=BAYES_00,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,GIT_PATCH_0,RCVD_IN_DNSWL_NONE,RCVD_IN_MSPIKE_H3,RCVD_IN_MSPIKE_WL,SPF_HELO_NONE,SPF_PASS,TXREP autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on server2.sourceware.org List-Id: On 2024-04-24 12:08, Florian Weimer wrote: > This avoids potential memory corruption when the underlying NSS > callback function does not use the buffer space to store all strings > (e.g., for constant strings). > > Instead of custom buffer management, two scratch buffers are used. > This increases stack usage somewhat. > > Scratch buffer allocation failure is handled by return -1 > (an invalid timeout value) instead of terminating the process. > This fixes bug 31679. > --- LGTM. Reviewed-by: Siddhesh Poyarekar > nscd/netgroupcache.c | 219 ++++++++++++++++++++++++------------------- > 1 file changed, 121 insertions(+), 98 deletions(-) > > diff --git a/nscd/netgroupcache.c b/nscd/netgroupcache.c > index 8f9eb84e39..8b5ebfaf31 100644 > --- a/nscd/netgroupcache.c > +++ b/nscd/netgroupcache.c > @@ -23,6 +23,7 @@ > #include > #include > #include > +#include > > #include "../nss/netgroup.h" > #include "nscd.h" > @@ -65,6 +66,16 @@ struct dataset > char strdata[0]; > }; > > +/* Send a notfound response to FD. Always returns -1 to indicate an > + ephemeral error. */ > +static time_t > +send_notfound (int fd) > +{ > + if (fd != -1) > + TEMP_FAILURE_RETRY (send (fd, ¬found, sizeof (notfound), MSG_NOSIGNAL)); > + return -1; > +} > + > /* 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 > @@ -83,8 +94,7 @@ do_notfound (struct database_dyn *db, int fd, request_header *req, > total = sizeof (notfound); > timeout = time (NULL) + db->negtimeout; > > - if (fd != -1) > - TEMP_FAILURE_RETRY (send (fd, ¬found, total, MSG_NOSIGNAL)); > + send_notfound (fd); > > dataset = mempool_alloc (db, sizeof (struct dataset) + req->key_len, 1); > /* If we cannot permanently store the result, so be it. */ > @@ -109,11 +119,78 @@ do_notfound (struct database_dyn *db, int fd, request_header *req, > return cacheable; > } > > +struct addgetnetgrentX_scratch > +{ > + /* This is the result that the caller should use. It can be NULL, > + point into buffer, or it can be in the cache. */ > + struct dataset *dataset; > + > + struct scratch_buffer buffer; > + > + /* Used internally in addgetnetgrentX as a staging area. */ > + struct scratch_buffer tmp; > + > + /* Number of bytes in buffer that are actually used. */ > + size_t buffer_used; > +}; > + > +static void > +addgetnetgrentX_scratch_init (struct addgetnetgrentX_scratch *scratch) > +{ > + scratch->dataset = NULL; > + scratch_buffer_init (&scratch->buffer); > + scratch_buffer_init (&scratch->tmp); > + > + /* Reserve space for the header. */ > + scratch->buffer_used = sizeof (struct dataset); > + static_assert (sizeof (struct dataset) < sizeof (scratch->tmp.__space), > + "initial buffer space"); > + memset (scratch->tmp.data, 0, sizeof (struct dataset)); > +} > + > +static void > +addgetnetgrentX_scratch_free (struct addgetnetgrentX_scratch *scratch) > +{ > + scratch_buffer_free (&scratch->buffer); > + scratch_buffer_free (&scratch->tmp); > +} > + > +/* Copy LENGTH bytes from S into SCRATCH. Returns NULL if SCRATCH > + could not be resized, otherwise a pointer to the copy. */ > +static char * > +addgetnetgrentX_append_n (struct addgetnetgrentX_scratch *scratch, > + const char *s, size_t length) > +{ > + while (true) > + { > + size_t remaining = scratch->buffer.length - scratch->buffer_used; > + if (remaining >= length) > + break; > + if (!scratch_buffer_grow_preserve (&scratch->buffer)) > + return NULL; > + } > + char *copy = scratch->buffer.data + scratch->buffer_used; > + memcpy (copy, s, length); > + scratch->buffer_used += length; > + return copy; > +} > + > +/* Copy S into SCRATCH, including its null terminator. Returns false > + if SCRATCH could not be resized. */ > +static bool > +addgetnetgrentX_append (struct addgetnetgrentX_scratch *scratch, const char *s) > +{ > + if (s == NULL) > + s = ""; > + return addgetnetgrentX_append_n (scratch, s, strlen (s) + 1) != NULL; > +} > + > +/* Caller must initialize and free *SCRATCH. If the return value is > + negative, this function has sent a notfound response. */ > static time_t > addgetnetgrentX (struct database_dyn *db, int fd, request_header *req, > const char *key, uid_t uid, struct hashentry *he, > - struct datahead *dh, struct dataset **resultp, > - void **tofreep) > + struct datahead *dh, struct addgetnetgrentX_scratch *scratch) > { > if (__glibc_unlikely (debug_level > 0)) > { > @@ -132,14 +209,10 @@ addgetnetgrentX (struct database_dyn *db, int fd, request_header *req, > > char *key_copy = NULL; > struct __netgrent data; > - size_t buflen = MAX (1024, sizeof (*dataset) + req->key_len); > - size_t buffilled = sizeof (*dataset); > - char *buffer = NULL; > size_t nentries = 0; > size_t group_len = strlen (key) + 1; > struct name_list *first_needed > = alloca (sizeof (struct name_list) + group_len); > - *tofreep = NULL; > > if (netgroup_database == NULL > && !__nss_database_get (nss_database_netgroup, &netgroup_database)) > @@ -151,8 +224,6 @@ addgetnetgrentX (struct database_dyn *db, int fd, request_header *req, > } > > memset (&data, '\0', sizeof (data)); > - buffer = xmalloc (buflen); > - *tofreep = buffer; > first_needed->next = first_needed; > memcpy (first_needed->name, key, group_len); > data.needed_groups = first_needed; > @@ -195,8 +266,8 @@ addgetnetgrentX (struct database_dyn *db, int fd, request_header *req, > while (1) > { > int e; > - status = getfct.f (&data, buffer + buffilled, > - buflen - buffilled - req->key_len, &e); > + status = getfct.f (&data, scratch->tmp.data, > + scratch->tmp.length, &e); > if (status == NSS_STATUS_SUCCESS) > { > if (data.type == triple_val) > @@ -204,68 +275,10 @@ addgetnetgrentX (struct database_dyn *db, int fd, request_header *req, > const char *nhost = data.val.triple.host; > const char *nuser = data.val.triple.user; > const char *ndomain = data.val.triple.domain; > - > - size_t hostlen = strlen (nhost ?: "") + 1; > - size_t userlen = strlen (nuser ?: "") + 1; > - size_t domainlen = strlen (ndomain ?: "") + 1; > - > - if (nhost == NULL || nuser == NULL || ndomain == NULL > - || nhost > nuser || nuser > ndomain) > - { > - const char *last = nhost; > - if (last == NULL > - || (nuser != NULL && nuser > last)) > - last = nuser; > - if (last == NULL > - || (ndomain != NULL && ndomain > last)) > - last = ndomain; > - > - size_t bufused > - = (last == NULL > - ? buffilled > - : last + strlen (last) + 1 - buffer); > - > - /* We have to make temporary copies. */ > - size_t needed = hostlen + userlen + domainlen; > - > - if (buflen - req->key_len - bufused < needed) > - { > - buflen += MAX (buflen, 2 * needed); > - /* Save offset in the old buffer. We don't > - bother with the NULL check here since > - we'll do that later anyway. */ > - size_t nhostdiff = nhost - buffer; > - size_t nuserdiff = nuser - buffer; > - size_t ndomaindiff = ndomain - buffer; > - > - char *newbuf = xrealloc (buffer, buflen); > - /* Fix up the triplet pointers into the new > - buffer. */ > - nhost = (nhost ? newbuf + nhostdiff > - : NULL); > - nuser = (nuser ? newbuf + nuserdiff > - : NULL); > - ndomain = (ndomain ? newbuf + ndomaindiff > - : NULL); > - *tofreep = buffer = newbuf; > - } > - > - nhost = memcpy (buffer + bufused, > - nhost ?: "", hostlen); > - nuser = memcpy ((char *) nhost + hostlen, > - nuser ?: "", userlen); > - ndomain = memcpy ((char *) nuser + userlen, > - ndomain ?: "", domainlen); > - } > - > - char *wp = buffer + buffilled; > - wp = memmove (wp, nhost ?: "", hostlen); > - wp += hostlen; > - wp = memmove (wp, nuser ?: "", userlen); > - wp += userlen; > - wp = memmove (wp, ndomain ?: "", domainlen); > - wp += domainlen; > - buffilled = wp - buffer; > + if (!(addgetnetgrentX_append (scratch, nhost) > + && addgetnetgrentX_append (scratch, nuser) > + && addgetnetgrentX_append (scratch, ndomain))) > + return send_notfound (fd); > ++nentries; > } > else > @@ -317,8 +330,8 @@ addgetnetgrentX (struct database_dyn *db, int fd, request_header *req, > } > else if (status == NSS_STATUS_TRYAGAIN && e == ERANGE) > { > - buflen *= 2; > - *tofreep = buffer = xrealloc (buffer, buflen); > + if (!scratch_buffer_grow (&scratch->tmp)) > + return send_notfound (fd); > } > else if (status == NSS_STATUS_RETURN > || status == NSS_STATUS_NOTFOUND > @@ -351,10 +364,17 @@ addgetnetgrentX (struct database_dyn *db, int fd, request_header *req, > goto maybe_cache_add; > } > > - total = buffilled; > + /* Capture the result size without the key appended. */ > + total = scratch->buffer_used; > + > + /* Make a copy of the key. The scratch buffer must not move after > + this point. */ > + key_copy = addgetnetgrentX_append_n (scratch, key, req->key_len); > + if (key_copy == NULL) > + return send_notfound (fd); > > /* Fill in the dataset. */ > - dataset = (struct dataset *) buffer; > + dataset = scratch->buffer.data; > timeout = datahead_init_pos (&dataset->head, total + req->key_len, > total - offsetof (struct dataset, resp), > he == NULL ? 0 : dh->nreloads + 1, > @@ -363,11 +383,7 @@ addgetnetgrentX (struct database_dyn *db, int fd, request_header *req, > dataset->resp.version = NSCD_VERSION; > dataset->resp.found = 1; > dataset->resp.nresults = nentries; > - dataset->resp.result_len = buffilled - sizeof (*dataset); > - > - assert (buflen - buffilled >= req->key_len); > - key_copy = memcpy (buffer + buffilled, key, req->key_len); > - buffilled += req->key_len; > + dataset->resp.result_len = total - sizeof (*dataset); > > /* Now we can determine whether on refill we have to create a new > record or not. */ > @@ -398,7 +414,7 @@ addgetnetgrentX (struct database_dyn *db, int fd, request_header *req, > if (__glibc_likely (newp != NULL)) > { > /* Adjust pointer into the memory block. */ > - key_copy = (char *) newp + (key_copy - buffer); > + key_copy = (char *) newp + (key_copy - (char *) dataset); > > dataset = memcpy (newp, dataset, total + req->key_len); > cacheable = true; > @@ -439,7 +455,7 @@ addgetnetgrentX (struct database_dyn *db, int fd, request_header *req, > } > > out: > - *resultp = dataset; > + scratch->dataset = dataset; > > return timeout; > } > @@ -460,6 +476,9 @@ addinnetgrX (struct database_dyn *db, int fd, request_header *req, > if (user != NULL) > key = strchr (key, '\0') + 1; > const char *domain = *key++ ? key : NULL; > + struct addgetnetgrentX_scratch scratch; > + > + addgetnetgrentX_scratch_init (&scratch); > > if (__glibc_unlikely (debug_level > 0)) > { > @@ -475,12 +494,8 @@ addinnetgrX (struct database_dyn *db, int fd, request_header *req, > group, group_len, > db, uid); > time_t timeout; > - void *tofree; > if (result != NULL) > - { > - timeout = result->head.timeout; > - tofree = NULL; > - } > + timeout = result->head.timeout; > else > { > request_header req_get = > @@ -489,7 +504,10 @@ addinnetgrX (struct database_dyn *db, int fd, request_header *req, > .key_len = group_len > }; > timeout = addgetnetgrentX (db, -1, &req_get, group, uid, NULL, NULL, > - &result, &tofree); > + &scratch); > + result = scratch.dataset; > + if (timeout < 0) > + goto out; > } > > struct indataset > @@ -601,7 +619,7 @@ addinnetgrX (struct database_dyn *db, int fd, request_header *req, > } > > out: > - free (tofree); > + addgetnetgrentX_scratch_free (&scratch); > return timeout; > } > > @@ -611,11 +629,12 @@ addgetnetgrentX_ignore (struct database_dyn *db, int fd, request_header *req, > const char *key, uid_t uid, struct hashentry *he, > struct datahead *dh) > { > - struct dataset *ignore; > - void *tofree; > - time_t timeout = addgetnetgrentX (db, fd, req, key, uid, he, dh, > - &ignore, &tofree); > - free (tofree); > + struct addgetnetgrentX_scratch scratch; > + addgetnetgrentX_scratch_init (&scratch); > + time_t timeout = addgetnetgrentX (db, fd, req, key, uid, he, dh, &scratch); > + addgetnetgrentX_scratch_free (&scratch); > + if (timeout < 0) > + timeout = 0; > return timeout; > } > > @@ -659,5 +678,9 @@ readdinnetgr (struct database_dyn *db, struct hashentry *he, > .key_len = he->len > }; > > - return addinnetgrX (db, -1, &req, db->data + he->key, he->owner, he, dh); > + int timeout = addinnetgrX (db, -1, &req, db->data + he->key, he->owner, > + he, dh); > + if (timeout < 0) > + timeout = 0; > + return timeout; > }