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

[Junio C Hamano <gitster@pobox.com>]

Aditya Garg <gargaditya08@live.com> writes:

> Ok so I was wrong here. The warning actually comes when we compile
> WITHOUT NO_OPENSSL, since auth_oauthbearer is a function there, and
> will attract waddress warnings,

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(-)

diff --git i/imap-send.c w/imap-send.c
index 6dbbf37125..bba2ef0cd0 100644
--- i/imap-send.c
+++ w/imap-send.c
@@ -1267,16 +1267,16 @@ static struct imap_store *imap_open_store(struct imap_server_conf *srvc, const c
 					goto bail;
 				}
 
-				#ifdef NO_OPENSSL
+				/* PLAIN */
+				memset(&cb, 0, sizeof(cb));
+				cb.cont = auth_plain;
+
+				if (NOT_CONSTANT(!cb.cont)) {
 				fprintf(stderr, "You are trying to use PLAIN authentication mechanism "
 					"with OpenSSL library, but its support has not been compiled in.");
 				goto bail;
-				#endif
-
-				/* PLAIN */
+				}
 
-				memset(&cb, 0, sizeof(cb));
-				cb.cont = auth_plain;
 				if (imap_exec(ctx, &cb, "AUTHENTICATE PLAIN") != RESP_OK) {
 					fprintf(stderr, "IMAP error: AUTHENTICATE PLAIN failed\n");
 					goto bail;
@@ -1289,16 +1289,16 @@ static struct imap_store *imap_open_store(struct imap_server_conf *srvc, const c
 					goto bail;
 				}
 
-				#ifdef NO_OPENSSL
-				fprintf(stderr, "If you want to use CRAM-MD5 authentication mechanism, "
-					"you have to build git-imap-send with OpenSSL library.");
-				goto bail;
-				#endif
 
 				/* 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;
@@ -1311,16 +1311,15 @@ static struct imap_store *imap_open_store(struct imap_server_conf *srvc, const c
 					goto bail;
 				}
 
-				#ifdef NO_OPENSSL
-				fprintf(stderr, "You are trying to use OAUTHBEARER authentication mechanism "
-					"with OpenSSL library, but its support has not been compiled in.");
-				goto bail;
-				#endif
-
 				/* OAUTHBEARER */
-
 				memset(&cb, 0, sizeof(cb));
 				cb.cont = auth_oauthbearer;
+
+				if (NOT_CONSTANT(!cb.cont)) {
+				fprintf(stderr, "You are trying to use OAUTHBEARER authentication mechanism "
+					"with OpenSSL library, but its support has not been compiled in.");
+				goto bail;
+				}
 				if (imap_exec(ctx, &cb, "AUTHENTICATE OAUTHBEARER") != RESP_OK) {
 					fprintf(stderr, "IMAP error: AUTHENTICATE OAUTHBEARER failed\n");
 					goto bail;
@@ -1333,16 +1332,16 @@ static struct imap_store *imap_open_store(struct imap_server_conf *srvc, const c
 					goto bail;
 				}
 
-				#ifdef NO_OPENSSL
+				/* XOAUTH2 */
+				memset(&cb, 0, sizeof(cb));
+				cb.cont = auth_xoauth2;
+
+				if (NOT_CONSTANT(!cb.cont)) {
 				fprintf(stderr, "You are trying to use XOAUTH2 authentication mechanism "
 					"with OpenSSL library, but its support has not been compiled in.");
 				goto bail;
-				#endif
-
-				/* XOAUTH2 */
+				}
 
-				memset(&cb, 0, sizeof(cb));
-				cb.cont = auth_xoauth2;
 				if (imap_exec(ctx, &cb, "AUTHENTICATE XOAUTH2") != RESP_OK) {
 					fprintf(stderr, "IMAP error: AUTHENTICATE XOAUTH2 failed\n");
 					goto bail;