Re: gputils: FTBFS on hurd-i386 (for review)
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];
> - 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);
Ditto as the other for loop.
> new->f = fopen(tryname, "rt");
>
> if (new->f != NULL) {
> new->name = strdup(tryname);
> + free(tryname);
> break;
> }
> }
Ditto leaking.
Thanks,
Guillem
Reply to: