Re: [PATCH] xen/public: fix flexible array definitions

[Juergen Gross <jgross@xxxxxxxx>]

On 30.11.23 09:24, Jan Beulich wrote:
> On 29.11.2023 12:58, Juergen Gross wrote:
> > On 09.08.23 11:42, Juergen Gross wrote:
> > > On 26.07.23 07:52, Jan Beulich wrote:
> > > > On 25.07.2023 18:59, Andrew Cooper wrote:
> > > > > On 25/07/2023 5:16 pm, Jan Beulich wrote:
> > > > > > On 25.07.2023 15:55, Juergen Gross wrote:
> > > > > > > Flexible arrays in public headers can be problematic with some
> > > > > > > compilers.
> > > > > > >
> > > > > > > Replace them with arr[XEN_FLEX_ARRAY_DIM] in order to avoid compilation
> > > > > > > errors.
> > > > > > >
> > > > > > > This includes arrays defined as "arr[1]", as seen with a recent Linux
> > > > > > > kernel [1].
> > > > > > >
> > > > > > > [1]: https://bugzilla.kernel.org/show_bug.cgi?id=217693
> > > > > > >
> > > > > > > Signed-off-by: Juergen Gross <jgross@xxxxxxxx>
> > > > > > I think we need to be careful here: What if someone somewhere applies
> > > > > > sizeof() to any of the types you alter?
> > > > > Then the code was most likely wrong already.
> > > > That's possible to judge only when seeing the code in question.
> > > >
> > > > > >    The resulting value would
> > > > > > change with the changes you propose, which we cannot allow to happen
> > > > > > in a stable interface. Therefore imo it can only be an opt-in feature
> > > > > > to have these arrays no longer be one-element ones.
> > > > > I don't consider this an issue.
> > > > >
> > > > > If people take an update to the headers and their code stops compiling,
> > > > > then of course they fix the compilation issue.  That's normal.
> > > > The code may continue to compile fine, and even appear to work initially.
> > > >
> > > > > It's unreasonable to take opt-in features to a set of headers intended
> > > > > to be vendored in the first place, to work around a corner case that's
> > > > > likely buggy already.
> > > > The original intention clearly was to allow use of these headers as is.
> > > > Anyway, I've voiced my view, yet if there are enough people agreeing
> > > > with you, then so be it.
> > > Any further thoughts?
> > >
> > > I have checked the code in the Linux kernel meanwhile. There should be no
> > > fallout resulting from this change, but I think there are some user mode
> > > backends outside of qemu which are probably using affected structs.
> > I've received another mail regarding the report [1] above. I think we should
> > _really_ come to a conclusion.
> >
> > I'm still in favor of applying my suggested patch.
> I think the change would be fine to make when adjusted to be conditional
> upon (suitably bumped) __XEN_LATEST_INTERFACE_VERSION__.
Okay, fine with me.

> Yet while looking at the patch and the headers again, it also looks as if
> there might be another small issue: ring.h uses XEN_FLEX_ARRAY_DIM without
> itself including xen.h. That's probably okay considering that all headers
> including ring.h also include grant_table.h (which in turn includes xen.h),
> but this dependency may still want making explicit.
Yes, I'll add that.

> Finally - is the change actually going to help everywhere (not just in
> Linux)? It effectively depends on people enabling C99 mode. Older gcc for
> example didn't even define __STDC_VERSION__ when -std wasn't used. Linux
> doesn't permit use of such old gcc versions anymore, but recall we're
> aiming to be C89 compatible. Therefore I think that in addition we'd need
> a way for consumers of the headers to indicate that the C99 form of
> XEN_FLEX_ARRAY_DIM can be used even when __STDC_VERSION__ isn't defined.
> (This may as well simply be done by allowing people to pre-define
> XEN_FLEX_ARRAY_DIM before including any Xen headers.)
Will the problem even occur with such an old gcc? I don't think so, as only
rather recent compilers showed the "array out of bounds" failure. Otherwise
we would have heard complaints much earlier.

Attachment: OpenPGP_0xB0DE9DD628BF132F.asc
Description: OpenPGP public key

Attachment: OpenPGP_signature.asc
Description: OpenPGP digital signature