[review v5] localedef: Add verbose messages for failure paths.

["Adhemerval Zanella (Code Review)" <gerrit at gnutoolchain-gerrit dot osci dot io>]

Adhemerval Zanella has posted comments on this change.

Change URL: https://gnutoolchain-gerrit.osci.io/r/c/glibc/+/303
......................................................................


Patch Set 5:

(2 comments)

| --- locale/programs/localedef.c
| +++ locale/programs/localedef.c
| @@ -526,14 +518,8 @@ construct_output_path (char *path)
| -		      (int) (startp - path), path, normal, endp, '\0');
| -
| -      if (n < 0)
| -	return NULL;
| -
| -      endp = result + n - 1;
| +	result = xasprintf ("%s%s/%.*s%s%s/",
| +			    output_prefix ?: "", COMPLOCALEDIR,
| +			    (int) (startp - path), path, normal, endp ?: "");
| +      /* Free the allocated normalized codeset name.  */

PS4, Line 521:

My understanding from that gnulib discussion was although it might not
fail on current hardware that will eventually use 2-complement
operations, it is still undefined behavior in some cases:

$ cat print.c
#include <stdio.h>
#include <inttypes.h>

int main ()
{
  char *path  = (char*) 0x7fffc000;
  char *startp = (char*) 0x80003000;
  printf ("%" PRIdPTR "\n", (int) (startp - path));

  return 0;
}

$ gcc -Wall print.c -o print -m32 -fsanitize=undefined && ./print
print.c:8:3: runtime error: signed integer overflow: -2147471360 -
2147467264 cannot be represented in type 'int'
28672

Not sure if this is indeed an issue, but the resulting code being used
a string length limiter raised a red flag.

| +      free ((char *) normal);
|      }
|    else
|      {
| -      /* This is a user path.  Please note the additional byte in the
| -	 memory allocation.  */
| -      size_t len = strlen (path) + 1;
| -      result = xmalloc (len + 1);
| -      endp = mempcpy (result, path, len) - 1;

 ...

| @@ -559,8 +545,16 @@ construct_output_path (char *path)
| +			      _("cannot create output path \"%s\": %s"),
| +			      result, strerror (errno));
| +	      free (result);
| +	      return NULL;
| +	    }
| +	}
| +      else
| +	record_verbose (stderr,
| +			_("no write permission to output path \"%s\": %s"),
| +			result, strerror (errno));

PS4, Line 554:

Fair enough.

| +    }
|  
|    return result;
|  }
|  
|  
| -/* Normalize codeset name.  There is no standard for the codeset
| -   names.  Normalization allows the user to use any of the common
| -   names.  */

-- 
Gerrit-Project: glibc
Gerrit-Branch: master
Gerrit-Change-Id: I28b9f680711ff00252a2cb15625b774cc58ecb9d
Gerrit-Change-Number: 303
Gerrit-PatchSet: 5
Gerrit-Owner: Carlos O'Donell <carlos@redhat.com>
Gerrit-Reviewer: Carlos O'Donell <carlos@redhat.com>
Gerrit-Reviewer: Florian Weimer <fweimer@redhat.com>
Gerrit-CC: Adhemerval Zanella <adhemerval.zanella@linaro.org>
Gerrit-CC: Simon Marchi <simon.marchi@polymtl.ca>
Gerrit-Comment-Date: Tue, 17 Dec 2019 12:25:27 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Adhemerval Zanella <adhemerval.zanella@linaro.org>
Comment-In-Reply-To: Carlos O'Donell <carlos@redhat.com>
Gerrit-MessageType: comment