Replace a mess of snprintf() dances with easier to read code using

strlcat(). Shorter, clearer, fewer signed vs unsigned questions.

Shrink static buffer for the string version of an option value from
32K to 8K. Since the string version of the entire lease is constructed
in a 8K buffer, bigger option values are pointless.

Use 8K of the saved space for a static buffer for pretty_print_string()
and use it rather scribbling intermediate values into the final
destination.

No intentional functional change.

diff --git a/sbin/dhclient/dhclient.c b/sbin/dhclient/dhclient.c
index d3579f7..b00745d 100644 (file)
--- a/sbin/dhclient/dhclient.c
+++ b/sbin/dhclient/dhclient.c
@@ -1,4 +1,4 @@
-/*     $OpenBSD: dhclient.c,v 1.408 2017/04/07 15:03:00 krw Exp $      */
+/*     $OpenBSD: dhclient.c,v 1.409 2017/04/08 17:00:10 krw Exp $      */
 
 /*
  * Copyright 2004 Henning Brauer <henning@openbsd.org>
@@ -164,6 +164,7 @@ void rewrite_client_leases(struct interface_info *);
 void rewrite_option_db(struct interface_info *, struct client_lease *,
     struct client_lease *);
 char *lease_as_string(struct interface_info *, char *, struct client_lease *);
+void append_statement(char *, size_t, char *, char *);
 
 struct client_lease *packet_to_lease(struct interface_info *, struct in_addr,
     struct option_data *);
@@ -1958,89 +1959,61 @@ rewrite_option_db(struct interface_info *ifi, struct client_lease *offered,
        write_option_db(db, strlen(db));
 }
 
+void
+append_statement(char *string, size_t sz, char *s1, char *s2)
+{
+       strlcat(string, s1, sz);
+       strlcat(string, s2, sz);
+       strlcat(string, ";\n", sz);
+}
+
 char *
 lease_as_string(struct interface_info *ifi, char *type,
     struct client_lease *lease)
 {
-       static char leasestr[8192];
+       static char string[8192];
+       char timebuf[27];       /* to hold "6 2017/04/08 05:47:50 UTC;" */
        struct option_data *opt;
-       char *p;
-       size_t sz, rsltsz;
-       int i, rslt;
+       char *buf;
+       size_t rslt;
+       int i;
 
-       sz = sizeof(leasestr);
-       p = leasestr;
-       memset(p, 0, sz);
+       memset(string, 0, sizeof(string));
 
-       rslt = snprintf(p, sz, "%s {\n"
-           "%s  interface \"%s\";\n  fixed-address %s;\n",
-           type, (lease->is_bootp) ? "  bootp;\n" : "", ifi->name,
-           inet_ntoa(lease->address));
-       if (rslt == -1 || rslt >= sz)
+       strlcat(string, type, sizeof(string));
+       strlcat(string, " {\n", sizeof(string));
+       strlcat(string, (lease->is_bootp) ? "  bootp;\n" : "", sizeof(string));
+
+       buf = pretty_print_string(ifi->name, strlen(ifi->name), 1);
+       if (buf == NULL)
                return (NULL);
-       p += rslt;
-       sz -= rslt;
+       append_statement(string, sizeof(string), "  interface ", buf);
 
