| Message ID | 20230725135557.20518-1-jgross@suse.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | xen/public: fix flexible array definitions | expand |
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>
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
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
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
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
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
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
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
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 --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]; }; /*
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(-)