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

Message ID CAJDtP-S45N-_fbsFtiMAKZH+=XzzrLTRNuNxN8qvJkJ79OZfDA@mail.gmail.com
State New
Headers show
Series
  • [binutils/testsuite] debuginfod.exp: Improve port selection
Related show

Commit Message

Aaron Merey Feb. 13, 2020, 4:22 a.m.
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.

Comments

Andreas Schwab Feb. 13, 2020, 8:42 a.m. | #1
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."
Aaron Merey Feb. 13, 2020, 3:26 p.m. | #2
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
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}
Nick Clifton March 2, 2020, 12:48 p.m. | #3
Hi Aaron,

> Thanks Andreas, I've updated the patch.


Patch approved and applied.

Cheers
  Nick

Patch

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