Bug 83358 - [libcompat] [patch] improper handling of malloc failures within rexec(3)
Summary: [libcompat] [patch] improper handling of malloc failures within rexec(3)
Status: Open
Alias: None
Product: Base System
Classification: Unclassified
Component: kern (show other bugs)
Version: 5.4-STABLE
Hardware: Any Any
: Normal Affects Only Me
Assignee: freebsd-bugs mailing list
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2005-07-12 23:30 UTC by Dan Lukes
Modified: 2018-05-20 23:50 UTC (History)
0 users

See Also:


Attachments
patch (3.08 KB, patch)
2005-07-12 23:30 UTC, Dan Lukes
no flags Details | Diff
4_3_compat_update.patch (20.76 KB, patch)
2008-06-21 06:07 UTC, Garrett Cooper
no flags Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Dan Lukes 2005-07-12 23:30:08 UTC
	1. malloc() not tested for failures within ruserpass()
	2. port variable within rexec() can be uset uninitialized causing
possible close of random socket (within bad: part of code)
Comment 1 Garrett Cooper 2008-06-21 06:07:16 UTC
This patch includes the fixes provided previously (minus the port var
removal provided by the OP), plus some style fixes and source
updating.

Which makes me wonder: is libcompat-4.3's rexec.c used for compiling
purposes at all?

-Garrett
Comment 2 Dan Lukes 2008-06-22 14:43:42 UTC
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
Comment 3 Pedro F. Giffuni freebsd_committer 2015-02-19 20:32:23 UTC
FWIW, The undefined "port" variable was fixed in r278867.
Comment 4 Eitan Adler freebsd_committer freebsd_triage 2018-05-20 23:50:23 UTC
For bugs matching the following conditions:
- Status == In Progress
- Assignee == "bugs@FreeBSD.org"
- Last Modified Year <= 2017

Do
- Set Status to "Open"