From patchwork Thu Mar 27 20:32:07 2014 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Patchwork-Submitter: Ondrej Bilka X-Patchwork-Id: 323 Return-Path: X-Original-To: siddhesh@wilcox.dreamhost.com Delivered-To: siddhesh@wilcox.dreamhost.com Received: from homiemail-mx21.g.dreamhost.com (caibbdcaaahb.dreamhost.com [208.113.200.71]) by wilcox.dreamhost.com (Postfix) with ESMTP id B17E23605B8 for ; Thu, 27 Mar 2014 13:32:18 -0700 (PDT) Received: by homiemail-mx21.g.dreamhost.com (Postfix, from userid 14307373) id 55219129F49D; Thu, 27 Mar 2014 13:32:18 -0700 (PDT) X-Original-To: glibc@patchwork.siddhesh.in Delivered-To: x14307373@homiemail-mx21.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-mx21.g.dreamhost.com (Postfix) with ESMTPS id 1F13112A11B8 for ; Thu, 27 Mar 2014 13:32:18 -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=i8iDuWC3OT/Rk6ZtuBgfZLXiMs/zW9 AxemWcKNwYQ+SYcc9JMAxHt191M7Hl1FUsolkaiDVwdr4ELlowllrDSN/ZSmUtVF x59z0r6ZvZEh2Nw925C1YqqU8Kc+lMgCCWy8c6PGkFNKv8PniT/c7FaPT1Kroqf8 bQ6Ah+jhNQ2I0= 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=BxBdRGY2btd1SVcliGew6oEgjD8=; b=CPd2 PD1nSR9scrMnZYarso3jk3suiluW5DxS/GeXn0jdzNO8KfMzdXFst0EPUFMrU1Ao I/Ll4FFGZ+Ng7+2OncMmo05bEZk2hCUDD1aY0eIiD+wtK5h/XkMJ3fEBxrb14cfT G84VOyuiW/qDCAEVwjiiapaHD/pPwwAcDebuhSM= Received: (qmail 23721 invoked by alias); 27 Mar 2014 20:32:16 -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 23709 invoked by uid 89); 27 Mar 2014 20:32:15 -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 21:32:07 +0100 From: =?utf-8?B?T25kxZllaiBCw61sa2E=?= To: Mike Frysinger Cc: libc-alpha@sourceware.org, Siddhesh Poyarekar Subject: [PATCH] Clean up netgroupcache Message-ID: <20140327203207.GA2476@domone.podge> References: <20140327040406.GA26264@spoyarek.pnq.redhat.com> <1499542.yzGAIksTkn@vapier> <20140327192254.GC1982@domone.podge> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <20140327192254.GC1982@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 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. * nscd/netgroupcache.c: Clean up implementation. diff --git a/nscd/netgroupcache.c b/nscd/netgroupcache.c index 820d823..0a0aa87 100644 --- a/nscd/netgroupcache.c +++ b/nscd/netgroupcache.c @@ -159,6 +159,10 @@ addgetnetgrentX (struct database_dyn *db, int fd, request_header *req, memset (&data, '\0', sizeof (data)); buffer = xmalloc (buflen); + + size_t tempbuflen = 1024; + char *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); + tempbuf *= 2; + tempbuflen = xrealloc (tempbuf, tempbuflen); } } @@ -474,6 +435,7 @@ addgetnetgrentX (struct database_dyn *db, int fd, request_header *req, } out: + free (tempbuf); free (buffer); *resultp = dataset;