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

[Jeff King]

On Tue, Sep 15, 2015 at 08:51:26PM -0400, Eric Sunshine wrote:

> > if (strbuf_getwholeline(, 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'?

My thinking was rather the other way around: why doesn't "file" get the
same treatment as "name"?

I generally prefer xstrfmt to strbufs in these patches for two reasons:

  1. The result has fewer lines.

  2. The variable switches from an array to a pointer, so accessing it
 doesn't change. Whereas with a strbuf, you have to s/foo/foo.buf/
 wherever it is accessed.

We can do that easily with "name"; we allocate it, use it, and free it.
But the lifetime of "file" crosses the "goto out" boundaries, and so
it's simplest to clean it up in the "out" section. Doing that correctly
with a bare pointer is tricky (you have to re-NULL it every time you
free the old value), whereas the strbuf's invariants make it trivial.

I guess we could get away with always calling free() right before
assigning (the equivalent of strbuf_reset()), and then rely on exiting
the loop to "out" to do the final free. And then the result (versus the
original code, not my patch) would look like:

diff --git a/builtin/mailsplit.c b/builtin/mailsplit.c
index 9de06e3..a82dd0d 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];
+   char *file = NULL;
FILE *f = NULL;
int ret = -1;
int i;
@@ -161,7 +167,11 @@ 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);
+   char *name;
+
+   free(file);
+   file = xstrfmt("%s/%s", maildir, list.items[i].string);
+
f = fopen(file, "r");
if (!f) {
error("cannot open mail %s (%s)", file, 
strerror(errno));
@@ -173,8 +183,9 @@ static int split_maildir(const char *maildir, const char 
*dir,
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);
 
fclose(f);
f = NULL;
@@ -184,6 +195,7 @@ static int split_maildir(const char *maildir, const char 
*dir,
 out:
if (f)
fclose(f);
+   free(file);
string_list_clear(, 1);
return ret;
 }

which is not so bad.

Of course this is more allocations per loop than using a strbuf. I doubt
it matters in practice (we are about to fopen() and read into a strbuf,
after all!), but we could also follow the opposite direction and use
strbufs for both.

-Peff
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Jeff King]

On Wed, Sep 16, 2015 at 06:14:18AM -0400, Jeff King wrote:

> I guess we could get away with always calling free() right before
> assigning (the equivalent of strbuf_reset()), and then rely on exiting
> the loop to "out" to do the final free. And then the result (versus the
> original code, not my patch) would look like:

And here is the whole patch converted to that style. I'm planning to go
with this, as the resulting diff is much smaller and much more clear
that we are touching only the allocations.

I _hope_ the result is pretty easy to understand. There is some subtlety
to the loop assumptions:

 - on entering, the pointer is NULL or an allocated buffer to be
   recycled; either way, free() is OK

 - on leaving, the pointer is either NULL (if we never ran the loop) or
   a buffer to be freed. The free() at the end is necessary to handle
   this.

That is not too complicated, but it is not an idiom we use elsewhere
(whereas recycled strbufs are). I can switch the whole thing to strbufs
if that's the direction we want to go.

-- >8 --
Subject: [PATCH] mailsplit: make PATH_MAX buffers dynamic

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, and you could generally only overflow a few
numbers at the end of a path that approaches PATH_MAX (a
longer path would choke mailsplit long before). 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 
---
 builtin/mailsplit.c | 34 +++---
 1 file changed, 23 insertions(+), 11 deletions(-)

diff --git a/builtin/mailsplit.c b/builtin/mailsplit.c
index 9de06e3..104277a 100644
--- a/builtin/mailsplit.c
+++ b/builtin/mailsplit.c
@@ -98,30 +98,37 @@ static int populate_maildir_list(struct string_list *list, 
const char *path)
 {
DIR *dir;
struct dirent *dent;
-   char name[PATH_MAX];
+   char *name = NULL;
char *subs[] = { "cur", "new", NULL };
char **sub;
+   int ret = -1;
 
for (sub = subs; *sub; ++sub) {
-   snprintf(name, sizeof(name), "%s/%s", path, *sub);
+   free(name);
+   name = xstrfmt("%s/%s", path, *sub);
if ((dir = opendir(name)) == NULL) {
if (errno == ENOENT)
continue;
error("cannot opendir %s (%s)", name, strerror(errno));
-   return -1;
+   goto out;
}
 
while ((dent = readdir(dir)) != NULL) {
if (dent->d_name[0] == '.')
continue;
-   snprintf(name, sizeof(name), "%s/%s", *sub, 
dent->d_name);
+   free(name);
+   name = xstrfmt("%s/%s", *sub, dent->d_name);
string_list_insert(list, name);
}
 
closedir(dir);
}
 
-   return 0;
+   ret = 0;
+
+out:
+   free(name);
+   return ret;
 }
 
 static int maildir_filename_cmp(const char *a, const char *b)
