Bug#657116: libtar: FTBFS on hurd-i386
[Magnus Holmgren] I have not forwarded the patch eliminating MAXPATHLEN yet, if that is what you mean. I did ask on the mailing list about releasing a new version with what's currently in git though. Thank you for fixing libtar on Hurd and follow up on the issues with upstream. :) I hope the testsuite patch also make it upstream, to make it easier to ensure that libtar work after build. :) -- Happy hacking Petter Reinholdtsen -- To UNSUBSCRIBE, email to debian-bugs-dist-requ...@lists.debian.org with a subject of unsubscribe. Trouble? Contact listmas...@lists.debian.org
Bug#657116: libtar: FTBFS on hurd-i386
Hi again. Is there a new upstream version planned soon fixing this build problem, or is it better to patch the Debian package? -- Happy hacking Petter Reinholdtsen -- To UNSUBSCRIBE, email to debian-bugs-dist-requ...@lists.debian.org with a subject of unsubscribe. Trouble? Contact listmas...@lists.debian.org
Bug#657116: libtar: FTBFS on hurd-i386
torsdagen den 13 februari 2014 09.03.36 skrev Petter Reinholdtsen: Hi again. Is there a new upstream version planned soon fixing this build problem, or is it better to patch the Debian package? I have not forwarded the patch eliminating MAXPATHLEN yet, if that is what you mean. I did ask on the mailing list about releasing a new version with what's currently in git though. -- Magnus Holmgrenholmg...@debian.org Debian Developer -- To UNSUBSCRIBE, email to debian-bugs-dist-requ...@lists.debian.org with a subject of unsubscribe. Trouble? Contact listmas...@lists.debian.org
Bug#657116: libtar: FTBFS on hurd-i386
fredagen den 31 januari 2014 23.01.54 skrev du: I've attached an updated patch solving some of the issues upstream commented on in the last patch. I have not had time to update all of it yet, but thought it best to check if it would get accepted into Debian even if upstream refuse to include it. I think you've missed a couple of free()s, at least in the various loops in wrapper.c. The final free()s could be moved inside the loops, but the plan was to reuse the already allocated buffer if it's large enough, wasn't it? Using strlcpy after having already ascertained that the buffer is large enough is unnecessary (the existing code used strlcpy, it's not something you added, I know). -- Magnus Holmgrenholmg...@debian.org Debian Developer signature.asc Description: This is a digitally signed message part.
Bug#657116: libtar: FTBFS on hurd-i386
söndagen den 9 februari 2014 19.35.06 skrev Magnus Holmgren:
fredagen den 31 januari 2014 23.01.54 skrev du:
I've attached an updated patch solving some of the issues upstream
commented on in the last patch. I have not had time to update all of
it yet, but thought it best to check if it would get accepted into
Debian even if upstream refuse to include it.
I think you've missed a couple of free()s, at least in the various loops in
wrapper.c. The final free()s could be moved inside the loops, but the plan
was to reuse the already allocated buffer if it's large enough, wasn't it?
Using strlcpy after having already ascertained that the buffer is large
enough is unnecessary (the existing code used strlcpy, it's not something
you added, I know).
Also you seem to have messed upp savepath in tar_append_tree.
I've updated your updated patch. Any comments? Perhaps my realloc_if_needed()
code is overly complicated. I'm guessing that free()ing buf and malloc()ing it
again is in practice going to reuse the same memory anyway, and the overhead
is negligible.
Since 1.2.21 isn't released yet I'm going to include the commit that did away
with the static buffer and introduced th_pathname rather than creating an
orig.tar.gz from the latest git commit, although that might not be a bad idea
either.
--
Magnus Holmgrenholmg...@debian.org
Debian Developer Author: Svante Signell svante.sign...@telia.com
Author: Petter Reinholdtsen p...@hungry.com
Author: Magnus Holmgren mag...@debian.org
Bug-Debian: http://bugs.debian.org/657116
Description: Fix FTBFS on Hurd by dynamically allocating path names.
Depends on no_static_buffers.patch, which introduced the th_pathname field.
--- a/compat/basename.c
+++ b/compat/basename.c
@@ -34,13 +34,25 @@ static char rcsid[] = $OpenBSD: basenam
#include errno.h
#include string.h
#include sys/param.h
+#include stdlib.h
char *
openbsd_basename(path)
const char *path;
{
- static char bname[MAXPATHLEN];
+ static char *bname = NULL;
+ static size_t allocated = 0;
register const char *endp, *startp;
+ int len = 0;
+
+ if (!allocated) {
+ allocated = 64;
+ bname = malloc(allocated);
+ if (!bname) {
+ allocated = 0;
+ return NULL;
+ }
+ }
/* Empty or NULL string gets treated as . */
if (path == NULL || *path == '\0') {
@@ -64,11 +76,19 @@ openbsd_basename(path)
while (startp path *(startp - 1) != '/')
startp--;
- if (endp - startp + 1 sizeof(bname)) {
- errno = ENAMETOOLONG;
- return(NULL);
+ len = endp - startp + 1;
+
+ if (len + 1 allocated) {
+ size_t new_allocated = 2*(len+1);
+ void *new_bname = malloc(new_allocated);
+ if (!new_bname)
+ return NULL;
+ allocated = new_allocated;
+ free(bname);
+ bname = new_bname;
}
- (void)strncpy(bname, startp, endp - startp + 1);
- bname[endp - startp + 1] = '\0';
+
+ (void)strncpy(bname, startp, len);
+ bname[len] = '\0';
return(bname);
}
--- a/compat/dirname.c
+++ b/compat/dirname.c
@@ -34,13 +34,25 @@ static char rcsid[] = $OpenBSD: dirname
#include errno.h
#include string.h
#include sys/param.h
+#include stdlib.h
char *
openbsd_dirname(path)
const char *path;
{
- static char bname[MAXPATHLEN];
+ static char *bname = NULL;
+ static size_t allocated = 0;
register const char *endp;
+ int len;
+
+ if (!allocated) {
+ allocated = 64;
+ bname = malloc(allocated);
+ if (!bname) {
+ allocated = 0;
+ return NULL;
+ }
+ }
/* Empty or NULL string gets treated as . */
if (path == NULL || *path == '\0') {
--- a/lib/append.c
+++ b/lib/append.c
@@ -38,7 +38,7 @@ typedef struct tar_dev tar_dev_t;
struct tar_ino
{
ino_t ti_ino;
- char ti_name[MAXPATHLEN];
+ char ti_name[];
};
typedef struct tar_ino tar_ino_t;
@@ -61,7 +61,7 @@ tar_append_file(TAR *t, const char *real
libtar_hashptr_t hp;
tar_dev_t *td = NULL;
tar_ino_t *ti = NULL;
- char path[MAXPATHLEN];
+ char *path = NULL;
#ifdef DEBUG
printf(== tar_append_file(TAR=0x%lx (\%s\), realname=\%s\,
@@ -126,34 +126,39 @@ tar_append_file(TAR *t, const char *real
}
else
{
+ const char *name;
#ifdef DEBUG
printf(+++ adding entry: device (0x%lx,0x%lx), inode %ld
(\%s\)...\n, major(s.st_dev), minor(s.st_dev),
s.st_ino, realname);
#endif
- ti = (tar_ino_t *)calloc(1, sizeof(tar_ino_t));
+ name = savename ? savename : realname;
+ ti = (tar_ino_t *)calloc(1, sizeof(tar_ino_t) + strlen(name) + 1);
if (ti == NULL)
return -1;
ti-ti_ino = s.st_ino;
- snprintf(ti-ti_name, sizeof(ti-ti_name), %s,
- savename ? savename : realname);
+ snprintf(ti-ti_name, strlen(name) + 1, %s, name);
libtar_hash_add(td-td_h, ti);
}
/* check if it's a symlink */
if (TH_ISSYM(t))
{
- i = readlink(realname, path, sizeof(path));
+ if ((path = malloc(s.st_size + 1)) == NULL)
+ return -1;
+ i = readlink(realname, path, s.st_size);
if (i == -1)
+ {
+ free(path);
return -1;
- if (i = MAXPATHLEN)
- i = MAXPATHLEN - 1;
+ }
path[i] = '\0';
#ifdef
Bug#657116: libtar: FTBFS on hurd-i386
[Magnus Holmgren] I've updated your updated patch. Any comments? Perhaps my realloc_if_needed() code is overly complicated. I'm guessing that free()ing buf and malloc()ing it again is in practice going to reuse the same memory anyway, and the overhead is negligible. Tried to apply it on the unstable source to test it on Hurd, but it failed to apply on decode.c. It did apply to the git upstream repo, though, with a bit fuzzying. :) And after applying the patch in URL: http://bugs.debian.org/737258 , 'make check' seemed to work too. What more should I test? Since 1.2.21 isn't released yet I'm going to include the commit that did away with the static buffer and introduced th_pathname rather than creating an orig.tar.gz from the latest git commit, although that might not be a bad idea either. Great! :) -- Happy hacking Petter Reinholdtsen -- To UNSUBSCRIBE, email to debian-bugs-dist-requ...@lists.debian.org with a subject of unsubscribe. Trouble? Contact listmas...@lists.debian.org
Bug#657116: libtar: FTBFS on hurd-i386
[Petter Reinholdtsen] I've attached an updated patch solving some of the issues upstream commented on in the last patch. I have not had time to update all of it yet, but thought it best to check if it would get accepted into Debian even if upstream refuse to include it. I've now had time to review the entire patch, and found nothing that had to be changed based on comments from upstream. Please apply this patch to libtar in Debian to allow vlc and a few other packages to try to build on Hurd. :) I plan to NMU libtar with the patches in BTS for #731860 and #657116 in a few days, assuming you are too busy to fix this now. Let me know if this is unwanted. -- Happy hacking Petter Reinholdtsen -- To UNSUBSCRIBE, email to debian-bugs-dist-requ...@lists.debian.org with a subject of unsubscribe. Trouble? Contact listmas...@lists.debian.org
Bug#657116: libtar: FTBFS on hurd-i386
Hi.
This libtar build failure on hurd block vlc and a few other packages
from building on Debian/Hurd. Any chance to get a added to the Debian
package while we wait for upstream to fix it?
I've attached an updated patch solving some of the issues upstream
commented on in the last patch. I have not had time to update all of
it yet, but thought it best to check if it would get accepted into
Debian even if upstream refuse to include it.
--
Happy hacking
Petter Reinholdtsen
diff --git a/compat/basename.c b/compat/basename.c
index 2ac1e13..1ec6f51 100644
--- a/compat/basename.c
+++ b/compat/basename.c
@@ -34,17 +34,29 @@ static char rcsid[] = $OpenBSD: basename.c,v 1.4
1999/05/30 17:10:30 espie Exp
#include errno.h
#include string.h
#include sys/param.h
+#include stdlib.h
char *
openbsd_basename(path)
const char *path;
{
- static char bname[MAXPATHLEN];
+ static char *bname = NULL;
+ static size_t allocated = 0;
register const char *endp, *startp;
+ int len = 0;
+
+ if (!allocated) {
+ allocated = 64;
+ bname = malloc(allocated);
+ if (!bname) {
+ allocated = 0;
+ return NULL;
+ }
+ }
/* Empty or NULL string gets treated as . */
if (path == NULL || *path == '\0') {
- (void)strcpy(bname, .);
+ strcpy(bname, .);
return(bname);
}
@@ -53,7 +65,7 @@ openbsd_basename(path)
while (endp path *endp == '/')
endp--;
- /* All slashes becomes / */
+ /* The base of / is / */
if (endp == path *endp == '/') {
(void)strcpy(bname, /);
return(bname);
@@ -64,11 +76,19 @@ openbsd_basename(path)
while (startp path *(startp - 1) != '/')
startp--;
- if (endp - startp + 1 sizeof(bname)) {
- errno = ENAMETOOLONG;
- return(NULL);
+ len = endp - startp + 1;
+
+ if (len + 1 allocated) {
+ size_t new_allocated = 2*(len+1);
+ void *new_bname = malloc(new_allocated);
+ if (!new_bname)
+ return NULL;
+ allocated = new_allocated;
+ free(bname);
+ bname = new_bname;
}
- (void)strncpy(bname, startp, endp - startp + 1);
- bname[endp - startp + 1] = '\0';
+
+ (void)strncpy(bname, startp, len);
+ bname[len] = '\0';
return(bname);
}
diff --git a/compat/dirname.c b/compat/dirname.c
index 986db4a..407ad47 100644
--- a/compat/dirname.c
+++ b/compat/dirname.c
@@ -34,17 +34,29 @@ static char rcsid[] = $OpenBSD: dirname.c,v 1.4 1999/05/30
17:10:30 espie Exp $
#include errno.h
#include string.h
#include sys/param.h
+#include stdlib.h
char *
openbsd_dirname(path)
const char *path;
{
- static char bname[MAXPATHLEN];
+ static char *bname = NULL;
+ static size_t allocated = 0;
register const char *endp;
+ int len;
+
+ if (!allocated) {
+ allocated = 64;
+ bname = malloc(allocated);
+ if (!bname) {
+ allocated = 0;
+ return NULL;
+ }
+ }
/* Empty or NULL string gets treated as . */
if (path == NULL || *path == '\0') {
- (void)strcpy(bname, .);
+ strcpy(bname, .);
return(bname);
}
@@ -67,11 +79,18 @@ openbsd_dirname(path)
} while (endp path *endp == '/');
}
- if (endp - path + 1 sizeof(bname)) {
- errno = ENAMETOOLONG;
- return(NULL);
+ len = endp - path + 1;
+ if (len + 1 allocated) {
+ size_t new_allocated = 2*(len+1);
+ void *new_bname = malloc(new_allocated);
+ if (!new_bname)
+ return NULL;
+ allocated = new_allocated;
+ free(bname);
+ bname = new_bname;
}
- (void)strncpy(bname, path, endp - path + 1);
- bname[endp - path + 1] = '\0';
+
+ (void)strncpy(bname, path, len);
+ bname[len] = '\0';
return(bname);
}
diff --git a/lib/append.c b/lib/append.c
index 32622f3..4d857ae 100644
--- a/lib/append.c
+++ b/lib/append.c
@@ -40,7 +40,7 @@ typedef struct tar_dev tar_dev_t;
struct tar_ino
{
ino_t ti_ino;
- char ti_name[MAXPATHLEN];
+ char ti_name[];
};
typedef struct tar_ino tar_ino_t;
@@ -63,7 +63,7 @@ tar_append_file(TAR *t, const char *realname, const char
*savename)
libtar_hashptr_t hp;
tar_dev_t *td = NULL;
tar_ino_t *ti = NULL;
- char path[MAXPATHLEN];
+ char *path = NULL;
#ifdef DEBUG
printf(== tar_append_file(TAR=0x%lx (\%s\), realname=\%s\,
@@ -128,34 +128,39 @@ tar_append_file(TAR *t, const char
Bug#657116: libtar: FTBFS on hurd-i386
On tisdagen den 24 januari 2012, you stated the following: This patch solves the build problems for GNU/Hurd due to MAXPATHLEN issues. The solution is to make dynamic string allocations instead of using fixed length buffers. The functions mkdirhier, basename and dirname have all been checked with separate test programs on Linux and Hurd and with valgrind on Linux without problems. Furthermore, parts of the the code has been reviewed by GN/Hurd developers and Debian GNU/Hurd developers and maintainers. Thanks! I've been thinking of doing something about this myself but there were so many instances of MAXPATHLEN that I got tired before I got started. :-) Build and run tested on both Linux and Hurd without problems. Also heap and leak tested on Linux with valgrind. No heap errors, but still memory leaks (coming from the original code). Removing the memory leaks will need another deep investigation of the original code, which time does not permit right now. Leaks other than the ones in th_get_pathname()? Since there is no upstream, the wish is that the Debian Maintainer takes a look at this patch and checks if it can be applied. Test programs are available on request. This patch will affect all architectures, completely removing the MAXPATHLEN dependencies (except for a few unused compat functions). I'll have a look and forward it to the libtar mailing list. There actually is sort of an official unofficial upstream at http://repo.or.cz/w/libtar.git since a while back. -- Magnus Holmgrenholmg...@debian.org Debian Developer signature.asc Description: This is a digitally signed message part.
Bug#657116: libtar: FTBFS on hurd-i386
On Thu, 2012-01-26 at 22:19 +0100, Magnus Holmgren wrote: On tisdagen den 24 januari 2012, you stated the following: This patch solves the build problems for GNU/Hurd due to MAXPATHLEN issues. The solution is to make dynamic string allocations instead of using fixed length buffers. The functions mkdirhier, basename and dirname have all been checked with separate test programs on Linux and Hurd and with valgrind on Linux without problems. Furthermore, parts of the the code has been reviewed by GN/Hurd developers and Debian GNU/Hurd developers and maintainers. Thanks! I've been thinking of doing something about this myself but there were so many instances of MAXPATHLEN that I got tired before I got started. :-) Yes, I realized that too, but after starting with the compat functions and mkdirhier things went on. Build and run tested on both Linux and Hurd without problems. Also heap and leak tested on Linux with valgrind. No heap errors, but still memory leaks (coming from the original code). Removing the memory leaks will need another deep investigation of the original code, which time does not permit right now. Leaks other than the ones in th_get_pathname()? Don't remember right now. I can run valgrind again if you are interested. Since there is no upstream, the wish is that the Debian Maintainer takes a look at this patch and checks if it can be applied. Test programs are available on request. This patch will affect all architectures, completely removing the MAXPATHLEN dependencies (except for a few unused compat functions). I'll have a look and forward it to the libtar mailing list. There actually is sort of an official unofficial upstream at http://repo.or.cz/w/libtar.git since a while back. Good to know about this repo. Regarding mailing list the link https://lists.feep.net:8080/mailman/listinfo.cgi/libtar times out all the time for me. Is another mailing list used? -- To UNSUBSCRIBE, email to debian-bugs-dist-requ...@lists.debian.org with a subject of unsubscribe. Trouble? Contact listmas...@lists.debian.org
Bug#657116: libtar: FTBFS on hurd-i386
On torsdagen den 26 januari 2012, Svante Signell wrote: On Thu, 2012-01-26 at 22:19 +0100, Magnus Holmgren wrote: I'll have a look and forward it to the libtar mailing list. There actually is sort of an official unofficial upstream at http://repo.or.cz/w/libtar.git since a while back. Good to know about this repo. Regarding mailing list the link https://lists.feep.net:8080/mailman/listinfo.cgi/libtar times out all the time for me. Is another mailing list used? No, that's the one AFAIK. -- Magnus Holmgrenholmg...@debian.org Debian Developer signature.asc Description: This is a digitally signed message part.
Bug#657116: libtar: FTBFS on hurd-i386
Source: libtar
Version: 1.2.11-8
Severity: important
Tags: patch
User: debian-h...@lists.debian.org
Usertags: hurd
Hi,
This patch solves the build problems for GNU/Hurd due to MAXPATHLEN
issues. The solution is to make dynamic string allocations instead of
using fixed length buffers. The functions mkdirhier, basename and
dirname have all been checked with separate test programs on Linux and
Hurd and with valgrind on Linux without problems. Furthermore, parts of
the the code has been reviewed by GN/Hurd developers and Debian GNU/Hurd
developers and maintainers.
Build and run tested on both Linux and Hurd without problems. Also heap
and leak tested on Linux with valgrind. No heap errors, but still memory
leaks (coming from the original code). Removing the memory leaks will
need another deep investigation of the original code, which time does
not permit right now.
Since there is no upstream, the wish is that the Debian Maintainer
takes a look at this patch and checks if it can be applied. Test
programs are available on request. This patch will affect all
architectures, completely removing the MAXPATHLEN dependencies (except
for a few unused compat functions).
There might be some remaining unneeded header file inclusions, they are
remnants of debugging methods used, both gdb and printf.
Thanks!
diff -ur libtar-1.2.11/compat/basename.c libtar-1.2.11.modified/compat/basename.c
--- libtar-1.2.11/compat/basename.c 2000-10-27 18:16:34.0 +0200
+++ libtar-1.2.11.modified/compat/basename.c 2012-01-19 14:20:20.0 +0100
@@ -34,17 +34,22 @@
#include errno.h
#include string.h
#include sys/param.h
+#include stdlib.h
char *
openbsd_basename(path)
const char *path;
{
- static char bname[MAXPATHLEN];
+ static char *bname = NULL;
register const char *endp, *startp;
+ int len = 0;
+
+ if (bname != NULL)
+ free(bname);
/* Empty or NULL string gets treated as . */
if (path == NULL || *path == '\0') {
- (void)strcpy(bname, .);
+ bname = strdup(.);
return(bname);
}
@@ -55,7 +60,7 @@
/* All slashes becomes / */
if (endp == path *endp == '/') {
- (void)strcpy(bname, /);
+ bname = strdup(/);
return(bname);
}
@@ -64,11 +69,9 @@
while (startp path *(startp - 1) != '/')
startp--;
- if (endp - startp + 1 sizeof(bname)) {
- errno = ENAMETOOLONG;
- return(NULL);
- }
- (void)strncpy(bname, startp, endp - startp + 1);
- bname[endp - startp + 1] = '\0';
+ len = endp - startp + 1;
+ bname = malloc(len + 1);
+ (void)strncpy(bname, startp, len);
+ bname[len] = '\0';
return(bname);
}
diff -ur libtar-1.2.11/compat/dirname.c libtar-1.2.11.modified/compat/dirname.c
--- libtar-1.2.11/compat/dirname.c 2000-10-27 18:16:34.0 +0200
+++ libtar-1.2.11.modified/compat/dirname.c 2012-01-18 23:06:20.0 +0100
@@ -34,17 +34,30 @@
#include errno.h
#include string.h
#include sys/param.h
+#include stdio.h
+#include stdlib.h
char *
openbsd_dirname(path)
const char *path;
{
- static char bname[MAXPATHLEN];
+ static char *bname = NULL;
+ static unsigned allocated = 0;
register const char *endp;
+ int len;
+
+ if (!allocated) {
+ allocated = 64;
+ bname = malloc(allocated);
+ if (!bname) {
+ allocated = 0;
+ return NULL;
+ }
+ }
/* Empty or NULL string gets treated as . */
if (path == NULL || *path == '\0') {
- (void)strcpy(bname, .);
+ strcpy(bname, .);
return(bname);
}
@@ -67,11 +80,18 @@
} while (endp path *endp == '/');
}
- if (endp - path + 1 sizeof(bname)) {
- errno = ENAMETOOLONG;
- return(NULL);
+ len = endp - path + 1;
+ if (len + 1 allocated) {
+ unsigned new_allocated = 2*(len+1);
+ void *new_bname = malloc(new_allocated);
+ if (!new_bname)
+ return NULL;
+ allocated = new_allocated;
+ free(bname);
+ bname = new_bname;
}
- (void)strncpy(bname, path, endp - path + 1);
- bname[endp - path + 1] = '\0';
+
+ (void)strncpy(bname, path, len);
+ bname[len] = '\0';
return(bname);
}
diff -ur libtar-1.2.11/lib/append.c libtar-1.2.11.modified/lib/append.c
--- libtar-1.2.11/lib/append.c 2003-01-07 02:40:59.0 +0100
+++ libtar-1.2.11.modified/lib/append.c 2012-01-20 14:28:57.0 +0100
@@ -17,6 +17,7 @@
#include fcntl.h
#include sys/param.h
#include sys/types.h
+#include sys/stat.h
#ifdef STDC_HEADERS
# include stdlib.h
@@ -38,7 +39,7 @@
struct tar_ino
{
ino_t ti_ino;
- char ti_name[MAXPATHLEN];
+ char *ti_name;
};
typedef struct tar_ino tar_ino_t;
@@ -61,7 +62,7 @@
libtar_hashptr_t hp;
tar_dev_t *td = NULL;
tar_ino_t *ti = NULL;
- char path[MAXPATHLEN];
+ char *path = NULL, *name = NULL;
#ifdef DEBUG
printf(== tar_append_file(TAR=0x%lx (\%s\), realname=\%s\,
@@ -135,25 +136,33 @@
if (ti == NULL)
return -1;
ti-ti_ino = s.st_ino;
- snprintf(ti-ti_name, sizeof(ti-ti_name), %s,
- savename ? savename : realname);
+ name = savename ? savename : realname;
+ if ((ti-ti_name = malloc(strlen(name) + 1)) == NULL)
+ return -1;
+ snprintf(ti-ti_name, strlen(name) + 1,
