[PATCH] tar: remove leading / and ../ on reading and writing

[Alexander Shishkin]

On Mon, Feb 28, 2011 at 05:57:37PM +0100, walter harms wrote:
> 

Thanks for your comments, I'll see if I can come up with something smaller.

> 
> Am 28.02.2011 11:24, schrieb Alexander Shishkin:
> > Currently, tar will add members with names starting with
> > the prefixes to an archive unmodified, and will then refuse
> > to extract from such archive. However, GNU tar will strip
> > these prefixes upon creating the archive and reading from
> > it.
> > 
> > function                                             old     new   delta
> > strip_leading_junk                                     -     130    +130
> > static.pfx                                             -      32     +32
> > run_applet_and_exit                                  811     810      -1
> > .rodata                                           147143  147142      -1
> > writeFileToTarball                                   535     500     -35
> > get_header_tar                                      1642    1598     -44
> > ------------------------------------------------------------------------------
> > (add/remove: 2/0 grow/shrink: 0/4 up/down: 162/-81)            Total: 81 bytes
> > 
> > Signed-off-by: Alexander Shishkin <virtuoso at slind.org>
> > ---
> >  archival/libarchive/get_header_tar.c |   48 +++++++++++++++++++++++++++------
> >  archival/tar.c                       |   11 +-------
> >  include/archive.h                    |    2 +
> >  3 files changed, 42 insertions(+), 19 deletions(-)
> > 
> > diff --git a/archival/libarchive/get_header_tar.c b/archival/libarchive/get_header_tar.c
> > index 2e03327..d4e904c 100644
> > --- a/archival/libarchive/get_header_tar.c
> > +++ b/archival/libarchive/get_header_tar.c
> > @@ -118,11 +118,44 @@ static char *get_selinux_sctx_from_pax_hdr(archive_handle_t *archive_handle, uns
> >  }
> >  #endif
> >  
> > +FAST_FUNC const char *strip_leading_junk(const char *file_name)
> > +{
> > +	static struct {
> > +		const char *prefix;
> > +		int len;
> > +	} pfx[] = {
> > +		{"/", 1},
> > +		{"../", 3}
> > +	};
> > +
> > +	while (*file_name) {
> > +		static smallint warned;
> > +		int i;
> > +
> > +		for (i = 0; i < ARRAY_SIZE(pfx); i++) {
> > +			if (!strncmp(file_name, pfx[i].prefix, pfx[i].len)) {
> > +				file_name += pfx[i].len;
> > +				break;
> > +			}
> > +		}
> 
> 
> do you expect much more ? maybe 2 * strcmp() is smaller ?

Not if you move the warning inside.

> 
> 
> > +		if (i == ARRAY_SIZE(pfx))
> > +			break;
> 
> 	perhaps you can move the warning into the if strcmp() and make a function from this loop ?

Not sure if there's any value in that.

> 	btw: you are warning only once, ok but the pattern changes from "/" to "../" i will not see
> 	the warning again. is that intended ?

GNU tar will print a warning once per prefix. I could add another field
to the structure to track that instead, but I'm not sure if there's much
value in that warning too.

> 
> 
> > +		if (!warned) {
> > +			bb_error_msg("removing leading '%s' from member names",
> > +				     pfx[i].prefix);
> > +			warned = 1;
> > +		}
> > +	}
> > +
> > +	return file_name;
> > +}
> > +
> >  char FAST_FUNC get_header_tar(archive_handle_t *archive_handle)
> >  {
> >  	file_header_t *file_header = archive_handle->file_header;
> >  	struct tar_header_t tar;
> > -	char *cp;
> > +	char *cp, *real_name;
> >  	int i, sum_u, sum;
> >  #if ENABLE_FEATURE_TAR_OLDSUN_COMPATIBILITY
> >  	int sum_s;
> > @@ -422,12 +455,9 @@ char FAST_FUNC get_header_tar(archive_handle_t *archive_handle)
> >  		p_linkname = NULL;
> >  	}
> >  #endif
> > -	if (strncmp(file_header->name, "/../"+1, 3) == 0
> > -	 || strstr(file_header->name, "/../")
> > -	) {
> > -		bb_error_msg_and_die("name with '..' encountered: '%s'",
> > -				file_header->name);
> > -	}
> > +
> > +	real_name = file_header->name;
> > +	file_header->name = (char *)strip_leading_junk(real_name);
> 
> i guess this (char *) is caused by the cons char * of the function ? may be just removing const is simpler.

Technically, it's the same string. And writeFileToTarball() will call it with
a const char * parameter.

> 
> 
> >  	/* Strip trailing '/' in directories */
> >  	/* Must be done after mode is set as '/' is used to check if it's a directory */
> > @@ -443,10 +473,10 @@ char FAST_FUNC get_header_tar(archive_handle_t *archive_handle)
> >  		if (archive_handle->accept || archive_handle->reject)
> >  			llist_add_to(&archive_handle->passed, file_header->name);
> >  		else /* Caller isn't interested in list of unpacked files */
> > -			free(file_header->name);
> > +			free(real_name);
> >  	} else {
> >  		data_skip(archive_handle);
> > -		free(file_header->name);
> > +		free(real_name);
> >  	}
> >  	archive_handle->offset += file_header->size;
> >  
> > diff --git a/archival/tar.c b/archival/tar.c
> > index 1e3cecf..4f5087e 100644
> > --- a/archival/tar.c
> > +++ b/archival/tar.c
> > @@ -398,16 +398,7 @@ static int FAST_FUNC writeFileToTarball(const char *fileName, struct stat *statb
> >  	DBG("writeFileToTarball('%s')", fileName);
> >  
> >  	/* Strip leading '/' (must be before memorizing hardlink's name) */
> > -	header_name = fileName;
> > -	while (header_name[0] == '/') {
> > -		static smallint warned;
> > -
> > -		if (!warned) {
> > -			bb_error_msg("removing leading '/' from member names");
> > -			warned = 1;
> > -		}
> > -		header_name++;
> > -	}
> > +	header_name = strip_leading_junk(fileName);
> >  
> >  	if (header_name[0] == '\0')
> >  		return TRUE;
> > diff --git a/include/archive.h b/include/archive.h
> > index 49c4787..e619c0e 100644
> > --- a/include/archive.h
> > +++ b/include/archive.h
> > @@ -224,6 +224,8 @@ int bbunpack(char **argv,
> >  	    const char *expected_ext
> >  ) FAST_FUNC;
> >  
> > +const char *strip_leading_junk(const char *file_name) FAST_FUNC;
> > +
> >  #if BB_MMU
> >  void open_transformer(int fd,
> >  	IF_DESKTOP(long long) int FAST_FUNC (*transformer)(int src_fd, int dst_fd)) FAST_FUNC;