[Devel] [PATCH] vzlist: dynamically allocated output buffer

Kir Kolyshkin kir at openvz.org
Mon Nov 24 11:12:31 PST 2014


Thanks, this looks better than the previous version.

Please see my comments inline.

Please send a new version to devel@, cc: kir@ (one email, not two 
separate ones).


On 11/24/2014 09:46 AM, Andrey Pushkar wrote:
> As pointed out in bug #2999, vzlist -o ip segfaults if the output string (IP addresses list in this case)
> exceeds the limit of 4096 characters. The issue is caused by static memory allocation, which has been changed
> to dynamic in this commit: a snprintf wrapper that implements automatic buffer resizing has been introduced.

1 Please make the description fit in 80 columns.

2 Please briefly describe the test case(s) you were using to check the 
patch.

>
> https://bugzilla.openvz.org/2999
>
> Reported-by: Michal Grzedzicki <lazy at iq.pl>
> Signed-off-by: Andrey Pushkar <andrey.s.pushkar at gmail.com>
> ---
>   src/vzlist.c |  118 +++++++++++++++++++++++++++++++++-------------------------
>   1 files changed, 67 insertions(+), 51 deletions(-)
>
> diff --git a/src/vzlist.c b/src/vzlist.c
> index 0b66f75..38bb7ae 100644
> --- a/src/vzlist.c
> +++ b/src/vzlist.c
> @@ -21,6 +21,7 @@
>   #include <stdlib.h>
>   #include <ctype.h>
>   #include <string.h>
> +#include <stdarg.h>
>   #include <sys/types.h>
>   #include <sys/stat.h>
>   #include <sys/socket.h>
> @@ -51,9 +52,10 @@
>   static struct Cveinfo *veinfo = NULL;
>   static int n_veinfo = 0;
>   
> -static char g_buf[4096] = "";
> -static char *p_buf = g_buf;
> -static char *e_buf = g_buf + sizeof(g_buf) - 1;
> +static char *g_buf = NULL;
> +static long g_buf_size = 4096;
> +static char *p_buf = NULL;
> +static char *e_buf = NULL;
>   static char *host_pattern = NULL;
>   static char *name_pattern = NULL;
>   static char *desc_pattern = NULL;
> @@ -90,6 +92,9 @@ static inline int get_run_ve(int update)
>   #define get_run_ve get_run_ve_proc
>   #endif
>   
> +static int x_snprintf(const char *format, ...);

1 This is no longer snprintf-like function, please rename. You can call 
it xprintf or something.

2 Please use __attribute__ (__format__ ( __printf__, 1, 2))) here, so C 
compiler can
check the arguments. See 
https://gcc.gnu.org/onlinedocs/gcc/Function-Attributes.html

> +static void *x_realloc(void *ptr, int size);
> +

I think you don't need this prototype here.

