[PATCH] tftp(d): fix wrong logic in get/put feature preprocessor work + some error handling enhancements

[Benjamin Cama]

Hi,

I tried to build tftpd applet with only "get" opcode support, but it
didn't work (no data is transfered). I also got some accidental file
overwriting problem which seems to be due to this problem.

Basically, the logic behind the get/put opcode feature macros is
brain-dead. Furthermore, there is a confusion in the code between the
actual get/put opcode and the meaning CMD_GET() and CMD_PUT() are
assigned in tftp_protocol(), which is, get is to receive data (so it's
get opcode for tftp, but put for tftpd) and put is to send it (same for
tftp, but means get for tftpd). I think it would be better to name them
XXX_RECV/XXX_SEND, but I leave that to someone else. It would make the
code far clearer.

So, I removed almost all the macro conditionnal stuff at the top, and
also fixed another bug in the middle (the test for the opcode against
TFTP_RRQ/WRQ was inverted). The previous logic also assumed every opcode
is what was enabled if there's only one opcode selected (so, a get
becomes a put if get support has not been enabled ...). I didn't like it
and find the "general case everywhere" approach better. (yes, you loose
some optimizations) This "general case" is needed anyway either when you
select at least the two applets (tftp and tftpd) or when you choose the
two opcodes.

I also separated one error case (which opcode is supported) and added
another error message saying we don't handle anything else than octet
mode (yes, I got caught by that for some time).

Tested working with different features enabled on 1.15.3, patch against
git master following.

Regards,
benjamin

Signed-off-by: Benjamin Cama <benoar at free.fr>
---
diff --git a/networking/tftp.c b/networking/tftp.c
index 0e5b48d..6629995 100644
--- a/networking/tftp.c
+++ b/networking/tftp.c
@@ -60,19 +60,14 @@ enum {
 	TFTPD_OPT_u = (1 << 10) * ENABLE_TFTPD,
 };
 
-#if ENABLE_FEATURE_TFTP_GET && !ENABLE_FEATURE_TFTP_PUT
-#define IF_GETPUT(...)
-#define CMD_GET(cmd) 1
-#define CMD_PUT(cmd) 0
-#elif !ENABLE_FEATURE_TFTP_GET && ENABLE_FEATURE_TFTP_PUT
-#define IF_GETPUT(...)
-#define CMD_GET(cmd) 0
-#define CMD_PUT(cmd) 1
-#else
+#if ENABLE_FEATURE_TFTP_GET && ENABLE_FEATURE_TFTP_PUT
 #define IF_GETPUT(...) __VA_ARGS__
+#else
+#define IF_GETPUT(...)
+#endif
+
 #define CMD_GET(cmd) ((cmd) & TFTP_OPT_GET)
 #define CMD_PUT(cmd) ((cmd) & TFTP_OPT_PUT)
-#endif
 /* NB: in the code below
  * CMD_GET(cmd) and CMD_PUT(cmd) are mutually exclusive
  */
@@ -667,7 +662,7 @@ int tftp_main(int argc UNUSED_PARAM, char **argv)
 # endif
 	int result;
 	int port;
-	IF_GETPUT(int opt;)
+	int opt;
 
 	INIT_G();
 
@@ -675,7 +670,7 @@ int tftp_main(int argc UNUSED_PARAM, char **argv)
 	opt_complementary = "" IF_FEATURE_TFTP_GET("g:") IF_FEATURE_TFTP_PUT("p:")
 			IF_GETPUT("g--p:p--g:");
 
-	IF_GETPUT(opt =) getopt32(argv,
+	opt = getopt32(argv,
 			IF_FEATURE_TFTP_GET("g") IF_FEATURE_TFTP_PUT("p")
 				"l:r:" IF_FEATURE_TFTP_BLOCKSIZE("b:"),
 			&local_file, &remote_file
@@ -774,13 +769,16 @@ int tftpd_main(int argc UNUSED_PARAM, char **argv)
 	opcode = ntohs(*(uint16_t*)block_buf);
 	if (result < 4 || result >= sizeof(block_buf)
 	 || block_buf[result-1] != '\0'
-	 || (IF_FEATURE_TFTP_PUT(opcode != TFTP_RRQ) /* not download */
-	     IF_GETPUT(&&)
-	     IF_FEATURE_TFTP_GET(opcode != TFTP_WRQ) /* not upload */
-	    )
 	) {
 		goto err;
 	}
+	if (IF_FEATURE_TFTP_GET(opcode != TFTP_RRQ) /* if not download */
+	    IF_GETPUT(&&)
+	    IF_FEATURE_TFTP_PUT(opcode != TFTP_WRQ) /* if not upload */
+	) {
+		error_msg = "unsupported opcode";
+		goto err;
+	}
 	local_file = block_buf + 2;
 	if (local_file[0] == '.' || strstr(local_file, "/.")) {
 		error_msg = "dot in file name";
@@ -788,6 +786,7 @@ int tftpd_main(int argc UNUSED_PARAM, char **argv)
 	}
 	mode = local_file + strlen(local_file) + 1;
 	if (mode >= block_buf + result || strcmp(mode, "octet") != 0) {
+		error_msg = "only octet mode is supported";
 		goto err;
 	}
 # if ENABLE_FEATURE_TFTP_BLOCKSIZE
@@ -815,16 +814,26 @@ int tftpd_main(int argc UNUSED_PARAM, char **argv)
 	}
 # endif
 
-	if (!ENABLE_FEATURE_TFTP_PUT || opcode == TFTP_WRQ) {
+#if ENABLE_FEATURE_TFTP_PUT
+	if (opcode == TFTP_WRQ) {
 		if (opt & TFTPD_OPT_r) {
 			/* This would mean "disk full" - not true */
 			/*error_pkt_reason = ERR_WRITE;*/
 			error_msg = bb_msg_write_error;
 			goto err;
 		}
-		IF_GETPUT(option_mask32 |= TFTP_OPT_GET;) /* will receive file's data */
-	} else {
-		IF_GETPUT(option_mask32 |= TFTP_OPT_PUT;) /* will send file's data */
+		option_mask32 |= TFTP_OPT_GET; /* will receive file's data */
+	}
+	else
+#endif
+#if ENABLE_FEATURE_TFTP_GET
+	if (opcode == TFTP_RRQ) {
+		option_mask32 |= TFTP_OPT_PUT; /* will send file's data */
+	}
+	else
+#endif
+	{
+		goto err;
 	}
 
 	/* NB: if error_pkt_str or error_pkt_reason is set up,