[PATCH] tar --to-command

Denys Vlasenko vda.linux at googlemail.com
Thu Jun 24 02:51:49 UTC 2010


On Monday 21 June 2010 19:41, Ladislav Michl wrote:
> On Mon, Jun 21, 2010 at 04:19:43PM +0200, Ladislav Michl wrote:
> > On Sat, Jun 19, 2010 at 08:41:13PM +0200, Denys Vlasenko wrote:
> > > On Friday 18 June 2010 11:11, Ladislav Michl wrote:
> > > +                       xsetenv("TAR_FILENAME", file_header->name);
> > > +                       xsetenv("TAR_SIZE", file_header->size);
> > > 
> > > This might leak memory on parent side.
> > 
> > Detect NOMMU and unsetenv then?
> > 
> > Other comments addressed in v2 below
> 
> Well, I had some spare time this afternoon, so here is v3 with unsets env
> on NOMMU.

No, unsetenv does not free the string, because it does not know
whether the string *can* be freed. Consider:

	putenv("VAR=VAL");
        ...
        unsetenv("VAR"); // kaboom

Here is how mdev.c does it:

                                char *s = xasprintf("%s=%s", "MDEV", node_name);
                                char *s1 = xasprintf("%s=%s", "SUBSYSTEM", G.subsystem);
                                putenv(s);
                                putenv(s1);
                                if (system(command) == -1)
                                        bb_perror_msg("can't run '%s'", command);
                                bb_unsetenv(s1);
                                free(s1);
                                bb_unsetenv(s);
                                free(s);


> +static const char *tar_env[] = {

static const char *const tar_env[] =
                   ^^^^^

> +	"TAR_FILETYPE",
> +	"TAR_MODE",
> +	"TAR_FILENAME",
> +	"TAR_REALNAME",
> +#if ENABLE_FEATURE_TAR_UNAME_GNAME
> +	"TAR_UNAME",
> +	"TAR_GNAME",
> +#endif
> +	"TAR_SIZE",
> +	"TAR_UID",
> +	"TAR_GID",
> +};

Since you will need to do xasprintf("%s=%s", tar_env[i], val)
anyway to deal with setenv leak, use
xasprintf("TAR_%s=%s", tar_env[i], val) and drop "TAR_"
prefix from every variable.


> +		static const char *sh = "/bin/sh";

static const char sh[] = "/bin/sh",
but better just use literal "/bin/sh" where you need the string.

> +		xpipe(p);
> +		sig_pipe = signal(SIGPIPE, SIG_IGN);
> +		pid = BB_MMU ? fork() : vfork();
> +		switch (pid) {
> +		case -1:
> +			bb_perror_msg_and_die("can't fork %d\n", errno);

Use simple bb_perror_msg_and_die("fork")

> +             char buf[8];
....
> +		case 0:
> +			/* Child */
> +			xdup2(p[0], STDIN_FILENO);
> +			close(p[1]);
> +
> +			xsetenv(tar_env[TAR_FILETYPE], "f");
> +			snprintf(buf, sizeof buf, "0%lo", (unsigned long) file_header->mode);

char buf[sizeof(int)*3];
....
sprintf(buf, "0%o", (unsigned) file_header->mode);



+               sighandler_t sig_pipe;
sighandler_t is a glibc'ism, some other libcs do not have
that typedef. Use explicit void (*f)(int) type.
Give sig_pipe better name (saved_sigpipe?).

+               sig_pipe = signal(SIGPIPE, SIG_IGN);
...
> +		signal(SIGPIPE, sig_pipe);

Move signal(SIGPIPE, SIG_IGN) / signal(SIGPIPE, sig_pipe)
out of the loop.


> +
> +		while (wait(&status) == -1)
> +			if (errno != EINTR)
> +				bb_perror_msg_and_die("error %d waiting for child\n", errno);

bb_perror_msg_and_die("wait");

> +		if (WIFEXITED(status) && WEXITSTATUS(status))
> +			bb_perror_msg_and_die("'%s' returned status %d\n",
> +				archive_handle->tar__to_command, WEXITSTATUS(status));
> +		if (WIFSIGNALED(status))
> +			bb_perror_msg_and_die("'%s' terminated on signal %d\n",
> +				archive_handle->tar__to_command, WTERMSIG(status));

"error", not "perror" - you do not want to print errno.


-- 
vda


More information about the busybox mailing list