Re: [PATCH v13 02/10] imap-send: add support for OAuth2.0 authentication

Aditya Garg <gargaditya08@live.com> writes:

But the patch in this thread, when compiled with NO_OPENSSL, gives "-Wunreachable".

--- * ------ * ------ * ------ * ------ * ---

$ make V=1 CC=clang DEVELOPER=YesPlease CFLAGS=-Waddress NO_OPENSSL=NoThanks imap-send.o
    * new build flags
clang -o imap-send.o -c -MF ./.depend/imap-send.o.d -MQ imap-send.o -MMD -MP  -Werror -Wall -pedantic -Wpedantic -std=gnu99 -Wdeclaration-after-statement -Wformat-security -Wold-style-definition -Woverflow -Wpointer-arith -Wstrict-prototypes -Wunused -Wvla -Wwrite-strings -fno-common -Wunreachable-code -Wcomma -Wtautological-constant-out-of-range-compare -Wextra -Wmissing-prototypes -Wno-empty-body -Wno-missing-field-initializers  -Waddress  -I. -DGIT_HOST_CPU="\"x86_64\"" -DHAVE_ALLOCA_H  -DUSE_CURL_FOR_IMAP_SEND -DNO_OPENSSL -DSUPPORTS_SIMPLE_IPC -DSHA1_DC -DSHA1DC_NO_STANDARD_INCLUDES -DSHA1DC_INIT_SAFE_HASH_DEFAULT=0 -DSHA1DC_CUSTOM_INCLUDE_SHA1_C="\"git-compat-util.h\"" -DSHA1DC_CUSTOM_INCLUDE_UBC_CHECK_C="\"git-compat-util.h\"" -DSHA256_BLK  -DHAVE_PATHS_H -DHAVE_DEV_TTY -DHAVE_CLOCK_GETTIME -DHAVE_CLOCK_MONOTONIC -DHAVE_SYNC_FILE_RANGE -DHAVE_SYSINFO -DHAVE_GETDELIM -DHAVE_GETRANDOM -DFREAD_READS_DIRECTORIES -DNO_STRLCPY -DSHELL_PATH='"/bin/sh"'  imap-send.c
imap-send.c:1344:5: error: code will never be executed [-Werror,-Wunreachable-code]
 1344 |                                 memset(&cb, 0, sizeof(cb));
      |                                 ^~~~~~
imap-send.c:1322:5: error: code will never be executed [-Werror,-Wunreachable-code]
 1322 |                                 memset(&cb, 0, sizeof(cb));
      |                                 ^~~~~~
imap-send.c:1300:5: error: code will never be executed [-Werror,-Wunreachable-code]
 1300 |                                 memset(&cb, 0, sizeof(cb));
      |                                 ^~~~~~
imap-send.c:1278:5: error: code will never be executed [-Werror,-Wunreachable-code]
 1278 |                                 memset(&cb, 0, sizeof(cb));
      |                                 ^~~~~~
4 errors generated.
make: *** [Makefile:2820: imap-send.o] Error 1

--- * ------ * ------ * ------ * ------ * ---

See the patch attached at the end [*] that you can apply to 'seen' to try out; the result compiles with all combinations of gcc/clang and with and without NO_OPENSSL.

Having said that, I think you should first clean up the repetitions before piling a lot of different auth_method on top.

A part of the resulting code after applying [*] to 'seen' looks like this:

	} else if (!strcmp(srvc->auth_method, "CRAM-MD5")) {
		if (!CAP(AUTH_CRAM_MD5)) {
			fprintf(stderr, "You specified "
				"CRAM-MD5 as authentication method, "
				"but %s doesn't support it.\n", srvc->host);
			goto bail;
		}

		/* CRAM-MD5 */
		memset(&cb, 0, sizeof(cb));
		cb.cont = auth_cram_md5;

		if (NOT_CONSTANT(!cb.cont)) {
		fprintf(stderr, "If you want to use CRAM-MD5 authentication mechanism, "
			"you have to build git-imap-send with OpenSSL library.");
		goto bail;
		}
		if (imap_exec(ctx, &cb, "AUTHENTICATE CRAM-MD5") != RESP_OK) {
			fprintf(stderr, "IMAP error: AUTHENTICATE CRAM-MD5 failed\n");
			goto bail;
		}

This pattern repeats for different auth methods as you add copy-and-paste, which made the code way too long and too deeply indented to be readable. Why not make the above into a single helper function that you can reuse, so that the call site can become something like

	} else if (!strcmp(srvc->auth_method, "PLAIN")) {
	    if (try_auth_method(srvc, ctx, "PLAIN", AUTH_PLAIN, auth_plain))
		goto bail;
	} else if (!strcmp(srvc->auth_method, "CRAM-MD5")) {
	    if (try_auth_method(srvc, ctx, "CRAM-MD5", AUTH_CRAM_MD5, auth_cram_md5))
		goto bail;
	} ...

If you did it at the very beginning of the series (i.e. when you only have CRAM_MD5 you inherited from the original before adding all the other new ones), each addition would become a lot simpler and main flow in the the resulting code would become quite simple like the above.

static int try_auth_method(struct imap_server_conf *srvc,
			   struct imap_store *ctx,
			   const char *auth_method,
			   enum CAPABILITY cap,
			   int (*fn)(struct imap_store *, const char *))
{
        struct imap_cmd_cb cb = {0};

	if (!CAP(cap)) {
		fprintf(stderr, "You specified "
			"%s as authentication method, "
			"but %s doesn't support it.\n",
			auth_method, srvc->host);
		return -1;
	}
	cb.cont = fn;

	if (NOT_CONSTANT(!cb.cont)) {
		fprintf(stderr, "If you want to use %s authentication mechanism, "
			"you have to build git-imap-send with OpenSSL library.",
			auth_method);
		return -1;
	}
	if (imap_exec(ctx, &cb, "AUTHENTICATE %s", auth_method) != RESP_OK) {
		fprintf(stderr, "IMAP error: AUTHENTICATE %s failed\n",
                	auth_method);
		return -1;
	}
        return 0;
}

So here is that [*] patch that is incorrectly indented to avoid even deeper nesting.

---- >8 ----

 imap-send.c | 49 ++++++++++++++++++++++++-------------------------
 1 file changed, 24 insertions(+), 25 deletions(-)