On Wed, Sep 16, 2015 at 12:24:38AM -0400, Eric Sunshine wrote:
> On Tue, Sep 15, 2015 at 11:45 AM, Jeff King <peff@xxxxxxxx> 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?
Thanks, fixed both.
> > --- 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?
Hrm, I didn't notice that. We actually feed "resp_len" directly to
EVP_EncodeBlock, meaning it it _would_ include the trailing NUL in the
encoded output. So my modification is not wrong memory-wise (we allocate
enough to hold the result), but does produce different output.
I'm not at all convinced that the original is correct, but if we are
going to fix it, it should definitely not be part of this patch. I'll
switch it to:
resp_len = strlen(response) + 1;
which matches the original.
Thanks for being such a careful reviewer. :)
-Peff
--
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