[DTrace-devel] [PATCH 02/14] Replace not-NULL test with more general runtime test

[Eugene Loh]

On 5/2/23 22:38, Kris Van Hees wrote:

> On Mon, May 01, 2023 at 11:47:10PM -0400, eugene.loh--- via DTrace-devel wrote:
>> From: Eugene Loh <eugene.loh at oracle.com>
>>
>> When we dereference a pointer, it should generally be safe to
>> dereference D (including alloca) pointers, even if a more robust
>> mechanism is needed to guard against dereferencing unsafe pointers
>> in the more general case.  We do, however, check whether a pointer
>> is NULL.
>>
>> There are times, however, when even a D pointer cannot safely be
>> dereferenced.  This will especially be true once NULL string pointers
>> will be supported.
> I am not sure I get this...  Wouldn't a NULL string pointer be NULL, i.e.
> when you load a string value and it has the 'magic value' you should make
> the value of the string pointer NULL, and from that point on it truly is a
> NULL pointer.  Right?

Right.  But:

*)  As far as this patch goes, it's not yet about NULL strings.

*)  You could do some s+1 thing (where s is a string).

But I confess that I don't understand this stuff.

>> So drop the NULL-pointer check and implement dereferencing with a
>> more general scalar load.
> See below why that is a bad idea.
>
>> Signed-off-by: Eugene Loh <eugene.loh at oracle.com>
>> ---
>>   libdtrace/dt_cg.c                   | 6 +-----
>>   test/unittest/pointers/tst.basic2.d | 3 +--
>>   2 files changed, 2 insertions(+), 7 deletions(-)
>>
>> diff --git a/libdtrace/dt_cg.c b/libdtrace/dt_cg.c
>> index 483603ef..c1fd46c0 100644
>> --- a/libdtrace/dt_cg.c
>> +++ b/libdtrace/dt_cg.c
>> @@ -5884,7 +5884,6 @@ dt_cg_node(dt_node_t *dnp, dt_irlist_t *dlp, dt_regset_t *drp)
>>   			uint_t	op;
>>   			ssize_t	size;
>>   
>> -			dt_cg_check_notnull(dlp, drp, dnp->dn_reg);
>>   			op = dt_cg_ldsize(dnp, ctfp, dnp->dn_type, &size);
>>   
>>   			/*
>> @@ -5899,10 +5898,7 @@ dt_cg_node(dt_node_t *dnp, dt_irlist_t *dlp, dt_regset_t *drp)
>>   						 dnp->dn_reg);
>>   			}
>>   
>> -			if (dnp->dn_child->dn_flags & (DT_NF_ALLOCA | DT_NF_DPTR))
>> -				emit(dlp, BPF_LOAD(op, dnp->dn_reg, dnp->dn_reg, 0));
>> -			else
>> -				dt_cg_load_scalar(dnp, op, size, dlp, drp);
>> +			dt_cg_load_scalar(dnp, op, size, dlp, drp);
> The problem here is that you end up going back to the general case of
> treating all pointers here as arbitrary pointers which bypasses the BPF
> verifier checks that the load instruction is able to count on.

What's wrong with relying on the general mechanism?  It's not like we're 
going to end up with unsafe programs.

> The main problem you are experiencing is that *assoc[val] is not being
> recognized as dereferencing an arbitrary pointer (and it has to be an
> arbitrary pointee we cannot store pointers to D managed storage as real
> pointers in an associative array.  So, I think this code needs to be updated
> to recognize this specific case and not treat assoc[val] as a DPTR just
> because assoc is a DPTR.

I'm not sure I understand.

Anyhow, I just sent out a v2 with my best guess as to what to do. The 
patch number is still 02/14, but now it's titled "[PATCH v2 02/14] Catch 
special DPTR cases in the parser".  By the way, if folks are happy with 
the new patch except that the string portion should be deferred until 
the "NULL string" patch, I'm happy to do that.  First, though, I'd like 
buy-in on this approach.

An alternative to the "string" part of the fix is to have the parser 
strip DPTR for (ptr+int) and (int+ptr) combinations.

(I just don't get the parser.)

>>   		}
>>   		break;
>>   
>> diff --git a/test/unittest/pointers/tst.basic2.d b/test/unittest/pointers/tst.basic2.d
>> -/* @@xfail: dtv2 */