[lxc-devel] [PATCH v2] Fix calculations in clone_update_unexp_hooks()

[Serge Hallyn]

Quoting Christian Brauner (christianvanbrauner at gmail.com):
> (1) This commit fixes some long standing miscalculation when updating paths in
>     lxc.hooks.* entries. We now also update conf->unexpandend_alloced which
>     hasn't been done prior to this commit. According to my testing valgrind now
>     no longer reports "x bytes definitely lost" when using lxc-clone.
> 
> (2) Also we use the stricter check:
> 
>     	if (p >= lend)
>     		continue;
> 
>     This should deal better with invalid config files.
> 
> (3) Insert some spaces between operators to increase readability.
> 
> (4) Use gotos to simplify function and increase readability.
> 
> Signed-off-by: Christian Brauner <christianvanbrauner at gmail.com>
> ---
>  src/lxc/confile.c | 60 +++++++++++++++++++++++++++----------------------------
>  1 file changed, 29 insertions(+), 31 deletions(-)
> 
> diff --git a/src/lxc/confile.c b/src/lxc/confile.c
> index d584964..0565973 100644
> --- a/src/lxc/confile.c
> +++ b/src/lxc/confile.c
> @@ -2695,7 +2695,8 @@ next:
>  }
>  
>  bool clone_update_unexp_hooks(struct lxc_conf *conf, const char *oldpath,
> -	const char *newpath, const char *oldname, const char *newname)
> +			      const char *newpath, const char *oldname,
> +			      const char *newname)
>  {
>  	const char *key = "lxc.hook";
>  	int ret;
> @@ -2705,13 +2706,13 @@ bool clone_update_unexp_hooks(struct lxc_conf *conf, const char *oldpath,
>  	char *olddir = alloca(olddirlen + 1);
>  	char *newdir = alloca(newdirlen + 1);
>  
> -	ret = snprintf(olddir, olddirlen+1, "%s/%s", oldpath, oldname);
> -	if (ret < 0 || ret >= olddirlen+1) {
> +	ret = snprintf(olddir, olddirlen + 1, "%s/%s", oldpath, oldname);
> +	if (ret < 0 || ret >= olddirlen + 1) {
>  		ERROR("Bug in %s", __func__);
>  		return false;
>  	}
> -	ret = snprintf(newdir, newdirlen+1, "%s/%s", newpath, newname);
> -	if (ret < 0 || ret >= newdirlen+1) {
> +	ret = snprintf(newdir, newdirlen + 1, "%s/%s", newpath, newname);
> +	if (ret < 0 || ret >= newdirlen + 1) {
>  		ERROR("Bug in %s", __func__);
>  		return false;
>  	}
> @@ -2723,56 +2724,53 @@ bool clone_update_unexp_hooks(struct lxc_conf *conf, const char *oldpath,
>  			lend = lstart + strlen(lstart);
>  		else
>  			lend++;
> -		if (strncmp(lstart, key, strlen(key)) != 0) {
> -			lstart = lend;
> -			continue;
> -		}
> -		p = strchr(lstart+strlen(key), '=');
> -		if (!p) {
> -			lstart = lend;
> -			continue;
> -		}
> +		if (strncmp(lstart, key, strlen(key)) != 0)
> +                        goto next;
> +		p = strchr(lstart + strlen(key), '=');
> +		if (!p)
> +                        goto next;
>  		p++;
>  		while (isblank(*p))
>  			p++;
> -		if (!*p)
> -			return true;
> -		if (strncmp(p, olddir, strlen(olddir)) != 0) {
> -			lstart = lend;
> -			continue;
> -		}
> +                if (p >= lend)
> +                        goto next;
> +		if (strncmp(p, olddir, strlen(olddir)) != 0)
> +                        goto next;
>  		/* replace the olddir with newdir */
>  		if (olddirlen >= newdirlen) {
>  			size_t diff = olddirlen - newdirlen;
>  			memcpy(p, newdir, newdirlen);
>  			if (olddirlen != newdirlen) {
> -				memmove(lend-diff, lend, strlen(lend)+1);
> +				memmove(p + newdirlen, p + newdirlen + diff,
> +					strlen(p) - newdirlen - diff + 1);
>  				lend -= diff;
>  				conf->unexpanded_len -= diff;
>  			}
> -			lstart = lend;
>  		} else {
>  			char *new;
>  			size_t diff = newdirlen - olddirlen;
>  			size_t oldlen = conf->unexpanded_len;
>  			size_t newlen = oldlen + diff;
> -			size_t poffset = p - conf->unexpanded_config;
> -			new = realloc(conf->unexpanded_config, newlen);
> +			size_t poffset = q - conf->unexpanded_config;

q is not defined anywhere that I can see