@@ -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];
+   char *file = NULL;
FILE *f = NULL;
int ret = -1;
int i;
@@ -161,7 +167,11 @@ 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);
+   char *name;
+
+   free(file);
+   file = xstrfmt("%s/%s", maildir, list.items[i].string);
+
f = fopen(file, "r");
if (!f) {
error("cannot open mail %s (%s)", file, 
strerror(errno));
@@ -173,8 +183,9 @@ static int split_maildir(const char *maildir, const char 
*dir,
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);
 
fclose(f);
f = NULL;
@@ -184,6 +195,7 @@ static int split_maildir(const char *maildir, const char 
*dir,
 out:
if (f)
fclose(f);
+   free(file);
string_list_clear(, 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 

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

[Junio C Hamano]

Jeff King  writes:

> + free(file);
> + file = xstrfmt("%s/%s", maildir, list.items[i].string);

Repeated pattern makes one wonder if a thin wrapper

xstrfmt_to(, "%s/%s", maildir, list.items[i].string);

that first frees the existing value and then overwrites is an
overall win.  Perhaps not, as you would (1) initialize the variable
to NULL before doing a series of xstrfmt_to(), and (2) free the final
one yourself.

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Jeff King]

On Wed, Sep 16, 2015 at 11:13:37AM -0700, Junio C Hamano wrote:

> Jeff King  writes:
> 
> > +   free(file);
> > +   file = xstrfmt("%s/%s", maildir, list.items[i].string);
> 
> Repeated pattern makes one wonder if a thin wrapper
> 
>   xstrfmt_to(, "%s/%s", maildir, list.items[i].string);
> 
> that first frees the existing value and then overwrites is an
> overall win.  Perhaps not, as you would (1) initialize the variable
> to NULL before doing a series of xstrfmt_to(), and (2) free the final
> one yourself.

Yeah, exactly. If you want to wrap it up in something that understands
invariants, I think strbuf is the way to go. I dunno. Maybe I should
just have done this whole thing with strbufs.

-Peff
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Eric Sunshine]

On Tue, Sep 15, 2015 at 11:28 AM, Jeff King  wrote:
> 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 
> ---
> 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();
> +   strbuf_addf(, "%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(, 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'?

> 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();
> string_list_clear(, 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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Jeff King]

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 
---
 builtin/mailsplit.c | 46 +-
 1 file changed, 29 insertions(+), 17 deletions(-)

diff --git a/builtin/mailsplit.c b/builtin/mailsplit.c
index 9de06e3..fb0bc08 100644
--- a/builtin/mailsplit.c
+++ b/builtin/mailsplit.c
@@ -98,30 +98,37 @@ static int populate_maildir_list(struct string_list *list, 
const char *path)
 {
DIR *dir;
struct dirent *dent;
-   char name[PATH_MAX];
+   struct strbuf name = STRBUF_INIT;
char *subs[] = { "cur", "new", NULL };
char **sub;
+   int ret = -1;
 
for (sub = subs; *sub; ++sub) {
-   snprintf(name, sizeof(name), "%s/%s", path, *sub);
-   if ((dir = opendir(name)) == NULL) {
+   strbuf_reset();
+   strbuf_addf(, "%s/%s", path, *sub);
+   if ((dir = opendir(name.buf)) == NULL) {
if (errno == ENOENT)
continue;
-   error("cannot opendir %s (%s)", name, strerror(errno));
-   return -1;
+   error("cannot opendir %s (%s)", name.buf, 
strerror(errno));
+   goto out;
}
 
while ((dent = readdir(dir)) != NULL) {
if (dent->d_name[0] == '.')
continue;
-   snprintf(name, sizeof(name), "%s/%s", *sub, 
dent->d_name);
-   string_list_insert(list, name);
+   strbuf_reset();
+   strbuf_addf(, "%s/%s", *sub, dent->d_name);
+   string_list_insert(list, name.buf);
}
 
closedir(dir);
}
 
-   return 0;
+   ret = 0;
+
+out:
+   strbuf_release();
+   return ret;
 }
 
 static int maildir_filename_cmp(const char *a, const char *b)
@@ -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();
+   strbuf_addf(, "%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(, 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);
 
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();
string_list_clear(, 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);
}
 
if (f != stdin)
-- 
2.6.0.rc2.408.ga2926b9

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html