Re: [Nbd] Patches to fix a coredump and improve error reporting

[Wouter Verhelst <w@...112...>]

On Thu, Oct 25, 2007 at 08:37:37AM -0500, Corey Minyard wrote:
> The functions getpwnam and getgrnam return NULL on error, but this
> wasn't being checked.  Add checks.
> 
> Index: nbd-2.9.6/nbd-server.c
> ===================================================================
> --- nbd-2.9.6.orig/nbd-server.c
> +++ nbd-2.9.6/nbd-server.c
> @@ -1536,11 +1536,19 @@ void dousers(void) {
>  	struct group *gr;
>  	if(runuser) {
>  		pw=getpwnam(runuser);
> +		if(!pw) {
> +			g_message("Invalid user name: %s", runuser);
> +			exit(EXIT_FAILURE);
> +		}
>  		if(setuid(pw->pw_uid)<0)
>  			msg3(LOG_DEBUG, "Could not set UID: %s", strerror(errno));
>  	}
>  	if(rungroup) {
>  		gr=getgrnam(rungroup);
> +		if(!gr) {
> +			g_message("Invalid group name: %s", rungroup);
> +			exit(EXIT_FAILURE);
> +		}
>  		if(setgid(gr->gr_gid)<0)
>  			msg3(LOG_DEBUG, "Could not set GID: %s", strerror(errno));
>  	}

Good catch, thanks.

> @@ -1596,9 +1604,9 @@ int main(int argc, char *argv[]) {
>  		g_message("Nothing to do! Bye!");
>  		exit(EXIT_FAILURE);
>  	}
> +	dousers();
>  	daemonize(serve);
>  	setup_servers(servers);
> -	dousers();

No. That will change documented behaviour: user/group are currently
defined as "the user or group nbd-server will change to after opening
ports, but before opening files". For reference, opening ports is done
in setup_serve, and and opening files as part of serveloop.

That plus the fact that you've prepared this patch against 2.9.6 whereas
I'm working on 2.9.8 in subversion ATM probably also means your next
patch will probably need to be slightly modified, which I'm not going to
do at 2:15 AM. I'll do so tomorrow -- unless you beat me to it ;-)

-- 
<Lo-lan-do> Home is where you have to wash the dishes.
  -- #debian-devel, Freenode, 2004-09-22