[Pkg-shadow-devel] Bug#750480: Bug#750480: shadow: FTBFS on hurd-i386

[Bob Proulx]

Svante Signell wrote:
> PATH_MAX is not recommended to use, see e.g.
> http://insanecoding.blogspot.se/2007/11/pathmax-simply-isnt.html
> so using PATH_MAX for all architectures but Hurd is not recommended,
> 
> Attached is my first proposed solution to the debian-hurd mailing list.
> This solution does not use PATH_MAX at all, but I was recommended to use
> a fixed value instead since the format is /proc/%u/ so the size is
> limited. (max field length for an unsigned integer is 10)
> 
> This patch (or the previous one) should preferably be forwarded upstream
> for review.
>
> -	char proc_dir_name[PATH_MAX];
> +	char *proc_dir_name = NULL;
>...
> +	len = 6 + 10 + 1 + 1;
> +	proc_dir_name = malloc(len);
> +	if (proc_dir_name == NULL)
> +	  {
> +	    fprintf(stderr, "%s: malloc failed: %s\n", 
> +			Prog, strerror(errno));
> +	    return EXIT_FAILURE;
> +	  }
> +	written = snprintf(proc_dir_name, len, "/proc/%u/", target);
> -	if ((written <= 0) || (written >= sizeof(proc_dir_name))) {
> +	if ((written <= 0) || (written >= len)) {
>...
> +	free (proc_dir_name);

This feels less than good to me.  It is introducing malloc()/free()
where it isn't needed to be used.  Which introduces another point of
possible failure.  Which should be localized.  That overhead is
usually okay when the string is going to be resized dynamically if it
isn't large enough.  Then dynamic memory is needed.  But in the above
there isn't any resize capability added.  Therefore the overhead is
introduced but without the benefit.

Basically it looks like the substance of the patch is this where
PATH_MAX is changed for 6 + 10 + 1 + 1.

> -	char proc_dir_name[PATH_MAX];
> +	char *proc_dir_name = NULL;
>...
> +	len = 6 + 10 + 1 + 1;

I am not opposing the hard coded calculation of the potential max
size.  I am saying that all of the above could be reduced to this one
line change and it would be just as good without introducing the
malloc and free and the other changes.

-	char proc_dir_name[PATH_MAX];
+	char proc_dir_name[6 + 10 + 1 + 1];

That trades one hard coded value for another which is going to trigger
immune responses.  I can imagine other better constructs.  Each of
those will be more complicated.  But that gets into matters of style
and taste and bike shedding so I will avoid going there.

I fear the hard coded 10 value size for PID_MAX size is moving the
problem from one place to another.

Bob
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 181 bytes
Desc: Digital signature
URL: <http://lists.alioth.debian.org/pipermail/pkg-shadow-devel/attachments/20140610/bd61ffbe/attachment.sig>