[RFC,v4,08/24] sysdeps/stat: Copy the statx struct to stat instead of stat64
Commit Message
When copying the statx struct to the stat stuct use the original stat
struct instead of the stat64 struct. As the padding in the original is
type 'unsigned short int' but the padding in the stat64 is 'unsigned int'
the copy can result in misallgined data. This would then incorrectly
trigger the stat_overflow() failure.
This would be very obvious when using a 64-bit ino_t type on a 32-bit
system, such as the RV32 port.
Signed-off-by: Alistair Francis <alistair.francis@wdc.com>
---
.../sysv/linux/generic/wordsize-32/fxstat.c | 2 +-
.../sysv/linux/generic/wordsize-32/fxstatat.c | 2 +-
.../sysv/linux/generic/wordsize-32/lxstat.c | 2 +-
.../sysv/linux/generic/wordsize-32/xstat.c | 2 +-
sysdeps/unix/sysv/linux/statx_cp.c | 24 +++++++++++++++++++
sysdeps/unix/sysv/linux/statx_cp.h | 3 +++
6 files changed, 31 insertions(+), 4 deletions(-)
Comments
On Fri, 9 Aug 2019, Alistair Francis wrote:
> When copying the statx struct to the stat stuct use the original stat
> struct instead of the stat64 struct. As the padding in the original is
> type 'unsigned short int' but the padding in the stat64 is 'unsigned int'
> the copy can result in misallgined data. This would then incorrectly
> trigger the stat_overflow() failure.
This indicates there's something else wrong with the port. By design, the
following apply for linux/generic/wordsize-32 ports of glibc:
* The layout of struct stat and struct stat64 is identical, except that
some bytes that are padding in struct stat serve as high parts of fields
that are wider in struct stat64 (and thus have endian-dependent positions
as determined by the __field64 macro in bits/stat.h).
* Conversions from statx have to go to stat64, including setting those
high parts as appropriate, so that the subsequent overflow checks (which
work by examining those padding fields) can correctly detect whether
overflow occurred and set errno to EOVERFLOW accordingly. See my C-SKY
port reviews that resulted in the code we have now
<https://sourceware.org/ml/libc-alpha/2018-11/msg00624.html>
<https://sourceware.org/ml/libc-alpha/2018-11/msg00668.html>.
> This would be very obvious when using a 64-bit ino_t type on a 32-bit
> system, such as the RV32 port.
If those types are 64-bit, you should not have padding around them in
struct stat, so as to preserve the property that struct stat and struct
stat64 have the same layout. I suppose this means bits/stat.h needs to
check further macros such as __OFF_T_MATCHES_OFF64_T.
You'll also need to ensure that XSTAT_IS_XSTAT64 is defined to 1. And
you'll need to make wordsize-32/overflow.h define trivial versions of the
*_overflow functions in cases where the types match (this should be done
in that file, rather than making an RV32-specific copy, to benefit future
ports that make the same choices as RV32).
On Mon, Aug 12, 2019 at 1:01 PM Joseph Myers <joseph@codesourcery.com> wrote:
>
> On Fri, 9 Aug 2019, Alistair Francis wrote:
>
> > When copying the statx struct to the stat stuct use the original stat
> > struct instead of the stat64 struct. As the padding in the original is
> > type 'unsigned short int' but the padding in the stat64 is 'unsigned int'
> > the copy can result in misallgined data. This would then incorrectly
> > trigger the stat_overflow() failure.
>
> This indicates there's something else wrong with the port. By design, the
> following apply for linux/generic/wordsize-32 ports of glibc:
>
> * The layout of struct stat and struct stat64 is identical, except that
> some bytes that are padding in struct stat serve as high parts of fields
> that are wider in struct stat64 (and thus have endian-dependent positions
> as determined by the __field64 macro in bits/stat.h).
__ino_t is a 64-bit value in RV32, so in both stat and stat64 it's the
same thing. The __pad1 changes in size between the two structs as one
is a short and one is just an int. I don't see how they are identical.
>
> * Conversions from statx have to go to stat64, including setting those
> high parts as appropriate, so that the subsequent overflow checks (which
> work by examining those padding fields) can correctly detect whether
> overflow occurred and set errno to EOVERFLOW accordingly. See my C-SKY
> port reviews that resulted in the code we have now
> <https://sourceware.org/ml/libc-alpha/2018-11/msg00624.html>
> <https://sourceware.org/ml/libc-alpha/2018-11/msg00668.html>.
Ok, this makes sense.
>
> > This would be very obvious when using a 64-bit ino_t type on a 32-bit
> > system, such as the RV32 port.
>
> If those types are 64-bit, you should not have padding around them in
> struct stat, so as to preserve the property that struct stat and struct
> stat64 have the same layout. I suppose this means bits/stat.h needs to
> check further macros such as __OFF_T_MATCHES_OFF64_T.
Changing the padding would fix the problem. Just to be clear is that
what you are suggesting?
>
> You'll also need to ensure that XSTAT_IS_XSTAT64 is defined to 1. And
> you'll need to make wordsize-32/overflow.h define trivial versions of the
> *_overflow functions in cases where the types match (this should be done
> in that file, rather than making an RV32-specific copy, to benefit future
> ports that make the same choices as RV32).
I think I tested that and it didn't fix the problem so I dropped it.
I'll add the XSTAT_IS_XSTAT64 define back.
I'm not sure what you mean by then the types match?
Alistair
>
> --
> Joseph S. Myers
> joseph@codesourcery.com
On Mon, 12 Aug 2019, Alistair Francis wrote:
> > * The layout of struct stat and struct stat64 is identical, except that
> > some bytes that are padding in struct stat serve as high parts of fields
> > that are wider in struct stat64 (and thus have endian-dependent positions
> > as determined by the __field64 macro in bits/stat.h).
>
> __ino_t is a 64-bit value in RV32, so in both stat and stat64 it's the
> same thing. The __pad1 changes in size between the two structs as one
> is a short and one is just an int. I don't see how they are identical.
What file are you looking at, in what version of the source tree?
I'm looking at sysdeps/unix/sysv/linux/generic/bits/stat.h. As far as I
can tell, your patch series doesn't change that file. That's the version
used by RV64 and it should be used by RV32 (the headers installed for RV32
and RV64 should be identical apart from the special cases of
gnu/lib-names-*.h and gnu/stubs-*.h; if it isn't, that needs to be fixed).
In that file, __pad1 is of type __dev_t in both struct stat and struct
stat64.
> > If those types are 64-bit, you should not have padding around them in
> > struct stat, so as to preserve the property that struct stat and struct
> > stat64 have the same layout. I suppose this means bits/stat.h needs to
> > check further macros such as __OFF_T_MATCHES_OFF64_T.
>
> Changing the padding would fix the problem. Just to be clear is that
> what you are suggesting?
Yes.
Either change the definition of __field64, or the uses of it.
Changing the definition runs into the complication that it's used three
times and each has its own macro to say whether the two types in question
match, but you could always have a #error if some match and others don't.
> > You'll also need to ensure that XSTAT_IS_XSTAT64 is defined to 1. And
> > you'll need to make wordsize-32/overflow.h define trivial versions of the
> > *_overflow functions in cases where the types match (this should be done
> > in that file, rather than making an RV32-specific copy, to benefit future
> > ports that make the same choices as RV32).
>
> I think I tested that and it didn't fix the problem so I dropped it.
> I'll add the XSTAT_IS_XSTAT64 define back.
>
> I'm not sure what you mean by then the types match?
off_t and off64_t match if they have the same size and alignment (if
__OFF_T_MATCHES_OFF64_T is defined). Likewise for the other pairs.
If off_t and off64_t match, there is no need for the "buf->__st_size_pad
== 0" check in stat_overflow (and once you've removed the padding in that
case, that check won't even compile because __st_size_pad won't exist).
Likewise for the other pairs. In practice, you can probably just make
that function return 0 if all three macros are defined, rather than
allowing for arbitrary subsets of the types matching or not matching.
Similar considerations apply to statfs_overflow if the types and padding
used in struct statfs match those used in struct statfs64. So apply
similar considerations to the two versions of that structure.
You probably don't need to do anything special with lseek_overflow because
the compiler should just optimize that away when off_t and loff_t are the
same type.
On Mon, Aug 12, 2019 at 4:02 PM Joseph Myers <joseph@codesourcery.com> wrote:
>
> On Mon, 12 Aug 2019, Alistair Francis wrote:
>
> > > * The layout of struct stat and struct stat64 is identical, except that
> > > some bytes that are padding in struct stat serve as high parts of fields
> > > that are wider in struct stat64 (and thus have endian-dependent positions
> > > as determined by the __field64 macro in bits/stat.h).
> >
> > __ino_t is a 64-bit value in RV32, so in both stat and stat64 it's the
> > same thing. The __pad1 changes in size between the two structs as one
> > is a short and one is just an int. I don't see how they are identical.
>
> What file are you looking at, in what version of the source tree?
I'm looking at sysdeps/unix/sysv/linux/bits/stat.h, it has a different
definition then the one you are looking at.
>
> I'm looking at sysdeps/unix/sysv/linux/generic/bits/stat.h. As far as I
> can tell, your patch series doesn't change that file. That's the version
> used by RV64 and it should be used by RV32 (the headers installed for RV32
> and RV64 should be identical apart from the special cases of
> gnu/lib-names-*.h and gnu/stubs-*.h; if it isn't, that needs to be fixed).
It looks like __field64(__ino_t, __ino64_t, st_ino) will use type int
for RV32 (I don't think __USE_FILE_OFFSET64 is defined) which might
need some changes.
>
> In that file, __pad1 is of type __dev_t in both struct stat and struct
> stat64.
In the file you are looking at that is correct.
>
> > > If those types are 64-bit, you should not have padding around them in
> > > struct stat, so as to preserve the property that struct stat and struct
> > > stat64 have the same layout. I suppose this means bits/stat.h needs to
> > > check further macros such as __OFF_T_MATCHES_OFF64_T.
> >
> > Changing the padding would fix the problem. Just to be clear is that
> > what you are suggesting?
>
> Yes.
>
> Either change the definition of __field64, or the uses of it.
Yep, in the file you are looking at that seems the best way to go.
>
> Changing the definition runs into the complication that it's used three
> times and each has its own macro to say whether the two types in question
> match, but you could always have a #error if some match and others don't.
>
> > > You'll also need to ensure that XSTAT_IS_XSTAT64 is defined to 1. And
> > > you'll need to make wordsize-32/overflow.h define trivial versions of the
> > > *_overflow functions in cases where the types match (this should be done
> > > in that file, rather than making an RV32-specific copy, to benefit future
> > > ports that make the same choices as RV32).
> >
> > I think I tested that and it didn't fix the problem so I dropped it.
> > I'll add the XSTAT_IS_XSTAT64 define back.
> >
> > I'm not sure what you mean by then the types match?
>
> off_t and off64_t match if they have the same size and alignment (if
> __OFF_T_MATCHES_OFF64_T is defined). Likewise for the other pairs.
>
> If off_t and off64_t match, there is no need for the "buf->__st_size_pad
> == 0" check in stat_overflow (and once you've removed the padding in that
> case, that check won't even compile because __st_size_pad won't exist).
> Likewise for the other pairs. In practice, you can probably just make
> that function return 0 if all three macros are defined, rather than
> allowing for arbitrary subsets of the types matching or not matching.
Ok, I think I understand. I'll update the patch series.
>
> Similar considerations apply to statfs_overflow if the types and padding
> used in struct statfs match those used in struct statfs64. So apply
> similar considerations to the two versions of that structure.
Ok
Alistair
>
> You probably don't need to do anything special with lseek_overflow because
> the compiler should just optimize that away when off_t and loff_t are the
> same type.
>
> --
> Joseph S. Myers
> joseph@codesourcery.com
On Mon, 12 Aug 2019, Alistair Francis wrote:
> I'm looking at sysdeps/unix/sysv/linux/bits/stat.h, it has a different
> definition then the one you are looking at.
That file is not relevant for any Linux kernel port added in the past
several years. All recent ports should use the asm-generic structure
layouts.
On Mon, Aug 12, 2019 at 4:31 PM Joseph Myers <joseph@codesourcery.com> wrote:
>
> On Mon, 12 Aug 2019, Alistair Francis wrote:
>
> > I'm looking at sysdeps/unix/sysv/linux/bits/stat.h, it has a different
> > definition then the one you are looking at.
>
> That file is not relevant for any Linux kernel port added in the past
> several years. All recent ports should use the asm-generic structure
> layouts.
Ok, now I understand. I have made all of the changes you requested.
Alistair
>
> --
> Joseph S. Myers
> joseph@codesourcery.com
@@ -43,7 +43,7 @@ __fxstat (int vers, int fd, struct stat *buf)
int rc = INLINE_SYSCALL (statx, 5, fd, "", AT_EMPTY_PATH,
STATX_BASIC_STATS, &tmp);
if (rc == 0)
- __cp_stat64_statx ((struct stat64 *)buf, &tmp);
+ __cp_stat_statx (buf, &tmp);
# endif
return rc ?: stat_overflow (buf);
}
@@ -44,7 +44,7 @@ __fxstatat (int vers, int fd, const char *file, struct stat *buf, int flag)
AT_NO_AUTOMOUNT | flag,
STATX_BASIC_STATS, &tmp);
if (rc == 0)
- __cp_stat64_statx ((struct stat64 *)buf, &tmp);
+ __cp_stat_statx (buf, &tmp);
# endif
return rc ?: stat_overflow (buf);
}
@@ -44,7 +44,7 @@ __lxstat (int vers, const char *name, struct stat *buf)
AT_NO_AUTOMOUNT | AT_SYMLINK_NOFOLLOW,
STATX_BASIC_STATS, &tmp);
if (rc == 0)
- __cp_stat64_statx ((struct stat64 *)buf, &tmp);
+ __cp_stat_statx (buf, &tmp);
#endif
return rc ?: stat_overflow (buf);
}
@@ -42,7 +42,7 @@ __xstat (int vers, const char *name, struct stat *buf)
int rc = INLINE_SYSCALL (statx, 5, AT_FDCWD, name, AT_NO_AUTOMOUNT,
STATX_BASIC_STATS, &tmp);
if (rc == 0)
- __cp_stat64_statx ((struct stat64 *)buf, &tmp);
+ __cp_stat_statx (buf, &tmp);
# endif
return rc ?: stat_overflow (buf);
}
@@ -46,4 +46,28 @@ __cp_stat64_statx (struct stat64 *to, struct statx *from)
to->st_blocks = from->stx_blocks;
to->st_blksize = from->stx_blksize;
}
+
+void
+__cp_stat_statx (struct stat *to, struct statx *from)
+{
+ memset (to, 0, sizeof (struct stat64));
+ to->st_dev = ((from->stx_dev_minor & 0xff) | (from->stx_dev_major << 8)
+ | ((from->stx_dev_minor & ~0xff) << 12));
+ to->st_rdev = ((from->stx_rdev_minor & 0xff) | (from->stx_rdev_major << 8)
+ | ((from->stx_rdev_minor & ~0xff) << 12));
+ to->st_ino = from->stx_ino;
+ to->st_mode = from->stx_mode;
+ to->st_nlink = from->stx_nlink;
+ to->st_uid = from->stx_uid;
+ to->st_gid = from->stx_gid;
+ to->st_atime = from->stx_atime.tv_sec;
+ to->st_atim.tv_nsec = from->stx_atime.tv_nsec;
+ to->st_mtime = from->stx_mtime.tv_sec;
+ to->st_mtim.tv_nsec = from->stx_mtime.tv_nsec;
+ to->st_ctime = from->stx_ctime.tv_sec;
+ to->st_ctim.tv_nsec = from->stx_ctime.tv_nsec;
+ to->st_size = from->stx_size;
+ to->st_blocks = from->stx_blocks;
+ to->st_blksize = from->stx_blksize;
+}
#endif
@@ -18,3 +18,6 @@
extern void __cp_stat64_statx (struct stat64 *to, struct statx *from)
attribute_hidden;
+
+extern void __cp_stat_statx (struct stat *to, struct statx *from)
+ attribute_hidden;