0.4.7.2-alpha FTBFS on i386

assigned to @ahf

changed milestone to %Tor: 0.4.7.x-stable

We should fix this before next 0.4.7.x.

added CI label

removed CI label

The following patch seems to make the test pass, but unfortunately by skipping the stat_filename test:

diff --git a/src/lib/sandbox/sandbox.c b/src/lib/sandbox/sandbox.c
index aed7e3706f..4a980284c5 100644
--- a/src/lib/sandbox/sandbox.c
+++ b/src/lib/sandbox/sandbox.c
@@ -257,7 +257,6 @@ static int filter_nopar_gen[] = {
 
     SCMP_SYS(madvise),
 #ifdef __NR_stat64
-    // getaddrinfo uses this..
     SCMP_SYS(stat64),
 #endif
 
@@ -624,6 +623,33 @@ sb_chown(scmp_filter_ctx ctx, sandbox_cfg_t *filter)
   return 0;
 }
 
+#ifdef __NR_chown32
+static int
+sb_chown32(scmp_filter_ctx ctx, sandbox_cfg_t *filter)
+{
+  int rc;
+  sandbox_cfg_t *elem = NULL;
+
+  // for each dynamic parameter filters
+  for (elem = filter; elem != NULL; elem = elem->next) {
+    smp_param_t *param = elem->param;
+
+    if (param != NULL && param->prot == 1 && param->syscall
+        == SCMP_SYS(chown32)) {
+      rc = seccomp_rule_add_1(ctx, SCMP_ACT_ALLOW, SCMP_SYS(chown32),
+            SCMP_CMP_STR(0, SCMP_CMP_EQ, param->value));
+      if (rc != 0) {
+        log_err(LD_BUG,"(Sandbox) failed to add chown32 syscall, received "
+            "libseccomp error %d", rc);
+        return rc;
+      }
+    }
+  }
+
+  return 0;
+}
+#endif // __NR_chown32
+
 /**
  * Function responsible for setting up the rename syscall for
  * the seccomp filter sandbox.
@@ -1271,6 +1297,9 @@ static sandbox_filter_func_t filter_func[] = {
     sb_mmap2,
 #endif
     sb_chown,
+#ifdef __NR_chown32
+    sb_chown32,
+#endif
     sb_chmod,
     sb_open,
     sb_openat,
@@ -1597,7 +1626,14 @@ sandbox_cfg_allow_chown_filename(sandbox_cfg_t **cfg, char *file)
   elem = new_element(SCMP_SYS(chown), file);
 
   elem->next = *cfg;
+
+#if  __NR_chown32
+  elem = new_element(SCMP_SYS(chown32), file);
+
+  elem->next = *cfg;
+  *cfg = elem;
   *cfg = elem;
+#endif
 
   return 0;
 }
diff --git a/src/test/test_sandbox.c b/src/test/test_sandbox.c
index e5064c58ec..c8366924e7 100644
--- a/src/test/test_sandbox.c
+++ b/src/test/test_sandbox.c
@@ -337,7 +337,7 @@ struct testcase_t sandbox_tests[] = {
  *
  * Skip testing sandbox_cfg_allow_stat_filename() if it seems the likely the
  * function will have no effect and the test will therefore not succeed. */
-#if !defined(__NR_newfstatat) && (!defined(__NR_stat) || defined(__NR_stat64))
+#if !defined(__NR_newfstatat) && (!defined(__NR_stat) || !defined(__NR_stat64))
   SANDBOX_TEST_IN_SANDBOX(stat_filename),
 #else
   SANDBOX_TEST_SKIPPED(stat_filename),

The change in src/test/test_sandbox.c is not correct as it is right now.

I suspect this might be related to #40382 (closed) too. The i386 debian stable I tested with was using glibc 2.31 and seems to use stat64() to implement stat() with.

I wonder if @nickm or @ssouth have opinions about the x86-32 situatoin here with our Sandbox tests and stat(). I think the chown32() code above is fine, but I am of course open to suggestions :-)

I wonder if @nickm or @ssouth have opinions

Yes, I'm busy setting up a test environment and will have more to say shortly.

Some extra debug info. This is a Debian i386 running Debian 11.1 (bullseye).

The glibc version:

GNU C Library (Debian GLIBC 2.31-13+deb11u2) stable release version 2.31.
Copyright (C) 2020 Free Software Foundation, Inc.
This is free software; see the source for copying conditions.
There is NO warranty; not even for MERCHANTABILITY or FITNESS FOR A
PARTICULAR PURPOSE.
Compiled by GNU CC version 10.2.1 20210110.
libc ABIs: UNIQUE IFUNC ABSOLUTE
For bug reporting instructions, please see:
<http://www.debian.org/Bugs/>.

A small test C program to see what stat() turns into system-call wise:

#include <sys/stat.h>
#include <unistd.h>
#include <stdio.h>

int main(int argc, char *argv[]) {
	struct stat st;
	if (stat("foobar", &st))
		perror("stat");

	return 0;
}