-       rslt = snprintf(p, sz, "  next-server %s;\n",
+       append_statement(string, sizeof(string), "  fixed-address ",
+           inet_ntoa(lease->address));
+       append_statement(string, sizeof(string), "  next-server ",
            inet_ntoa(lease->next_server));
-       if (rslt == -1 || rslt >= sz)
-               return (NULL);
-       p += rslt;
-       sz -= rslt;
 
        if (lease->filename) {
-               rslt = snprintf(p, sz, "  filename ");
-               if (rslt == -1 || rslt >= sz)
-                       return (NULL);
-               p += rslt;
-               sz -= rslt;
-               rslt = pretty_print_string(p, sz, lease->filename,
+               buf = pretty_print_string(lease->filename,
                    strlen(lease->filename), 1);
-               if (rslt == -1 || rslt >= sz)
-                       return (NULL);
-               p += rslt;
-               sz -= rslt;
-               rslt = snprintf(p, sz, ";\n");
-               if (rslt == -1 || rslt >= sz)
+               if (buf == NULL)
                        return (NULL);
-               p += rslt;
-               sz -= rslt;
+               append_statement(string, sizeof(string), "  filename ", buf);
        }
        if (lease->server_name) {
-               rslt = snprintf(p, sz, "  server-name ");
-               if (rslt == -1 || rslt >= sz)
-                       return (NULL);
-               p += rslt;
-               sz -= rslt;
-               rslt = pretty_print_string(p, sz, lease->server_name,
+               buf = pretty_print_string(lease->server_name,
                    strlen(lease->server_name), 1);
-               if (rslt == -1 || rslt >= sz)
-                       return (NULL);
-               p += rslt;
-               sz -= rslt;
-               rslt = snprintf(p, sz, ";\n");
-               if (rslt == -1 || rslt >= sz)
+               if (buf == NULL)
                        return (NULL);
-               p += rslt;
-               sz -= rslt;
+               append_statement(string, sizeof(string), "  server-name ",
+                   buf);
        }
        if (lease->ssid_len != 0) {
-               rslt = snprintf(p, sz, "  ssid ");
-               if (rslt == -1 || rslt >= sz)
-                       return (NULL);
-               p += rslt;
-               sz -= rslt;
-               rslt = pretty_print_string(p, sz, lease->ssid,
-                   lease->ssid_len, 1);
-               if (rslt == -1 || rslt >= sz)
-                       return (NULL);
-               p += rslt;
-               sz -= rslt;
-               rslt = snprintf(p, sz, ";\n");
-               if (rslt == -1 || rslt >= sz)
+               buf = pretty_print_string(lease->ssid, lease->ssid_len, 1);
+               if (buf == NULL)
                        return (NULL);
-               p += rslt;
-               sz -= rslt;
+               append_statement(string, sizeof(string), "  ssid ", buf);
        }
 
        for (i = 0; i < 256; i++) {
@@ -2048,37 +2021,37 @@ lease_as_string(struct interface_info *ifi, char *type,
                if (opt->len == 0)
                        continue;
 
-               rslt = snprintf(p, sz, "  option %s %s;\n",
-                   dhcp_options[i].name, pretty_print_option(i, opt,  1));
-               if (rslt == -1 || rslt >= sz)
+               buf = pretty_print_option(i, opt, 1);
+               if (buf == NULL)
                        return (NULL);
-               p += rslt;
-               sz -= rslt;
+               strlcat(string, "  option ", sizeof(string));
+               strlcat(string, dhcp_options[i].name, sizeof(string));
+               append_statement(string, sizeof(string), " ", buf);
        }
 
-       rsltsz = strftime(p, sz, "  renew " DB_TIMEFMT ";\n",
+       rslt = strftime(timebuf, sizeof(timebuf), DB_TIMEFMT,
            gmtime(&lease->renewal));
-       if (rsltsz == 0)
+       if (rslt == 0)
                return (NULL);
-       p += rsltsz;
-       sz -= rsltsz;
-       rsltsz = strftime(p, sz, "  rebind " DB_TIMEFMT ";\n",
+       append_statement(string, sizeof(string), "  renew ", timebuf);
+
+       rslt = strftime(timebuf, sizeof(timebuf), DB_TIMEFMT,
            gmtime(&lease->rebind));
-       if (rsltsz == 0)
+       if (rslt == 0)
                return (NULL);
-       p += rsltsz;
-       sz -= rsltsz;
-       rsltsz = strftime(p, sz, "  expire " DB_TIMEFMT ";\n",
+       append_statement(string, sizeof(string), "  rebind ", timebuf);
+
+       rslt = strftime(timebuf, sizeof(timebuf), DB_TIMEFMT,
            gmtime(&lease->expiry));
-       if (rsltsz == 0)
+       if (rslt == 0)
                return (NULL);
-       p += rsltsz;
-       sz -= rsltsz;
-       rslt = snprintf(p, sz, "}\n");
-       if (rslt == -1 || rslt >= sz)
+       append_statement(string, sizeof(string), "  expire ", timebuf);
+
+       rslt = strlcat(string, "}\n", sizeof(string));
+       if (rslt >= sizeof(string))
                return (NULL);
 
-       return (leasestr);
+       return (string);
 }
 
 void

diff --git a/sbin/dhclient/dhcpd.h b/sbin/dhclient/dhcpd.h
index 8eaac84..6104b8a 100644 (file)
--- a/sbin/dhclient/dhcpd.h
+++ b/sbin/dhclient/dhcpd.h
@@ -1,4 +1,4 @@
-/*     $OpenBSD: dhcpd.h,v 1.166 2017/04/07 15:03:00 krw Exp $ */
+/*     $OpenBSD: dhcpd.h,v 1.167 2017/04/08 17:00:10 krw Exp $ */
 
 /*
  * Copyright (c) 2004 Henning Brauer <henning@openbsd.org>
@@ -188,7 +188,7 @@ extern struct in_addr active_addr;
 int cons_options(struct interface_info *, struct option_data *);
 char *pretty_print_option(unsigned int, struct option_data *, int);
 char *pretty_print_domain_search(unsigned char *, size_t);
-int pretty_print_string(unsigned char *, size_t, unsigned char *, size_t, int);
+char *pretty_print_string(unsigned char *, size_t, int);
 int pretty_print_classless_routes(unsigned char *, size_t, unsigned char *,
     size_t);
 void do_packet(struct interface_info *, unsigned int, struct in_addr,

diff --git a/sbin/dhclient/options.c b/sbin/dhclient/options.c
index aad2385..affb0e2 100644 (file)
--- a/sbin/dhclient/options.c
+++ b/sbin/dhclient/options.c
@@ -1,4 +1,4 @@
-/*     $OpenBSD: options.c,v 1.84 2017/04/07 15:03:01 krw Exp $        */
+/*     $OpenBSD: options.c,v 1.85 2017/04/08 17:00:10 krw Exp $        */
 
 /* DHCP options parsing and reassembly. */
 
@@ -215,52 +215,34 @@ cons_options(struct interface_info *ifi, struct option_data *options)
  * represented as '"' delimited strings and safely passed to scripts. Surround
  * result with double quotes if emit_punct is true.
  */
-int
-pretty_print_string(unsigned char *dst, size_t dstlen, unsigned char *src,
-    size_t srclen, int emit_punct)
+char *
+pretty_print_string(unsigned char *src, size_t srclen, int emit_punct)
 {
+       static char string[8196];
        char visbuf[5];
        unsigned char *origsrc = src;
-       int opcount = 0, total = 0;
+       size_t rslt = 0;
 
-       if (emit_punct) {
-               opcount = snprintf(dst, dstlen, "\"");
-               if (opcount == -1)
-                       return (-1);
-               total += opcount;
-               if (opcount >= dstlen)
-                       goto done;
-               dstlen -= opcount;
-               dst += opcount;
-       }
+       memset(string, 0, sizeof(string));
+
+       if (emit_punct)
+               rslt = strlcat(string, "\"", sizeof(string));
 
        for (; src < origsrc + srclen; src++) {
                if (*src && strchr("\"'$`\\", *src))
                        vis(visbuf, *src, VIS_ALL | VIS_OCTAL, *src+1);
                else
                        vis(visbuf, *src, VIS_OCTAL, *src+1);
-               opcount = snprintf(dst, dstlen, "%s", visbuf);
-               if (opcount == -1)
-                       return (-1);
-               total += opcount;
-               if (opcount >= dstlen)
-                       goto done;
-               dstlen -= opcount;
-               dst += opcount;
+               rslt = strlcat(string, visbuf, sizeof(string));
        }
 
-       if (emit_punct) {
-               opcount = snprintf(dst, dstlen, "\"");
-               if (opcount == -1)
-                       return (-1);
-               total += opcount;
-               if (opcount >= dstlen)
-                       goto done;
-               dstlen -= opcount;
-               dst += opcount;
-       }
-done:
-       return (total);
+       if (emit_punct)
+               rslt = strlcat(string, "\"", sizeof(string));
+
+       if (rslt >= sizeof(string))
+               return (NULL);
+
+       return (string);
 }
 
 /*
@@ -428,9 +410,9 @@ char *
 pretty_print_option(unsigned int code, struct option_data *option,
     int emit_punct)
 {
-       static char optbuf[32768]; /* XXX */
+       static char optbuf[8192]; /* XXX */
        int hunksize = 0, numhunk = -1, numelem = 0;
-       char fmtbuf[32], *op = optbuf;
+       char fmtbuf[32], *op = optbuf, *buf;
        int i, j, k, opleft = sizeof(optbuf);
        unsigned char *data = option->data;
        unsigned char *dp = data;
@@ -563,8 +545,11 @@ pretty_print_option(unsigned int code, struct option_data *option,
                for (j = 0; j < numelem; j++) {
                        switch (fmtbuf[j]) {
                        case 't':
-                               opcount = pretty_print_string(op, opleft,
-                                   dp, len, emit_punct);
+                               buf = pretty_print_string(dp, len, emit_punct);
+                               if (buf == NULL)
+                                       opcount = -1;
+                               else
+                                       opcount = strlcat(op, buf, opleft);
                                break;
                        case 'I':
                                memcpy(&foo.s_addr, dp, sizeof(foo.s_addr));