Re: gputils: FTBFS on hurd-i386 (for review)

2017-04-03 Thread Svante Signell 
On Fri, 2017-03-31 at 18:07 +0100, James Clarke wrote:
> On Fri, Mar 31, 2017 at 03:13:03PM +0200, Svante Signell wrote:
> > Source: gputils
> > Version: 1.4.0-0.1
> > Severity: important
> > Tags: patch
> > User: debian-hurd@lists.debian.org
> > Usertags: hurd
...
> >  for (i = 0; i < state.numpaths; i++) {
> > -  snprintf(file_name, sizeof(file_name), "%s" COPY_CHAR "%s",
> > state.paths[i], name);
> > +  len = snprintf(NULL, 0, "%s" COPY_CHAR "%s", state.paths[i], name);
> 
> This one seems to be missing a + 1.

Thanks James, OK now?
Index: gputils-1.4.0/gplink/gplink.c
===
--- gputils-1.4.0.orig/gplink/gplink.c
+++ gputils-1.4.0/gplink/gplink.c
@@ -321,9 +321,11 @@ gplink_open_coff(const char *name)
   gp_object_type *object;
   gp_archive_type *archive;
   FILE *coff;
-  char file_name[PATH_MAX + 1];
+  char *file_name = NULL;
+  int len = strlen(name) + 1;
 
-  strncpy(file_name, name, sizeof(file_name));
+  file_name = malloc(len);
+  strncpy(file_name, name, len);
 
   coff = fopen(file_name, "rb");
   if ((coff == NULL) && (strchr(file_name, PATH_CHAR) == 0)) {
@@ -331,7 +333,9 @@ gplink_open_coff(const char *name)
 int i;
 
 for (i = 0; i < state.numpaths; i++) {
-  snprintf(file_name, sizeof(file_name), "%s" COPY_CHAR "%s", state.paths[i], name);
+  len = snprintf(NULL, 0, "%s" COPY_CHAR "%s", state.paths[i], name) + 1;
+  file_name = realloc(file_name, len);
+  snprintf(file_name, len, "%s" COPY_CHAR "%s", state.paths[i], name);
   coff = fopen(file_name, "rb");
 
   if (coff != NULL) {
@@ -341,6 +345,7 @@ gplink_open_coff(const char *name)
   }
 
   if (coff == NULL) {
+free(file_name);
 perror(name);
 exit(1);
   }
@@ -353,17 +358,21 @@ gplink_open_coff(const char *name)
 /* read the object */
 object = gp_read_coff(file_name);
 object_append(object, file_name);
+free(file_name);
 break;
   case archive_file:
 /* read the archive */
 archive = gp_archive_read(file_name);
 archive_append(archive, file_name);
+free(file_name);
 break;
   case sys_err_file:
 gp_error("Can't open file \"%s\".", file_name);
+free(file_name);
 break;
   case unknown_file:
 gp_error("\"%s\" is not a valid coff object or archive.", file_name);
+free(file_name);
 break;
   default:
 assert(0);
Index: gputils-1.4.0/gplink/lst.c
===
--- gputils-1.4.0.orig/gplink/lst.c
+++ gputils-1.4.0/gplink/lst.c
@@ -35,9 +35,9 @@ static gp_section_type *line_section;
 static void
 open_src(const char *name, gp_symbol_type *symbol)
 {
-  char file_name[PATH_MAX + 1];
+  char *file_name = NULL;
   struct list_context *new = malloc(sizeof(*new));
-  int i;
+  int i, len;
 
   assert(name != NULL);
 
@@ -45,10 +45,13 @@ open_src(const char *name, gp_symbol_typ
   if (new->f == NULL) {
 /* Try searching include pathes */
 for (i = 0; i < state.numpaths; i++) {
-  snprintf(file_name, sizeof(file_name), "%s" COPY_CHAR "%s", state.paths[i], name);
+  len = snprintf(NULL, 0, "%s" COPY_CHAR "%s", state.paths[i], name) + 1;
+  file_name = realloc(file_name, len);
+  snprintf(file_name, len, "%s" COPY_CHAR "%s", state.paths[i], name);
   new->f = fopen(file_name, "rb");
   if (new->f != NULL) {
 name = file_name;
+free(file_name);
 break;
   }
 }
@@ -57,11 +60,15 @@ open_src(const char *name, gp_symbol_typ
   const char *p = strrchr(name, PATH_CHAR);
 
   if (p != NULL) {
+file_name = NULL;
 for (i = 0; i < state.numpaths; i++) {
-  snprintf(file_name, sizeof(file_name), "%s%s", state.paths[i], p);
+  len = snprintf(NULL, 0, "%s%s", state.paths[i], p) + 1;
+  file_name = realloc(file_name, len);
+  snprintf(file_name, len, "%s%s", state.paths[i], p);
   new->f = fopen(file_name, "rb");
   if (new->f != NULL) {
 name = file_name;
+free(file_name);
 break;
   }
 }
Index: gputils-1.4.0/gpasm/scan.l
===
--- gputils-1.4.0.orig/gpasm/scan.l
+++ gputils-1.4.0/gpasm/scan.l
@@ -626,16 +626,18 @@ a'{STR_QCHAR}'   {
 static void
 search_paths(struct source_context *new, const char *name)
 {
-  char tryname[PATH_MAX + 1];
-  int i;
+  char *tryname = NULL;
+  int i, len;
 
   for (i = 0; i < state.path_num; i++) {
-snprintf(tryname, sizeof(tryname),
- "%s" COPY_CHAR "%s", state.paths[i], name);
+len = snprintf(NULL, 0, "%s" COPY_CHAR "%s", state.paths[i], name) + 1;
+tryname = realloc(tryname, len);
+snprintf(tryname, len, "%s" COPY_CHAR "%s", state.paths[i], name);
 new->f = fopen(tryname, "rt");
 
 if (new->f != NULL) {
   new->name = strdup(tryname);
+  free(tryname);
   break;
 }
   }

Still Failing: g-i-installation_debian_sid_daily_hurd_lxde/335

2017-04-03 Thread jenkins 
See 
https://jenkins.debian.net/job/g-i-installation_debian_sid_daily_hurd_lxde/335/ 
and 
https://jenkins.debian.net/job/g-i-installation_debian_sid_daily_hurd_lxde/335//console
 and 
https://jenkins.debian.net/job/g-i-installation_debian_sid_daily_hurd_lxde/335//artifact/results/
 if there are any.

Re: gputils: FTBFS on hurd-i386 (for review)

2017-04-03 Thread Guillem Jover 
Hi!

On Fri, 2017-03-31 at 15:13:03 +0200, Svante Signell wrote:
> Source: gputils
> Version: 1.4.0-0.1
> Severity: important
> Tags: patch
> User: debian-hurd@lists.debian.org
> Usertags: hurd

> gputils currently FTBFS on GNU/Hurd due to PATH_MAX not being defined. The
> attached patch fixes this issue by allocating strings dynamically and remove
> them when not needed any more.

> Index: gputils-1.4.0/gplink/gplink.c
> ===
> --- gputils-1.4.0.orig/gplink/gplink.c
> +++ gputils-1.4.0/gplink/gplink.c
> @@ -321,9 +321,11 @@ gplink_open_coff(const char *name)
>gp_object_type *object;
>gp_archive_type *archive;
>FILE *coff;
> -  char file_name[PATH_MAX + 1];
> +  char *file_name = NULL;
> +  int len = strlen(name) + 1;
>  
> -  strncpy(file_name, name, sizeof(file_name));
> +  file_name = malloc(len);
> +  strncpy(file_name, name, len);

strdup(), and you should really be checking return values for error
conditions, such as ENOMEM.

>coff = fopen(file_name, "rb");
>if ((coff == NULL) && (strchr(file_name, PATH_CHAR) == 0)) {
> @@ -331,7 +333,9 @@ gplink_open_coff(const char *name)
>  int i;
>  
>  for (i = 0; i < state.numpaths; i++) {
> -  snprintf(file_name, sizeof(file_name), "%s" COPY_CHAR "%s", 
> state.paths[i], name);
> +  len = snprintf(NULL, 0, "%s" COPY_CHAR "%s", state.paths[i], name);
> +  file_name = realloc(file_name, len);
> +  snprintf(file_name, len, "%s" COPY_CHAR "%s", state.paths[i], name);
>coff = fopen(file_name, "rb");

asprintf()? I don't think the possibly slow down from not using
realloc is worth it.

>if (coff != NULL) {
> @@ -341,6 +345,7 @@ gplink_open_coff(const char *name)
>}
>  
>if (coff == NULL) {
> +free(file_name);
>  perror(name);
>  exit(1);
>}
> @@ -353,17 +358,21 @@ gplink_open_coff(const char *name)
>  /* read the object */
>  object = gp_read_coff(file_name);
>  object_append(object, file_name);
> +free(file_name);
>  break;
>case archive_file:
>  /* read the archive */
>  archive = gp_archive_read(file_name);
>  archive_append(archive, file_name);
> +free(file_name);
>  break;
>case sys_err_file:
>  gp_error("Can't open file \"%s\".", file_name);
> +free(file_name);
>  break;
>case unknown_file:
>  gp_error("\"%s\" is not a valid coff object or archive.", file_name);
> +free(file_name);
>  break;
>default:
>  assert(0);

I've not seen the context, but why not simply free immediately after
the switch, instead of on each case, which seems more error prone?

> Index: gputils-1.4.0/gplink/lst.c
> ===
> --- gputils-1.4.0.orig/gplink/lst.c
> +++ gputils-1.4.0/gplink/lst.c
> @@ -35,9 +35,9 @@ static gp_section_type *line_section;
>  static void
>  open_src(const char *name, gp_symbol_type *symbol)
>  {
> -  char file_name[PATH_MAX + 1];
> +  char *file_name = NULL;
>struct list_context *new = malloc(sizeof(*new));
> -  int i;
> +  int i, len;
>  
>assert(name != NULL);
>  
> @@ -45,10 +45,13 @@ open_src(const char *name, gp_symbol_typ
>if (new->f == NULL) {
>  /* Try searching include pathes */
>  for (i = 0; i < state.numpaths; i++) {
> -  snprintf(file_name, sizeof(file_name), "%s" COPY_CHAR "%s", 
> state.paths[i], name);
> +  len = snprintf(NULL, 0, "%s" COPY_CHAR "%s", state.paths[i], name) + 1;
> +  file_name = realloc(file_name, len);
> +  snprintf(file_name, len, "%s" COPY_CHAR "%s", state.paths[i], name);

Ditto as the other for loop.

>new->f = fopen(file_name, "rb");
>if (new->f != NULL) {
>  name = file_name;
> +free(file_name);
>  break;
>}
>  }

I guess this leaks, at least when the loop exits and there was no
error from fopen().

> @@ -57,11 +60,15 @@ open_src(const char *name, gp_symbol_typ
>const char *p = strrchr(name, PATH_CHAR);
>  
>if (p != NULL) {
> +file_name = NULL;
>  for (i = 0; i < state.numpaths; i++) {
> -  snprintf(file_name, sizeof(file_name), "%s%s", state.paths[i], p);
> +  len = snprintf(NULL, 0, "%s%s", state.paths[i], p) + 1;
> +  file_name = realloc(file_name, len);
> +  snprintf(file_name, len, "%s%s", state.paths[i], p);

Ditto as the other for loop.

>new->f = fopen(file_name, "rb");
>if (new->f != NULL) {
>  name = file_name;
> +free(file_name);
>  break;
>}
>  }

Ditto leaking.

> Index: gputils-1.4.0/gpasm/scan.l
> ===
> --- gputils-1.4.0.orig/gpasm/scan.l
> +++ gputils-1.4.0/gpasm/scan.l
> @@ -626,16 +626,18 @@ a'{STR_QCHAR}'   {
>  static void
>  search_paths(struct source_context *new, const char *name)
>  {
> -  char tryname[PATH_MAX + 1