portability: locale_t need not be a pointer type

[ischwarze]

I think src/hb-common.cc and src/hb-machinery.hh make non-portable assumptions about the properties of the type locale_t. All that POSIX requires is that something of type locale_t is an "object", and i think the word "object" is to be understood in the sense of the C standard. In particular, POSIX does not require locale_t to be a pointer type, and even less so that it points to anything. If i read POSIX correctly, having "typedef int locale_t" in <locale.h> would be perfectly valid.

However, src/hb-common.cc invokes "hb_remove_pointer<locale_t>" which can only work if locale_t is a pointer type. Even worse, the parent class of hb_C_locale_lazy_loader_t, that is, the class hb_lazy_loader_t declared in src/hb-machinery.hh, assumes that locale_t is a pointer to a non-void type: in the declaration of the "operator *" method of hb_lazy_loader_t, it takes a reference to the type pointed to by locale_t:

const Returned & operator * () const  { return *get (); }

In case of "typedef void *locale_t", the "void &" contained in this line is a syntax error:

In file included from ../harfbuzz-3.3.1/src/hb-common.cc:30:
../harfbuzz-3.3.1/src/hb-machinery.hh:197:18: error: cannot form a reference to 'void'
const Returned & operator * () const  { return *get (); }
               ^
../harfbuzz-3.3.1/src/hb-common.cc:1067:43: note: in instantiation
of template class 'hb_lazy_loader_t<void, hb_C_locale_lazy_loader_t, void, 0>' requested here
static struct hb_C_locale_lazy_loader_t : hb_lazy_loader_t<hb_remove_pointer<hb_locale_t>,

My impression is that all these assumptions are completely gratuitious. I inspected the files src/hb-common.cc, src/hb-machinery.hh, and src/hb-meta.hh and failed to find any code that might actually dereference a locale_t pointer. Also, application code cannot do that because the type hb_C_locale_lazy_loader_t is local to the file src/hb-common.cc rather than contained in any public header, so applications cannot instantiate such an object, and the only object instantiated internally, static_C_locale, is static in the same file, so application code cannot access that object either. As far as i can see, using the excessively generic parent class is not required for any practical purpose, and the only effect is to kill the build. Besides, actually dereferencing locale_t would also be useless because the size and layout of locale_t is system-dependent, and even within any given operating system, the ABI may change from one OS version to the next, so harfbuzz could not usefully peek into the object even if it wanted to.

I think it would be best to clean up your code and let it respect POSIX such that it works with "typedef int locale_t" - or at least with "typedef void *locale_t".

The issue was found by Antoine Jacoutot while trying to port harfbuzz-3.3.1 to the upcoming OpenBSD 7.1 release (OpenBSD is an example of a real-world system using "typedef void *locale_t"). Part of the analysis was done by Marc Espie, part by me.

I'm also considering to change the OpenBSD libc implementation to "typedef struct _locale *locale_t" with a dummy struct, but i'd hate having to do that because we don't really need it and POSIX does not require it. Apart from harfbuzz, i'm not aware of any other software that makes such assumptions, so changing the C library just for harfbuzz feels like an extreme measure to me, in particular given that even harfbuzz doesn't seem to do anything useful with it either.

[behdad]

You are correct in your analysis. We just had a lazy-loader implementation that we could readily use. We have to duplicate and modify that otherwise to work with portable locale_t, and since sizeof locale_t can be that of int or pointer, doing so portably would be a pain. If you can typedef struct _locale *locale_t, that would really make our life easier.

[behdad]

All of this to say, there's no easy way to atomically assign a locale_t just from the POSIX specification.

[behdad]

I'm also considering to change the OpenBSD libc implementation to "typedef struct _locale *locale_t" with a dummy struct,

You don't even need to ever define such struct... So, I don't see the downside of that, over void *.

[ischwarze]

The downside of declaring "struct _locale" without actually using it is that it makes the code harder to understand. We like to keep our code simple, and to avoid needless abstractions. Should application software constrain C libraries to particular implementation choices? I'm not really convinced just yet.

there's no easy way to atomically assign a locale_t just from the POSIX specification.

True, i don't think you can atomically assign a locale_t at all.

sizeof locale_t can be that of int or pointer,

According to POSIX, sizeof(locale_t) might even be 42 or any other positive integer number.

Unless other operating systems contact you where locale_t is actually a struct or an array rather than a pointer, and if you hesitate to do a full cleanup right now, a compromise would be to just make your code work with "void *" and postpone dealing with non-pointer implementations until they are reported to you, hoping that day might never come.

Unless i'm missing something, the "operator *" method in the parent class is the only problem that prevents "void *" from working. Do you really need that method, or was it defined just for interface completeness, "just in case"? It clearly isn't used for hb_C_locale_lazy_loader_t. Even if somethings wants to have it, could it be moved to just those child classes of hb_lazy_loader_t that really need it?

[behdad]

Unless i'm missing something, the "operator *" method in the parent class is the only problem that prevents "void *" from working.

Okay. Let me try to make that conditional.

[behdad]

@ischwarze see if #3428 helps. Thanks.

[ischwarze]

I am happy to confirm that with your patch to src/hb-machinery.hh, harfbuzz-3.3.1 compiles on OpenBSD-amd64-current with clang 13. Antoine is planning to do more extensive testing later. We will let you know of any further issues, but right now, i see no particular reason to expect any.

Your friendly and efficient handling of this issue is highly appreciated. Thank you very much!

(edit: only a typo fix, no content change)

[behdad]

Thank you @ischwarze. We'll get this into a release soon. cc @khaledhosny

[khaledhosny]

We'll get this into a release soon. cc @khaledhosny

https://github.com/harfbuzz/harfbuzz/releases/tag/3.4.0