Major memory leak

[Gary Coady]

See details on the libtar mailing list at https://lists.feep.net:8080/pipermail/libtar/2006-April/000222.html

Here is a patch that fixes all occurences of strdup() being used without
free()'ing the allocated memory.

Apply the patch on libtar 1.2.11.

[Adam Niedling (krychek)]

Has this issue been fixed already?

[Mikael Strom (mikael-sesamiq)]

Is this issue resolved, or are there still leaks in the current distribution?

Thanks,
Mike

[Magnus Holmgren (holmgren)]

No, it's not resolved.

I was applying this patch to the package in Debian, but after looking a bit closer I'm not so sure anymore. The problem that the patch attempts to solve is that th_get_pathname() in some cases (namely, if gnu_longname is not set) strdup()s a local buffer. But th_get_pathname() is a part of the public API, so all applications that use libtar might get *more* memory leaks until they are updated. And they can't free the memory unless they know that it's malloc()ed, so it's an incompatible change.

Then there's the similar function th_get_pathname(), which never returns a strdup()ed string.

Another suggestion is to make the buffer static:
http://downloads.diy-linux.org/patches/OLD/PM/libtar-1.2.11.patch

[Launchpad Janitor (janitor)]

This bug was fixed in the package libtar - 1.2.11-8

---------------
libtar (1.2.11-8) unstable; urgency=low

  * libtool.patch: Set SHELL to the configured shell in those Makefile.in
    where libtool is used; otherwise libtool fails when /bin/sh is dash
    but bash is expected (Closes: #621935).
  * man_hyphen_minus.patch (new): Escape hyphens that should be minus
    signs in man pages.
  * Rename libtar as libtar0 to follow policy.

libtar (1.2.11-7) unstable; urgency=low

  * New maintainer (Closes: #526618).
  * Change source format to 3.0 (quilt), clean up Debian diff and split
    into several patches:
    * libtool.patch: Using libtool to build dynamic library;
    * autoreconf.patch: Changes needed to call autoreconf (bug 511741);
    * memleak.patch: Fix memory leaks;
    * bad_ptrtoint.patch: Document stupidity of tartype_t in libtar.c
      (bug 309945).
  * Increase Debhelper compat level to 7.
  * Use dh_autoreconf to avoid having to keep track of files to clean.
  * memleak2.patch (new): Applied instead of memleak.patch. Fix memory
    leak by making th_get_pathname() return a pointer to a static buffer
    instead of a pointer to a copy of a local buffer (LP: #41804).
  * Add homepage field and watch file (in case there is ever a new
    upstream release).
  * Upgrade to Standards-Version 3.9.1.
 -- Ubuntu Archive Auto-Sync <email address hidden> Sat, 30 Apr 2011 13:11:08 +0000

[Tomas Skäre (tomas-skare)]

IMO, this is not an acceptable solution.

th_get_pathname() is part of the public API, and must not change behaviour
unless the major version of libtar is changed. This severely breaks compatibility
with existing programs, and affects portability a lot.

If a program uses libtar 1.2.11 on Solaris, th_get_pathname() returns a
strdup():ed pointer which must be free:d by the caller. If the same program
is compiled (also using libtar 1.2.11!) on Debian or Ubuntu, it must not free
the returned pointer. There is no way to know from the code if it must be
free:d or not.

If the original memory leak only happens for internal usage of th_get_pathname,
then another internal function should be added, and th_get_pathname can
use that function, but strdup() the return value.

[Magnus Holmgren (holmgren)]

I believe that technically, the public API hasn't been changed. The API doesn't specify whether the returned pointer should be free()d, and is broken by sometimes returning a pointer that needs to be free()d and sometimes one that mustn't, so one must never free() the pointer. Some distributions decided to fix that by making th_get_pathname() always return a free()able pointer, but portable programs can't rely on until _all_ distributions do the same.

Using a static buffer of course makes the function non reentrant, but I figured that's not typically a problem, and that therefore it would be better if other distros chose the same solution, but if that won't happen (I haven't even got around to bringing it up on the libtar mailing list yet), I'll reconsider.

It's really unfortunate that there's no active upstream to set this straight as well as fix the other API misdesigns and bugs.