Thread (104 messages) flat view 104 messages, 5 authors, 2016-06-15

Re: [PATCH 10/67] mailsplit: make PATH_MAX buffers dynamic

From: Eric Sunshine <hidden>
Date: 2016-06-15 23:06:34

On Tue, Sep 15, 2015 at 11:28 AM, Jeff King [off-list ref] wrote:
quoted hunk ↗ jump to hunk
There are several static PATH_MAX-sized buffers in
mailsplit, along with some questionable uses of sprintf.
These are not really of security interest, as local
mailsplit pathnames are not typically under control of an
attacker.  But it does not hurt to be careful, and as a
bonus we lift some limits for systems with too-small
PATH_MAX varibles.

Signed-off-by: Jeff King <redacted>
---
diff --git a/builtin/mailsplit.c b/builtin/mailsplit.c
index 9de06e3..fb0bc08 100644
--- a/builtin/mailsplit.c
+++ b/builtin/mailsplit.c
@@ -148,8 +155,7 @@ static int maildir_filename_cmp(const char *a, const char *b)
 static int split_maildir(const char *maildir, const char *dir,
        int nr_prec, int skip)
 {
-       char file[PATH_MAX];
-       char name[PATH_MAX];
+       struct strbuf file = STRBUF_INIT;
        FILE *f = NULL;
        int ret = -1;
        int i;
@@ -161,20 +167,25 @@ static int split_maildir(const char *maildir, const char *dir,
                goto out;

        for (i = 0; i < list.nr; i++) {
-               snprintf(file, sizeof(file), "%s/%s", maildir, list.items[i].string);
-               f = fopen(file, "r");
+               char *name;
+
+               strbuf_reset(&file);
+               strbuf_addf(&file, "%s/%s", maildir, list.items[i].string);
+
+               f = fopen(file.buf, "r");
                if (!f) {
-                       error("cannot open mail %s (%s)", file, strerror(errno));
+                       error("cannot open mail %s (%s)", file.buf, strerror(errno));
                        goto out;
                }

                if (strbuf_getwholeline(&buf, f, '\n')) {
-                       error("cannot read mail %s (%s)", file, strerror(errno));
+                       error("cannot read mail %s (%s)", file.buf, strerror(errno));
                        goto out;
                }

-               sprintf(name, "%s/%0*d", dir, nr_prec, ++skip);
+               name = xstrfmt("%s/%0*d", dir, nr_prec, ++skip);
                split_one(f, name, 1);
+               free(name);
Hmm, why does 'file' become a strbuf which is re-used each time
through the loop, but 'name' is treated differently and gets
re-allocated upon each iteration? Why doesn't 'name' deserve the same
treatment as 'file'?
quoted hunk ↗ jump to hunk
                fclose(f);
                f = NULL;
@@ -184,6 +195,7 @@ static int split_maildir(const char *maildir, const char *dir,
 out:
        if (f)
                fclose(f);
+       strbuf_release(&file);
        string_list_clear(&list, 1);
        return ret;
 }
@@ -191,7 +203,6 @@ out:
 static int split_mbox(const char *file, const char *dir, int allow_bare,
                      int nr_prec, int skip)
 {
-       char name[PATH_MAX];
        int ret = -1;
        int peek;
@@ -218,8 +229,9 @@ static int split_mbox(const char *file, const char *dir, int allow_bare,
        }

        while (!file_done) {
-               sprintf(name, "%s/%0*d", dir, nr_prec, ++skip);
+               char *name = xstrfmt("%s/%0*d", dir, nr_prec, ++skip);
                file_done = split_one(f, name, allow_bare);
+               free(name);
Same question, pretty much: Why not make 'name' a strbuf which is
re-used in the loop? (I don't have a strong preference; I'm just
trying to understand the apparent inconsistency of treatment.)
        }

        if (f != stdin)
--
2.6.0.rc2.408.ga2926b9
Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help