From patchwork Thu Mar 27 21:15:16 2014 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Patchwork-Submitter: Ondrej Bilka X-Patchwork-Id: 324 Return-Path: X-Original-To: siddhesh@wilcox.dreamhost.com Delivered-To: siddhesh@wilcox.dreamhost.com Received: from homiemail-mx20.g.dreamhost.com (caibbdcaaahc.dreamhost.com [208.113.200.72]) by wilcox.dreamhost.com (Postfix) with ESMTP id C497C360567 for ; Thu, 27 Mar 2014 14:15:26 -0700 (PDT) Received: by homiemail-mx20.g.dreamhost.com (Postfix, from userid 14307373) id 7592F40B24337; Thu, 27 Mar 2014 14:15:26 -0700 (PDT) X-Original-To: glibc@patchwork.siddhesh.in Delivered-To: x14307373@homiemail-mx20.g.dreamhost.com Received: from sourceware.org (server1.sourceware.org [209.132.180.131]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by homiemail-mx20.g.dreamhost.com (Postfix) with ESMTPS id 4CBA740B0A552 for ; Thu, 27 Mar 2014 14:15:26 -0700 (PDT) DomainKey-Signature: a=rsa-sha1; c=nofws; d=sourceware.org; h=list-id :list-unsubscribe:list-subscribe:list-archive:list-post :list-help:sender:date:from:to:cc:subject:message-id:references :mime-version:content-type:content-transfer-encoding :in-reply-to; q=dns; s=default; b=FMddzZVHm0mI+GkbKmxM2hJhP6yOYG EjrL8E7N4yX8Ziie4l9YoHRWYVxI1gA2bG9Ux+WZJ78p+tq/sCQk0lH68liPKva7 bb3WszuKLb8Xex6T161Lj341fk+6JOsM6Apb851NwFvJ26vEw8bYHd/b7z7fCue4 N6vZ7O8my0Ppg= DKIM-Signature: v=1; a=rsa-sha1; c=relaxed; d=sourceware.org; h=list-id :list-unsubscribe:list-subscribe:list-archive:list-post :list-help:sender:date:from:to:cc:subject:message-id:references :mime-version:content-type:content-transfer-encoding :in-reply-to; s=default; bh=1fm8COwYWYPYwL5LeE2Ue+B+peM=; b=MsBo 5pybTArK1CKJ/+sQ6CilimNTTo2wJXUknmn/r6cRD4qm/r5EQSKYomLJ0Av2L0au EXH2equwyO3ELROPkUYr+UVL0eBNNdYLm5cmq+NT+MXeEjUbE2cdA1XkWCqpn+RE DEUSH2bI85e5cjkjz+fjYJJs2+CDij6DxYgUpq8= Received: (qmail 17446 invoked by alias); 27 Mar 2014 21:15:24 -0000 Mailing-List: contact libc-alpha-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Unsubscribe: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: libc-alpha-owner@sourceware.org Delivered-To: mailing list libc-alpha@sourceware.org Received: (qmail 17432 invoked by uid 89); 27 Mar 2014 21:15:23 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-0.7 required=5.0 tests=AWL, BAYES_00, FREEMAIL_FROM, SPF_NEUTRAL autolearn=no version=3.3.2 X-HELO: popelka.ms.mff.cuni.cz Date: Thu, 27 Mar 2014 22:15:16 +0100 From: =?utf-8?B?T25kxZllaiBCw61sa2E=?= To: Mike Frysinger Cc: libc-alpha@sourceware.org, Siddhesh Poyarekar Subject: Re: [PATCH 1.1] Clean up netgroupcache Message-ID: <20140327211516.GB2476@domone.podge> References: <20140327040406.GA26264@spoyarek.pnq.redhat.com> <1499542.yzGAIksTkn@vapier> <20140327192254.GC1982@domone.podge> <20140327203207.GA2476@domone.podge> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <20140327203207.GA2476@domone.podge> User-Agent: Mutt/1.5.20 (2009-06-14) X-DH-Original-To: glibc@patchwork.siddhesh.in On Thu, Mar 27, 2014 at 09:32:07PM +0100, Ondřej Bílka wrote: > On Thu, Mar 27, 2014 at 08:22:54PM +0100, Ondřej Bílka wrote: > > On Thu, Mar 27, 2014 at 03:34:11AM -0400, Mike Frysinger wrote: > > > On Thu 27 Mar 2014 09:34:06 Siddhesh Poyarekar wrote: > > > > Calls to stpcpy from nscd netgroups code will have overlapping source > > > > and destination when all three values in the returned triplet are > > > > non-NULL and in the expected (host,user,domain) order. This is seen > > > > in valgrind as: > > > > > > > > Fix this by using memmove instead of stpcpy. Tested x86_64 using > > > > various combinations of triplets (including NULL and non-NULL ones) to > > > > verify that this works correctly and there are no regressions. > > > > > This could work only with additional assertion that we do not move host > > forward otherwise it could overwrite user. > > > > > i feel like we've wanted an equivalent of stpcpy/memccpy for memmove. good > > > time to add it ? :) > > > > > Yes, it would be better to use this at least internally, perhaps this > > patch instead is cleaner. > > > > Other possibility is keep these in separate header like second snippet, > > do you have better name for that? Also I could make a stpcat and move > > equivalent, not sure with what name. > > > > Her I would fix a root cause of these bugs which is bad design. We mix > > temporary buffer with building result. If we use separate buffers for > > that a code would be lot simpler, I will prepare patch for it. > > > Here is patch that uses separate buffers. > Had typo there, here is fixed patch. * nscd/netgroupcache.c: Clean up implementation. diff --git a/nscd/netgroupcache.c b/nscd/netgroupcache.c index 820d823..1412113 100644 --- a/nscd/netgroupcache.c +++ b/nscd/netgroupcache.c @@ -138,8 +138,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 tempbuflen = 1024; size_t buffilled = sizeof (*dataset); char *buffer = NULL; + char *tempbuf; size_t nentries = 0; size_t group_len = strlen (key) + 1; union @@ -159,6 +161,8 @@ addgetnetgrentX (struct database_dyn *db, int fd, request_header *req, memset (&data, '\0', sizeof (data)); buffer = xmalloc (buflen); + tempbuf = xmalloc (tempbuflen); + first_needed.elem.next = &first_needed.elem; memcpy (first_needed.elem.name, key, group_len); data.needed_groups = &first_needed.elem; @@ -201,8 +205,7 @@ 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, tempbuf, tempbuflen, &e); if (status == NSS_STATUS_RETURN || status == NSS_STATUS_NOTFOUND) /* This was either the last one for this group or the @@ -220,53 +223,11 @@ addgetnetgrentX (struct database_dyn *db, int fd, request_header *req, size_t userlen = strlen (nuser ?: "") + 1; size_t domainlen = strlen (ndomain ?: "") + 1; - if (nhost == NULL || nuser == NULL || ndomain == NULL - || nhost > nuser || nuser > ndomain) + size_t newlen = buffilled + hostlen + userlen + domainlen; + if (newlen + req->key_len > buflen) { - 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); - buffer = newbuf; - } - - nhost = memcpy (buffer + bufused, - nhost ?: "", hostlen); - nuser = memcpy ((char *) nhost + hostlen, - nuser ?: "", userlen); - ndomain = memcpy ((char *) nuser + userlen, - ndomain ?: "", domainlen); + buflen = 2 * newlen + req->key_len; + buffer = xrealloc (buffer, buflen); } char *wp = buffer + buffilled; @@ -328,8 +289,8 @@ addgetnetgrentX (struct database_dyn *db, int fd, request_header *req, } else if (status == NSS_STATUS_UNAVAIL && e == ERANGE) { - buflen *= 2; - buffer = xrealloc (buffer, buflen); + tempbuflen *= 2; + tempbuf = xrealloc (tempbuf, tempbuflen); } } @@ -474,6 +435,7 @@ addgetnetgrentX (struct database_dyn *db, int fd, request_header *req, } out: + free (tempbuf); free (buffer); *resultp = dataset;