[DTrace-devel] [PATCH] Fix length stored by strjoin() and optimise the implementation

[Eugene Loh]

Reviewed-by: Eugene Loh <eugene.loh at oracle.com>

You say "Fix length stored by strjoin()."  So the commit message should 
say what was wrong, and the patch should add a test to show that the 
problem was fixed.

Also,

On 11/15/21 12:32 PM, Kris Van Hees via DTrace-devel wrote:
> Signed-off-by: Kris Van Hees <kris.van.hees at oracle.com>
> ---
>   bpf/strjoin.S | 63 +++++++++++++++++++++++++++++----------------------
>   1 file changed, 36 insertions(+), 27 deletions(-)
>
> diff --git a/bpf/strjoin.S b/bpf/strjoin.S
> index da7afa3e..2f78695a 100644
> --- a/bpf/strjoin.S
> +++ b/bpf/strjoin.S
> @@ -19,39 +19,48 @@ dt_strjoin:
>   	mov	%r7, %r2		/* %r7 = s1 */
>   	mov	%r8, %r3		/* %r8 = s2 */
>   
> -	mov	%r1, %r7
> -	call	dt_strlen
> -	mov	%r6, %r0		/* len = dt_strlen(s1) */
> -	add	%r7, DT_STRLEN_BYTES	/* s1 += 2 */
> -
> -	mov	%r1, %r8
> -	call	dt_strlen
> -	add	%r6, %r0		/* len += dt_strlen(s2) */
> -	add	%r8, DT_STRLEN_BYTES	/* s2 += 2 */
> -
> -	mov	%r1, %r6
> -	mov	%r2, %r9
> -	call	dt_strlen_store		/* dt_strlen_store(len, dst) */
> -	add	%r9, DT_STRLEN_BYTES	/* dst += 2 */
> -
>   	lddw	%r6, STRSZ
> -	add	%r6, 1
> -	and	%r6, 0xffffffff		/* len = (STRSZ + 1) & 0xffffffff */
> +	add	%r6, 1			/* cnt = STRSZ + 1 */
>   
> -	mov	%r1, %r9
> +	add	%r1, DT_STRLEN_BYTES	/* dst is in %r1 already */
>   	mov	%r2, %r6
>   	mov	%r3, %r7
> -	call	BPF_FUNC_probe_read_str	/* cnt = bpf_probe_read_str(dst, len s1) */
> -	jslt	%r0, 0, .L1		/* if (cnt < 0) goto .L1 */
> -	jslt	%r0, 1, .L2		/* if (cnt < 1) goto .L2 */
> -	sub	%r0, 1			/* cnt-- */
> -	add	%r9, %r0		/* dst += cnt */
> -	sub	%r6, %r0		/* len -= cnt */
> -.L2:
> +	add	%r3, DT_STRLEN_BYTES
> +	call	BPF_FUNC_probe_read_str	/*
> +					 * rc = probe_read_str(
> +					 *	  &dst[DT_STRLEN_BYTES],
> +					 *	  cnt,
> +					 *	  &s1[DT_STRLEN_BYTES]);
> +					 */
> +	jslt	%r0, 0, .Lexit		/* if (rc < 0) goto .Lexit; */
> +	mov	%r7, %r0		/* len = rc */
> +	jslt	%r7, 1, .Ls2_only	/* if (len < 1) goto .Ls2_only; */

After the probe_read_str() call, the return value cannot be 0. Either it 
is negative (error) or it is strictly positive.  So, the first test 
might as well be "jsle".  Then the second test (and the Ls2_only label) 
are no longer needed.

Now, let's say probe_read_str() errors out.  Then we silently return 
garbage?  Are we happy with that?  (Maybe we are.  I don't know.)

For comments regarding instructions with signed conditionals, might as 
well use (e.g.) "s<", "s<=", and the like.

> +	sub	%r7, 1			/* len-- */
> +	jsge	%r7, %r6, .Lset_len	/* if (len >= cnt) goto .Lset_len; */

Actually, at this stage, we know len<cnt.  Even the BPF verifier knows 
this.  So the test and the Lset_len label can be dropped.

> +
> +.Ls2_only:
>   	mov	%r1, %r9
> +	add	%r1, DT_STRLEN_BYTES
> +	add	%r1, %r7
>   	mov	%r2, %r6
> +	sub	%r2, %r7
>   	mov	%r3, %r8
> -	call	BPF_FUNC_probe_read_str	/* bpf_probe_read_str(dst, len, s2 */
> -.L1:
> +	add	%r3, DT_STRLEN_BYTES
> +	call	BPF_FUNC_probe_read_str	/*
> +					 * rc = probe_read_str(
> +					 *	  &dst[DT_STRLEN_BYTES + len],
> +					 *	  cnt - len,
> +					 *	  &s2[DT_STRLEN_BYTES]);
> +					 */
> +	jslt	%r0, 0, .Lexit		/* if (rc < 0) goto .Lset_len; */
> +	add	%r7, %r0
> +	sub	%r7, 1			/* len += rc - 1 */
> +
> +.Lset_len:
> +	mov	%r1, %r7
> +	mov	%r2, %r9
> +	call	dt_strlen_store		/* dt_strlen_store(len - 1, dst) */
> +
> +.Lexit:
>   	exit
>   	.size	dt_strjoin, .-dt_strjoin