Poisoning in HotSpot

[Kim Barrett]

On 12/1/24 2:55 AM, Julian Waters wrote:
> This seems to change the forbidden interface, which by itself is ok,
> but I have some questions

Changing the syntax also doesn't bother me. I've always expected that if 
the underlying mechanism got changed then the usage syntax might need to 
change. There aren't that many uses (there really shouldn't be lots of 
"allows"). I think there are currently ~35 allows?

> 1. Changing from signature to a name might make the forbidden function
> less specific, as information on exactly which function is forbidden
> is lost. Would this create problems in the event that another function
> with the same name but different signature which is not forbidden is
> called?
The purpose of this mechanism is to prevent the (non-casual) use of 
certain C
library functions.  C doesn't support overloading.  So different 
signatures is
not an issue.

It also doesn't matter, since we're manipulating name lookup. If there were
two overloads of malloc (or frob, in my example), the forbidding still 
blocks
access and the permitting just has to call with the proper arguments. That's
also a reason to *not* base the forbidding declaration on the function being
blocked. It can be anything, and making it a deleted and deprecated function
isn't useful. Mine works fine even if there are multiple overloaded
declarations of the global frob function.

> 2. ALLOW_C_FUNCTION used to take a statement, changing it to a call
> expression would mean every use site would have to change as well.
> Many uses of the macro also use qualified names as well.
Again, the syntax change doesn't bother me.

The statement-based syntax for the old ALLOW_C_FUNCTION is a matter of 
history
and the mechanism used to implement the feature.  The implementation 
involved
setting up a surrounding pragma context, which required a statement.  At 
that
time I had not yet realized that lambdas provide some of the functionality
needed to be expression-based while being able to establish such a 
surrounding
pragma context.  (gcc supports statements and declarations in expressions as
an extension.  lambda provides a portable mechanism for doing that.)  Had I
realized that at the time, I would have made ALLOW_C_FUNCTION 
expression-based.

>   There's also
> the VC specific warning disable in compilerWarnings, which I'm not
> sure why it was there in the first place when FORBID_C_FUNCTION does
> nothing on VC:https://urldefense.com/v3/__https://github.com/openjdk/jdk/blob/28ae281b42cd00f471e275db544a5d23a42df59c/src/hotspot/share/utilities/compilerWarnings_visCPP.hpp*L64__;Iw!!ACWV5N9M2RV99hQ!L0gUwZ8rkVwClErEUKrf4NKjn2-hjhuJu4_a1D6pE2GMOOc8KLuxs31FpxcObtQDkzbcsTmx25hpInegCuhgUM3C$ 
> Now that ALLOW_C_FUNCTION takes an expression, I don't think that same
> warning disable can be done, would that cause issues for
> ALLOW_C_FUNCTION when VC is the compiler?
The comment about the Windows definition of ALLOW_C_FUNCTIONS discusses 
why it
exists. At the time, it seemed like it might be useful for suppressing
deprecation warnings from other sources, such as the "security" warnings and
from _CRT_DECLARE_NONSTDC_NAMES. But we ultimately decided to just globally
use the warning suppression macros for those, so didn't need to use the 
ALLOW
macro for that purpose. So the current definition doesn't really serve any
purpose.

> 3. None of the big 3 compilers inline lambdas by default, only when
> optimizations are enabled. In practice this means slowdebug HotSpot
> will have the wrapper lambda in the final assembly instead of a direct
> call to the C library code. I don't know if this issue is a deal
> breaker
Regarding non-inlining of lambdas in unoptimized code, I don't really 
care all
that much. If it's *really* an issue, `__attribute__((always_inline))` or
`[[gnu::always_inline]]` can help. But the approach of requiring the use 
of a
specially qualified name instead of a context macro does have some 
appeal. But
I think I'd want a less convenient name. The point of this mechanism is to
make it intentionally inconvenient and obvious that the blockade is being
bypassed, because it shouldn't be done casually or "quietly".
>
> In any case I tried again after seeing the new example, but using
> separate macros to handle the permitted namespace and the actual
> forbidding of the function itself. I used the older style signature
> based forbidding rather than the new name based solution (Due to
> concerns over #1), but I included a toggle to switch between name
> based and signature based variants anyway, so a comparison between the
> 2 can easily be made. Rather than ALLOW_C_FUNCTION this example
> directly does permitted::symbol, the meaning of which is also rather
> clear
>
> https://urldefense.com/v3/__https://godbolt.org/z/hdhrh5eev__;!!ACWV5N9M2RV99hQ!L0gUwZ8rkVwClErEUKrf4NKjn2-hjhuJu4_a1D6pE2GMOOc8KLuxs31FpxcObtQDkzbcsTmx25hpInegCsNpF1Dt$ 
I think separate macros for registering the bypass and registering the
forbidding is a mistake, and just has the potential for accidental misuse.
The bypass *must* be registered first, and there's no point to it 
without the
forbidding, so one macro is better.

I've also dropped the "alternative" string. It doesn't do anything useful in
the code. Depending on which compiler is used, it may or may not appear 
in the
error message. I think just a comment at the point of forbidding would
suffice, and can be more extensive than one would want in a string, if/when
that's warranted. Anyone who is encountering a failure clearly has the 
source
code available, and the error messages from all three compilers seems to
provide information about where in the source to look.

How about this:

void* frob(unsigned);
void* frob(unsigned, unsigned);

// name must be unqualified.
#define FORBID_C_FUNCTION(name)                                 \
   namespace permit_forbidden_functions { using ::name; }        \
   inline namespace forbidden_functions { const int name = 0; }  \
   /* */

void* permit_frob(unsigned x) {
   return permit_forbidden_functions::frob(x);
}

void* permit_frob(unsigned x, unsigned y) {
   return permit_forbidden_functions::frob(x, y);
}

// demonstrate error for unqualified call.
#if 0 // change to 1 to trigger error
void* bad_frob1(unsigned x) {
   return frob(x);
}
#endif

// demonstrate error for qualified call.
#if 0 // change to 1 to trigger error
void* bad_frob2(unsigned x) {
   return ::frob(x);
}
#endif
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.openjdk.org/pipermail/hotspot-dev/attachments/20241201/4847480e/attachment-0001.htm>