| Message ID | 20201116211743.2228063-1-adhemerval.zanella@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | linux: mips: Fix getdents64 fallback on mips64-n32 | expand |
* Adhemerval Zanella via Libc-alpha: > GCC mainline shows the following error: > > ../sysdeps/unix/sysv/linux/mips/mips64/getdents64.c: In function '__getdents64': > ../sysdeps/unix/sysv/linux/mips/mips64/getdents64.c:121:7: error: 'memcpy' forming offset [4, 7] is out of the bounds [0, 4] [-Werror=array-bounds] > 121 | memcpy (((char *) dp + offsetof (struct dirent64, d_ino)), > | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ > 122 | KDP_MEMBER (kdp, d_ino), sizeof ((struct dirent64){0}.d_ino)); > | ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ > ../sysdeps/unix/sysv/linux/mips/mips64/getdents64.c:123:7: error: 'memcpy' forming offset [4, 7] is out of the bounds [0, 4] [-Werror=array-bounds] > 123 | memcpy (((char *) dp + offsetof (struct dirent64, d_off)), > | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ > 124 | KDP_MEMBER (kdp, d_off), sizeof ((struct dirent64){0}.d_off)); > | ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ > > The issue is due both d_ino and d_off fields for mips64-n32 > kernel_dirent are 32-bits, while this is using memcpy to copy 64 bits > from it into the glibc dirent64. > > The fix is to use a temporary variable to read the correct type > from kernel_dirent. I think I suggested to cut back on the macro magic and simply have appropriately defined structs, with a sequence like this: memcpy to first temporary struct field-by-field assignment from first temporary struct to second struct memcpy from second temporary struct Would that work? (Sorry if my message got through and this suggestion was already considered.)
On Nov 16 2020, Adhemerval Zanella via Libc-alpha wrote: > diff --git a/sysdeps/unix/sysv/linux/mips/mips64/getdents64.c b/sysdeps/unix/sysv/linux/mips/mips64/getdents64.c > index d18a5297dc..5b7597c99b 100644 > --- a/sysdeps/unix/sysv/linux/mips/mips64/getdents64.c > +++ b/sysdeps/unix/sysv/linux/mips/mips64/getdents64.c > @@ -91,15 +91,18 @@ __getdents64 (int fd, void *buf, size_t nbytes) > while ((char *) kdp < (char *) skdp + r) > { > /* This macro is used to avoid aliasing violation. */ > -#define KDP_MEMBER(src, member) \ > - (__typeof__((struct kernel_dirent){0}.member) *) \ > - memcpy (&((__typeof__((struct kernel_dirent){0}.member)){0}), \ > - ((char *)(src) + offsetof (struct kernel_dirent, member)),\ > - sizeof ((struct kernel_dirent){0}.member)) > +#define KDP_MEMBER(member) \ > + ({ \ > + __typeof ((struct kernel_dirent){0}.member) kdp_tmp; \ > + memcpy (&kdp_tmp, \ > + ((char *)(kdp) + offsetof (struct kernel_dirent, member)), \ > + sizeof (kdp_tmp)); \ > + kdp_tmp; \ > + }) Why can't this just be kdp->member? Andreas.
On 16/11/2020 18:22, Florian Weimer wrote: > * Adhemerval Zanella via Libc-alpha: > >> GCC mainline shows the following error: >> >> ../sysdeps/unix/sysv/linux/mips/mips64/getdents64.c: In function '__getdents64': >> ../sysdeps/unix/sysv/linux/mips/mips64/getdents64.c:121:7: error: 'memcpy' forming offset [4, 7] is out of the bounds [0, 4] [-Werror=array-bounds] >> 121 | memcpy (((char *) dp + offsetof (struct dirent64, d_ino)), >> | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ >> 122 | KDP_MEMBER (kdp, d_ino), sizeof ((struct dirent64){0}.d_ino)); >> | ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ >> ../sysdeps/unix/sysv/linux/mips/mips64/getdents64.c:123:7: error: 'memcpy' forming offset [4, 7] is out of the bounds [0, 4] [-Werror=array-bounds] >> 123 | memcpy (((char *) dp + offsetof (struct dirent64, d_off)), >> | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ >> 124 | KDP_MEMBER (kdp, d_off), sizeof ((struct dirent64){0}.d_off)); >> | ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ >> >> The issue is due both d_ino and d_off fields for mips64-n32 >> kernel_dirent are 32-bits, while this is using memcpy to copy 64 bits >> from it into the glibc dirent64. >> >> The fix is to use a temporary variable to read the correct type >> from kernel_dirent. > > I think I suggested to cut back on the macro magic and simply have > appropriately defined structs, with a sequence like this: > > memcpy to first temporary struct > field-by-field assignment from first temporary struct to second struct > memcpy from second temporary struct > > Would that work? > > (Sorry if my message got through and this suggestion was already > considered.) Would it be more in line of what you have suggested? --- [PATCH] linux: mips: Fix getdents64 fallback on mips64-n32 GCC mainline shows the following error: ../sysdeps/unix/sysv/linux/mips/mips64/getdents64.c: In function '__getdents64': ../sysdeps/unix/sysv/linux/mips/mips64/getdents64.c:121:7: error: 'memcpy' forming offset [4, 7] is out of the bounds [0, 4] [-Werror=array-bounds] 121 | memcpy (((char *) dp + offsetof (struct dirent64, d_ino)), | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ 122 | KDP_MEMBER (kdp, d_ino), sizeof ((struct dirent64){0}.d_ino)); | ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ ../sysdeps/unix/sysv/linux/mips/mips64/getdents64.c:123:7: error: 'memcpy' forming offset [4, 7] is out of the bounds [0, 4] [-Werror=array-bounds] 123 | memcpy (((char *) dp + offsetof (struct dirent64, d_off)), | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ 124 | KDP_MEMBER (kdp, d_off), sizeof ((struct dirent64){0}.d_off)); | ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ The issue is due both d_ino and d_off fields for mips64-n32 kernel_dirent are 32-bits, while this is using memcpy to copy 64 bits from it into the glibc dirent64. The fix is to use a temporary buffer to read the correct type from kernel_dirent. Checked with a build-many-glibcs.py for mips64el-linux-gnu and I also checked the tst-getdents64 on mips64el 4.1.4 kernel with and without fallback enabled (by manually setting the getdents64_supported). --- .../unix/sysv/linux/mips/mips64/getdents64.c | 37 +++++++++---------- sysdeps/unix/sysv/linux/tst-getdents64.c | 29 ++++++++++++--- 2 files changed, 41 insertions(+), 25 deletions(-) diff --git a/sysdeps/unix/sysv/linux/mips/mips64/getdents64.c b/sysdeps/unix/sysv/linux/mips/mips64/getdents64.c index d18a5297dc..368128244e 100644 --- a/sysdeps/unix/sysv/linux/mips/mips64/getdents64.c +++ b/sysdeps/unix/sysv/linux/mips/mips64/getdents64.c @@ -90,17 +90,14 @@ __getdents64 (int fd, void *buf, size_t nbytes) while ((char *) kdp < (char *) skdp + r) { - /* This macro is used to avoid aliasing violation. */ -#define KDP_MEMBER(src, member) \ - (__typeof__((struct kernel_dirent){0}.member) *) \ - memcpy (&((__typeof__((struct kernel_dirent){0}.member)){0}), \ - ((char *)(src) + offsetof (struct kernel_dirent, member)),\ - sizeof ((struct kernel_dirent){0}.member)) - /* This is a conservative approximation, since some of size_diff might fit into the existing padding for alignment. */ - unsigned short int k_reclen = *KDP_MEMBER (kdp, d_reclen); - unsigned short int new_reclen = ALIGN_UP (k_reclen + size_diff, + + /* Obtain the d_ino, d_off, and d_reclen from kernel filled buffer. */ + struct kernel_dirent kdirent; + memcpy (&kdirent, kdp, sizeof (struct kernel_dirent)); + + unsigned short int new_reclen = ALIGN_UP (kdirent.d_reclen + size_diff, _Alignof (struct dirent64)); if (nb + new_reclen > nbytes) { @@ -118,19 +115,19 @@ __getdents64 (int fd, void *buf, size_t nbytes) } nb += new_reclen; - memcpy (((char *) dp + offsetof (struct dirent64, d_ino)), - KDP_MEMBER (kdp, d_ino), sizeof ((struct dirent64){0}.d_ino)); - memcpy (((char *) dp + offsetof (struct dirent64, d_off)), - KDP_MEMBER (kdp, d_off), sizeof ((struct dirent64){0}.d_off)); - last_offset = *KDP_MEMBER (kdp, d_off); - memcpy (((char *) dp + offsetof (struct dirent64, d_reclen)), - &new_reclen, sizeof (new_reclen)); - dp->d_type = *((char *) kdp + k_reclen - 1); - memcpy (dp->d_name, kdp->d_name, - k_reclen - offsetof (struct kernel_dirent, d_name)); + struct dirent64 d64; + d64.d_ino = kdirent.d_ino; + d64.d_off = kdirent.d_off; + d64.d_reclen = new_reclen; + d64.d_type = *((char *) kdp + kdirent.d_reclen - 1); + memcpy (d64.d_name, kdp->d_name, + kdirent.d_reclen - offsetof (struct kernel_dirent, d_name)); + + memcpy (dp, &d64, new_reclen); + last_offset = kdirent.d_off; dp = (struct dirent64 *) ((char *) dp + new_reclen); - kdp = (struct kernel_dirent *) (((char *) kdp) + k_reclen); + kdp = (struct kernel_dirent *) (((char *) kdp) + kdirent.d_reclen); } return (char *) dp - (char *) buf; diff --git a/sysdeps/unix/sysv/linux/tst-getdents64.c b/sysdeps/unix/sysv/linux/tst-getdents64.c index 25b4755fb9..97857d22f3 100644 --- a/sysdeps/unix/sysv/linux/tst-getdents64.c +++ b/sysdeps/unix/sysv/linux/tst-getdents64.c @@ -76,8 +76,18 @@ large_buffer_checks (int fd) } } -static int -do_test (void) +static void +do_test_large_size (void) +{ + int fd = xopen (".", O_RDONLY | O_DIRECTORY, 0); + TEST_VERIFY (fd >= 0); + large_buffer_checks (fd); + + xclose (fd); +} + +static void +do_test_by_size (size_t buffer_size) { /* The test compares the iteration order with readdir64. */ DIR *reference = opendir ("."); @@ -98,7 +108,7 @@ do_test (void) non-existing data. */ struct { - char buffer[1024]; + char buffer[buffer_size]; struct dirent64 pad; } data; @@ -153,10 +163,19 @@ do_test (void) rewinddir (reference); } - large_buffer_checks (fd); - xclose (fd); closedir (reference); +} + +static int +do_test (void) +{ + do_test_by_size (512); + do_test_by_size (1024); + do_test_by_size (4096); + + do_test_large_size (); + return 0; }
On Nov 17 2020, Adhemerval Zanella wrote: > diff --git a/sysdeps/unix/sysv/linux/mips/mips64/getdents64.c b/sysdeps/unix/sysv/linux/mips/mips64/getdents64.c > index d18a5297dc..368128244e 100644 > --- a/sysdeps/unix/sysv/linux/mips/mips64/getdents64.c > +++ b/sysdeps/unix/sysv/linux/mips/mips64/getdents64.c > @@ -90,17 +90,14 @@ __getdents64 (int fd, void *buf, size_t nbytes) > > while ((char *) kdp < (char *) skdp + r) > { > - /* This macro is used to avoid aliasing violation. */ > -#define KDP_MEMBER(src, member) \ > - (__typeof__((struct kernel_dirent){0}.member) *) \ > - memcpy (&((__typeof__((struct kernel_dirent){0}.member)){0}), \ > - ((char *)(src) + offsetof (struct kernel_dirent, member)),\ > - sizeof ((struct kernel_dirent){0}.member)) > - > /* This is a conservative approximation, since some of size_diff might > fit into the existing padding for alignment. */ > - unsigned short int k_reclen = *KDP_MEMBER (kdp, d_reclen); > - unsigned short int new_reclen = ALIGN_UP (k_reclen + size_diff, > + > + /* Obtain the d_ino, d_off, and d_reclen from kernel filled buffer. */ > + struct kernel_dirent kdirent; > + memcpy (&kdirent, kdp, sizeof (struct kernel_dirent)); How good is the compiler at eliding the memcpy? What would change if kdp would just be void *? Andreas.
On 17/11/2020 10:38, Andreas Schwab wrote: > On Nov 17 2020, Adhemerval Zanella wrote: > >> diff --git a/sysdeps/unix/sysv/linux/mips/mips64/getdents64.c b/sysdeps/unix/sysv/linux/mips/mips64/getdents64.c >> index d18a5297dc..368128244e 100644 >> --- a/sysdeps/unix/sysv/linux/mips/mips64/getdents64.c >> +++ b/sysdeps/unix/sysv/linux/mips/mips64/getdents64.c >> @@ -90,17 +90,14 @@ __getdents64 (int fd, void *buf, size_t nbytes) >> >> while ((char *) kdp < (char *) skdp + r) >> { >> - /* This macro is used to avoid aliasing violation. */ >> -#define KDP_MEMBER(src, member) \ >> - (__typeof__((struct kernel_dirent){0}.member) *) \ >> - memcpy (&((__typeof__((struct kernel_dirent){0}.member)){0}), \ >> - ((char *)(src) + offsetof (struct kernel_dirent, member)),\ >> - sizeof ((struct kernel_dirent){0}.member)) >> - >> /* This is a conservative approximation, since some of size_diff might >> fit into the existing padding for alignment. */ >> - unsigned short int k_reclen = *KDP_MEMBER (kdp, d_reclen); >> - unsigned short int new_reclen = ALIGN_UP (k_reclen + size_diff, >> + >> + /* Obtain the d_ino, d_off, and d_reclen from kernel filled buffer. */ >> + struct kernel_dirent kdirent; >> + memcpy (&kdirent, kdp, sizeof (struct kernel_dirent)); > > How good is the compiler at eliding the memcpy? What would change if > kdp would just be void *? Not that good, I see a large stack usage (1456 vs 1152 from my initial version) and some more memory load/store memory instructions. The kdp type change does not change the code generation. I don't have a strong preference here, in any case this should be used only as fallback on older kernels.
* Adhemerval Zanella via Libc-alpha: > Not that good, I see a large stack usage (1456 vs 1152 from my initial > version) and some more memory load/store memory instructions. The > kdp type change does not change the code generation. That's why I suggested to use two separately defined struct types for this.
On 17/11/2020 14:51, Florian Weimer wrote: > * Adhemerval Zanella via Libc-alpha: > >> Not that good, I see a large stack usage (1456 vs 1152 from my initial >> version) and some more memory load/store memory instructions. The >> kdp type change does not change the code generation. > > That's why I suggested to use two separately defined struct types for > this. > How different than my second attempt [1] is your suggestion? I tried to follow your description on this one. [1] https://sourceware.org/pipermail/libc-alpha/2020-November/119688.html
* Adhemerval Zanella via Libc-alpha: > Would it be more in line of what you have suggested? Yes, thanks. > while ((char *) kdp < (char *) skdp + r) > { > + > + /* Obtain the d_ino, d_off, and d_reclen from kernel filled buffer. */ > + struct kernel_dirent kdirent; > + memcpy (&kdirent, kdp, sizeof (struct kernel_dirent)); I believe this should use offsetof (struct kernel_dirent, d_name) for the size of the copy. Technically, the padding at the end of the struct may not be present in the buffer. > @@ -118,19 +115,19 @@ __getdents64 (int fd, void *buf, size_t nbytes) > } > nb += new_reclen; > > + struct dirent64 d64; > + d64.d_ino = kdirent.d_ino; > + d64.d_off = kdirent.d_off; > + d64.d_reclen = new_reclen; > + d64.d_type = *((char *) kdp + kdirent.d_reclen - 1); > + memcpy (d64.d_name, kdp->d_name, > + kdirent.d_reclen - offsetof (struct kernel_dirent, d_name)); > + > + memcpy (dp, &d64, new_reclen); > + last_offset = kdirent.d_off; > > dp = (struct dirent64 *) ((char *) dp + new_reclen); > + kdp = (struct kernel_dirent *) (((char *) kdp) + kdirent.d_reclen); > } I think the first memcpy is wrong: It has to go into the result buffer because only that will be large enough. 256 bytes for d_name may not be enough. The second memcpy should only copy the header (before d_name). Thanks, Florian
On 22/01/2021 09:49, Florian Weimer wrote: > * Adhemerval Zanella via Libc-alpha: > >> Would it be more in line of what you have suggested? > > Yes, thanks. >> while ((char *) kdp < (char *) skdp + r) >> { >> + >> + /* Obtain the d_ino, d_off, and d_reclen from kernel filled buffer. */ >> + struct kernel_dirent kdirent; >> + memcpy (&kdirent, kdp, sizeof (struct kernel_dirent)); > > I believe this should use offsetof (struct kernel_dirent, d_name) > for the size of the copy. Technically, the padding at the end of the > struct may not be present in the buffer. Right, I was working the assumption that kernel always returned at least one byte do d_name ('\0'). > >> @@ -118,19 +115,19 @@ __getdents64 (int fd, void *buf, size_t nbytes) >> } >> nb += new_reclen; >> >> + struct dirent64 d64; >> + d64.d_ino = kdirent.d_ino; >> + d64.d_off = kdirent.d_off; >> + d64.d_reclen = new_reclen; >> + d64.d_type = *((char *) kdp + kdirent.d_reclen - 1); >> + memcpy (d64.d_name, kdp->d_name, >> + kdirent.d_reclen - offsetof (struct kernel_dirent, d_name)); >> + >> + memcpy (dp, &d64, new_reclen); >> + last_offset = kdirent.d_off; >> >> dp = (struct dirent64 *) ((char *) dp + new_reclen); >> + kdp = (struct kernel_dirent *) (((char *) kdp) + kdirent.d_reclen); >> } > > I think the first memcpy is wrong: It has to go into the result buffer > because only that will be large enough. 256 bytes for d_name may not be > enough. The second memcpy should only copy the header (before d_name). > Indeed, I fixed on the patch below. The tst-getdents64 does pass on mips64 and mips64-n32 on gccfarm machine (I explicit disabled the getdents64 call for the test). -- [PATCH] linux: mips: Fix getdents64 fallback on mips64-n32 GCC mainline shows the following error: ../sysdeps/unix/sysv/linux/mips/mips64/getdents64.c: In function '__getdents64': ../sysdeps/unix/sysv/linux/mips/mips64/getdents64.c:121:7: error: 'memcpy' forming offset [4, 7] is out of the bounds [0, 4] [-Werror=array-bounds] 121 | memcpy (((char *) dp + offsetof (struct dirent64, d_ino)), | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ 122 | KDP_MEMBER (kdp, d_ino), sizeof ((struct dirent64){0}.d_ino)); | ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ ../sysdeps/unix/sysv/linux/mips/mips64/getdents64.c:123:7: error: 'memcpy' forming offset [4, 7] is out of the bounds [0, 4] [-Werror=array-bounds] 123 | memcpy (((char *) dp + offsetof (struct dirent64, d_off)), | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ 124 | KDP_MEMBER (kdp, d_off), sizeof ((struct dirent64){0}.d_off)); | ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ The issue is due both d_ino and d_off fields for mips64-n32 kernel_dirent are 32-bits, while this is using memcpy to copy 64 bits from it into the glibc dirent64. The fix is to use a temporary buffer to read the correct type from kernel_dirent. Checked with a build-many-glibcs.py for mips64el-linux-gnu and I also checked the tst-getdents64 on mips64el 4.1.4 kernel with and without fallback enabled (by manually setting the getdents64_supported). --- .../unix/sysv/linux/mips/mips64/getdents64.c | 37 +++++++++---------- sysdeps/unix/sysv/linux/tst-getdents64.c | 29 ++++++++++++--- 2 files changed, 42 insertions(+), 24 deletions(-) diff --git a/sysdeps/unix/sysv/linux/mips/mips64/getdents64.c b/sysdeps/unix/sysv/linux/mips/mips64/getdents64.c index a218f68104..ed6589ab94 100644 --- a/sysdeps/unix/sysv/linux/mips/mips64/getdents64.c +++ b/sysdeps/unix/sysv/linux/mips/mips64/getdents64.c @@ -90,17 +90,14 @@ __getdents64 (int fd, void *buf, size_t nbytes) while ((char *) kdp < (char *) skdp + r) { - /* This macro is used to avoid aliasing violation. */ -#define KDP_MEMBER(src, member) \ - (__typeof__((struct kernel_dirent){0}.member) *) \ - memcpy (&((__typeof__((struct kernel_dirent){0}.member)){0}), \ - ((char *)(src) + offsetof (struct kernel_dirent, member)),\ - sizeof ((struct kernel_dirent){0}.member)) - /* This is a conservative approximation, since some of size_diff might fit into the existing padding for alignment. */ - unsigned short int k_reclen = *KDP_MEMBER (kdp, d_reclen); - unsigned short int new_reclen = ALIGN_UP (k_reclen + size_diff, + + /* Obtain the d_ino, d_off, and d_reclen from kernel filled buffer. */ + struct kernel_dirent kdirent; + memcpy (&kdirent, kdp, offsetof (struct kernel_dirent, d_name)); + + unsigned short int new_reclen = ALIGN_UP (kdirent.d_reclen + size_diff, _Alignof (struct dirent64)); if (nb + new_reclen > nbytes) { @@ -118,19 +115,21 @@ __getdents64 (int fd, void *buf, size_t nbytes) } nb += new_reclen; - memcpy (((char *) dp + offsetof (struct dirent64, d_ino)), - KDP_MEMBER (kdp, d_ino), sizeof ((struct dirent64){0}.d_ino)); - memcpy (((char *) dp + offsetof (struct dirent64, d_off)), - KDP_MEMBER (kdp, d_off), sizeof ((struct dirent64){0}.d_off)); - last_offset = *KDP_MEMBER (kdp, d_off); - memcpy (((char *) dp + offsetof (struct dirent64, d_reclen)), - &new_reclen, sizeof (new_reclen)); - dp->d_type = *((char *) kdp + k_reclen - 1); + struct dirent64 d64; + d64.d_ino = kdirent.d_ino; + d64.d_off = kdirent.d_off; + d64.d_reclen = new_reclen; + d64.d_type = *((char *) kdp + kdirent.d_reclen - 1); + /* First copy only the header. */ + memcpy (dp, &d64, offsetof (struct dirent64, d_name)); + /* And then the d_name. */ memcpy (dp->d_name, kdp->d_name, - k_reclen - offsetof (struct kernel_dirent, d_name)); + kdirent.d_reclen - offsetof (struct kernel_dirent, d_name)); + + last_offset = kdirent.d_off; dp = (struct dirent64 *) ((char *) dp + new_reclen); - kdp = (struct kernel_dirent *) (((char *) kdp) + k_reclen); + kdp = (struct kernel_dirent *) (((char *) kdp) + kdirent.d_reclen); } return (char *) dp - (char *) buf; diff --git a/sysdeps/unix/sysv/linux/tst-getdents64.c b/sysdeps/unix/sysv/linux/tst-getdents64.c index 379ecbbcc6..691444d56e 100644 --- a/sysdeps/unix/sysv/linux/tst-getdents64.c +++ b/sysdeps/unix/sysv/linux/tst-getdents64.c @@ -76,8 +76,18 @@ large_buffer_checks (int fd) } } -static int -do_test (void) +static void +do_test_large_size (void) +{ + int fd = xopen (".", O_RDONLY | O_DIRECTORY, 0); + TEST_VERIFY (fd >= 0); + large_buffer_checks (fd); + + xclose (fd); +} + +static void +do_test_by_size (size_t buffer_size) { /* The test compares the iteration order with readdir64. */ DIR *reference = opendir ("."); @@ -98,7 +108,7 @@ do_test (void) non-existing data. */ struct { - char buffer[1024]; + char buffer[buffer_size]; struct dirent64 pad; } data; @@ -153,10 +163,19 @@ do_test (void) rewinddir (reference); } - large_buffer_checks (fd); - xclose (fd); closedir (reference); +} + +static int +do_test (void) +{ + do_test_by_size (512); + do_test_by_size (1024); + do_test_by_size (4096); + + do_test_large_size (); + return 0; }
* Adhemerval Zanella: > Indeed, I fixed on the patch below. The tst-getdents64 does pass on > mips64 and mips64-n32 on gccfarm machine (I explicit disabled the > getdents64 call for the test). This version looks okay to me. Thanks, Florian
diff --git a/sysdeps/unix/sysv/linux/mips/mips64/getdents64.c b/sysdeps/unix/sysv/linux/mips/mips64/getdents64.c index d18a5297dc..5b7597c99b 100644 --- a/sysdeps/unix/sysv/linux/mips/mips64/getdents64.c +++ b/sysdeps/unix/sysv/linux/mips/mips64/getdents64.c @@ -91,15 +91,18 @@ __getdents64 (int fd, void *buf, size_t nbytes) while ((char *) kdp < (char *) skdp + r) { /* This macro is used to avoid aliasing violation. */ -#define KDP_MEMBER(src, member) \ - (__typeof__((struct kernel_dirent){0}.member) *) \ - memcpy (&((__typeof__((struct kernel_dirent){0}.member)){0}), \ - ((char *)(src) + offsetof (struct kernel_dirent, member)),\ - sizeof ((struct kernel_dirent){0}.member)) +#define KDP_MEMBER(member) \ + ({ \ + __typeof ((struct kernel_dirent){0}.member) kdp_tmp; \ + memcpy (&kdp_tmp, \ + ((char *)(kdp) + offsetof (struct kernel_dirent, member)), \ + sizeof (kdp_tmp)); \ + kdp_tmp; \ + }) /* This is a conservative approximation, since some of size_diff might fit into the existing padding for alignment. */ - unsigned short int k_reclen = *KDP_MEMBER (kdp, d_reclen); + unsigned short int k_reclen = KDP_MEMBER (d_reclen); unsigned short int new_reclen = ALIGN_UP (k_reclen + size_diff, _Alignof (struct dirent64)); if (nb + new_reclen > nbytes) @@ -118,11 +121,17 @@ __getdents64 (int fd, void *buf, size_t nbytes) } nb += new_reclen; - memcpy (((char *) dp + offsetof (struct dirent64, d_ino)), - KDP_MEMBER (kdp, d_ino), sizeof ((struct dirent64){0}.d_ino)); - memcpy (((char *) dp + offsetof (struct dirent64, d_off)), - KDP_MEMBER (kdp, d_off), sizeof ((struct dirent64){0}.d_off)); - last_offset = *KDP_MEMBER (kdp, d_off); +#define COPY_MEMBER(member) \ + ({ \ + __typeof ((struct dirent64){0}.member) dp_tmp \ + = KDP_MEMBER (member); \ + memcpy ((char *) dp + offsetof (struct dirent64, member), \ + &dp_tmp, sizeof (dp_tmp)); \ + }) + + COPY_MEMBER (d_ino); + COPY_MEMBER (d_off); + last_offset = KDP_MEMBER (d_off); memcpy (((char *) dp + offsetof (struct dirent64, d_reclen)), &new_reclen, sizeof (new_reclen)); dp->d_type = *((char *) kdp + k_reclen - 1); diff --git a/sysdeps/unix/sysv/linux/tst-getdents64.c b/sysdeps/unix/sysv/linux/tst-getdents64.c index 25b4755fb9..97857d22f3 100644 --- a/sysdeps/unix/sysv/linux/tst-getdents64.c +++ b/sysdeps/unix/sysv/linux/tst-getdents64.c @@ -76,8 +76,18 @@ large_buffer_checks (int fd) } } -static int -do_test (void) +static void +do_test_large_size (void) +{ + int fd = xopen (".", O_RDONLY | O_DIRECTORY, 0); + TEST_VERIFY (fd >= 0); + large_buffer_checks (fd); + + xclose (fd); +} + +static void +do_test_by_size (size_t buffer_size) { /* The test compares the iteration order with readdir64. */ DIR *reference = opendir ("."); @@ -98,7 +108,7 @@ do_test (void) non-existing data. */ struct { - char buffer[1024]; + char buffer[buffer_size]; struct dirent64 pad; } data; @@ -153,10 +163,19 @@ do_test (void) rewinddir (reference); } - large_buffer_checks (fd); - xclose (fd); closedir (reference); +} + +static int +do_test (void) +{ + do_test_by_size (512); + do_test_by_size (1024); + do_test_by_size (4096); + + do_test_large_size (); + return 0; }
GCC mainline shows the following error: ../sysdeps/unix/sysv/linux/mips/mips64/getdents64.c: In function '__getdents64': ../sysdeps/unix/sysv/linux/mips/mips64/getdents64.c:121:7: error: 'memcpy' forming offset [4, 7] is out of the bounds [0, 4] [-Werror=array-bounds] 121 | memcpy (((char *) dp + offsetof (struct dirent64, d_ino)), | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ 122 | KDP_MEMBER (kdp, d_ino), sizeof ((struct dirent64){0}.d_ino)); | ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ ../sysdeps/unix/sysv/linux/mips/mips64/getdents64.c:123:7: error: 'memcpy' forming offset [4, 7] is out of the bounds [0, 4] [-Werror=array-bounds] 123 | memcpy (((char *) dp + offsetof (struct dirent64, d_off)), | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ 124 | KDP_MEMBER (kdp, d_off), sizeof ((struct dirent64){0}.d_off)); | ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ The issue is due both d_ino and d_off fields for mips64-n32 kernel_dirent are 32-bits, while this is using memcpy to copy 64 bits from it into the glibc dirent64. The fix is to use a temporary variable to read the correct type from kernel_dirent. Checked with a build-many-glibcs.py for mips64el-linux-gnu and I also checked the tst-getdents64 on mips64el 4.1.4 kernel with and without fallback enabled (by manually setting the getdents64_supported). --- .../unix/sysv/linux/mips/mips64/getdents64.c | 31 ++++++++++++------- sysdeps/unix/sysv/linux/tst-getdents64.c | 29 ++++++++++++++--- 2 files changed, 44 insertions(+), 16 deletions(-)