From: Eric Sunshine <sunshine@sunshineco.com>
Date: Wed, 16 Sep 2015 04:24:38 GMT
Subject: Re: [PATCH 26/67] replace trivial malloc + sprintf /strcpy calls to xstrfmt
Message-ID: <CAPig+cQTGbE4jC2o8zta0HR3ske3y3MoFTd7zBvwMTg6vp3Vag@mail.gmail.com>
In-Reply-To: <20150915154510.GZ29753@sigill.intra.peff.net>

On Tue, Sep 15, 2015 at 11:45 AM, Jeff King <peff@peff.net> wrote:
> replace trivial malloc + sprintf /strcpy calls to xstrfmt

s/to/with/

Also, do you want either to add a space after '/' or drop the one before it?

> It's a common pattern to do:
>
>   foo = xmalloc(strlen(one) + strlen(two) + 1 + 1);
>   sprintf(foo, "%s %s", one, two);
>
> (or possibly some variant with strcpy()s or a more
> complicated length computation).  We can switch these to use
> xstrfmt, which is shorter, involves less error-prone manual
> computation, and removes many sprintf and strcpy calls which
> make it harder to audit the code for real buffer overflows.
>
> Signed-off-by: Jeff King <peff@peff.net>
> ---
> --- a/imap-send.c
> +++ b/imap-send.c
> @@ -889,9 +889,8 @@ static char *cram(const char *challenge_64, const char *user, const char *pass)
>         }
>
>         /* response: "<user> <digest in hex>" */
> -       resp_len = strlen(user) + 1 + strlen(hex) + 1;
> -       response = xmalloc(resp_len);
> -       sprintf(response, "%s %s", user, hex);
> +       response = xstrfmt("%s %s", user, hex);
> +       resp_len = strlen(response);
>
>         response_64 = xmalloc(ENCODED_SIZE(resp_len) + 1);

The original resp_len calculation included the NUL but the revised
does not. If I'm reading this correctly, the revised calculation is
correct, and the original was over-allocating response_64, right?

>         encoded_len = EVP_EncodeBlock((unsigned char *)response_64,