[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