Bug 589056 - review of patches addressing memory leaks in libtar
review of patches addressing memory leaks in libtar
Status: CLOSED RAWHIDE
Product: Fedora
Classification: Fedora
Component: libtar (Show other bugs)
rawhide
All Linux
low Severity medium
: ---
: ---
Assigned To: Kamil Dudka
Fedora Extras Quality Assurance
:
Depends On:
Blocks: 595635
  Show dependency treegraph
 
Reported: 2010-05-05 05:23 EDT by Kamil Dudka
Modified: 2010-05-27 10:16 EDT (History)
2 users (show)

See Also:
Fixed In Version: libtar-1.2.11-19.fc14
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
: 595635 (view as bug list)
Environment:
Last Closed: 2010-05-27 10:16:41 EDT
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---


Attachments (Terms of Use)
patch #1 (9.43 KB, patch)
2010-05-05 05:26 EDT, Kamil Dudka
no flags Details | Diff
patch #2 (964 bytes, patch)
2010-05-05 05:27 EDT, Kamil Dudka
no flags Details | Diff
revisited patch (3.30 KB, patch)
2010-05-25 04:57 EDT, Kamil Dudka
no flags Details | Diff
revisited patch V2 (11.19 KB, patch)
2010-05-27 05:18 EDT, Kamil Dudka
huzaifas: review+
Details | Diff
revisited patch V2 (11.19 KB, patch)
2010-05-27 05:20 EDT, Kamil Dudka
no flags Details | Diff

  None (edit)
Description Kamil Dudka 2010-05-05 05:23:20 EDT
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 05:26:32 EDT
Created attachment 411524 [details]
patch #1

I only removed two unintentional hunks removing the <stdlib.h> include.
Comment 2 Kamil Dudka 2010-05-05 05:27:37 EDT
Created attachment 411525 [details]
patch #2
Comment 3 Huzaifa S. Sidhpurwala 2010-05-05 05:29:20 EDT
Patches work, stdlib removal was non-intentional though
Comment 4 Kamil Dudka 2010-05-05 06:38:15 EDT
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 06:42:00 EDT
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 04:57:33 EDT
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 09:02:37 EDT
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 05:18:34 EDT
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 05:20:49 EDT
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 05:23:11 EDT
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 10:16:41 EDT
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.