Should we rename os:: functions that are named like standard C- or Posix-functions?

[Thomas Stüfe]

I am preparing a patch to forbid C-heap allocation functions in hotspot as
you proposed (https://github.com/openjdk/jdk/pull/9356).

Interestingly, not all occurrences of forbidden functions are found
everywhere. I found that if I compile on Ubuntu 20.04 with gcc 10.3., it
does not complain about "realpath" even though I forbade it. If I build on
Alpine, gcc 10.3.1, it finds occurrences of realpath.

This may have to do with the way realpath is defined:

glibc:   extern char *realpath (const char *__restrict __name, char
*__restrict __resolved) __THROW __wur;
(__THROW becomes throw())

muslc:  char *realpath (const char *__restrict, char *__restrict);

For a test, I added "throw()" to the realpath prototype in
"FORBID_C_FUNCTION", but that did not help either. gcc just did not pick up
my use of raw realpath.


On Sat, Jul 2, 2022 at 8:48 PM Kim Barrett <kim.barrett at oracle.com> wrote:

> > On Jul 2, 2022, at 12:57 PM, Thomas Stüfe <thomas.stuefe at gmail.com>
> wrote:
> >
> > On Sat, Jul 2, 2022 at 5:22 PM Kim Barrett <kim.barrett at oracle.com>
> wrote:
> > > On Jun 30, 2022, at 1:21 AM, David Holmes <david.holmes at oracle.com>
> wrote:
> > >
> > > Hi Thomas,
> > >
> > > On 30/06/2022 2:57 pm, Thomas Stüfe wrote:
> > >> Hi,
> > >> several functions in the os:: name scope are deliberately named like
> the official counterparts they replace:
> > >> os::malloc, os::free, os::strdup, os::realloc, os::recv, os::send,
> os::connect, os::signal...
> > >> There may be more. Some of them argument-match their counterparts
> (e.g. os::free), while others don't.
> > >> Since the os:: variants can be called inside the os:: namespace with
> omitting the leading os::, name confusions are possible. "free(p)" means
> something different in global scope or inside an os:: function.
> > >> This can lead to problems that are difficult to find, e.g.,
> mismatched (os::)malloc->(os::)free with the potential to corrupt the
> C-heap:
> > >> […] So I wonder if we should do that. Rename os::<function> to
> something like os::<prefix><function>. And what the prefix or suffix would
> be.
> > >
> > > It annoys me that we have to do such things. It would have made more
> sense for the standard C library routines to have a prefix that marked them
> as reserved identifiers rather than polluting the global namespace the way
> they did. But no one thinks of these things initially and by the time it is
> standardised it is too late to make such changes. :(
> > >
> > > I'm not sure this is a problem we have to address, but if we choose to
> then I think we should try to make a general improvement to the way os is
> used.
> > >
> > > Maybe, as I think has been suggested before, we can move these out of
> the os class as they are not really about the os but the C library, and
> then any renaming that includes a prefix may not look so bad?
> > >
> > > Maybe lib::C_free(), lib::C_malloc() etc?
> >
> > A reminder that JDK-8214976 allows us to "poison" a function, other than
> in
> > explicitly marked places. When I introduced that feature I only marked a
> small
> > number of functions that were "easy". There are many other functions
> that seem
> > like good candidates, but had more fannout than I wanted in that change.
> For
> > example, we could mark ::malloc, ::calloc, ::free, &etc as normally
> forbidden.
> >
> >
> > I really like this, in addition to the name change. Note however that we
> may still need to expose the raw functions to hotspot code for outlier
> cases. Something like "os::raw_malloc()" and "os::raw_free()" for "when you
> really really mean it".
>
> Remember that JDK-8214976 provides a way to disable the poisoning in a
> specific context.
> So you can still use “::malloc()” (for example) where you need it; you
> just need to also say
> that’s really what you meant.
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.openjdk.org/pipermail/hotspot-dev/attachments/20220703/405db37f/attachment.htm>