From owner-freebsd-bugs@FreeBSD.ORG Sun Jun 22 13:50:03 2008 Return-Path: Delivered-To: freebsd-bugs@hub.freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:4f8:fff6::34]) by hub.freebsd.org (Postfix) with ESMTP id 7E103106564A for ; Sun, 22 Jun 2008 13:50:03 +0000 (UTC) (envelope-from gnats@FreeBSD.org) Received: from freefall.freebsd.org (freefall.freebsd.org [IPv6:2001:4f8:fff6::28]) by mx1.freebsd.org (Postfix) with ESMTP id 729858FC0A for ; Sun, 22 Jun 2008 13:50:03 +0000 (UTC) (envelope-from gnats@FreeBSD.org) Received: from freefall.freebsd.org (gnats@localhost [127.0.0.1]) by freefall.freebsd.org (8.14.2/8.14.2) with ESMTP id m5MDo37p077772 for ; Sun, 22 Jun 2008 13:50:03 GMT (envelope-from gnats@freefall.freebsd.org) Received: (from gnats@localhost) by freefall.freebsd.org (8.14.2/8.14.1/Submit) id m5MDo3p8077771; Sun, 22 Jun 2008 13:50:03 GMT (envelope-from gnats) Date: Sun, 22 Jun 2008 13:50:03 GMT Message-Id: <200806221350.m5MDo3p8077771@freefall.freebsd.org> To: freebsd-bugs@FreeBSD.org From: Dan Lukes Cc: Subject: Re: bin/83358: [patch] improper handling of malloc failures within rexec() X-BeenThere: freebsd-bugs@freebsd.org X-Mailman-Version: 2.1.5 Precedence: list Reply-To: Dan Lukes List-Id: Bug reports List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Sun, 22 Jun 2008 13:50:03 -0000 The following reply was made to PR bin/83358; it has been noted by GNATS. From: Dan Lukes To: Garrett Cooper Cc: bug-followup@FreeBSD.org Subject: Re: bin/83358: [patch] improper handling of malloc failures within rexec() Date: Sun, 22 Jun 2008 15:43:42 +0200 Garrett Cooper wrote: > This patch includes the fixes provided previously (minus the port var > removal provided by the OP), plus some style fixes and source > updating. > + if (*aname == 0) { > + *aname = malloc(strlen(tokval)+1); > + if (*aname == NULL) { > + warnx("malloc for aname failed\n"); > + goto bad; > + } > + strcpy(*aname, tokval); Minor: 1. Are you hate strdup() for a reason (in place of malloc+strcpy)? Very minor: 2. Are you sure the average user will be more happy with "malloc for aname failed" than with "Cannot allocate memory for user name" ? > bad: > - (void) fclose(cfile); > - return (-1); > + ret_code = -1; > +done: > + fclose(cfile); > + return ret_code; > } Major: 3. I found no exact specification of library function ruserpass(). It seems you may got *aname, *apass, *aacct NULL or non-NULL on enter. In the case you got NULL on enter, the memory may be allocated during the processing and returned to the caller. Now what about proper cleanup (against memory leaks) ? If the function exit with 0, it's clear - the caller must free the allocated memories. But what when the function fail ? To be on the safe side I would recommend to free() the memory if allocated by the failing ruserpass() But if you are sure (e.g. you know the specification of ...) the caller will free it even in the case of nonzero return code, it's not necesarry. > Which makes me wonder: is libcompat-4.3's rexec.c used for compiling > purposes at all? The cpio and tar seems to use it. By the way, it's very hard to analyze the patch containing both functional and stylistic changes ... Dan