Bug 589056

Created attachment 411524 [details]
patch #1

I only removed two unintentional hunks removing the <stdlib.h> include.

Created attachment 411525 [details]
patch #2

Patches work, stdlib removal was non-intentional though

Comment on attachment 411524 [details]
patch #1

>@@ -29,7 +29,7 @@ th_get_pathname(TAR *t)
> 	char filename[MAXPATHLEN];

We agreed with Huzaifa that making the 'filename' static would lead to easier code since all the strdup/free would be no more necessary.

>@@ -92,10 +92,6 @@ gzopen_frontend(char *pathname, int oflags, int mode)
> 		return -1;
> 	}
> 
>-	/* This is a bad thing to do on big-endian lp64 systems, where the
>-	   size and placement of integers is different than pointers.
>-	   However, to fix the problem 4 wrapper functions would be needed and
>-	   an extra bit of data associating GZF with the wrapper functions.  */

As the comment still seems to be relevant, we may keep it there.

Comment on attachment 411525 [details]
patch #2

>@@ -245,7 +245,12 @@ extract(char *tarfile, char *rootdir)
> #endif
> 	if (tar_extract_all(t, rootdir) != 0)
> 	{
>+		
> 		fprintf(stderr, "tar_extract_all(): %s\n", strerror(errno));
>+		if (t->h)
>+			libtar_hash_free(t->h,NULL);
>+		if (t)		
>+			free (t);

The second condition can be never evaluated as false.  I suggest something like this:

    if (t)
    {
        if (t->h)
            libtar_hash_free(t->h,NULL);

        free (t);
    }

>@@ -255,6 +260,10 @@ extract(char *tarfile, char *rootdir)
> 	if (tar_close(t) != 0)
> 	{
> 		fprintf(stderr, "tar_close(): %s\n", strerror(errno));
>+		if (t->h)
>+			libtar_hash_free(t->h, NULL);
>+		if (t)
>+			free (t);

The same here.

Overall the patches look good.  Thank you for working on them!

Created attachment 416325 [details]
revisited patch

I've just put the patches together and addressed the review comments.

Huzaifa, could you please have a look whether it still works fine for you?

Thanks in advance!

Comment on attachment 416325 [details]
revisited patch

Looking once again, I considered use of the static array as really bad idea.  It would work fairly well in tar(1), but not so well in a library which may be used from a multi-threaded environment.

Created attachment 417159 [details]
revisited patch V2

reverted the static array change and fixed some other memory leaks in libtar.c

Created attachment 417160 [details]
revisited patch V2

reverted the static array change and fixed some other memory leaks in libtar.c

Comment on attachment 417160 [details]
revisited patch V2

Oops, race condition.  No reason the have the same patch twice here...

built as libtar-1.2.11-19.fc14