gcc-10 warns about writes to the empty freeSpaceTable[] array, with
many instances like:
fs/udf/balloc.c: In function 'udf_bitmap_new_block':
fs/udf/balloc.c:101:36: error: array subscript 65535 is outside the bounds of an interior zero-length array '__le32[0]' {aka 'unsigned int[0]'} [-Werror=zero-length-bounds]
101 | le32_add_cpu(&lvid->freeSpaceTable[partition], cnt);
| ~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~
In file included from fs/udf/udfdecl.h:7,
from fs/udf/balloc.c:22:
fs/udf/ecma_167.h:363:11: note: while referencing 'freeSpaceTable'
363 | __le32 freeSpaceTable[0];
| ^~~~~~~~~~~~~~
These can all be avoided by using a flexible array member instead.
Another warning is a bit more obscure:
fs/udf/super.c: In function 'udf_count_free':
fs/udf/super.c:2521:26: warning: array subscript '(<unknown>) + 4294967295' is outside the bounds of an interior zero-length array '__le32[0]' {aka 'unsigned int[0]'} [-Wzero-length-bounds]
2521 | lvid->freeSpaceTable[part]);
Work around this one by changing the array access to equivalent
pointer arithmetic, as there cannot be multiple flexible-array
members in a single struct.
Signed-off-by: Arnd Bergmann <[email protected]>
---
fs/udf/ecma_167.h | 2 +-
fs/udf/super.c | 2 +-
2 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/fs/udf/ecma_167.h b/fs/udf/ecma_167.h
index 736ebc5dc441..14ffe27342bc 100644
--- a/fs/udf/ecma_167.h
+++ b/fs/udf/ecma_167.h
@@ -360,9 +360,9 @@ struct logicalVolIntegrityDesc {
uint8_t logicalVolContentsUse[32];
__le32 numOfPartitions;
__le32 lengthOfImpUse;
- __le32 freeSpaceTable[0];
__le32 sizeTable[0];
uint8_t impUse[0];
+ __le32 freeSpaceTable[];
} __packed;
/* Integrity Type (ECMA 167r3 3/10.10.3) */
diff --git a/fs/udf/super.c b/fs/udf/super.c
index f747bf72edbe..379867888c36 100644
--- a/fs/udf/super.c
+++ b/fs/udf/super.c
@@ -117,7 +117,7 @@ struct logicalVolIntegrityDescImpUse *udf_sb_lvidiu(struct super_block *sb)
}
/* The offset is to skip freeSpaceTable and sizeTable arrays */
offset = partnum * 2 * sizeof(uint32_t);
- return (struct logicalVolIntegrityDescImpUse *)&(lvid->impUse[offset]);
+ return (struct logicalVolIntegrityDescImpUse *)(lvid->impUse + offset);
}
/* UDF filesystem type */
--
2.26.0
On Thursday 30 April 2020 23:30:51 Arnd Bergmann wrote:
> gcc-10 warns about writes to the empty freeSpaceTable[] array, with
> many instances like:
>
> fs/udf/balloc.c: In function 'udf_bitmap_new_block':
> fs/udf/balloc.c:101:36: error: array subscript 65535 is outside the bounds of an interior zero-length array '__le32[0]' {aka 'unsigned int[0]'} [-Werror=zero-length-bounds]
> 101 | le32_add_cpu(&lvid->freeSpaceTable[partition], cnt);
> | ~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~
> In file included from fs/udf/udfdecl.h:7,
> from fs/udf/balloc.c:22:
> fs/udf/ecma_167.h:363:11: note: while referencing 'freeSpaceTable'
> 363 | __le32 freeSpaceTable[0];
> | ^~~~~~~~~~~~~~
Hi Arnd! This looks like a false-positive warning.
> These can all be avoided by using a flexible array member instead.
>
> Another warning is a bit more obscure:
>
> fs/udf/super.c: In function 'udf_count_free':
> fs/udf/super.c:2521:26: warning: array subscript '(<unknown>) + 4294967295' is outside the bounds of an interior zero-length array '__le32[0]' {aka 'unsigned int[0]'} [-Wzero-length-bounds]
> 2521 | lvid->freeSpaceTable[part]);
>
> Work around this one by changing the array access to equivalent
> pointer arithmetic, as there cannot be multiple flexible-array
> members in a single struct.
Well, this code uses GNU extension for zero-length arrays because it was
written in pre-C99 era when C99 flexible arrays did not exist yet.
> Signed-off-by: Arnd Bergmann <[email protected]>
> ---
> fs/udf/ecma_167.h | 2 +-
> fs/udf/super.c | 2 +-
> 2 files changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/fs/udf/ecma_167.h b/fs/udf/ecma_167.h
> index 736ebc5dc441..14ffe27342bc 100644
> --- a/fs/udf/ecma_167.h
> +++ b/fs/udf/ecma_167.h
> @@ -360,9 +360,9 @@ struct logicalVolIntegrityDesc {
> uint8_t logicalVolContentsUse[32];
> __le32 numOfPartitions;
> __le32 lengthOfImpUse;
> - __le32 freeSpaceTable[0];
> __le32 sizeTable[0];
> uint8_t impUse[0];
> + __le32 freeSpaceTable[];
Please do not change order of members in these structures. Order is
strictly defined by ECMA 167 standard and changing them you would just
confuse reader. In LVID is free space table before size table.
If you do not like GNU C extension for zero-length arrays then just
replace it by standard C99 flexible arrays. I think that there is no
reason to not use standard C99 language constructions, just nobody had
motivation or time to change (working) code.
Also this file is semi-synchronized with udftools project in which I
already replaced all GNU C zero-length arrays by C99 flexible arrays.
You can take inspiration what I did with logicalVolIntegrityDesc:
https://github.com/pali/udftools/commit/f851d84478ce881d516a76018745fa163f803880#diff-1e1a5b89f620d380f22b973f9449aeaeL381-R384
Anyway, if you have a better idea what to do with such on-disk structure
and how to represent it in C struct syntax, let me know as it could be
updated also in udftools project.
> } __packed;
>
> /* Integrity Type (ECMA 167r3 3/10.10.3) */
> diff --git a/fs/udf/super.c b/fs/udf/super.c
> index f747bf72edbe..379867888c36 100644
> --- a/fs/udf/super.c
> +++ b/fs/udf/super.c
> @@ -117,7 +117,7 @@ struct logicalVolIntegrityDescImpUse *udf_sb_lvidiu(struct super_block *sb)
> }
> /* The offset is to skip freeSpaceTable and sizeTable arrays */
> offset = partnum * 2 * sizeof(uint32_t);
> - return (struct logicalVolIntegrityDescImpUse *)&(lvid->impUse[offset]);
> + return (struct logicalVolIntegrityDescImpUse *)(lvid->impUse + offset);
> }
>
> /* UDF filesystem type */
> --
> 2.26.0
>
On Thu, Apr 30, 2020 at 11:54 PM Pali Rohár <[email protected]> wrote:
>
> On Thursday 30 April 2020 23:30:51 Arnd Bergmann wrote:
> > gcc-10 warns about writes to the empty freeSpaceTable[] array, with
> > many instances like:
> >
> > fs/udf/balloc.c: In function 'udf_bitmap_new_block':
> > fs/udf/balloc.c:101:36: error: array subscript 65535 is outside the bounds of an interior zero-length array '__le32[0]' {aka 'unsigned int[0]'} [-Werror=zero-length-bounds]
> > 101 | le32_add_cpu(&lvid->freeSpaceTable[partition], cnt);
> > | ~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~
> > In file included from fs/udf/udfdecl.h:7,
> > from fs/udf/balloc.c:22:
> > fs/udf/ecma_167.h:363:11: note: while referencing 'freeSpaceTable'
> > 363 | __le32 freeSpaceTable[0];
> > | ^~~~~~~~~~~~~~
>
> Hi Arnd! This looks like a false-positive warning.
Right, sorry for not making that clearer in the changelog.
> > These can all be avoided by using a flexible array member instead.
> >
> > Another warning is a bit more obscure:
> >
> > fs/udf/super.c: In function 'udf_count_free':
> > fs/udf/super.c:2521:26: warning: array subscript '(<unknown>) + 4294967295' is outside the bounds of an interior zero-length array '__le32[0]' {aka 'unsigned int[0]'} [-Wzero-length-bounds]
> > 2521 | lvid->freeSpaceTable[part]);
> >
> > Work around this one by changing the array access to equivalent
> > pointer arithmetic, as there cannot be multiple flexible-array
> > members in a single struct.
>
> > @@ -360,9 +360,9 @@ struct logicalVolIntegrityDesc {
> > uint8_t logicalVolContentsUse[32];
> > __le32 numOfPartitions;
> > __le32 lengthOfImpUse;
> > - __le32 freeSpaceTable[0];
> > __le32 sizeTable[0];
> > uint8_t impUse[0];
> > + __le32 freeSpaceTable[];
>
> Please do not change order of members in these structures. Order is
> strictly defined by ECMA 167 standard and changing them you would just
> confuse reader. In LVID is free space table before size table.
Ok
> If you do not like GNU C extension for zero-length arrays then just
> replace it by standard C99 flexible arrays. I think that there is no
> reason to not use standard C99 language constructions, just nobody had
> motivation or time to change (working) code.
No, the problem is that only the last member can be a flexible array,
so when impUse[] is the last member, freeSpaceTable has to be a zero
length array.
[]> Also this file is semi-synchronized with udftools project in which I
> already replaced all GNU C zero-length arrays by C99 flexible arrays.
>
> You can take inspiration what I did with logicalVolIntegrityDesc:
> https://github.com/pali/udftools/commit/f851d84478ce881d516a76018745fa163f803880#diff-1e1a5b89f620d380f22b973f9449aeaeL381-R384
Right, this is likely the best workaround.
> Anyway, if you have a better idea what to do with such on-disk structure
> and how to represent it in C struct syntax, let me know as it could be
> updated also in udftools project.
The trick I used for impUse[] would also work for freeSpaceTable[] to avoid
the gcc warning, it's still not great, but maybe you like this better:
arnd@threadripper:~/arm-soc$ git diff
diff --git a/fs/udf/balloc.c b/fs/udf/balloc.c
index 02f03fadb75b..666d022eb00b 100644
--- a/fs/udf/balloc.c
+++ b/fs/udf/balloc.c
@@ -98,7 +98,7 @@ static void udf_add_free_space(struct super_block
*sb, u16 partition, u32 cnt)
return;
lvid = (struct logicalVolIntegrityDesc *)sbi->s_lvid_bh->b_data;
- le32_add_cpu(&lvid->freeSpaceTable[partition], cnt);
+ le32_add_cpu(lvid->freeSpaceTable + partition, cnt);
udf_updated_lvid(sb);
}
diff --git a/fs/udf/ecma_167.h b/fs/udf/ecma_167.h
index 14ffe27342bc..215d97d7edc4 100644
--- a/fs/udf/ecma_167.h
+++ b/fs/udf/ecma_167.h
@@ -360,9 +360,9 @@ struct logicalVolIntegrityDesc {
uint8_t logicalVolContentsUse[32];
__le32 numOfPartitions;
__le32 lengthOfImpUse;
__le32 freeSpaceTable[0];
__le32 sizeTable[0];
- uint8_t impUse[0];
+ uint8_t impUse[];
} __packed;
/* Integrity Type (ECMA 167r3 3/10.10.3) */
diff --git a/fs/udf/super.c b/fs/udf/super.c
index 379867888c36..a1fc51c2261e 100644
--- a/fs/udf/super.c
+++ b/fs/udf/super.c
@@ -2517,8 +2517,8 @@ static unsigned int udf_count_free(struct super_block *sb)
(struct logicalVolIntegrityDesc *)
sbi->s_lvid_bh->b_data;
if (le32_to_cpu(lvid->numOfPartitions) > part) {
- accum = le32_to_cpu(
- lvid->freeSpaceTable[part]);
+ accum = le32_to_cpup(
+ (lvid->freeSpaceTable + part));
if (accum == 0xFFFFFFFF)
accum = 0;
}
This version could easily be backported to stable kernels to let them be
compiled with gcc-10, and then synchronizing with the udftools version of
the header needs additional changes on top, which do not need to be
backported.
Arnd
On Fri 01-05-20 22:30:27, Arnd Bergmann wrote:
> On Thu, Apr 30, 2020 at 11:54 PM Pali Roh?r <[email protected]> wrote:
> > > @@ -360,9 +360,9 @@ struct logicalVolIntegrityDesc {
> > > uint8_t logicalVolContentsUse[32];
> > > __le32 numOfPartitions;
> > > __le32 lengthOfImpUse;
> > > - __le32 freeSpaceTable[0];
> > > __le32 sizeTable[0];
> > > uint8_t impUse[0];
> > > + __le32 freeSpaceTable[];
> >
> > Please do not change order of members in these structures. Order is
> > strictly defined by ECMA 167 standard and changing them you would just
> > confuse reader. In LVID is free space table before size table.
>
> Ok
>
> > If you do not like GNU C extension for zero-length arrays then just
> > replace it by standard C99 flexible arrays. I think that there is no
> > reason to not use standard C99 language constructions, just nobody had
> > motivation or time to change (working) code.
>
> No, the problem is that only the last member can be a flexible array,
> so when impUse[] is the last member, freeSpaceTable has to be a zero
> length array.
>
> []> Also this file is semi-synchronized with udftools project in which I
> > already replaced all GNU C zero-length arrays by C99 flexible arrays.
> >
> > You can take inspiration what I did with logicalVolIntegrityDesc:
> > https://github.com/pali/udftools/commit/f851d84478ce881d516a76018745fa163f803880#diff-1e1a5b89f620d380f22b973f9449aeaeL381-R384
>
> Right, this is likely the best workaround.
>
> > Anyway, if you have a better idea what to do with such on-disk structure
> > and how to represent it in C struct syntax, let me know as it could be
> > updated also in udftools project.
>
> The trick I used for impUse[] would also work for freeSpaceTable[] to avoid
> the gcc warning, it's still not great, but maybe you like this better:
I like Pali's version somewhat better because whenever I look at several
(obviously flexible) arrays in one struct, I start wondering what's going
on. So let's not define members of struct whose offset we actually don't
know (and thus we cannot sanely use them anyway).
Honza
> arnd@threadripper:~/arm-soc$ git diff
> diff --git a/fs/udf/balloc.c b/fs/udf/balloc.c
> index 02f03fadb75b..666d022eb00b 100644
> --- a/fs/udf/balloc.c
> +++ b/fs/udf/balloc.c
> @@ -98,7 +98,7 @@ static void udf_add_free_space(struct super_block
> *sb, u16 partition, u32 cnt)
> return;
>
> lvid = (struct logicalVolIntegrityDesc *)sbi->s_lvid_bh->b_data;
> - le32_add_cpu(&lvid->freeSpaceTable[partition], cnt);
> + le32_add_cpu(lvid->freeSpaceTable + partition, cnt);
> udf_updated_lvid(sb);
> }
>
> diff --git a/fs/udf/ecma_167.h b/fs/udf/ecma_167.h
> index 14ffe27342bc..215d97d7edc4 100644
> --- a/fs/udf/ecma_167.h
> +++ b/fs/udf/ecma_167.h
> @@ -360,9 +360,9 @@ struct logicalVolIntegrityDesc {
> uint8_t logicalVolContentsUse[32];
> __le32 numOfPartitions;
> __le32 lengthOfImpUse;
> __le32 freeSpaceTable[0];
> __le32 sizeTable[0];
> - uint8_t impUse[0];
> + uint8_t impUse[];
> } __packed;
>
> /* Integrity Type (ECMA 167r3 3/10.10.3) */
> diff --git a/fs/udf/super.c b/fs/udf/super.c
> index 379867888c36..a1fc51c2261e 100644
> --- a/fs/udf/super.c
> +++ b/fs/udf/super.c
> @@ -2517,8 +2517,8 @@ static unsigned int udf_count_free(struct super_block *sb)
> (struct logicalVolIntegrityDesc *)
> sbi->s_lvid_bh->b_data;
> if (le32_to_cpu(lvid->numOfPartitions) > part) {
> - accum = le32_to_cpu(
> - lvid->freeSpaceTable[part]);
> + accum = le32_to_cpup(
> + (lvid->freeSpaceTable + part));
> if (accum == 0xFFFFFFFF)
> accum = 0;
> }
>
>
>
> This version could easily be backported to stable kernels to let them be
> compiled with gcc-10, and then synchronizing with the udftools version of
> the header needs additional changes on top, which do not need to be
> backported.
>
> Arnd
--
Jan Kara <[email protected]>
SUSE Labs, CR
On Friday 01 May 2020 22:30:27 Arnd Bergmann wrote:
> On Thu, Apr 30, 2020 at 11:54 PM Pali Rohár <[email protected]> wrote:
> >
> > On Thursday 30 April 2020 23:30:51 Arnd Bergmann wrote:
> > > gcc-10 warns about writes to the empty freeSpaceTable[] array, with
> > > many instances like:
> > >
> > > fs/udf/balloc.c: In function 'udf_bitmap_new_block':
> > > fs/udf/balloc.c:101:36: error: array subscript 65535 is outside the bounds of an interior zero-length array '__le32[0]' {aka 'unsigned int[0]'} [-Werror=zero-length-bounds]
> > > 101 | le32_add_cpu(&lvid->freeSpaceTable[partition], cnt);
> > > | ~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~
> > > In file included from fs/udf/udfdecl.h:7,
> > > from fs/udf/balloc.c:22:
> > > fs/udf/ecma_167.h:363:11: note: while referencing 'freeSpaceTable'
> > > 363 | __le32 freeSpaceTable[0];
> > > | ^~~~~~~~~~~~~~
> >
> > Hi Arnd! This looks like a false-positive warning.
>
> Right, sorry for not making that clearer in the changelog.
>
> > > These can all be avoided by using a flexible array member instead.
> > >
> > > Another warning is a bit more obscure:
> > >
> > > fs/udf/super.c: In function 'udf_count_free':
> > > fs/udf/super.c:2521:26: warning: array subscript '(<unknown>) + 4294967295' is outside the bounds of an interior zero-length array '__le32[0]' {aka 'unsigned int[0]'} [-Wzero-length-bounds]
> > > 2521 | lvid->freeSpaceTable[part]);
> > >
> > > Work around this one by changing the array access to equivalent
> > > pointer arithmetic, as there cannot be multiple flexible-array
> > > members in a single struct.
> >
>
> > > @@ -360,9 +360,9 @@ struct logicalVolIntegrityDesc {
> > > uint8_t logicalVolContentsUse[32];
> > > __le32 numOfPartitions;
> > > __le32 lengthOfImpUse;
> > > - __le32 freeSpaceTable[0];
> > > __le32 sizeTable[0];
> > > uint8_t impUse[0];
> > > + __le32 freeSpaceTable[];
> >
> > Please do not change order of members in these structures. Order is
> > strictly defined by ECMA 167 standard and changing them you would just
> > confuse reader. In LVID is free space table before size table.
>
> Ok
>
> > If you do not like GNU C extension for zero-length arrays then just
> > replace it by standard C99 flexible arrays. I think that there is no
> > reason to not use standard C99 language constructions, just nobody had
> > motivation or time to change (working) code.
>
> No, the problem is that only the last member can be a flexible array,
I know, that is why I replaced those 3 zero-length arrays by just one
flexible array in udftools project.
> so when impUse[] is the last member, freeSpaceTable has to be a zero
> length array.
>
> > Also this file is semi-synchronized with udftools project in which I
> > already replaced all GNU C zero-length arrays by C99 flexible arrays.
> >
> > You can take inspiration what I did with logicalVolIntegrityDesc:
> > https://github.com/pali/udftools/commit/f851d84478ce881d516a76018745fa163f803880#diff-1e1a5b89f620d380f22b973f9449aeaeL381-R384
>
> Right, this is likely the best workaround.
>
> > Anyway, if you have a better idea what to do with such on-disk structure
> > and how to represent it in C struct syntax, let me know as it could be
> > updated also in udftools project.
>
> The trick I used for impUse[] would also work for freeSpaceTable[] to avoid
> the gcc warning, it's still not great, but maybe you like this better:
>
> arnd@threadripper:~/arm-soc$ git diff
> diff --git a/fs/udf/balloc.c b/fs/udf/balloc.c
> index 02f03fadb75b..666d022eb00b 100644
> --- a/fs/udf/balloc.c
> +++ b/fs/udf/balloc.c
> @@ -98,7 +98,7 @@ static void udf_add_free_space(struct super_block
> *sb, u16 partition, u32 cnt)
> return;
>
> lvid = (struct logicalVolIntegrityDesc *)sbi->s_lvid_bh->b_data;
> - le32_add_cpu(&lvid->freeSpaceTable[partition], cnt);
> + le32_add_cpu(lvid->freeSpaceTable + partition, cnt);
> udf_updated_lvid(sb);
> }
>
> diff --git a/fs/udf/ecma_167.h b/fs/udf/ecma_167.h
> index 14ffe27342bc..215d97d7edc4 100644
> --- a/fs/udf/ecma_167.h
> +++ b/fs/udf/ecma_167.h
> @@ -360,9 +360,9 @@ struct logicalVolIntegrityDesc {
> uint8_t logicalVolContentsUse[32];
> __le32 numOfPartitions;
> __le32 lengthOfImpUse;
> __le32 freeSpaceTable[0];
> __le32 sizeTable[0];
> - uint8_t impUse[0];
> + uint8_t impUse[];
> } __packed;
>
> /* Integrity Type (ECMA 167r3 3/10.10.3) */
> diff --git a/fs/udf/super.c b/fs/udf/super.c
> index 379867888c36..a1fc51c2261e 100644
> --- a/fs/udf/super.c
> +++ b/fs/udf/super.c
> @@ -2517,8 +2517,8 @@ static unsigned int udf_count_free(struct super_block *sb)
> (struct logicalVolIntegrityDesc *)
> sbi->s_lvid_bh->b_data;
> if (le32_to_cpu(lvid->numOfPartitions) > part) {
> - accum = le32_to_cpu(
> - lvid->freeSpaceTable[part]);
> + accum = le32_to_cpup(
> + (lvid->freeSpaceTable + part));
> if (accum == 0xFFFFFFFF)
> accum = 0;
> }
>
This is much better as it does not change order of members in LVID
structure. I'm fine with it.
> This version could easily be backported to stable kernels to let them be
> compiled with gcc-10
I do not know what triggers that false-positive warning. But if you
think that this change is enough to "hide" that warning, you can add my
Acked-by: Pali Rohár <[email protected]>
For sure it is better to have just small changes needed for backporting.
> and then synchronizing with the udftools version of
> the header needs additional changes on top, which do not need to be
> backported.
Both header files (ECMA and OSTA) should be in-sync with udftools since
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=614644676394951e73194ea96b3f026c1adf5443
Differences in kernel code are: usage of zero-length array members,
usage of integer types and usage of structure attributes.
If you are planning in future to do some changes in those ECMA or OSTA
header files, please send updates also for udftools. So we will have
header files synchronized as much as possible.