Re: [PATCH 1/2] imap-send: fix bug causing cfg->folder being set to NULL

From: Eric Sunshine <sunshine@sunshineco.com>
Date: May 22, 2025, 18:25 UTC
Message-ID: <CAPig+cTJmeczzUcGrn98svMfK7aODYS-Ha8FxJHuKU2c2+R-FQ@mail.gmail.com>
In-Reply-To: <PN3PR01MB95971AADEF1C768E58187419B899A@PN3PR01MB9597.INDPRD01.PROD.OUTLOOK.COM>

On Thu, May 22, 2025 at 2:21 PM Aditya Garg <gargaditya08@live.com> wrote:

Show 11 quoted lines

> On 22-05-2025 11:30 pm, Eric Sunshine wrote:
> > Squinting at the code a bit more, am I correct in thinking that
> > 6d1f198f34 missed a case and that the function is still leaking
> > `cfg->host` in the "imap.host" conditional? I haven't traced the code
> > or all the callers, but I wonder if server_fill_credential() in the
> > same file may also be leaky. In any event, the `cfg->host` and the
> > possible server_fill_credential() leaks are outside the scope of this
> > bug-fix patch.
>
> Not sure about server_fill_credential(), but I think this is also
> a potential memory leak

Agreed.

Show 16 quoted lines

> static int auth_cram_md5(struct imap_store *ctx, const char *prompt)
> {
>         int ret;
>         char *response;
>
>         response = cram(prompt, ctx->cfg->user, ctx->cfg->pass);
>
>         ret = socket_write(&ctx->imap->buf.sock, response, strlen(response));
>         if (ret != strlen(response))
> +               free(response); // fix for the leak
>                 return error("IMAP error: sending response failed");
>
>         free(response);
>
>         return 0;
> }

It's subjective, but I would probably fix this a little bit differently and (to my mind) more simply:

    response = cram(prompt, ctx->cfg->user, ctx->cfg->pass);

    ret = socket_write(&ctx->imap->buf.sock, response, strlen(response));
    free(response);
    if (ret != strlen(response))
        return error("IMAP error: sending response failed");
    return 0;