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

Mon, 24 Nov 2014 12:36:35 -0800 

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 <[email protected]>
Signed-off-by: Andrey Pushkar <[email protected]>
---
  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; 
  }


_______________________________________________
Devel mailing list
[email protected]
https://lists.openvz.org/mailman/listinfo/devel

Reply via email to