Bug#1043201: slirp: immediate exit at startup

[Ferenc Wágner]

Package: slirp
Severity: important
Tags: patch upstream

Dear Maintainer,

When trying to use slirp with user-mode-linux (eth0=slirp) under Debian
bookworm (amd64), I noticed that as soon as I up the virtual interface,
slirp prints its usual banner and immediately exits with status 1. The
slirp process started by UML becomes a zombie and there's no network
connectivity. Some debugging led me to believe that the cause is
signedness difference at src/main.c:955 in the

if (timeout.tv_usec == -1) { ... }

comparison, which makes the condition always false, thus reaching the
select() call with timeout.tv_usec (of unsiged type) set to 0xffffffff
(that is -1 converted to unsigned) and thus hitting EINVAL. Adding an
explicit cast via the below patch helped. This problem might affect
64-bit architectures only; I tested on amd64. Please consider fixing
this one way or another.

Thanks,
Feri.

$ cat slirp-1.0.17/debian/patches/018-tv_usec-is-unsigned.patch
Index: slirp-1.0.17/src/main.c
===================================================================
--- slirp-1.0.17.orig/src/main.c 2023-08-07 11:49:03.000000000 +0200
+++ slirp-1.0.17/src/main.c 2023-08-07 11:57:06.850518113 +0200
@@ -859,7 +859,7 @@
* First, see the timeout needed by *timo
*/
timeout.tv_sec = 0;
- timeout.tv_usec = -1;
+ timeout.tv_usec = (unsigned)-1;
/*
* If a slowtimo is needed, set timeout to 500ms from the last
* slow timeout. If a fast timeout is needed, set timeout within
@@ -952,7 +952,7 @@
* This will make timings (like idle timer and "wait" timer)
* up to 10 seconds late, but will be more system friendly
*/
- if (timeout.tv_usec == -1) {
+ if (timeout.tv_usec == (unsigned)-1) {
timeout.tv_usec = 0;
timeout.tv_sec = 5; /* XXX */
}

[Roberto Lumbreras]

Hi Ferenc,

timeout is defined as "struct timeval":

       struct timeval {
           time_t       tv_sec;   /* Seconds */
           suseconds_t  tv_usec;  /* Microseconds */
       };

timeout.tv_used is suseconds_t == time_t == int, so I can't understand why it's unsigned in your case.

If you dig in src/main.c there are more lines with timeout.tv_used being compared with negative numbers, your patch may work but I think it has to be something else.

Have you tried just compiling the slirp package? Maybe it's a problem with libc6 linking, https://tracker.debian.org/pkg/slirp warns that slirp "Fails to build during reproducibility testing". I'm looking into this.

--

Regards,

Roberto Lumbreras

Debian developer

[Ferenc Wágner]

Roberto Lumbreras <ro...@debian.org> writes:

> timeout is defined as "struct timeval":
>
> struct timeval {
> time_t tv_sec; /* Seconds */
> suseconds_t tv_usec; /* Microseconds */
> };
>
> timeout.tv_used is suseconds_t == time_t == int, so I can't understand
> why it's unsigned in your case.

Hi Roberto,

I probably missed the starting 's', suseconds_t is indeed documented as
signed, and is actually a long int here. So my patch shouldn't work.
What's sure:

# gdb slirp
GNU gdb (Debian 13.1-3) 13.1
Copyright (C) 2023 Free Software Foundation, Inc.
License GPLv3+: GNU GPL version 3 or later <http://gnu.org/licenses/gpl.html>
This is free software: you are free to change and redistribute it.
There is NO WARRANTY, to the extent permitted by law.
Type "show copying" and "show warranty" for details.
This GDB was configured as "x86_64-linux-gnu".
Type "show configuration" for configuration details.
For bug reporting instructions, please see:
<https://www.gnu.org/software/gdb/bugs/>.
Find the GDB manual and other documentation resources online at:
<http://www.gnu.org/software/gdb/documentation/>.

For help, type "help".
Type "apropos word" to search for commands related to "word"...
Reading symbols from slirp...
Reading symbols from /usr/lib/debug/.build-id/e1/1ce5371d887d06c3b97a0c6a68463ba1cc8bcf.debug...
(gdb) b main.c:960
Breakpoint 1 at 0x1408b: file ./main.c, line 960.
(gdb) r
Starting program: /usr/bin/slirp
warning: Error disabling address space randomization: Operation not permitted
[Thread debugging using libthread_db enabled]
Using host libthread_db library "/lib/x86_64-linux-gnu/libthread_db.so.1".
Slirp v1.0.17 (BETA)

Copyright (c) 1995,1996 Danny Gasparovski and others.
All rights reserved.
This program is copyrighted, free software.
Please read the file COPYRIGHT that came with the Slirp
package for the terms and conditions of the copyright.

IP address of Slirp host: 172.17.0.2
IP address of your DNS(s): 10.12.1.5, 10.10.1.10
Your address is 10.0.2.15
(or anything else you want)

Type five zeroes (0) to exit.

[autodetect SLIP/CSLIP, MTU 1500, MRU 1500, 115200 baud]

SLiRP Ready ...

Breakpoint 1, main_loop () at ./main.c:960
warning: Source file is more recent than executable.
960 ret = select(nfds+1, &readfds, &writefds, &xfds, &timeout);
(gdb) p timeout
$1 = {tv_sec = 0, tv_usec = 4294967295}
(gdb) p/x timeout.tv_usec
$2 = 0xffffffff
(gdb) p nfds
$3 = 0
(gdb) n
962 if (ret < 0) {
(gdb) p ret
$4 = -1
(gdb) p errno
$5 = 22
(gdb) n
963 if (errno == EINTR)
(gdb)
965 slirp_exit(1);
(gdb)
[Inferior 1 (process 4349) exited with code 01]

> If you dig in src/main.c there are more lines with timeout.tv_used
> being compared with negative numbers, your patch may work but I think
> it has to be something else.

Yes, I've got another theory now: timeout.tv_usec is a long int of
8 bytes size, but at main.c:935 it gets assigned a 4-byte unsigned:

if ((timeout.tv_usec < 0) || (tmp_time >= 0 && tmp_time < timeout.tv_usec))
timeout.tv_usec = (u_int)tmp_time;

This results in the value of 4294967295 above instead of the expected
-1, so timeout does not get reset to {5, 0} and select() gets the
invalid timeout value, immediately breaking the loop.

> Have you tried just compiling the slirp package? Maybe it's a problem
> with libc6 linking, https://tracker.debian.org/pkg/slirp warns that
> slirp "Fails to build during reproducibility testing". I'm looking
> into this.

Reproducibility probably does not factor into this, but I'll do some
more checks as I get to it.
--
Regards,
Feri.

[Ferenc Wágner]

Hi,

I finally managed to get back to this and arrived at the patch bundle
below. The first patch fixes the actual problem for me, the second one
aligns the corresponding debug logs (which aren't enabled in the current
build, but were useful during debugging), and the third one fixes a
compilation error with debugging enabled. The explanation is in the
commit message. I didn't test on 32-bit architectures, only on 64-bit
under UML, but briefly contemplated adding an autopkgtest. Nothing too
involved, because I doubt slirp sees wide use, but we can talk about
this later if you're interested.

Please review and consider incorporating the fix, or I can do an NMU if
you prefer that.

Thanks,
Feri.

From 247d6e3563512cf41d8af279a8be23d22699f80d Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Ferenc=20W=C3=A1gner?= <wf...@debian.org>
Date: Fri, 20 Oct 2023 21:05:31 +0200
Subject: [PATCH 1/3] Do not convert tmp_time to unsiged before assigning to
tv_usec

When tmp_time is set to the sentinel value -1 that conversion results
2^32-1, which is out of range for suseconds_t on 32-bit platforms, so
the assignment invokes undefined behaviour (which apparently happened
to give -1, working good enough for the task by chance). However, on
64-bit platforms 2^32-1 fits in the range of suseconds_t (long int)
and definitely does not equal -1 in the following check, leading to
EINVAL when passed into the select() call and immediately exiting
slirp on startup.
---
src/main.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/main.c b/src/main.c
index 6e15a74..1c01732 100644
--- a/src/main.c
+++ b/src/main.c
@@ -933,7 +933,7 @@ cont_1:
* Take the minimum of the above calculated timeouts
*/

if ((timeout.tv_usec < 0) || (tmp_time >= 0 && tmp_time < timeout.tv_usec))

- timeout.tv_usec = (u_int)tmp_time;
+ timeout.tv_usec = tmp_time;
#endif
DEBUG_MISC((dfd, " timeout.tv_usec = %u",
(u_int)timeout.tv_usec));
--
2.39.2


From d20acd8f16cc884611bcf3fd8ad5665876a22f0a Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Ferenc=20W=C3=A1gner?= <wf...@debian.org>
Date: Fri, 20 Oct 2023 21:22:46 +0200
Subject: [PATCH 2/3] Log tv_usec as the signed long it is

---
src/main.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/src/main.c b/src/main.c
index 1c01732..6384810 100644
--- a/src/main.c
+++ b/src/main.c
@@ -935,8 +935,8 @@ cont_1:

if ((timeout.tv_usec < 0) || (tmp_time >= 0 && tmp_time < timeout.tv_usec))

timeout.tv_usec = tmp_time;
#endif
- DEBUG_MISC((dfd, " timeout.tv_usec = %u",
- (u_int)timeout.tv_usec));
+ DEBUG_MISC((dfd, " timeout.tv_usec = %ld",
+ (long)timeout.tv_usec));
if (time_fasttimo) {
DEBUG_MISC((dfd, ", need fasttimo\n"));
} else {
--
2.39.2


From ed538e8fae21a6f802e412b5b38a19944ac5d631 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Ferenc=20W=C3=A1gner?= <wf...@debian.org>
Date: Sat, 21 Oct 2023 17:06:00 +0200
Subject: [PATCH 3/3] Add missing FILE argument to DEBUG_ERROR macro invocation

---
src/misc.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/misc.c b/src/misc.c
index e2849b8..dc6e593 100644
--- a/src/misc.c
+++ b/src/misc.c
@@ -988,7 +988,7 @@ static int slirp_vsnprintf(char *str, size_t size,
int rv = vsnprintf(str, size, format, args);

if (rv < 0) {
- DEBUG_ERROR(("vsnprintf() failed: %s", strerror(errno)));
+ DEBUG_ERROR((dfd, "vsnprintf() failed: %s", strerror(errno)));
}

return rv;
--
2.39.2