Re: lm-sensors: FTBFS on hurd-i386 (for review)

[Samuel Thibault <sthibault@debian.org>]

Hello,

Svante Signell, le Wed 26 Aug 2015 17:52:59 +0200, a écrit :
> Currently lm-sensors fails to build on GNU/Hurd due to PAH_MAX issues,
> see [1]. The attached patch 15-PATH_MAX.patch solves this by allocating
> the strings 'path' in lib/init.c and 'buf' in lib/access.c dynamically
> and free them if needed.

See below.

> Additionally the dependency on librrd2-dev should be changed to
> librrd-dev, see debian_control.patch, since the latest version of
> rrdtool 1.5.4-5 no longer provides this library. (packages ntop and
> ganglia also have this dependency and should be modified too)

librrd-dev has stopped providing librrd2-dev indeed.  Reporting bugs
against ntop and ganglia would be useful.

> Index: lm-sensors-3.4.0/lib/init.c
> ===================================================================
> --- lm-sensors-3.4.0.orig/lib/init.c
> +++ lm-sensors-3.4.0/lib/init.c
> @@ -141,21 +141,25 @@ static int add_config_from_dir(const cha
>  	}
>  
>  	for (res = 0, i = 0; !res && i < count; i++) {
> -		int len;
> -		char path[PATH_MAX];
> +		int len, path_len;
> +		char *path = NULL;
>  		FILE *input;
>  		struct stat st;
> -
> -		len = snprintf(path, sizeof(path), "%s/%s", dir,
> +		
> +		path_len = strlen(dir) + 1 + strlen(namelist[i]->d_name) + 1;
> +		path = malloc(path_len);

You should check for malloc failure, in which case do
{ res = -SENSORS_ERR_PARSE; continue } too

> +		len = snprintf(path, path_len, "%s/%s", dir,
>  			       namelist[i]->d_name);
> -		if (len < 0 || len >= (int)sizeof(path)) {
> +		if (len != path_len - 1) {
>  			res = -SENSORS_ERR_PARSE;
>  			continue;



> @@ -183,10 +183,12 @@ char *sensors_get_label(const sensors_ch
>  			}
>  
>  	/* No user specified label, check for a _label sysfs file */
> -	snprintf(buf, PATH_MAX, "%s/%s_label", name->path, feature->name);
> +	len = strlen(name->path) + 1 + strlen(feature->name) + 6 + 1;
> +	buf = malloc(len);
> +	snprintf(buf, len, "%s/%s_label", name->path, feature->name);
>  	
>  	if ((f = fopen(buf, "r"))) {
> -		i = fread(buf, 1, sizeof(buf), f);
> +		i = fread(buf, 1, len, f);

Uh, so upstream re-used the buf[PATH_MAX] array to store what is read
from the sysfs file... Using len may be big enough, but that's not
really sure. It'd probably be saner to free buf, and reallocate one with
a "reasonable" size, like 128 (it is a sensor label, it will probably
not be very big).

Also, you could treat allocation errors like is done below.

>  		fclose(f);
>  		if (i > 0) {
>  			/* i - 1 to strip the '\n' at the end */
> @@ -197,10 +199,9 @@ char *sensors_get_label(const sensors_ch
>  	}
>  
>  	/* No label, return the feature name instead */
> -	label = feature->name;
> +	label = strdup(feature->name);
>  	
>  sensors_get_label_exit:
> -	label = strdup(label);
>  	if (!label)
>  		sensors_fatal_error(__func__, "Allocating label text");
>  	return label;

Samuel