From owner-freebsd-current@FreeBSD.ORG Wed Nov 26 03:33:51 2008 Return-Path: Delivered-To: freebsd-current@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:4f8:fff6::34]) by hub.freebsd.org (Postfix) with ESMTP id 1B76D106564A; Wed, 26 Nov 2008 03:33:51 +0000 (UTC) (envelope-from delphij@delphij.net) Received: from tarsier.delphij.net (delphij-pt.tunnel.tserv2.fmt.ipv6.he.net [IPv6:2001:470:1f03:2c9::2]) by mx1.freebsd.org (Postfix) with ESMTP id 94C108FC1B; Wed, 26 Nov 2008 03:33:49 +0000 (UTC) (envelope-from delphij@delphij.net) Received: from tarsier.geekcn.org (tarsier.geekcn.org [211.166.10.233]) (using TLSv1 with cipher ADH-CAMELLIA256-SHA (256/256 bits)) (No client certificate requested) by tarsier.delphij.net (Postfix) with ESMTPS id D05D928449; Wed, 26 Nov 2008 11:33:48 +0800 (CST) Received: from localhost (tarsier.geekcn.org [211.166.10.233]) by tarsier.geekcn.org (Postfix) with ESMTP id BA21AEB653B; Wed, 26 Nov 2008 11:33:47 +0800 (CST) X-Virus-Scanned: amavisd-new at geekcn.org Received: from tarsier.geekcn.org ([211.166.10.233]) by localhost (mail.geekcn.org [211.166.10.233]) (amavisd-new, port 10024) with ESMTP id BIWMOM-B2tZs; Wed, 26 Nov 2008 11:33:41 +0800 (CST) Received: from delta.delphij.net (c-76-103-40-85.hsd1.ca.comcast.net [76.103.40.85]) (using TLSv1 with cipher DHE-RSA-AES256-SHA (256/256 bits)) (No client certificate requested) by tarsier.geekcn.org (Postfix) with ESMTPSA id 4A6E5EB63A4; Wed, 26 Nov 2008 11:33:40 +0800 (CST) DomainKey-Signature: a=rsa-sha1; s=default; d=delphij.net; c=nofws; q=dns; h=message-id:date:from:reply-to:organization:user-agent: mime-version:to:cc:subject:references:in-reply-to: x-enigmail-version:openpgp:content-type; b=C3ohUBgauWPsAhUkBc78d267R1ygi5zr4Ysgvk+doB3vkC2ZagMbDIsyE0/OQCmnl 3Fft4AiyZof+lK0iSoYoQ== Message-ID: <492CC391.2070207@delphij.net> Date: Tue, 25 Nov 2008 19:33:37 -0800 From: Xin LI Organization: The Geek China Organization User-Agent: Thunderbird 2.0.0.18 (X11/20081123) MIME-Version: 1.0 To: insomniac References: <20081126032214.03d8517a@slackware.it> In-Reply-To: <20081126032214.03d8517a@slackware.it> X-Enigmail-Version: 0.95.7 OpenPGP: id=18EDEBA0; url=http://www.delphij.net/delphij.asc Content-Type: multipart/mixed; boundary="------------000809080902040307050305" Cc: freebsd-current@freebsd.org, kensmith@freebsd.org Subject: Re: Patch for bin/54446 X-BeenThere: freebsd-current@freebsd.org X-Mailman-Version: 2.1.5 Precedence: list Reply-To: d@delphij.net List-Id: Discussions about the use of FreeBSD-current List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 26 Nov 2008 03:33:51 -0000 This is a multi-part message in MIME format. --------------000809080902040307050305 Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit -----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1 Hi, insomniac wrote: > Hi to everyone, > I wrote a patch for the bin/54446 PR, fixing pkg_delete(1) that doesn't > honour symlinks, and portupgrades leads to failing services. > > Actually, this patch fixes that for all the utilities as it acts > directly in the lib. > > I tested the patch on a few x86 machines, ranging from 7.0 to -HEAD. > Testing and further reviewing are welcome and encouraged. > > pkg_delete now seems to work fine; moreover I found other bugs, like > memory leaks, missing checks of function return values, and wrong return > values. > > The patch has already been reviewed by attilio@ , it applies to > src/usr.sbin/pkg_install/lib/plist.c and is located at > > http://insomniac.slackware.it/plist.c.diff I have made a small change: use malloc() here and use strlcpy(). Other parts looks just fine. (BTW I think we need to cc portmgr@ for approval) Cheers, - -- Xin LI http://www.delphij.net/ FreeBSD - The Power to Serve! -----BEGIN PGP SIGNATURE----- Version: GnuPG v2.0.9 (FreeBSD) iEYEARECAAYFAkksw5EACgkQi+vbBBjt66CVaACfRah4OMrOiFZKzJ3DvzjTnl3K sE8AnRQeL3lKC/fSnzJn89IQHMAgoudI =loiW -----END PGP SIGNATURE----- --------------000809080902040307050305 Content-Type: text/x-patch; name="plist.diff" Content-Transfer-Encoding: 7bit Content-Disposition: inline; filename="plist.diff" Index: usr.sbin/pkg_install/lib/plist.c =================================================================== --- usr.sbin/pkg_install/lib/plist.c (revision 185325) +++ usr.sbin/pkg_install/lib/plist.c (working copy) @@ -544,45 +544,90 @@ delete_package(Boolean ign_err, Boolean nukedirs, int delete_hierarchy(const char *dir, Boolean ign_err, Boolean nukedirs) { - char *cp1, *cp2; + char *cp1, *cp2, *realdir; - cp1 = cp2 = strdup(dir); - if (!fexists(dir)) { + realdir = malloc(FILENAME_MAX); + if (realdir == NULL) { + warnx("Couldn't allocate enough memory\n"); + return (ign_err ? SUCCESS : FAIL); + } + + if (issymlink(dir) && readlink(dir, realdir, FILENAME_MAX-1) == -1) + return (ign_err ? SUCCESS : FAIL); + + strlcpy(realdir, dir, FILENAME_MAX); + + cp1 = cp2 = strdup(realdir); + if (cp1 == NULL) { + warnx("Couldn't allocate enough memory\n"); + return (ign_err ? SUCCESS : FAIL); + } + + if (!fexists(realdir)) { if (!ign_err) warnx("%s '%s' doesn't exist", - isdir(dir) ? "directory" : "file", dir); - return !ign_err; + isdir(realdir) ? "directory" : "file", realdir); + free(cp1); + free(realdir); + return (ign_err ? SUCCESS : FAIL); } else if (nukedirs) { - if (vsystem("%s -r%s %s", REMOVE_CMD, (ign_err ? "f" : ""), dir)) - return 1; + if (vsystem("%s -r%s %s", REMOVE_CMD, (ign_err ? "f" : ""), realdir)) { + free(cp1); + free(realdir); + return (ign_err ? SUCCESS : FAIL); + } } - else if (isdir(dir) && !issymlink(dir)) { - if (RMDIR(dir) && !ign_err) - return 1; + else if (isdir(realdir)) { + if (RMDIR(realdir)) { + free(cp1); + free(realdir); + return (ign_err ? SUCCESS : FAIL); + } } else { - if (REMOVE(dir, ign_err)) - return 1; + if (REMOVE(realdir, ign_err)) { + free(cp1); + free(realdir); + return (ign_err ? SUCCESS : FAIL); + } } - if (!nukedirs) - return 0; + if (!nukedirs) { + free(cp1); + free(realdir); + return (SUCCESS); + } while (cp2) { if ((cp2 = strrchr(cp1, '/')) != NULL) *cp2 = '\0'; - if (!isemptydir(dir)) - return 0; - if (RMDIR(dir) && !ign_err) { - if (!fexists(dir)) - warnx("directory '%s' doesn't exist", dir); - else - return 1; + if (!isemptydir(realdir)) { + free(cp1); + free(realdir); + return (SUCCESS); } + if (RMDIR(realdir) && !ign_err) { + if (!fexists(realdir)) { + warnx("directory '%s' doesn't exist", realdir); + return (SUCCESS); + } else { + free(cp1); + free(realdir); + return (FAIL); + } + } /* back up the pathname one component */ if (cp2) { - cp1 = strdup(dir); + free(cp1); + cp1 = strdup(realdir); + if (cp1 == NULL) { + warnx("Couldn't allocate enough memory\n"); + return (ign_err ? SUCCESS : FAIL); + } } } - return 0; + free(cp1); + free(realdir); + return (SUCCESS); } + --------------000809080902040307050305--