diff mbox series

xen/public: fix flexible array definitions

Message ID 20230725135557.20518-1-jgross@suse.com (mailing list archive)
State New, archived
Headers show
Series xen/public: fix flexible array definitions | expand

Commit Message

Jürgen Groß July 25, 2023, 1:55 p.m. UTC
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@suse.com>
---
 xen/include/public/io/cameraif.h | 2 +-
 xen/include/public/io/displif.h  | 2 +-
 xen/include/public/io/fsif.h     | 4 ++--
 xen/include/public/io/pvcalls.h  | 2 +-
 xen/include/public/io/ring.h     | 4 ++--
 xen/include/public/io/sndif.h    | 2 +-
 6 files changed, 8 insertions(+), 8 deletions(-)

Comments

Andrew Cooper July 25, 2023, 4:06 p.m. UTC | #1
On 25/07/2023 2:55 pm, 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@suse.com>

Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>
Jan Beulich July 25, 2023, 4:16 p.m. UTC | #2
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@suse.com>

I think we need to be careful here: What if someone somewhere applies
sizeof() to any of the types you alter? 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.

Jan
Andrew Cooper July 25, 2023, 4:59 p.m. UTC | #3
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@suse.com>
> 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.

>  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.

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.

~Andrew
Jan Beulich July 26, 2023, 5:52 a.m. UTC | #4
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@suse.com>
>> 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.

Jan
Jürgen Groß Aug. 9, 2023, 9:42 a.m. UTC | #5
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@suse.com>
>>> 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.


Juergen
Jürgen Groß Nov. 29, 2023, 11:58 a.m. UTC | #6
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@suse.com>
>>>> 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.


Juergen
Andrew Cooper Nov. 29, 2023, 1:20 p.m. UTC | #7
On 25/07/2023 2:55 pm, 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@suse.com>

I know this is a change in the public headers, and I know it will cause
changes in the behaviour of sizeof() against these, but

1) We expect people to copy these files, so the change here isn't
breaking others, and
2) The use of sizeof() on these structs is buggy in the first place, and
3) The use of sizeof() with these structs is unlikely because they're
variadic
4) It really genuinely is UB as reported by toolchains

It may not be great, but it's the least bad of a lot of bad options.

This definitely needs a note in CHANGELOG.  Subject to something
suitable there, Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>

~Andrew
Jan Beulich Nov. 30, 2023, 8:24 a.m. UTC | #8
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@suse.com>
>>>>> 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__.

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.

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.)

Jan
Jürgen Groß Nov. 30, 2023, 9:21 a.m. UTC | #9
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@suse.com>
>>>>>> 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.
diff mbox series

Patch

diff --git a/xen/include/public/io/cameraif.h b/xen/include/public/io/cameraif.h
index 13763abef9..d6c69d6e1c 100644
--- a/xen/include/public/io/cameraif.h
+++ b/xen/include/public/io/cameraif.h
@@ -763,7 +763,7 @@  struct xencamera_buf_create_req {
  */
 struct xencamera_page_directory {
     grant_ref_t gref_dir_next_page;
-    grant_ref_t gref[1]; /* Variable length */
+    grant_ref_t gref[XEN_FLEX_ARRAY_DIM];
 };
 
 /*
diff --git a/xen/include/public/io/displif.h b/xen/include/public/io/displif.h
index 73d0cbdf15..4b9a27e960 100644
--- a/xen/include/public/io/displif.h
+++ b/xen/include/public/io/displif.h
@@ -537,7 +537,7 @@  struct xendispl_dbuf_create_req {
 
 struct xendispl_page_directory {
     grant_ref_t gref_dir_next_page;
-    grant_ref_t gref[1]; /* Variable length */
+    grant_ref_t gref[XEN_FLEX_ARRAY_DIM];
 };
 
 /*
diff --git a/xen/include/public/io/fsif.h b/xen/include/public/io/fsif.h
index ec57850233..0e1fba994a 100644
--- a/xen/include/public/io/fsif.h
+++ b/xen/include/public/io/fsif.h
@@ -40,7 +40,7 @@  struct fsif_read_request {
     int32_t pad;
     uint64_t len;
     uint64_t offset;
-    grant_ref_t grefs[1];  /* Variable length */
+    grant_ref_t grefs[XEN_FLEX_ARRAY_DIM];
 };
 
 struct fsif_write_request {
@@ -48,7 +48,7 @@  struct fsif_write_request {
     int32_t pad;
     uint64_t len;
     uint64_t offset;
-    grant_ref_t grefs[1];  /* Variable length */
+    grant_ref_t grefs[XEN_FLEX_ARRAY_DIM];
 };
 
 struct fsif_stat_request {
diff --git a/xen/include/public/io/pvcalls.h b/xen/include/public/io/pvcalls.h
index 230b0719e3..c8c7602470 100644
--- a/xen/include/public/io/pvcalls.h
+++ b/xen/include/public/io/pvcalls.h
@@ -30,7 +30,7 @@  struct pvcalls_data_intf {
     uint8_t pad2[52];
 
     RING_IDX ring_order;
-    grant_ref_t ref[];
+    grant_ref_t ref[XEN_FLEX_ARRAY_DIM];
 };
 DEFINE_XEN_FLEX_RING(pvcalls);
 
diff --git a/xen/include/public/io/ring.h b/xen/include/public/io/ring.h
index 0cae4367be..fa43396318 100644
--- a/xen/include/public/io/ring.h
+++ b/xen/include/public/io/ring.h
@@ -110,7 +110,7 @@  struct __name##_sring {                                                 \
         uint8_t pvt_pad[4];                                             \
     } pvt;                                                              \
     uint8_t __pad[44];                                                  \
-    union __name##_sring_entry ring[1]; /* variable-length */           \
+    union __name##_sring_entry ring[XEN_FLEX_ARRAY_DIM];                \
 };                                                                      \
                                                                         \
 /* "Front" end's private variables */                                   \
@@ -479,7 +479,7 @@  struct name##_data_intf {                                                     \
     uint8_t pad2[56];                                                         \
                                                                               \
     RING_IDX ring_order;                                                      \
-    grant_ref_t ref[];                                                        \
+    grant_ref_t ref[XEN_FLEX_ARRAY_DIM];                                      \
 };                                                                            \
 DEFINE_XEN_FLEX_RING(name)
 
diff --git a/xen/include/public/io/sndif.h b/xen/include/public/io/sndif.h
index 4234a47c87..32f1fde4d6 100644
--- a/xen/include/public/io/sndif.h
+++ b/xen/include/public/io/sndif.h
@@ -659,7 +659,7 @@  struct xensnd_open_req {
 
 struct xensnd_page_directory {
     grant_ref_t gref_dir_next_page;
-    grant_ref_t gref[1]; /* Variable length */
+    grant_ref_t gref[XEN_FLEX_ARRAY_DIM];
 };
 
 /*