Merge pull request #990 from rust-lang/report-segfault

Author: shepmaster

compiler/base/orchestrator/src/coordinator.rs

@@ -28,9 +28,9 @@ use tracing::{instrument, trace, trace_span, warn, Instrument};
 use crate::{
     bincode_input_closed,
     message::{
-        CoordinatorMessage, DeleteFileRequest, ExecuteCommandRequest, JobId, Multiplexed,
-        OneToOneResponse, ReadFileRequest, ReadFileResponse, SerializedError, WorkerMessage,
-        WriteFileRequest,
+        CoordinatorMessage, DeleteFileRequest, ExecuteCommandRequest, ExecuteCommandResponse,
+        JobId, Multiplexed, OneToOneResponse, ReadFileRequest, ReadFileResponse, SerializedError,
+        WorkerMessage, WriteFileRequest,
     },
     DropErrorDetailsExt,
 };
@@ -233,6 +233,7 @@ impl CargoTomlModifier for ExecuteRequest {
 #[derive(Debug, Clone)]
 pub struct ExecuteResponse {
     pub success: bool,
+    pub exit_detail: String,
 }
 
 #[derive(Debug, Clone)]
@@ -338,6 +339,7 @@ impl CargoTomlModifier for CompileRequest {
 #[derive(Debug, Clone)]
 pub struct CompileResponse {
     pub success: bool,
+    pub exit_detail: String,
     pub code: String,
 }
 
@@ -627,11 +629,17 @@ impl Container {
             .context(CouldNotStartCargoSnafu)?;
 
         let task = async move {
-            let success = task
+            let ExecuteCommandResponse {
+                success,
+                exit_detail,
+            } = task
                 .await
                 .context(CargoTaskPanickedSnafu)?
                 .context(CargoFailedSnafu)?;
-            Ok(ExecuteResponse { success })
+            Ok(ExecuteResponse {
+                success,
+                exit_detail,
+            })
         }
         .boxed();
 
@@ -693,7 +701,10 @@ impl Container {
 
         let commander = self.commander.clone();
         let task = async move {
-            let success = task
+            let ExecuteCommandResponse {
+                success,
+                exit_detail,
+            } = task
                 .await
                 .context(CargoTaskPanickedSnafu)?
                 .context(CargoFailedSnafu)?;
@@ -711,7 +722,11 @@ impl Container {
             // TODO: This is synchronous...
             let code = request.postprocess_result(code);
 
-            Ok(CompileResponse { success, code })
+            Ok(CompileResponse {
+                success,
+                exit_detail,
+                code,
+            })
         }
         .boxed();
 
@@ -744,7 +759,7 @@ impl Container {
 
                     match container_msg {
                         WorkerMessage::ExecuteCommand(resp) => {
-                            return Ok(resp.success);
+                            return Ok(resp);
                         }
                         WorkerMessage::StdoutPacket(packet) => {
                             stdout_tx.send(packet).await.ok(/* Receiver gone, that's OK */);
@@ -869,7 +884,7 @@ pub enum CompileError {
 }
 
 struct SpawnCargo {
-    task: JoinHandle<Result<bool, SpawnCargoError>>,
+    task: JoinHandle<Result<ExecuteCommandResponse, SpawnCargoError>>,
     stdout_rx: mpsc::Receiver<String>,
     stderr_rx: mpsc::Receiver<String>,
 }
@@ -2060,6 +2075,31 @@ mod tests {
         Ok(())
     }
 
+    #[tokio::test]
+    #[snafu::report]
+    async fn exit_due_to_signal_is_reported() -> Result<()> {
+        let coordinator = new_coordinator().await;
+
+        let req = ExecuteRequest {
+            channel: Channel::Stable,
+            mode: Mode::Release,
+            edition: Edition::Rust2021,
+            crate_type: CrateType::Binary,
+            tests: false,
+            backtrace: false,
+            code: r#"fn main() { std::process::abort(); }"#.into(),
+        };
+
+        let res = coordinator.execute(req.clone()).await.unwrap();
+
+        assert!(!res.success);
+        assert_contains!(res.exit_detail, "abort");
+
+        coordinator.shutdown().await?;
+
+        Ok(())
+    }
+
     fn new_execution_limited_request() -> ExecuteRequest {
         ExecuteRequest {
             channel: Channel::Stable,

compiler/base/orchestrator/src/message.rs

@@ -117,6 +117,7 @@ pub struct ExecuteCommandRequest {
 #[derive(Debug, Serialize, Deserialize)]
 pub struct ExecuteCommandResponse {
     pub success: bool,
+    pub exit_detail: String,
 }
 
 #[derive(Debug, Serialize, Deserialize)]

compiler/base/orchestrator/src/worker.rs

@@ -36,7 +36,7 @@ use std::{
     collections::HashMap,
     io,
     path::{Path, PathBuf},
-    process::Stdio,
+    process::{ExitStatus, Stdio},
 };
 use tokio::{
     fs,
@@ -509,7 +509,111 @@ async fn process_end(
     }
 
     let success = status.success();
-    Ok(ExecuteCommandResponse { success })
+    let exit_detail = extract_exit_detail(status);
+
+    Ok(ExecuteCommandResponse {
+        success,
+        exit_detail,
+    })
+}
+
+mod signals {
+    mod descriptions {
+        #![allow(dead_code)]
+
+        pub const SIGABRT: &str = "abort program";
+        pub const SIGALRM: &str = "real-time timer expired";
+        pub const SIGBUS: &str = "bus error";
+        pub const SIGEMT: &str = "emulate instruction executed";
+        pub const SIGFPE: &str = "floating-point exception";
+        pub const SIGHUP: &str = "terminal line hangup";
+        pub const SIGILL: &str = "illegal instruction";
+        pub const SIGINT: &str = "interrupt program";
+        pub const SIGKILL: &str = "kill program";
+        pub const SIGPIPE: &str = "write on a pipe with no reader";
+        pub const SIGQUIT: &str = "quit program";
+        pub const SIGSEGV: &str = "segmentation violation";
+        pub const SIGSYS: &str = "non-existent system call invoked";
+        pub const SIGTERM: &str = "software termination signal";
+        pub const SIGTRAP: &str = "trace trap";
+        pub const SIGUSR1: &str = "user-defined signal 1";
+        pub const SIGUSR2: &str = "user-defined signal 2";
+    }
+
+    type Pair = (&'static str, &'static str);
+
+    macro_rules! sigtable {
+        [$($name:ident,)*] => {
+            [
+                $((stringify!($name), descriptions::$name),)*
+            ]
+        };
+    }
+
+    #[cfg(target_os = "macos")]
+    const SIGNALS: [Pair; 15] = sigtable![
+        SIGHUP,  //  1
+        SIGINT,  //  2
+        SIGQUIT, //  3
+        SIGILL,  //  4
+        SIGTRAP, //  5
+        SIGABRT, //  6
+        SIGEMT,  //  7
+        SIGFPE,  //  8
+        SIGKILL, //  9
+        SIGBUS,  // 10
+        SIGSEGV, // 11
+        SIGSYS,  // 12
+        SIGPIPE, // 13
+        SIGALRM, // 14
+        SIGTERM, // 15
+    ];
+
+    #[cfg(target_os = "linux")]
+    const SIGNALS: [Pair; 15] = sigtable![
+        SIGHUP,  //  1
+        SIGINT,  //  2
+        SIGQUIT, //  3
+        SIGILL,  //  4
+        SIGTRAP, //  5
+        SIGABRT, //  6
+        SIGBUS,  //  7
+        SIGFPE,  //  8
+        SIGKILL, //  9
+        SIGUSR1, // 10
+        SIGSEGV, // 11
+        SIGUSR2, // 12
+        SIGPIPE, // 13
+        SIGALRM, // 14
+        SIGTERM, // 15
+    ];
+
+    const SIG_UNKNOWN: Pair = ("???", "Unknown signal");
+
+    pub fn get(signal: i32) -> Pair {
+        let details = (|| {
+            let signal = usize::try_from(signal).ok()?;
+            let signal = signal.checked_sub(1)?;
+            SIGNALS.get(signal).copied()
+        })();
+
+        details.unwrap_or(SIG_UNKNOWN)
+    }
+}
+
+fn extract_exit_detail(status: ExitStatus) -> String {
+    use std::os::unix::process::ExitStatusExt;
+
+    if let Some(code) = status.code() {
+        return format!("Exited with status {code}");
+    }
+
+    if let Some(signal) = status.signal() {
+        let (name, description) = signals::get(signal);
+        return format!("Exited with signal {signal} ({name}): {description}");
+    }
+
+    String::new()
 }
 
 #[derive(Debug, Snafu)]

tests/Gemfile.lock

@@ -20,7 +20,7 @@ GEM
       addressable (~> 2.8)
     matrix (0.4.2)
     mini_mime (1.1.5)
-    mini_portile2 (2.8.4)
+    mini_portile2 (2.8.5)
     nokogiri (1.15.4)
       mini_portile2 (~> 2.8.2)
       racc (~> 1.4)
@@ -29,7 +29,7 @@ GEM
     rack (3.0.8)
     rack-test (2.1.0)
       rack (>= 1.3)
-    regexp_parser (2.8.1)
+    regexp_parser (2.8.2)
     rexml (3.2.6)
     rspec (3.12.0)
       rspec-core (~> 3.12.0)
@@ -45,11 +45,11 @@ GEM
       rspec-support (~> 3.12.0)
     rspec-support (3.12.1)
     rubyzip (2.3.2)
-    selenium-webdriver (4.12.0)
+    selenium-webdriver (4.14.0)
       rexml (~> 3.2, >= 3.2.5)
       rubyzip (>= 1.2.2, < 3.0)
       websocket (~> 1.0)
-    websocket (1.2.9)
+    websocket (1.2.10)
     xpath (3.2.0)
       nokogiri (~> 1.8)
 

tests/spec/features/segfault_spec.rb

@@ -0,0 +1,29 @@
+require 'spec_helper'
+require 'support/editor'
+require 'support/playground_actions'
+
+RSpec.feature "When the code triggers a signal", type: :feature, js: true do
+  include PlaygroundActions
+
+  before { visit '/' }
+
+  scenario "when a crate type is provided" do
+    code = <<~EOF
+      fn main() {
+          unsafe { *(0xDECAF_000 as *mut usize) = 1 };
+      }
+    EOF
+    editor.set(code)
+
+    click_on("Run")
+    within(:output, :error) do
+      expect(page).to have_content "signal 11"
+      expect(page).to have_content "SIGSEGV"
+      expect(page).to have_content "segmentation violation"
+    end
+  end
+
+  def editor
+    Editor.new(page)
+  end
+end

ui/frontend/reducers/output/execute.ts

@@ -51,6 +51,7 @@ const { action: wsExecuteEnd, schema: wsExecuteEndSchema } = createWebsocketResp
   'output/execute/wsExecuteEnd',
   z.object({
     success: z.boolean(),
+    exitDetail: z.string(),
   }),
 );
 
@@ -68,6 +69,7 @@ export interface ExecuteRequestBody {
 
 interface ExecuteResponseBody {
   success: boolean;
+  exitDetail: string;
   stdout: string;
   stderr: string;
 }
@@ -113,8 +115,12 @@ const slice = createSlice({
         state.requestsInProgress += 1;
       })
       .addCase(performExecute.fulfilled, (state, action) => {
-        const { stdout, stderr } = action.payload;
+        const { success, exitDetail, stdout, stderr } = action.payload;
         Object.assign(state, { stdout, stderr });
+        delete state.error;
+        if (!success) {
+          state.error = exitDetail;
+        }
         state.requestsInProgress -= 1;
       })
       .addCase(performExecute.rejected, (state, action) => {
@@ -130,6 +136,7 @@ const slice = createSlice({
           state.requestsInProgress = 1; // Only tracking one request
           state.stdout = '';
           state.stderr = '';
+          delete state.error;
         }),
       )
       .addCase(
@@ -146,8 +153,12 @@ const slice = createSlice({
       )
       .addCase(
         wsExecuteEnd,
-        sequenceNumberMatches((state) => {
+        sequenceNumberMatches((state, payload) => {
           state.requestsInProgress = 0; // Only tracking one request
+
+          if (!payload.success) {
+            state.error = payload.exitDetail;
+          }
         }),
       );
   },

ui/src/main.rs

@@ -284,6 +284,8 @@ struct CompileRequest {
 #[derive(Debug, Clone, Serialize)]
 struct CompileResponse {
     success: bool,
+    #[serde(rename = "exitDetail")]
+    exit_detail: String,
     code: String,
     stdout: String,
     stderr: String,
@@ -306,6 +308,8 @@ struct ExecuteRequest {
 #[derive(Debug, Clone, Serialize)]
 struct ExecuteResponse {
     success: bool,
+    #[serde(rename = "exitDetail")]
+    exit_detail: String,
     stdout: String,
     stderr: String,
 }