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

[Benjamin Cama]

Ping ?
Is no one interested in committing this fix ? Is there any problem in
the patch ?

Regards,
benjamin

Le lundi 15 février 2010 à 05:23 +0100, Benjamin Cama a écrit :
> 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,
> 
> 
> _______________________________________________
> busybox mailing list
> busybox at busybox.net
> http://lists.busybox.net/mailman/listinfo/busybox