On 22-05-2025 11:30 pm, Eric Sunshine wrote:
> On Thu, May 22, 2025 at 1:29 PM Aditya Garg <gargaditya08@xxxxxxxx> wrote:
>> Upon setting up imap-send config file, I encountered the very first bug.
>> An error showing "no imap store specified" was being displayed on the
>> terminal. Upon investigating further, in static int git_imap_config,
>> cfg->folder was being incorrectly set to NULL in case imap.user, imap.pass,
>> imap.tunnel and imap.authmethod were defined, and the values that these configs
>> intended to set were not being set at all. Because of this, git imap-send was
>> basically not usable at all. The bug seems to be there for quite a while, and
>> has not yet been detected, likely due to better options like git send-email
>> being available.
>>
>> Signed-off-by: Aditya Garg <gargaditya08@xxxxxxxx>
>> ---
>> diff --git a/imap-send.c b/imap-send.c
>> @@ -1316,16 +1316,16 @@ static int git_imap_config(const char *var, const char *val,
>> FREE_AND_NULL(cfg->folder);
>> return git_config_string(&cfg->folder, var, val);
>> } else if (!strcmp("imap.user", var)) {
>> - FREE_AND_NULL(cfg->folder);
>> + FREE_AND_NULL(cfg->user);
>> return git_config_string(&cfg->user, var, val);
>> } else if (!strcmp("imap.pass", var)) {
>> - FREE_AND_NULL(cfg->folder);
>> + FREE_AND_NULL(cfg->pass);
>> return git_config_string(&cfg->pass, var, val);
>> } else if (!strcmp("imap.tunnel", var)) {
>> - FREE_AND_NULL(cfg->folder);
>> + FREE_AND_NULL(cfg->tunnel);
>> return git_config_string(&cfg->tunnel, var, val);
>> } else if (!strcmp("imap.authmethod", var)) {
>> - FREE_AND_NULL(cfg->folder);
>> + FREE_AND_NULL(cfg->auth_method);
>> return git_config_string(&cfg->auth_method, var, val);
>
> Okay, makes sense. It might be worth mentioning in the commit message
> that these copy/paste bugs were introduced by 6d1f198f34 (imap-send:
> fix leaking memory in `imap_server_conf`, 2024-06-07).
>
> 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
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;
}
