Skip to content

Commit f8ee2e3

Browse files
jmcphersjmcphers
committed
deal with bad SHELL values
1 parent 710f405 commit f8ee2e3

File tree

2 files changed

+199
-46
lines changed

2 files changed

+199
-46
lines changed

‎crates/kcserver/src/kernel_session.rs‎

Lines changed: 81 additions & 43 deletions
Original file line numberDiff line numberDiff line change
@@ -466,50 +466,81 @@ impl KernelSession {
466466
let run_in_shell = self.model.run_in_shell.unwrap_or(false);
467467

468468
// Create the command to start the kernel with the processed arguments
469-
let mut command = if run_in_shell {
469+
let shell_command = if run_in_shell {
470470
#[cfg(not(target_os = "windows"))]
471471
{
472-
// On Unix systems, use a login shell if requested
473-
// Get the shell from the environment, or use bash as fallback
474-
let shell_path =
475-
std::env::var("SHELL").unwrap_or_else(|_| String::from("/bin/bash"));
476-
477-
log::debug!(
478-
"[session {}] Running kernel in login shell: {}",
479-
self.model.session_id,
480-
shell_path
481-
);
472+
let candidates = vec![
473+
std::env::var("SHELL").unwrap_or_else(|_| String::from("")),
474+
String::from("/bin/bash"),
475+
String::from("/bin/sh"),
476+
];
477+
478+
let mut login_shell = None;
479+
for i in 0..candidates.len() {
480+
let shell_path = &candidates[i];
481+
// Ignore if empty (happens if SHELL is not set)
482+
if shell_path.is_empty() {
483+
continue;
484+
}
485+
// Found a valid shell path
486+
if fs::metadata(&shell_path).is_ok() {
487+
login_shell = Some(shell_path);
488+
break;
489+
} else if i == 0 {
490+
// The first candidate comes from $SHELL. If it doesn't exist,
491+
// log a warning but continue to try the others.
492+
log::warn!(
493+
"[session {}] Shell path specified in $SHELL '{}' does not exist",
494+
self.model.session_id,
495+
shell_path
496+
);
497+
}
498+
}
482499

483-
// Create the original command as a string with proper shell escaping
484-
let mut original_command = argv
485-
.iter()
486-
.map(|arg| escape_for_shell(arg))
487-
.collect::<Vec<_>>()
488-
.join(" ");
489-
490-
// On macOS, if the DYLD_LIBRARY_PATH environment variable was
491-
// requested, set it explictly; it is not inherited by default
492-
// in login shells due to SIP.
493-
if let Some(dyld_path) = resolved_env.get("DYLD_LIBRARY_PATH") {
500+
// If we found a login shell, use it to run the kernel
501+
if let Some(login_shell) = login_shell {
494502
log::debug!(
495-
"[session {}] Explicitly forwarding DYLD_LIBRARY_PATH: {}",
503+
"[session {}] Running kernel in login shell: {}",
496504
self.model.session_id,
497-
dyld_path
505+
login_shell
498506
);
499-
original_command = format!(
500-
"DYLD_LIBRARY_PATH={} {}",
501-
escape_for_shell(dyld_path),
502-
original_command
507+
// Create the original command as a string with proper shell escaping
508+
let mut original_command = argv
509+
.iter()
510+
.map(|arg| escape_for_shell(arg))
511+
.collect::<Vec<_>>()
512+
.join(" ");
513+
514+
// On macOS, if the DYLD_LIBRARY_PATH environment variable was
515+
// requested, set it explictly; it is not inherited by default
516+
// in login shells due to SIP.
517+
if let Some(dyld_path) = resolved_env.get("DYLD_LIBRARY_PATH") {
518+
log::debug!(
519+
"[session {}] Explicitly forwarding DYLD_LIBRARY_PATH: {}",
520+
self.model.session_id,
521+
dyld_path
522+
);
523+
original_command = format!(
524+
"DYLD_LIBRARY_PATH={} {}",
525+
escape_for_shell(dyld_path),
526+
original_command
527+
);
528+
}
529+
530+
// Create a command that uses the login shell. Note that we use
531+
// the short form -l rather than --login to ensure compatibility
532+
// with shells that don't support the long form, such as
533+
// sh/dash.
534+
let mut cmd = tokio::process::Command::new(login_shell);
535+
cmd.args(&["-l", "-c", &original_command]);
536+
Some(cmd)
537+
} else {
538+
log::warn!(
539+
"[session {}] No valid login shell found; running kernel without a login shell",
540+
self.model.session_id
503541
);
542+
None
504543
}
505-
506-
// Create a command that uses the login shell. Note that we use
507-
// the short form -l rather than --login to ensure compatibility
508-
// with shells that don't support the long form, such as
509-
// sh/dash.
510-
let mut cmd = tokio::process::Command::new(shell_path);
511-
cmd.args(&["-l", "-c", &original_command]);
512-
cmd
513544
}
514545

515546
#[cfg(target_os = "windows")]
@@ -519,15 +550,22 @@ impl KernelSession {
519550
"[session {}] run_in_shell parameter ignored on Windows",
520551
self.model.session_id
521552
);
553+
None
554+
}
555+
} else {
556+
None
557+
};
558+
559+
// If we formed a shell command, start the kernel with the shell
560+
// command; otherwise, start it with the original command line
561+
// arguments.
562+
let mut cmd = match shell_command {
563+
Some(command) => command,
564+
None => {
522565
let mut cmd = tokio::process::Command::new(&argv[0]);
523566
cmd.args(&argv[1..]);
524567
cmd
525568
}
526-
} else {
527-
// Normal execution - no login shell
528-
let mut cmd = tokio::process::Command::new(&argv[0]);
529-
cmd.args(&argv[1..]);
530-
cmd
531569
};
532570

533571
// If a working directory was specified, test the working directory to
@@ -537,7 +575,7 @@ impl KernelSession {
537575
match fs::metadata(&working_directory) {
538576
Ok(metadata) => {
539577
if metadata.is_dir() {
540-
command.current_dir(&working_directory);
578+
cmd.current_dir(&working_directory);
541579
log::trace!(
542580
"[session {}] Using working directory '{}'",
543581
self.model.session_id.clone(),
@@ -571,7 +609,7 @@ impl KernelSession {
571609
}
572610

573611
// Attempt to actually start the kernel process
574-
let mut child = match command
612+
let mut child = match cmd
575613
.envs(&resolved_env)
576614
.stdout(Stdio::piped())
577615
.stderr(Stdio::piped())

‎crates/kcserver/tests/python_kernel_tests.rs‎

Lines changed: 118 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -17,7 +17,8 @@ use common::test_utils::{
1717
};
1818
use common::transport::{run_communication_test, CommunicationChannel, TransportType};
1919
use common::TestServer;
20-
use kallichore_api::models::{InterruptMode, NewSession};
20+
use kallichore_api::models::{InterruptMode, NewSession, VarAction, VarActionType};
21+
use kallichore_api::NewSessionResponse;
2122
use kcshared::jupyter_message::{JupyterChannel, JupyterMessage, JupyterMessageHeader};
2223
use kcshared::websocket_message::WebsocketMessage;
2324
use std::time::Duration;
@@ -1152,8 +1153,8 @@ async fn test_kernel_session_restart_with_environment_changes() {
11521153
println!("Restarting kernel session with environment changes...");
11531154
let restart_session = kallichore_api::models::RestartSession {
11541155
working_directory: None,
1155-
env: Some(vec![kallichore_api::models::VarAction {
1156-
action: kallichore_api::models::VarActionType::Replace,
1156+
env: Some(vec![VarAction {
1157+
action: VarActionType::Replace,
11571158
name: "RESTART_TEST_VAR".to_string(),
11581159
value: "restart_value".to_string(),
11591160
}]),
@@ -1346,3 +1347,117 @@ async fn test_multiple_session_shutdown_restart_cycle() {
13461347
println!("Multiple session shutdown/restart cycle test completed successfully");
13471348
drop(server);
13481349
}
1350+
1351+
#[cfg(not(target_os = "windows"))] // Shell behavior is Unix-specific
1352+
#[tokio::test]
1353+
async fn test_kernel_starts_with_bad_shell_env_var() {
1354+
let test_result = tokio::time::timeout(Duration::from_secs(30), async {
1355+
let python_cmd = if let Some(cmd) = get_python_executable().await {
1356+
cmd
1357+
} else {
1358+
println!("Skipping test: No Python executable found");
1359+
return;
1360+
};
1361+
1362+
if !is_ipykernel_available().await {
1363+
println!("Skipping test: ipykernel not available for {}", python_cmd);
1364+
return;
1365+
}
1366+
1367+
// Start a test server
1368+
let server = TestServer::start().await;
1369+
let client = server.create_client().await;
1370+
1371+
// Generate a unique session ID
1372+
let session_id = format!("test-bad-shell-{}", Uuid::new_v4());
1373+
1374+
// Create a session with run_in_shell=true but set SHELL to a non-existent path
1375+
let new_session = NewSession {
1376+
session_id: session_id.clone(),
1377+
display_name: "Test Bad Shell Session".to_string(),
1378+
language: "python".to_string(),
1379+
username: "testuser".to_string(),
1380+
input_prompt: "In [{}]: ".to_string(),
1381+
continuation_prompt: " ...: ".to_string(),
1382+
argv: vec![
1383+
python_cmd.clone(),
1384+
"-m".to_string(),
1385+
"ipykernel".to_string(),
1386+
"-f".to_string(),
1387+
"{connection_file}".to_string(),
1388+
],
1389+
working_directory: std::env::current_dir()
1390+
.unwrap()
1391+
.to_string_lossy()
1392+
.to_string(),
1393+
env: vec![
1394+
// Set SHELL to a non-existent path to simulate the bad shell scenario
1395+
VarAction {
1396+
action: VarActionType::Replace,
1397+
name: "SHELL".to_string(),
1398+
value: "/non/existent/shell".to_string(),
1399+
}
1400+
],
1401+
connection_timeout: Some(15),
1402+
interrupt_mode: InterruptMode::Message,
1403+
protocol_version: Some("5.3".to_string()),
1404+
run_in_shell: Some(true), // This is the key - enable run_in_shell with bad SHELL
1405+
};
1406+
1407+
println!("Creating session with run_in_shell=true and bad SHELL env var");
1408+
1409+
// Create the session - this should succeed despite the bad SHELL value
1410+
let _created_session_id = create_session_with_client(&client, new_session).await;
1411+
1412+
println!("Session created successfully: {}", session_id);
1413+
1414+
// Start the session - this should succeed despite the bad SHELL environment variable
1415+
println!("Starting kernel session with bad SHELL environment variable...");
1416+
let start_response = client
1417+
.start_session(session_id.clone())
1418+
.await
1419+
.expect("Failed to start session with bad SHELL");
1420+
1421+
println!("Start response: {:?}", start_response);
1422+
1423+
// Check if the session started successfully
1424+
match &start_response {
1425+
kallichore_api::StartSessionResponse::Started(_) => {
1426+
println!("Kernel started successfully despite bad SHELL environment variable");
1427+
}
1428+
kallichore_api::StartSessionResponse::StartFailed(error) => {
1429+
panic!("Kernel failed to start with bad SHELL: {:?}", error);
1430+
}
1431+
_ => {
1432+
panic!("Unexpected start response: {:?}", start_response);
1433+
}
1434+
}
1435+
1436+
// Wait for kernel to fully start
1437+
tokio::time::sleep(Duration::from_millis(2000)).await;
1438+
1439+
// Verify the session is actually running by checking its status
1440+
let sessions = client
1441+
.list_sessions()
1442+
.await
1443+
.expect("Failed to get sessions list");
1444+
1445+
let kallichore_api::ListSessionsResponse::ListOfActiveSessions(session_list) = sessions;
1446+
let session_found = session_list.sessions.iter().any(|s| s.session_id == session_id);
1447+
assert!(session_found, "Session should be in the active sessions list");
1448+
1449+
println!("Test passed: Kernel started successfully despite bad SHELL environment variable");
1450+
1451+
drop(server);
1452+
})
1453+
.await;
1454+
1455+
match test_result {
1456+
Ok(_) => {
1457+
println!("Bad shell environment variable test completed successfully");
1458+
}
1459+
Err(_) => {
1460+
panic!("Bad shell environment variable test timed out after 30 seconds");
1461+
}
1462+
}
1463+
}

0 commit comments

Comments
 (0)