On 22-05-2025 11:30 pm, Eric Sunshine wrote:

On Thu, May 22, 2025 at 1:29 PM Aditya Garg [off-list ref] wrote:

quoted

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 <redacted>
---

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;
}