Bug 254204 - devel/libtar: update to 1.2.20
Summary: devel/libtar: update to 1.2.20
Status: Open
Alias: None
Product: Ports & Packages
Classification: Unclassified
Component: Individual Port(s) (show other bugs)
Version: Latest
Hardware: Any Any
: --- Affects Only Me
Assignee: Steve Wills
URL:
Keywords: buildisok
Depends on:
Blocks:
 
Reported: 2021-03-10 20:10 UTC by Steve Wills
Modified: 2021-04-04 16:51 UTC (History)
2 users (show)

See Also:
bugzilla: maintainer-feedback? (manuelj.munoz)


Attachments
patch to update port (2.84 KB, patch)
2021-03-10 20:10 UTC, Steve Wills
swills: maintainer-approval? (manuelj.munoz)
Details | Diff
Test program (745 bytes, text/plain)
2021-03-24 20:20 UTC, Manuel Muñoz
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Steve Wills freebsd_committer 2021-03-10 20:10:17 UTC
Created attachment 223171 [details]
patch to update port
Comment 1 Automation User 2021-03-10 20:25:59 UTC
Build and package info is available at https://gitlab.com/swills/freebsd-ports/pipelines/268516198
Comment 2 Fernando Apesteguía freebsd_committer 2021-03-11 08:59:45 UTC
^Triage: Reporter is committer, assign accordingly.

^Triage: If there is a changelog or release notes URL available for this version, please add it to the URL field.

Thanks!
Comment 3 Manuel Muñoz 2021-03-23 15:18:03 UTC
Hello guys,

I am the maintainer, apologies for the delay.
It seems 1.2.20 was out 7 years ago.

I wrote an email in order to introduce myself to Mark Roth to his email account at University of Illinois but the account is gone and he seems to be working at Google, I cannot find his current address. 

So I am right now trying to build libtar from the port (after upgrading to 12.2) using the suggested Makefile.
Comment 4 Manuel Muñoz 2021-03-24 20:20:20 UTC
Created attachment 223551 [details]
Test program
Comment 5 Manuel Muñoz 2021-03-24 20:22:29 UTC
Thanks Steve...

First of all, sorry if I say or do anything stupid, I am pretty new here.

I have replace all Makefile, distinfo, pkg-desc, pkg-plist files inside /usr/ports/devel/libtar, and got 1.2.20 installed (# make install clean).

After that, I have checked by running a small .c program that makes use of libtar.

When running 1.2.20 I get the correct output but a "Core dump (Segmentation fault)" at the end.
Comment 6 Fernando Apesteguía freebsd_committer 2021-03-30 14:19:20 UTC
Hi Manuel, Steve,

I can confirm the segfault. I had a look and the problem arises trying to free(filename). According to the ChangeLog:

     Fixed thread-safe bug in th_get_pathname() (Sergey Zhitomirsky)

But they are returning the addres of a static alocated array:

char *
th_get_pathname(TAR *t)
{
        static TLS_THREAD char filename[MAXPATHLEN];

        if (t->th_buf.gnu_longname)
                return t->th_buf.gnu_longname;

        if (t->th_buf.prefix[0] != '\0')
        {
                snprintf(filename, sizeof(filename), "%.155s/%.100s",
                         t->th_buf.prefix, t->th_buf.name);
                return filename;
        }

        snprintf(filename, sizeof(filename), "%.100s", t->th_buf.name);
        return filename;
}

So it can not be freed from outside. I added a patch that just strdups the string before returning it so it can be freed. 

With the new version:

fernape@beastie:~/tmp/libtar$ touch one two three
fernape@beastie:~/tmp/libtar$ tar cvf mytar.tar one two three 
a one
a two
a three
fernape@beastie:~/tmp/libtar$ clang -ggdb -Wall -ltar -I /usr/local/include/ -L /usr/local/lib libtar-list.c 
fernape@beastie:~/tmp/libtar$ ./a.out mytar.tar 
one
two
three

Before the change, it segfaulted after "one".

There is this patch from a few days after the last release in 2013: https://github.com/tklauser/libtar/commit/ec613af2e9371d7a3e1f7c7a6822164a4255b4d1

th_get_pathname is used internally in the library assuming it returns a static allocated buffer and they never free filename. So I think that the above patch will trade segfaults for memory leaks.



Let me know what you think.

What surprises me is that I don't see that patch in OpenBSD or other Linux distributions...
Comment 7 Manuel Muñoz 2021-04-02 20:19:13 UTC
Hey Fernando,

Why do you say there could be a memory leak?
I can see in lib/handle.c they have added a "free(t->th_pathname)" statement.

/* close tarfile handle */
int
tar_close(TAR *t)
{
	int i;

	i = (*(t->type->closefunc))(t->fd);

	if (t->h != NULL)
		libtar_hash_free(t->h, ((t->oflags & O_ACCMODE) == O_RDONLY
					? free
					: (libtar_freefunc_t)tar_dev_free));
	free(t->th_pathname);
	free(t);

	return i;
}
Comment 8 Fernando Apesteguía freebsd_committer 2021-04-04 16:51:02 UTC
(In reply to Manuel Muñoz from comment #7)

I mean that by strduping the string returned from th_get_pathname, the function returns a _different_ string every time. For example, when called from tar_set_file_perms():

filename = (realname ? realname : th_get_pathname(t));

But filename is never freed(). So that string will be lost/leaked. The code as it is, assumes that the string returned by th_get_pathname is always the same. Similar problems are encountered in functions like tar_extract_regfile().

I would need to run a program through valgrind to confirm this.