On Wed, 2017-04-05 at 16:19:28 +0200, Svante Signell wrote:
> Thank you for the review. Attached is an updated patch. Is it OK now? I still
> have to check return values of snprintf and realloc and maybe also switch to
> using alloca (not POSIX though).
I've got the feeling we have covered similar ground in the past, but
let's see. And I'd keep away from alloca().
> Index: gputils-1.4.0/gplink/gplink.c
> ===================================================================
> --- gputils-1.4.0.orig/gplink/gplink.c
> +++ gputils-1.4.0/gplink/gplink.c
> @@ -321,17 +321,23 @@ gplink_open_coff(const char *name)
> gp_object_type *object;
> gp_archive_type *archive;
> FILE *coff;
> - char file_name[PATH_MAX + 1];
> -
> - strncpy(file_name, name, sizeof(file_name));
> + char *file_name = NULL;
The variable is unconditionally assigned almost immediately, there's
IMO no point in this assignment. Ditto other instances.
> + int len = 0;
>
> + file_name = strdup(name);
> + if (file_name == NULL) {
> + perror(name);
> + exit(1);
> + }
> coff = fopen(file_name, "rb");
> if ((coff == NULL) && (strchr(file_name, PATH_CHAR) == 0)) {
> /* If no "/" in name, try searching include pathes. */
> 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);
If you are going to keep using realloc, then it seems like a waste to
resize the buffer, when we need less memory for example. Ditto other
instances.
> + snprintf(file_name, len, "%s" COPY_CHAR "%s", state.paths[i], name);
> coff = fopen(file_name, "rb");
>
> if (coff != NULL) {
> @@ -341,6 +347,8 @@ gplink_open_coff(const char *name)
> }
>
> if (coff == NULL) {
> + if (file_name)
> + free(file_name);
There's never a need to check whether a pointer is NULL before a free().
Ditto other instances.
> perror(name);
> exit(1);
> }
Thanks,
Guillem
