Re: [PATCH 05/14] probe: get the size of the hash-lookup key right

[Nick Alcock <nick.alcock@oracle.com>]

On 25 Oct 2024, Kris Van Hees stated:

> On Thu, Oct 24, 2024 at 12:37:49PM +0100, Nick Alcock wrote:
>> This was allocated with alloca() but then snprintf()ed with a size of
>> INT_MAX.  This makes _FORTIFY_SOURCE rightly unhappy.
>> 
>> Passing in the size we actually allocated is trivial.
>
> Except for the fact that dt_probe_keylen() and dt_probe_key() are only
> ever used from dt_probe_lookup2().  So, getting rid of those two functions
> and rewriting the call as shown below seems to be a much better fix.

Yeah. I assumed you wanted to keep the alloca() for some reason, but if
you don't (and it seems to be not incredibly helpful), then I agree.

>> Signed-off-by: Nick Alcock <nick.alcock@oracle.com>
>> Bug: https://github.com/oracle/dtrace-utils/issues/78
>> ---
>>  libdtrace/dt_probe.c | 6 +++---
>>  1 file changed, 3 insertions(+), 3 deletions(-)
>> 
>> diff --git a/libdtrace/dt_probe.c b/libdtrace/dt_probe.c
>> index 686e2a661253..bb1773ee60d4 100644
>> --- a/libdtrace/dt_probe.c
>> +++ b/libdtrace/dt_probe.c
>> @@ -180,9 +180,9 @@ dt_probe_keylen(const dtrace_probedesc_t *pdp)
>>  }
>>  
>>  static char *
>> -dt_probe_key(const dtrace_probedesc_t *pdp, char *s)
>> +dt_probe_key(const dtrace_probedesc_t *pdp, char *s, size_t len)
>>  {
>> -	snprintf(s, INT_MAX, "%s:%s:%s", pdp->mod, pdp->fun, pdp->prb);
>> +	snprintf(s, len, "%s:%s:%s", pdp->mod, pdp->fun, pdp->prb);
>>  	return s;
>>  }
>>  
>> @@ -204,7 +204,7 @@ dt_probe_lookup2(dt_provider_t *pvp, const char *s)
>>  		return NULL; /* dt_errno is set for us */
>>  
>>  	keylen = dt_probe_keylen(&pd);
>> -	key = dt_probe_key(&pd, alloca(keylen));
>> +	key = dt_probe_key(&pd, alloca(keylen), keylen);
>
> Better would be:
> 	if (asprintf(&key, "%s:%s:%s", pdp->mod, pdp->fun, pdp->prb) == -1) {
> 		dt_set_errno(dtp, errno);
> 		return NULL;
> 	}

Went from that into a minor can of worms: the entire function is rusted
kernel-dtrace-era stuff, from a header comment that talks about caching
and interrogation of the kernel that the function never does, through a
probedesc that is always leaked, errno checks that succeed only randomly
(checking errno but no longer doing so after a function that can set
errno on failure, so basically random) which leads to the wrong error
return almost all the time... ... I think my latest version fixes all
those problems (more description in its commit comment, forthcoming).

Any problems in this one will likely be uncovered by the pre-release
valgrind run! but I'll do a full make check before mailing the series
out again anyway.

-- 
NULL && (void)