[DTrace-devel] [PATCH 11/14] map_value_or_null: add check for NULL pointer in load_var()

[Nick Alcock]

On 2 May 2023, eugene loh told this:

> From: Eugene Loh <eugene.loh at oracle.com>
>
> When we look up a dynamic pointer (e.g., associative array or thread-local
> variable), the BPF verifier sees that we get back a "map_value_or_null".
> Ultimately, we will check the validity of the value.  If we first add an
> offset to it, however, the BPF verifier objects.  This is a little strange,
> since the verifier is fine adding an integer to 0 or to a map value.
> Nevertheless, that's the situation.

There's even a comment in the verifier source that it's not worth doing
a better job, but without any reason given as to why (of course not: I'm
surprised there was even a comment).

> Check the value.  If 0, assign 0.  This seems like it doesn't do anything,
> but it allows the BPF verifier to branch and handle map_value_or_null as
> two separate cases.
> 
> Add tests.  While this fix addresses dynamic variables (associative arrays
> and thread-local variables), add tests for global and local variables also.

Reviewed-by: Nick Alcock <nick.alcock at oracle.com>

... but...

>  		if (dst->dn_flags & DT_NF_REF) {
> -			emit(dlp,  BPF_MOV_REG(dst->dn_reg, BPF_REG_0));
> +			uint_t	lbl_notnull = dt_irlist_label(dlp);
> +			uint_t	lbl_done = dt_irlist_label(dlp);
> +
> +			emit(dlp,  BPF_BRANCH_IMM(BPF_JNE, BPF_REG_0, 0, lbl_notnull));
> +			emit(dlp,  BPF_MOV_IMM(dst->dn_reg, 0));
> +			emit(dlp,  BPF_JUMP(lbl_done));
> +			emitl(dlp, lbl_notnull,
> +				   BPF_MOV_REG(dst->dn_reg, BPF_REG_0));
> +			emitl(dlp, lbl_done,
> +				   BPF_NOP());

This really needs a comment above the assignment explaining what you put
in the commit log.