On Tue, Sep 15, 2015 at 11:28 AM, Jeff King <peff@xxxxxxxx> 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 <peff@xxxxxxxx>
> ---
> 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'?
> 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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html