Note: This is a beta release of Red Hat Bugzilla 5.0. The data contained within is a snapshot of the live data so any changes you make will not be reflected in the production Bugzilla. Also email is disabled so feel free to test any aspect of the site that you want. File any problems you find or give feedback here.
Bug 589056 - review of patches addressing memory leaks in libtar
Summary: review of patches addressing memory leaks in libtar
Keywords:
Status: CLOSED RAWHIDE
Alias: None
Product: Fedora
Classification: Fedora
Component: libtar
Version: rawhide
Hardware: All
OS: Linux
low
medium
Target Milestone: ---
Assignee: Kamil Dudka
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
Depends On:
Blocks: 595635
TreeView+ depends on / blocked
 
Reported: 2010-05-05 09:23 UTC by Kamil Dudka
Modified: 2010-05-27 14:16 UTC (History)
2 users (show)

Fixed In Version: libtar-1.2.11-19.fc14
Doc Type: Bug Fix
Doc Text:
Clone Of:
: 595635 (view as bug list)
Environment:
Last Closed: 2010-05-27 14:16:41 UTC


Attachments (Terms of Use)
patch #1 (deleted)
2010-05-05 09:26 UTC, Kamil Dudka
no flags Details | Diff
patch #2 (deleted)
2010-05-05 09:27 UTC, Kamil Dudka
no flags Details | Diff
revisited patch (deleted)
2010-05-25 08:57 UTC, Kamil Dudka
no flags Details | Diff
revisited patch V2 (deleted)
2010-05-27 09:18 UTC, Kamil Dudka
huzaifas: review+
Details | Diff
revisited patch V2 (deleted)
2010-05-27 09:20 UTC, Kamil Dudka
no flags Details | Diff

Description Kamil Dudka 2010-05-05 09:23:20 UTC
Patches contributed by Huzaifa Sidhpurwala.

Version-Release number of selected component (if applicable):
libtar-1.2.11-18.fc14

Comment 1 Kamil Dudka 2010-05-05 09:26:32 UTC
Created attachment 411524 [details]
patch #1

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

Comment 2 Kamil Dudka 2010-05-05 09:27:37 UTC
Created attachment 411525 [details]
patch #2

Comment 3 Huzaifa S. Sidhpurwala 2010-05-05 09:29:20 UTC
Patches work, stdlib removal was non-intentional though

Comment 4 Kamil Dudka 2010-05-05 10:38:15 UTC
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 5 Kamil Dudka 2010-05-05 10:42:00 UTC
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!

Comment 6 Kamil Dudka 2010-05-25 08:57:33 UTC
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 7 Kamil Dudka 2010-05-26 13:02:37 UTC
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.

Comment 8 Kamil Dudka 2010-05-27 09:18:34 UTC
Created attachment 417159 [details]
revisited patch V2

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

Comment 9 Kamil Dudka 2010-05-27 09:20:49 UTC
Created attachment 417160 [details]
revisited patch V2

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

Comment 10 Kamil Dudka 2010-05-27 09:23:11 UTC
Comment on attachment 417160 [details]
revisited patch V2

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

Comment 11 Kamil Dudka 2010-05-27 14:16:41 UTC
built as libtar-1.2.11-19.fc14


Note You need to log in before you can comment on or make changes to this bug.