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:00 UTC
- Message-ID
- <CAPig+cRNyEC5LjK1GhGBbEtf3xRu_ZS4RKizFhwjE8fP8sGwTA@mail.gmail.com>
- In-Reply-To
- <PN3PR01MB95970F8CF7527648EC4BE907B899A@PN3PR01MB9597.INDPRD01.PROD.OUTLOOK.COM>
On Thu, May 22, 2025 at 1:29 PM Aditya Garg <gargaditya08@live.com> wrote:
Show 32 quoted lines
> 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@live.com>
> ---
> 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.