Running it under strace:

root@c8b916e33e7d:~# gcc -Wall -O2 -o test test.c
root@c8b916e33e7d:~# strace ./test
execve("./test", ["./test"], 0xffc1e530 /* 8 vars */) = 0
brk(NULL)                               = 0x61665000
access("/etc/ld.so.nohwcap", F_OK)      = -1 ENOENT (No such file or directory)
mmap2(NULL, 8192, PROT_READ|PROT_WRITE, MAP_PRIVATE|MAP_ANONYMOUS, -1, 0) = 0xf7c45000
access("/etc/ld.so.preload", R_OK)      = -1 ENOENT (No such file or directory)
openat(AT_FDCWD, "/etc/ld.so.cache", O_RDONLY|O_LARGEFILE|O_CLOEXEC) = 3
fstat64(3, {st_mode=S_IFREG|0644, st_size=12849, ...}) = 0
mmap2(NULL, 12849, PROT_READ, MAP_PRIVATE, 3, 0) = 0xf7c41000
close(3)                                = 0
access("/etc/ld.so.nohwcap", F_OK)      = -1 ENOENT (No such file or directory)
openat(AT_FDCWD, "/lib/i386-linux-gnu/libc.so.6", O_RDONLY|O_LARGEFILE|O_CLOEXEC) = 3
read(3, "\177ELF\1\1\1\3\0\0\0\0\0\0\0\0\3\0\3\0\1\0\0\0\360\357\1\0004\0\0\0"..., 512) = 512
fstat64(3, {st_mode=S_IFREG|0755, st_size=1993968, ...}) = 0
mmap2(NULL, 2002876, PROT_READ, MAP_PRIVATE|MAP_DENYWRITE, 3, 0) = 0xf7a58000
mprotect(0xf7a75000, 1859584, PROT_NONE) = 0
mmap2(0xf7a75000, 1396736, PROT_READ|PROT_EXEC, MAP_PRIVATE|MAP_FIXED|MAP_DENYWRITE, 3, 0x1d000) = 0xf7a75000
mmap2(0xf7bca000, 458752, PROT_READ, MAP_PRIVATE|MAP_FIXED|MAP_DENYWRITE, 3, 0x172000) = 0xf7bca000
mmap2(0xf7c3b000, 16384, PROT_READ|PROT_WRITE, MAP_PRIVATE|MAP_FIXED|MAP_DENYWRITE, 3, 0x1e2000) = 0xf7c3b000
mmap2(0xf7c3f000, 8124, PROT_READ|PROT_WRITE, MAP_PRIVATE|MAP_FIXED|MAP_ANONYMOUS, -1, 0) = 0xf7c3f000
close(3)                                = 0
set_thread_area({entry_number=-1, base_addr=0xf7c46100, limit=0x0fffff, seg_32bit=1, contents=0, read_exec_only=0, limit_in_pages=1, seg_not_present=0, useable=1}) = 0 (entry_number=12)
mprotect(0xf7c3b000, 8192, PROT_READ)   = 0
mprotect(0x600f7000, 4096, PROT_READ)   = 0
mprotect(0xf7c75000, 4096, PROT_READ)   = 0
munmap(0xf7c41000, 12849)               = 0
stat64("foobar", {st_mode=S_IFREG|0644, st_size=0, ...}) = 0
exit_group(0)                           = ?
+++ exited with 0 +++

It does look like stat() from libc gets called as the system call stat64().

FWIW, I support disabling tests that don't work, if we open a ticket to make them pass again.

mentioned in commit ahf/tor@d83c6bf8

mentioned in merge request !480 (merged)

I've created !480 (merged) with the changes I'd like to propose to resolve this.

Alexander (@ahf), this borrows from the code you posted but changes it a bit to fit in better with the existing sandbox code and to work reliably on x86-64 as well. There's also an additional change, to allow the clock_gettime64 system call, required to get the entire test suite passing on x86.

These changes work for me on a virtualized i686 with glibc 2.31, and I'll re-test with glibc 2.33 once I have my test system updated.

Very nice work, @ssouth - I will give it a review tomorrow and hopefully merge it in.

Thank you for helping out with this.

This has been merged already. @weasel can you confirm this is no longer an issue for you? :-)

This has been merged already. @weasel can you confirm this is no longer an issue for you? :-)

If jenkins is to be believed, this issue was fixed/merged on or about Nov 8, and both jenkins and gitlab-ci builds appear to succeed at this time.

Cheers,

...

On Mon, 15 Nov 2021, Alexander Færøy (@ahf) wrote:

                        |  .''`.       ** Debian **
  Peter Palfrader       | : :' :      The  universal

https://www.palfrader.org/ | . ' Operating System | `- https://www.debian.org/

Perfect. Thank you.

Thanks for the help here, Simon!

closed

0.4.7.2-alpha FTBFS on i386

Child items ...

Activity

On Mon, 15 Nov 2021, Alexander Færøy (@ahf) wrote: