Bug#1030638: cp -a fails to preserve ownership information on 32-bit arches
Control: tags -1 + patch On Wed, Mar 1, 2023 at 3:10 PM Shengjing Zhu wrote: > I realized there probably was no need for runtime detection after some > discussion with others. > > After all, it has already dispatched the right _time64 function. But > on i386, the only case to use _time64 function is when compiled with > D_TIME_BITS=64. > So there shouldn't be two variants of stat64 struct. It's just > fakeroot is using the wrong one. > fakeroot should compile its all time64 funcs with D_TIME_BITS=64, then > it should get the right struct. (only these _time64 parts, so be in > separate files.) > > I'm still exploring this idea, but anyone more familiar with autoconf > would be helpful! > Please see the patch https://salsa.debian.org/clint/fakeroot/-/merge_requests/22 -- Shengjing Zhu
Processed: Re: Bug#1030638: cp -a fails to preserve ownership information on 32-bit arches
Processing control commands: > tags -1 + patch Bug #1030638 [fakeroot] cp -a fails to preserve ownership information on 32-bit arches Added tag(s) patch. -- 1030638: https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=1030638 Debian Bug Tracking System Contact ow...@bugs.debian.org with problems
Bug#1030638: cp -a fails to preserve ownership information on 32-bit arches
On Wed, Mar 1, 2023 at 3:00 PM Johannes Schauer Marin Rodrigues wrote: > > Hi Shengjing, > > Quoting Shengjing Zhu (2023-03-01 06:40:38) > > I've debugged it as well and here is my write up. Though I don't have > > solution yet. > > you don't have a *good* solution yet but I think you are extremely close! > Thank > you so much for spending all the time debugging this *and* for this very > helpful writeup which is helpful for guys like me who could've never come this > far by themselves. Thank you! :) > > Would it maybe possible to choose the correct stat struct by trying to see > with > which struct the values make sense? For example it should be easy to check > which inode numbers actually exist. The other entries of the stat struct also > can be checked if they look legitimate, like the file mode (which should not > be > larger than 0o4777). I realized there probably was no need for runtime detection after some discussion with others. After all, it has already dispatched the right _time64 function. But on i386, the only case to use _time64 function is when compiled with D_TIME_BITS=64. So there shouldn't be two variants of stat64 struct. It's just fakeroot is using the wrong one. fakeroot should compile its all time64 funcs with D_TIME_BITS=64, then it should get the right struct. (only these _time64 parts, so be in separate files.) I'm still exploring this idea, but anyone more familiar with autoconf would be helpful! -- Shengjing Zhu
Bug#1030638: cp -a fails to preserve ownership information on 32-bit arches
Hi Shengjing, Quoting Shengjing Zhu (2023-03-01 06:40:38) > I've debugged it as well and here is my write up. Though I don't have > solution yet. you don't have a *good* solution yet but I think you are extremely close! Thank you so much for spending all the time debugging this *and* for this very helpful writeup which is helpful for guys like me who could've never come this far by themselves. Thank you! :) Would it maybe possible to choose the correct stat struct by trying to see with which struct the values make sense? For example it should be easy to check which inode numbers actually exist. The other entries of the stat struct also can be checked if they look legitimate, like the file mode (which should not be larger than 0o4777). What do you think? Thanks! cheers, josch
Bug#1030638: cp -a fails to preserve ownership information on 32-bit arches
X-Debbugs-Cc: z...@debian.org, cl...@debian.org, jo...@debian.org
Hi,
I've debugged it as well and here is my write up. Though I don't have solution
yet.
I use an i386 VM for testing and following code to simplify my test.
```c
int main() {
struct stat buf;
int r;
r = fstatat(AT_FDCWD, "./test.c", , 0);
printf("%d %d %d\n", r, buf.st_ino, buf.st_gid);
}
```
Compile it with:
$ gcc -g -D_FILE_OFFSET_BITS=64 -D_TIME_BITS=64 ./test.c
This same as coreutils that enables time64 support.
with the instruction in DEBUG file:
$ gdb ./a.out
(gdb) set env FAKEROOTKEY=430935964
(gdb) set env LD_PRELOAD=./prefix/lib/libfakeroot-0.so
(gdb) b main
Breakpoint 1 at 0x11b6: file ./test.c, line 10.
(gdb) r
Starting program: /home/debian/a.out
Breakpoint 1, main () at ./test.c:10
10 r = fstatat(AT_FDCWD, "./test.c", , 0);
(gdb) s
__fstatat64_time64 (dir_fd=-100, path=0x402008 "./test.c", st=0xb4d0,
flags=0) at libfakeroot.c:2700
2700 r=NEXT_FSTATAT64_TIME64(ver, dir_fd, path, st, flags);
(gdb) p st
$1 = (struct stat64 *) 0xb4d0
(gdb) p *st
$2 = {st_dev = 11676941228, __pad1 = 3086760800, __st_ino = 1, st_mode = 0,
st_nlink = 1, st_uid = 3087006272, st_gid = 0, st_rdev = 13257672154138279935,
__pad2 = 3087005192,
st_size = -5188164414456463360, st_blksize = 44, st_blocks =
-4611686117211635712, st_atim = {tv_sec = -1208183472, tv_nsec = 0}, st_mtim =
{tv_sec = -1210426833,
tv_nsec = -1208328120}, st_ctim = {tv_sec = -1208207344, tv_nsec =
-1208108853}, st_ino = 13257533047527070255}
(gdb) n
2701 if(r)
(gdb) p *st
$3 = {st_dev = 2049, __pad1 = 262484, __st_ino = 0, st_mode = 33188, st_nlink =
1, st_uid = 1000, st_gid = 1000, st_rdev = 0, __pad2 = 221, st_size =
17592186044416, st_blksize = 8,
st_blocks = 7205240262505791488, st_atim = {tv_sec = 0, tv_nsec = 29600},
st_mtim = {tv_sec = 0, tv_nsec = 1677600726}, st_ctim = {tv_sec = 0, tv_nsec =
9200},
st_ino = 7205240253915856896}
(gdb) p (struct __stat64_t64)(*st)
$4 = {st_dev = 2049, st_ino = 262484, st_mode = 33188, st_nlink = 1, st_uid =
1000, st_gid = 1000, st_rdev = 0, st_size = 221, st_blksize = 4096, st_blocks =
8, st_atim = {
tv_sec = 1677600728, tv_nsec = 29600}, st_mtim = {tv_sec = 1677600726,
tv_nsec = 9200}, st_ctim = {tv_sec = 1677600726, tv_nsec = 10800}}
We can see that st struct is decoded into a wrong layout.
It causes the message passed to faked has wrong inode.
To verify the guess, I use following dirty patch,
>From ea3eab6ea82604f9a16d658f7fc7ec5ce4bc337d Mon Sep 17 00:00:00 2001
From: Shengjing Zhu
Date: Wed, 1 Mar 2023 13:21:25 +0800
Subject: [PATCH] hack
---
communicate.c | 35 +++
communicate.h | 1 +
libfakeroot.c | 9 +++--
stat64.h | 15 +++
4 files changed, 58 insertions(+), 2 deletions(-)
create mode 100644 stat64.h
diff --git a/communicate.c b/communicate.c
index dec9cc6..f7bbaf4 100644
--- a/communicate.c
+++ b/communicate.c
@@ -63,6 +63,8 @@
#include "stats.h"
#endif
+#include "stat64.h"
+
#ifndef _UTSNAME_LENGTH
/* for LINUX libc5 */
# define _UTSNAME_LENGTH _SYS_NMLN
@@ -864,6 +866,39 @@ void send_get_stat64(struct stat64 *st
}
}
+void send_get_stat64_2(struct stat64 *st)
+{
+ struct fake_msg buf;
+
+#ifndef FAKEROOT_FAKENET
+ if(init_get_msg()!=-1)
+#endif /* ! FAKEROOT_FAKENET */
+ {
+ buf.st.mode =((struct x__stat64 *)st)->st_mode;
+ buf.st.ino =((struct x__stat64 *)st)->st_ino ;
+ buf.st.uid =((struct x__stat64 *)st)->st_uid ;
+ buf.st.gid =((struct x__stat64 *)st)->st_gid ;
+ buf.st.dev =((struct x__stat64 *)st)->st_dev ;
+ buf.st.rdev =((struct x__stat64 *)st)->st_rdev;
+ buf.st.nlink=((struct x__stat64 *)st)->st_nlink;
+
+ fprintf(stderr, "libfakeroot: before ino %ld uid %ld gid %ld \n",
buf.st.ino, buf.st.uid, buf.st.gid);
+
+buf.id=stat_func;
+send_get_fakem();
+
+
+ ((struct x__stat64 *)st)->st_mode =buf.st.mode;
+ ((struct x__stat64 *)st)->st_ino =buf.st.ino ;
+ ((struct x__stat64 *)st)->st_uid =buf.st.uid ;
+ ((struct x__stat64 *)st)->st_gid =buf.st.gid ;
+ ((struct x__stat64 *)st)->st_dev =buf.st.dev ;
+ ((struct x__stat64 *)st)->st_rdev =buf.st.rdev;
+
+ fprintf(stderr, "libfakeroot: after ino %ld uid %ld gid %ld \n", buf.st.ino,
buf.st.uid, buf.st.gid);
+ }
+}
+
void send_get_xattr64(struct stat64 *st
, xattr_args *xattr
#ifdef STUPID_ALPHA_HACK
diff --git a/communicate.h b/communicate.h
index a586108..5562402 100644
--- a/communicate.h
+++ b/communicate.h
@@ -211,6 +211,7 @@ extern void unlock_comm_sd(void);
#ifndef STUPID_ALPHA_HACK
extern void send_stat64(const struct stat64 *st, func_id_t f);
extern void send_get_stat64(struct stat64 *buf);
+extern void send_get_stat64_2(struct stat64 *buf);
extern void send_get_xattr64(struct stat64 *st, xattr_args *xattr);
#else
extern void send_stat64(const struct stat64 *st, func_id_t f, int ver);
diff --git a/libfakeroot.c b/libfakeroot.c
index 26a3e90..5eb35b3
Bug#1030638: cp -a fails to preserve ownership information on 32-bit arches
Package: fakeroot Followup-For: Bug #1030638 Hi josch, Are you able to confirm whether the repro environment(s) for this bugreport used emulated and/or native (non-emulated) 32-bit systems? To explain my question: a qemu bug[1] that I was reading about a while ago highlights that there are some challenges emulating file-related system calls for a 32-bit guest from a 64-bit host. (in the situation I was looking at, that caused directory listing functions to appear to return empty results, despite directory contents being visible on the host. it may not be relevant here - if repro was found on non-emulated systems then we can ignore this theory) Thanks, James [1] - https://gitlab.com/qemu-project/qemu/-/issues/263
Bug#1030638: cp -a fails to preserve ownership information on 32-bit arches
Hi,
Quoting Johannes Schauer Marin Rodrigues (2023-02-06 00:47:35)
> since glibc 2.34 and coreutils 9.1, fakeroot fails to preserve ownership
> information when running "cp -a" on a file owned by a user other than
> root. On armel, armhf and i386 (our 32 bit arches), you can reproduce
> this problem by running inside fakeroot:
>
> $ touch foo
> $ chown 0:42 foo
> $ ls -lha foo
> $ cp -a foo bar
> $ ls -lha bar"
>
> which will print this:
>
> -rw-r--r-- 1 root shadow 0 Feb 5 23:00 foo
> -rw-r--r-- 1 root root 0 Feb 5 23:00 bar
>
> I submitted an improvement to the `cp-a` test which adds a check for the
> ownership information in addition to the mode checks as a merge request
> for that test here:
>
> https://salsa.debian.org/clint/fakeroot/-/merge_requests/19
>
> Observe how the salsaci pipeline succeds for amd64 but fails on i386.
> The reason is that on i386, fakeroot will not retain the ownership
> information.
>
> A quick comparison of the strace output on arm64 (which does not have
> this problem) and armhf (which does have this problem) shows that arm64
> calls fchown() while armhf calls fchown32() which is not wrapped by
> fakeroot. Maybe that is the problem?
>
> This breaks my package mmdebstrap in a similar way as #1023286 did.
I have a little bit of more input. This is what "cp --preserve=ownership" does
on amd64 (according to ltrace):
open("bar", 2162688, 015412541474)
= -1
__errno_location()
= 0x7ff47c598398
fstatat(0xff9c, 0x7ffe6c2ac337, 0x7ffe6c2aae10, 0)
= 0
open("foo", 0, 00)
= 3
fstat(3, 0x7ffe6c2aafc0, 0, 0x7ff47c72ae51)
= 0
openat(0xff9c, 0x7ffe6c2ac33b, 193, 384)
= 4
__errno_location()
= 0x7ff47c598398
ioctl(4, 1074041865, 0x3)
= -1
fstat(4, 0x7ffe6c2aaf30, 0x, 0x7ff47c730bab)
= 0
Okay, nothing to see here. This works, after all. On i386 it looks like this:
open64("bar", 2162688, 012625311011)
= -1
__errno_location()
= 0xf7bf56bc
__fstatat64_time64(-100, 0xff958337, 0xff957948, 0)
= 0
open64("foo", 0, 00)
= 3
__fstat64_time64(3, 0xff957a8c, 0xff957948, 0)
= 0
openat64(-100, 0xff95833b, 193, 384)
= 4
__errno_location()
= 0xf7bf56bc
__ioctl_time64(4, 0x40049409, 3, 1)
= -1
__fstat64_time64(4, 0xff957a20, 0, 0)
= 0
So now this looks even more like #1023286 because it involves
__fstatat64_time64 and __fstat64_time64. I added this patch to fakeroot:
--- a/libfakeroot.c
+++ b/libfakeroot.c
@@ -2671,6 +2673,11 @@ int WRAP_FSTATAT64_TIME64 FSTATAT64_TIME64_ARG(int ver,
int r;
+#ifdef LIBFAKEROOT_DEBUGGING
+ if (fakeroot_debug) {
+fprintf(stderr, "fstatat64[time64] fd %d %s\n", dir_fd, path);
+ }
+#endif /* LIBFAKEROOT_DEBUGGING */
r=NEXT_FSTATAT64_TIME64(ver, dir_fd, path, st, flags);
if(r)
return -1;
And re-ran the t.cp-a test that I changed according to my merge request to
reproduce the problem on i386 like this:
env --chdir=test srcdir=$(pwd)/test VERBOSE=1 libfakeroot=libfakeroot-0.so sh
-x ./t.cp-a
(is there a better way to run individual tests with maximum verbosity?)
The relevant call to `cp -a` looks like this:
stat64 file_name /home/josch/usr/bin/cp
stat64 file_name /usr/local/bin/cp
stat64 file_name /usr/bin/cp
fstatat64[time64] fd -100 empty
load_library_symbols
fstat64[time64] fd 3
fstat64[time64] fd 4
flistxattr fd 3
flistxattr fd 3
fchmod fd 4
This seems to indicate that __fstatat64_time64 does get wrapped as expected.
The dirfd is set to -100 which is the special value AT_FDCWD.
I'm at a loss at how to proceed.
Can you find out more?
Thanks!
cheers, josch
signature.asc
Description: signature
Bug#1030638: cp -a fails to preserve ownership information on 32-bit arches
Package: fakeroot Version: 1.30.1-1.1 Severity: grave Control: affects -1 + mmdebstrap Hi, since glibc 2.34 and coreutils 9.1, fakeroot fails to preserve ownership information when running "cp -a" on a file owned by a user other than root. On armel, armhf and i386 (our 32 bit arches), you can reproduce this problem by running inside fakeroot: $ touch foo $ chown 0:42 foo $ ls -lha foo $ cp -a foo bar $ ls -lha bar" which will print this: -rw-r--r-- 1 root shadow 0 Feb 5 23:00 foo -rw-r--r-- 1 root root 0 Feb 5 23:00 bar I submitted an improvement to the `cp-a` test which adds a check for the ownership information in addition to the mode checks as a merge request for that test here: https://salsa.debian.org/clint/fakeroot/-/merge_requests/19 Observe how the salsaci pipeline succeds for amd64 but fails on i386. The reason is that on i386, fakeroot will not retain the ownership information. A quick comparison of the strace output on arm64 (which does not have this problem) and armhf (which does have this problem) shows that arm64 calls fchown() while armhf calls fchown32() which is not wrapped by fakeroot. Maybe that is the problem? This breaks my package mmdebstrap in a similar way as #1023286 did. Since I think that `cp -a` functionality is quite essential, I'm making this bug RC. Feel free to adjust accordingly. Thanks! cheers, josch
