Patches contributed by Huzaifa Sidhpurwala. Version-Release number of selected component (if applicable): libtar-1.2.11-18.fc14
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