>   /* Print functions */
>   static void print_json_str(const char *s)
>   {
> @@ -110,8 +115,7 @@ static void print_ ## funcname(struct Cveinfo *p, int index)	\
>   								\
>   	if (p->fieldname != NULL)				\
>   		str = p->fieldname;				\
> -	r = snprintf(p_buf, e_buf - p_buf,			\
> -		"%" #length "s", str);				\
> +	r = x_snprintf("%" #length "s", str);				\
>   	if (!is_last_field)					\
>   		r = abs(length);				\
>   	p_buf += r;						\
> @@ -177,7 +181,7 @@ static void print_strlist(char *list)
>   		if ((ch = strchr(str, ' ')) != NULL)
>   			*ch = 0;
>   	}
> -	r = snprintf(p_buf, e_buf - p_buf, "%-15s", str);
> +	r = x_snprintf("%-15s", str);
>   	if (!is_last_field)
>   		r = 15;
>   	p_buf += r;
> @@ -203,8 +207,7 @@ static void print_veid(struct Cveinfo *p, int index)
>   	if (fmt_json)
>   		printf("%d", p->veid);
>   	else
> -		p_buf += snprintf(p_buf, e_buf - p_buf,
> -				"%10d", p->veid);
> +		p_buf += x_snprintf("%10d", p->veid);
>   }
>   
>   static void print_smart_name(struct Cveinfo *p, int index)
> @@ -220,8 +223,7 @@ static void print_status(struct Cveinfo *p, int index)
>   	if (fmt_json)
>   		print_json_str(ve_status[p->status]);
>   	else
> -		p_buf += snprintf(p_buf, e_buf - p_buf,
> -				"%-9s", ve_status[p->status]);
> +		p_buf += x_snprintf("%-9s", ve_status[p->status]);
>   }
>   
>   static void print_laverage(struct Cveinfo *p, int index)
> @@ -230,8 +232,7 @@ static void print_laverage(struct Cveinfo *p, int index)
>   		if (fmt_json)
>   			printf("null");
>   		else
> -			p_buf += snprintf(p_buf, e_buf - p_buf,
> -					"%14s", "-");
> +			p_buf += x_snprintf("%14s", "-");
>   	}
>   	else {
>   		if (fmt_json)
> @@ -240,8 +241,7 @@ static void print_laverage(struct Cveinfo *p, int index)
>   					p->cpustat->la[1],
>   					p->cpustat->la[2]);
>   		else
> -			p_buf += snprintf(p_buf, e_buf - p_buf,
> -					"%1.2f/%1.2f/%1.2f",
> +			p_buf += x_snprintf("%1.2f/%1.2f/%1.2f",
>   					p->cpustat->la[0],
>   					p->cpustat->la[1],
>   					p->cpustat->la[2]);
> @@ -257,8 +257,7 @@ static void print_uptime(struct Cveinfo *p, int index)
>   	}
>   
>   	if (p->cpustat == NULL)
> -		p_buf += snprintf(p_buf, e_buf - p_buf,
> -				"%15s", "-");
> +		p_buf += x_snprintf("%15s", "-");
>   	else
>   	{
>   		unsigned int days, hours, min, secs;
> @@ -272,8 +271,7 @@ static void print_uptime(struct Cveinfo *p, int index)
>   				(60ull * min + 60ull * 60 * hours +
>   				 60ull * 60 * 24 * days));
>   
> -		p_buf += snprintf(p_buf, e_buf - p_buf,
> -				"%.3dd%.2dh:%.2dm:%.2ds",
> +		p_buf += x_snprintf("%.3dd%.2dh:%.2dm:%.2ds",
>   				days, hours, min, secs);
>   	}
>   }
> @@ -285,15 +283,13 @@ static void print_cpu ## name(struct Cveinfo *p, int index)	\
>   		if (fmt_json)					\
>   			printf("null");				\
>   		else						\
> -			p_buf += snprintf(p_buf, e_buf - p_buf,	\
> -					"%7s", "-");		\
> +			p_buf += x_snprintf("%7s", "-");		\
>   	}							\
>   	else {							\
>   		if (fmt_json)					\
>   			printf("%lu", p->cpu->name[index]);	\
>   		else						\
> -			p_buf += snprintf(p_buf, e_buf - p_buf,	\
> -				"%7lu", p->cpu->name[index]);	\
> +			p_buf += x_snprintf("%7lu", p->cpu->name[index]);	\
>   	}							\
>   }
>   
> @@ -311,11 +307,9 @@ static void print_io ## name(struct Cveinfo *p, int index)	\
>   	}							\
>   								\
>   	if (i < min)						\
> -		p_buf += snprintf(p_buf, e_buf - p_buf,		\
> -				fmt "s", "-");			\
> +		p_buf += x_snprintf(fmt "s", "-");			\
>   	else							\
> -		p_buf += snprintf(p_buf, e_buf - p_buf,		\
> -				fmt "d", i);			\
> +		p_buf += x_snprintf(fmt "d", i);			\
>   }
>   
>   PRINT_IO(prio, "%3", 0)
> @@ -328,8 +322,7 @@ static void print_bool(const char *fmt, int val)
>   	if (fmt_json)
>   		printf(val ? "true" : "false");
>   	else
> -		p_buf += snprintf(p_buf, e_buf-p_buf,
> -				fmt, val ? "yes" : "no");
> +		p_buf += x_snprintf(fmt, val ? "yes" : "no");
>   }
>   
>   static void print_onboot(struct Cveinfo *p, int index)
> @@ -341,7 +334,7 @@ static void print_onboot(struct Cveinfo *p, int index)
>   	if (fmt_json)
>   		printf("null");
>   	else
> -		p_buf += snprintf(p_buf, e_buf-p_buf, "%6s", "-");
> +		p_buf += x_snprintf("%6s", "-");
>   }
>   
>   static void print_bootorder(struct Cveinfo *p, int index)
> @@ -350,15 +343,13 @@ static void print_bootorder(struct Cveinfo *p, int index)
>   		if (fmt_json)
>   			printf("null");
>   		else
> -			p_buf += snprintf(p_buf, e_buf-p_buf,
> -				"%10s", "-");
> +			p_buf += x_snprintf("%10s", "-");
>   	}
>   	else {
>   		if (fmt_json)
>   			printf("%lu", p->bootorder[index]);
>   		else
> -			p_buf += snprintf(p_buf, e_buf-p_buf,
> -					"%10lu", p->bootorder[index]);
> +			p_buf += x_snprintf("%10lu", p->bootorder[index]);
>   	}
>   }
>   
> @@ -370,11 +361,9 @@ static void print_cpunum(struct Cveinfo *p, int index)
>   	}
>   
>   	if (p->cpunum <= 0)
> -		p_buf += snprintf(p_buf, e_buf - p_buf,
> -				"%5s", "-");
> +		p_buf += x_snprintf("%5s", "-");
>   	else
> -		p_buf += snprintf(p_buf, e_buf - p_buf,
> -				"%5d", p->cpunum);
> +		p_buf += x_snprintf("%5d", p->cpunum);
>   }
>   
>   static const char *layout2str(int layout)
> @@ -394,8 +383,7 @@ static void print_layout(struct Cveinfo *p, int index)
>   	if (fmt_json)
>   		print_json_str(layout2str(p->layout));
>   	else
> -		p_buf += snprintf(p_buf, e_buf - p_buf,
> -				"%-6s",	layout2str(p->layout));
> +		p_buf += x_snprintf("%-6s",	layout2str(p->layout));
>   }
>   
>   static void print_features(struct Cveinfo *p, int index)
> @@ -409,7 +397,7 @@ static void print_features(struct Cveinfo *p, int index)
>   
>   	features_mask2str(p->features_mask, p->features_known,
>   		       ",", str, sizeof(str));
> -	r = snprintf(p_buf, e_buf - p_buf, "%-15s", str);
> +	r = x_snprintf("%-15s", str);
>   	if (!is_last_field)
>   		r = 15;
>   	p_buf += r;
> @@ -440,9 +428,9 @@ static void print_ubc(struct Cveinfo *p, size_t res_off, int index)
>   	}
>   
>   	if (!res || (!running && (index == 0 || index == 1 || index == 4)))
> -		p_buf += snprintf(p_buf, e_buf - p_buf, "%10s", "-");
> +		p_buf += x_snprintf("%10s", "-");
>   	else
> -		p_buf += snprintf(p_buf, e_buf - p_buf, "%10lu",
> +		p_buf += x_snprintf("%10lu",
>   				res[index]);
>   }
>   
> @@ -493,10 +481,9 @@ static void print_dq(struct Cveinfo *p, size_t res_off, int index)
>   	}
>   
>   	if (!res || (index == 0 && res[index] == 0))
> -		p_buf += snprintf(p_buf, e_buf - p_buf, "%10s", "-");
> +		p_buf += x_snprintf("%10s", "-");
>   	else
> -		p_buf += snprintf(p_buf, e_buf - p_buf, "%10lu",
> -				res[index]);
> +		p_buf += x_snprintf("%10lu", res[index]);
>   }
>   
>   #define PRINT_DQ(name)							\
> @@ -517,10 +504,9 @@ static void print_vm_overcommit(struct Cveinfo *p, int index)
>   	}
>   
>   	if (p->vm_overcommit == 0)
> -		p_buf += snprintf(p_buf, e_buf - p_buf, "%6s", "-");
> +		p_buf += x_snprintf("%6s", "-");
>   	else
> -		p_buf += snprintf(p_buf, e_buf - p_buf, "%6g",
> -			p->vm_overcommit);
> +		p_buf += x_snprintf("%6g", p->vm_overcommit);
>   }
>   
>   /* Sort functions */
> @@ -790,6 +776,32 @@ static void *x_realloc(void *ptr, int size)
>   	return tmp;
>   }
>   
> +static int x_snprintf(const char *format, ...)
> +{
> +	va_list arglist, tmp_arglist;
> +	va_start(arglist, format);
> +	va_start(tmp_arglist, format);
> +		
> +	int write_len = vsnprintf(p_buf, e_buf - p_buf, format, tmp_arglist);
> +
> +	write_len++; // vsnprintf doesn't count the last \0
> +
> +	if (e_buf - p_buf <= write_len)
> +	{
> +		g_buf_size = write_len >= g_buf_size * 2 ? write_len : g_buf_size * 2;
> +		char *tmp = (char *)x_realloc(g_buf, sizeof(char) * g_buf_size);
> +
> +		p_buf = tmp + (p_buf - g_buf);
> +		g_buf = tmp;
> +		e_buf = g_buf + g_buf_size - 1;
> +	}
> +
> +	int ret = vsnprintf(p_buf, e_buf - p_buf, format, arglist);

1 Don't declare variables in the middle of the function

2 You don't need to print two times in fact -- only if you need to 
resize the buffer.

> +	va_end(arglist);
> +	va_end(tmp_arglist);
> +	return ret;
> +}
> +
>   static void usage()
>   {
>   	printf(
> @@ -843,8 +855,7 @@ static void print_hdr()
>   
>   	for (p = g_field_order; p != NULL; p = p->next) {
>   		f = p->order;
> -		p_buf += snprintf(p_buf, e_buf - p_buf,
> -				field_names[f].hdr_fmt, field_names[f].hdr);
> +		p_buf += x_snprintf(field_names[f].hdr_fmt, field_names[f].hdr);
>   		if (p_buf >= e_buf)
>   			break;
>   		if (p->next != NULL)
> @@ -1673,7 +1684,7 @@ static int get_ves_cpu()
>   		}
>   		if (id && !veid) {
>   			rate = rint((double)rate * 100 / 1024);
> -			weight = rint((double)MAXCPUUNITS / weight);
> +			weight = MAXCPUUNITS / weight;

Uh oh. Please rebase your stuff properly.

>   			update_cpu(id, rate, weight);
>   		}
>   	}
> @@ -1911,6 +1922,10 @@ int main(int argc, char **argv)
>   	char *ep, *p;
>   	int veid, c;
>   
> +	g_buf = (char *)malloc(sizeof(char) * g_buf_size);
> +	p_buf = g_buf;
> +	e_buf = g_buf + g_buf_size - 1;
> +
>   	while (1) {
>   		int option_index = -1;
>   		c = getopt_long(argc, argv, "HtSanjN:h:d:o:s:Le1",
> @@ -2015,6 +2030,7 @@ int main(int argc, char **argv)
>   	free(name_pattern);
>   	free(desc_pattern);
>   	free(f_order);
> +	free(g_buf);
>   
>   	return 0;
>   }




More information about the Devel mailing list