[PATCH] [binutils/testsuite] debuginfod.exp: Improve port selection

[PATCH] [binutils/testsuite] debuginfod.exp: Improve port selection

[Aaron Merey]

[-- Attachment #1: Type: text/plain, Size: 328 bytes --]

The procedure to find an unused port is susceptible to a TOCTOU failure.
Change port finding in order to avoid this. Also use 'expect' to interact
with the server process since we now use the server's output to determine
whether a port is in use.

* binutils/testsuite/binutils-all/debuginfod.exp: Improve port selection.

[-- Attachment #2: binutils-testsuite-debuginfod.exp-Improve-port-selec.diff --]
[-- Type: text/x-patch, Size: 2603 bytes --]

diff --git a/binutils/testsuite/binutils-all/debuginfod.exp b/binutils/testsuite/binutils-all/debuginfod.exp
index 7e1c6380a5..bb22a98a1e 100644
--- a/binutils/testsuite/binutils-all/debuginfod.exp
+++ b/binutils/testsuite/binutils-all/debuginfod.exp
@@ -72,9 +72,6 @@ if { ![binutils_assemble $srcdir/$subdir/linkdebug.s tmpdir/linkdebug.debug] } {
     fail "$test (assemble linkdebug)"
 }
 
-# Find an unused port
-set port [exec sh -c "while true; do PORT=`expr '(' \$RANDOM % 1000 ')' + 9000`; ss -atn | fgrep \":\$PORT\" || break; done; echo \$PORT"]
-
 set cache [file join [pwd] "tmpdir/.debuginfod_cache"]
 set db [file join [pwd] "tmpdir/.debuginfod.db"]
 
@@ -99,34 +96,36 @@ file delete -force $db
 set conf_objdump [binutils_run $OBJDUMP "-WK tmpdir/testprog"]
 set conf_readelf [binutils_run $READELF "-wK tmpdir/testprog"]
 
-set debuginfod_pid 0
-
-# Kill the server if we abort early
-proc sigint_handler {} {
-    global debuginfod_pid
+# Find an unused port
+set port 7999
+set found 0
+while { ! $found } {
+  incr port
+  if { $port == 65536 } {
+    fail "$test (no available ports)"
+    return
+  }
 
-    if { $debuginfod_pid != 0 } {
-        catch {exec kill -INT $debuginfod_pid}
+  spawn debuginfod -vvvv -d $db -p $port -F tmpdir/dbg
+  expect {
+    "started http server on IPv4 IPv6 port=$port" {
+      set found 1
     }
-
-    exit
-}
-
-trap sigint_handler INT
-
-# Start a debuginfod server.
-set debuginfod_pid [exec debuginfod -d $db -p $port -F tmpdir/dbg 2>/dev/null &]
-
-if { !$debuginfod_pid } {
-    fail "$test (server init)"
-    return
+    "Failed to bind to port" {
+      exec kill -INT -[exp_pid]
+      catch {close}; catch {wait -i $spawn_id}
+    }
+    timeout {
+      fail "$test (find port timeout)"
+      return
+    }
+  }
 }
 
 set metrics [list "ready 1" \
              "thread_work_total{role=\"traverse\"} 1" \
              "thread_work_pending{role=\"scan\"} 0" \
-             "thread_busy{role=\"scan\"} 0" \
-             "groom{statistic=\"buildids\"} 2"]
+             "thread_busy{role=\"scan\"} 0"]
 
 # Check server metrics to confirm init has completed.
 foreach m $metrics {
@@ -145,7 +144,8 @@ foreach m $metrics {
 
   if { $timelim == 0 } {
     fail "$test (server init timeout)"
-    catch {exec kill -INT $debuginfod_pid}
+    exec kill -INT -[exp_pid]
+    catch {close}; catch {wait -i $spawn_id}
     return
   }
 }
@@ -196,5 +196,3 @@ if { [regexp ".*DEBUGINFOD.*" $conf_readelf] } {
 } else {
     untested "$test (readelf not configured with debuginfod)"
 }
-
-catch {exec kill -INT $debuginfod_pid}
-- 
2.24.1

Re: [PATCH] [binutils/testsuite] debuginfod.exp: Improve port selection

[Andreas Schwab]

On Feb 12 2020, Aaron Merey wrote:

> @@ -99,34 +96,36 @@ file delete -force $db
>  set conf_objdump [binutils_run $OBJDUMP "-WK tmpdir/testprog"]
>  set conf_readelf [binutils_run $READELF "-wK tmpdir/testprog"]
>  
> -set debuginfod_pid 0
> -
> -# Kill the server if we abort early
> -proc sigint_handler {} {
> -    global debuginfod_pid
> +# Find an unused port
> +set port 7999
> +set found 0
> +while { ! $found } {
> +  incr port
> +  if { $port == 65536 } {
> +    fail "$test (no available ports)"

That should be unsupported or untested, not fail.

Andreas.

-- 
Andreas Schwab, SUSE Labs, schwab@suse.de
GPG Key fingerprint = 0196 BAD8 1CE9 1970 F4BE  1748 E4D4 88E3 0EEA B9D7
"And now for something completely different."

Re: [PATCH] [binutils/testsuite] debuginfod.exp: Improve port selection

[Aaron Merey]

[-- Attachment #1: Type: text/plain, Size: 367 bytes --]

On Thu, Feb 13, 2020 at 3:42 AM Andreas Schwab <schwab@suse.de> wrote:
> > +# Find an unused port
> > +set port 7999
> > +set found 0
> > +while { ! $found } {
> > +  incr port
> > +  if { $port == 65536 } {
> > +    fail "$test (no available ports)"
>
> That should be unsupported or untested, not fail.

Thanks Andreas, I've updated the patch.

Aaron

[-- Attachment #2: 0001-binutils-testsuite-debuginfod.exp-Improve-port-selec.patch --]
[-- Type: text/x-patch, Size: 3282 bytes --]

From 70266b489c43c3f7c64c477be1c997eb331d2922 Mon Sep 17 00:00:00 2001
From: Aaron Merey <amerey@redhat.com>
Date: Thu, 13 Feb 2020 10:21:50 -0500
Subject: [PATCH] [binutils/testsuite] debuginfod.exp: Improve port selection

The procedure to find an unused port is susceptible to a TOCTOU failure.
Change port finding in order to avoid this. Also use 'expect' to interact
with the server process since we now use the server's output to determine
whether a port is in use.

* binutils/testsuite/binutils-all/debuginfod.exp: Improve port selection.
---
 .../testsuite/binutils-all/debuginfod.exp     | 52 +++++++++----------
 1 file changed, 25 insertions(+), 27 deletions(-)

diff --git a/binutils/testsuite/binutils-all/debuginfod.exp b/binutils/testsuite/binutils-all/debuginfod.exp
index 7e1c6380a5..f057ad0155 100644
--- a/binutils/testsuite/binutils-all/debuginfod.exp
+++ b/binutils/testsuite/binutils-all/debuginfod.exp
@@ -72,9 +72,6 @@ if { ![binutils_assemble $srcdir/$subdir/linkdebug.s tmpdir/linkdebug.debug] } {
     fail "$test (assemble linkdebug)"
 }
 
-# Find an unused port
-set port [exec sh -c "while true; do PORT=`expr '(' \$RANDOM % 1000 ')' + 9000`; ss -atn | fgrep \":\$PORT\" || break; done; echo \$PORT"]
-
 set cache [file join [pwd] "tmpdir/.debuginfod_cache"]
 set db [file join [pwd] "tmpdir/.debuginfod.db"]
 
@@ -99,34 +96,36 @@ file delete -force $db
 set conf_objdump [binutils_run $OBJDUMP "-WK tmpdir/testprog"]
 set conf_readelf [binutils_run $READELF "-wK tmpdir/testprog"]
 
-set debuginfod_pid 0
-
-# Kill the server if we abort early
-proc sigint_handler {} {
-    global debuginfod_pid
+# Find an unused port
+set port 7999
+set found 0
+while { ! $found } {
+  incr port
+  if { $port == 65536 } {
+    untested "$test (no available ports)"
+    return
+  }
 
-    if { $debuginfod_pid != 0 } {
-        catch {exec kill -INT $debuginfod_pid}
+  spawn debuginfod -vvvv -d $db -p $port -F tmpdir/dbg
+  expect {
+    "started http server on IPv4 IPv6 port=$port" {
+      set found 1
     }
-
-    exit
-}
-
-trap sigint_handler INT
-
-# Start a debuginfod server.
-set debuginfod_pid [exec debuginfod -d $db -p $port -F tmpdir/dbg 2>/dev/null &]
-
-if { !$debuginfod_pid } {
-    fail "$test (server init)"
-    return
+    "Failed to bind to port" {
+      exec kill -INT -[exp_pid]
+      catch {close}; catch {wait -i $spawn_id}
+    }
+    timeout {
+      fail "$test (find port timeout)"
+      return
+    }
+  }
 }
 
 set metrics [list "ready 1" \
              "thread_work_total{role=\"traverse\"} 1" \
              "thread_work_pending{role=\"scan\"} 0" \
-             "thread_busy{role=\"scan\"} 0" \
-             "groom{statistic=\"buildids\"} 2"]
+             "thread_busy{role=\"scan\"} 0"]
 
 # Check server metrics to confirm init has completed.
 foreach m $metrics {
@@ -145,7 +144,8 @@ foreach m $metrics {
 
   if { $timelim == 0 } {
     fail "$test (server init timeout)"
-    catch {exec kill -INT $debuginfod_pid}
+    exec kill -INT -[exp_pid]
+    catch {close}; catch {wait -i $spawn_id}
     return
   }
 }
@@ -196,5 +196,3 @@ if { [regexp ".*DEBUGINFOD.*" $conf_readelf] } {
 } else {
     untested "$test (readelf not configured with debuginfod)"
 }
-
-catch {exec kill -INT $debuginfod_pid}
-- 
2.24.1

Re: [PATCH] [binutils/testsuite] debuginfod.exp: Improve port selection

[Nick Clifton]

Hi Aaron,

> Thanks Andreas, I've updated the patch.

Patch approved and applied.

Cheers
  Nick