[PATCH v1] strace: Enhance --string-limit/-s to remove max string length limit

[Dmitry V. Levin]

On Sun, Jan 21, 2024 at 04:16:02PM +0530, Gautam Menghani wrote:
> Enhance the -s STRSIZE/--string-limit=STRSIZE option to provide the
> ability to remove the max string length limit with the value argument
> 'inf'. This patch resolves issue 269 on github.

Another issue I see just by looking at the patch is potential integer
overflow followed by memory corruption:

> @@ -2562,10 +2564,15 @@ init(int argc, char *argv[])
>  				error_opt_arg(c, lopt, optarg);
>  			break;
>  		case 's':
> -			i = string_to_uint(optarg);
> -			if (i < 0 || (unsigned int) i > -1U / 4)
> +			if (isdigit(*optarg)) {
> +				i = string_to_uint(optarg);
> +				if (i < 0 || (unsigned int) i > -1U / 4)
> +					error_opt_arg(c, lopt, optarg);
> +				max_strlen = i;
> +			} else if (strcmp(optarg, "inf") == 0)
> +				truncate_output = false;
> +			else
>  				error_opt_arg(c, lopt, optarg);
> -			max_strlen = i;
>  			break;
>  		case 'S':
>  			set_sortby(optarg);

Note that before this change max_strlen was always less than -1U / 4.

> diff --git a/src/util.c b/src/util.c
> index 92c0577ce..68fb0277e 100644
> --- a/src/util.c
> +++ b/src/util.c
> @@ -22,6 +22,7 @@
>  # include <sys/xattr.h>
>  #endif
>  #include <sys/uio.h>
> +#include <sys/sysinfo.h>
>  
>  #include "largefile_wrappers.h"
>  #include "number_set.h"

> @@ -1258,30 +1259,39 @@ printstr_ex(struct tcb *const tcp, const kernel_ulong_t addr,
>  	static char *str;
>  	static char *outstr;
>  
> -	unsigned int size;
> +	unsigned long size;
>  	unsigned int style = user_style;
> +	unsigned long max_len = max_strlen;
>  	int rc;
>  	int ellipsis;
> +	struct sysinfo info;
>  
>  	if (!addr) {
>  		tprint_null();
>  		return -1;
>  	}
> +
> +	if (!truncate_output) {
> +		sysinfo(&info);

Note that expensive sysinfo() is now being called on every printstr_ex()
invocation.

> +		max_len = info.totalram;
> +	}
> +
>  	/* Allocate static buffers if they are not allocated yet. */
> +
>  	if (!str) {
>  		const unsigned int outstr_size =
> -			4 * max_strlen + /* for quotes and NUL */ 3;
> +			4 * max_len + /* for quotes and NUL */ 3;

Unlike former max_strlen, the max_len obtained from info.totalram can
easily exceed -1U / 4, resulting to outstr_size overflow.

>  		/*
>  		 * We can assume that outstr_size / 4 == max_strlen
>  		 * since we have a guarantee that max_strlen <= -1U / 4.
>  		 */
>  
> -		str = xmalloc(max_strlen + 1);
> +		str = xmalloc(max_len + 1);
>  		outstr = xmalloc(outstr_size);

If outstr_size ends up with a value that is less than 4 * max_len + 3,
then outstr would point to a small buffer, resulting to subsequent memory
corruption.

By the way, allocating 4 * totalram of memory is a very questionable idea.


-- 
ldv