[DTrace-devel] [PATCH] add BEGIN and END probes

[Eugene Loh]

Thanks for the comments.  I expect to post a new patch momentarily.  A 
few comments below.


On 03/19/2020 02:38 PM, Kris Van Hees wrote:
> On Thu, Mar 19, 2020 at 03:30:41PM -0400, eugene.loh at oracle.com wrote:
>> From: Eugene Loh <eugene.loh at oracle.com>
>> * Deal with the problem that there may be multiple uprobes (like
>>    using different libdtrace.so's, including some that are stale).
>>    Like, what if one person is using installed DTrace and someone
>>    else a personal build?
> I am not sure I see the problem here.

I stated the poorly and even incorrectly.  I'm actually even confused 
where that text came from and how it survived into the commit message.

I agree the problem is minor, but there is a problem that bit me during 
development.  I would update my libdtrace and yet still have old 
probes.  So, I saw a problem that I would not expect people generally to 
see.

Nevertheless, I still wonder if we should be cleaning up more after 
ourselves.  It does not seem to be very social behavior to define lots 
of probes that persist even after we go away.

Further, I do not understand how a personal install of DTrace will not 
step on a systemwide install.  When we write the event to 
/sys/kernel/debug/tracing/uprobe_events, I agree the entries will 
distinguish the two libdtrace paths since the full path name is 
recorded.  On the other hand, since the "nickname" we use in both cases 
is the same, when we hunt for the event in events/uprobes/$name/format 
(to extract ID, etc.) we are extracting an ID that is the same for both 
cases.  I do not understand how this works and I worry that it does 
not.  Actually, with the current code, we will simply look for 
events/uprobes/$name/format and use the ID that's there, never writing 
to uprobe_events at all.

I can try to explain my concerns better if they are not clear here.

>> +/*
>> + * FIXME:
>> + * DTrace v2 introduces functions of the form <provider>_probe_info.
>> + * Unfortunately, for the dtrace provider, this name conflicts with
>> + * the legacy dtrace_probe_info() function.  For now, name the new
>> + * function dtrace_prov_probe_info().  We will clean this mess up later.
> None of these functions are exported in any way so although there has been a
> pretty consistent naming used for functions in providers, there is no actual
> need for that.  We could also easily refer to the dtrace provider as dprov and
> use that as prefix for functions.  Or dtprv or whatever else feels reasonable.
>
> I would prefer to name all provider functions for a givne provider in a
> consistent manner.

Okay.  I hope I understand your comment.  Check my refreshed commit.

>> +	/*
>> +	 * We check to see if the event already exists in the tracing
>> +	 * sub-system.  If not, we try to register the probe with the tracing
>> +	 * sub-system, and try accessing it again.
>> +	 */
>> +again:
>> +	/*
>> +	 * FIXME: gcc complains:
>> +	 *     warning: ???%s??? directive output may be truncated writing up to 255 bytes
>> +	 *         into a region of size 215 [-Wformat-truncation=]
>> +	 *     note: ???snprintf??? output between 49 and 304 bytes into a destination of size 256
>> +	 * How do we make this complaint go away?  Checking the snprintf()
>> +	 * return value against the size of the buffer was supposed to do this.
> Well, given that probe_name can be up to 256 characters, and you are already
> writing a bunch of other characters in 's' (i.e. the constant string portions
> of the format) and s can be up to 256 characters, yes, gcc is right.  I do not
> think that gcc would actually use the earlier conditional on the number of
> bytes written as information for bounds checking here, but even if it did, the
> conditional merely ensure that the number of bytes written to probe_name does
> not exceed the size of probe_name.  Which incidentally is already guaranteed
> by specifying sizeof(probe_name) as size limit for snprint() in the first
> place.
>
> Anyway, why not just use BEGIN or END as probe name?  The probe name can be
> different from the function name that we use to trigger the probe.  I.e. the
> BEGIN probe can be attached to BEGIN_probe (or dtrace_BEGIN in your code).
>
> Since we know that probe_name is never more than 5 characters, you can use that
> information to ensure that the snprintf below never causes an out-of-bounds
> buffer access.
>
>> +	 */
>> +	if (snprintf(s, sizeof (s), "%suprobes/%s/format", EVENTSFS, probe_name) >= sizeof (s)) {
>> +		/* FIXME: now what? */
>> +	}

I'm not sure my comment was very clear.  There are two issues.  One is 
having the code correctly safeguard against buffer overrun.  The comment 
was with respect to something slightly different:  I would like to 
minimize how many compile-time warnings are being generated.  It should 
be possible to write code that does not trigger this warning.  I read 
that checking the snprintf() return value against the buffer size should 
be enough to tell the compiler that it does not need to emit that 
warning.  I think I've had success in cases with that approach.  It did 
not work here.  How can the code be rewritten so that the compiler 
warning is not generated?