Skip to content

Conversation

itislu
Copy link
Collaborator

@itislu itislu commented May 28, 2025

Test case:

mkdir -p /tmp/dir && cd /tmp/dir && rm -rf ../dir
<absolute path to minishell>
  • Fix leak:
    image
  • Don't exit (follows the behavior of bash).
  • Usually, bash always sets the PWD env variable itself and overwrites any exported value on startup. However, if it cannot get the cwd then it keeps the existing value, if one exists.
  • Print an error:
    image

@itislu itislu added bug Something isn't working leak There are memory leaks labels May 28, 2025
@itislu
Copy link
Collaborator Author

itislu commented May 28, 2025

@@ -32,7 +32,7 @@ bool setup_env_list(t_sh *shell)
if (!process_str_to_env_list(environ[i], &tmp_list, EXPORT_YES))
return (false);
if (!check_special_env_vars(&tmp_list))
return (ft_lstclear(&tmp_list, free), false);
return (ft_lstclear(&tmp_list, (void *)free_env_node), false);
Copy link
Collaborator Author

@itislu itislu May 29, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This caused the leak when starting in a non-existing directory.
check_special_env_vars() sets the PWD environment variable, and if getcwd() fails it returns false.
This issue existed even in the evaluation version.

Comment on lines 26 to +35
shell->exit_code = EXIT_SUCCESS;
if (!setup_env_list(shell))
return (false);
handle_signal_std(0, NULL, shell);
handle_signal_record(0, NULL, shell);
setup_signal(SIGINT, SIG_STANDARD);
setup_signal(SIGTERM, SIG_STANDARD);
setup_signal(SIGUSR1, SIG_STANDARD);
setup_signal(SIGQUIT, SIG_IGNORE);
setup_signal(SIGPIPE, SIG_STANDARD);
if (!setup_env_list(shell))
return (false);
Copy link
Collaborator Author

@itislu itislu May 29, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Moved env setup (which allocates memory and can print errors) after setting up the signal handlers so memory can be freed when a signal gets received.

Test case with SIGPIPE:

(rm -f /tmp/valgrind.log ; valgrind --log-file=/tmp/valgrind.log --leak-check=full --show-leak-kinds=all <absolute_path_to_minishell> 2>&1 | exit)
cat /tmp/valgrind.log

@itislu itislu force-pushed the fix-startup-in-non-existing-dir branch from b2fa705 to 002e7ec Compare June 10, 2025 19:17
@itislu itislu marked this pull request as ready for review June 22, 2025 20:47
@itislu itislu moved this from Backlog to Ready in minishell (🌊rash) Jun 22, 2025
itislu added 5 commits June 22, 2025 20:52
To test:
```
mkdir -p /tmp/dir && cd /tmp/dir && rm -rf ../dir
valgrind --leak-check=full <absolute path to minishell>
```
This follows the behavior of bash.
And usually bash always sets the `PWD` env variable itself and overwrites any exported value. However, if it cannot get the cwd then it keeps the existing value, if one exists.
1. Set up all signal handlers before any dynamic memory allocation.
2. Have all dynamic memory allocations reachable in the shell struct.
`pwd` stands for "print working directory", which is a verb/command and so does not really make sense for a variable name.
`cwd`, like in `getcwd()`, stands for "current working directory".
@itislu itislu force-pushed the fix-startup-in-non-existing-dir branch from 002e7ec to 11f2640 Compare June 22, 2025 20:53
@itislu itislu requested a review from Copilot June 23, 2025 07:33
Copilot

This comment was marked as outdated.

itislu added 2 commits June 23, 2025 09:56
Before, the new env node was added to the env list directly after it was created, and then possibly removed again by the functions checking for special env variables.
Now, those functions operate only on the new env node and don't have to modify the env list.
`ft_lstadd_back_tail()` does nothing if the node to add is `NULL`.
@itislu itislu requested a review from Copilot June 23, 2025 08:51
Copy link

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

Fix shell startup behavior when running in a non‐existent directory by handling PWD/OLDPWD initialization more robustly, preventing premature exit and leaks, and reporting errors like Bash.

  • Reordered setup_env_list in init_shell to avoid early return before signal setup.
  • Refactored environment list setup to handle special variables (PWD, OLDPWD) via handle_pwd and handle_oldpwd helpers.
  • Enhanced default PWD initialization to report getcwd errors (excluding ENOMEM) without exiting.

Reviewed Changes

Copilot reviewed 10 out of 10 changed files in this pull request and generated no comments.

Show a summary per file
File Description
source/shell/init.c Moved setup_env_list after signal registrations.
source/shell/env_list.c Added tail pointer, check_special_env_vars, and handlers for PWD/OLDPWD.
source/shell/default_env_list.c Improved add_default_pwd_env_node to log getcwd errors gracefully.
source/backend/builtins/cd/path.c Renamed pwd variables to cwd and updated file header.
source/backend/builtins/cd/env_pwd_update.c Updated file name in header comment.
source/backend/builtins/cd/dot_components.c Updated file name in header comment.
source/backend/builtins/cd/component_list_utils.c Updated file name in header comment.
source/backend/builtins/cd/component_list.c Updated file name in header comment and prototype rename.
include/cd.h Updated prototypes and header grouping to reflect renames.
build/source_files.mk Renamed dot_component.c to dot_components.c in build list.
Comments suppressed due to low confidence (3)

source/shell/env_list.c:16

  • [nitpick] The parameter name env_node in check_special_env_vars is ambiguous; consider renaming to env_list_node or similar to clarify that it refers to a single list node.
static bool	check_special_env_vars(t_list **env_node);

source/shell/default_env_list.c:57

  • Handling of getcwd failures (ENOMEM vs other errors) in add_default_pwd_env_node introduces new error-reporting branches; consider adding unit tests for PWD initialization when getcwd fails to validate behavior.
	if (!value)

source/shell/env_list.c:92

  • ft_lstdrop_node is called with the same pointer for both head pointer and node-to-drop; verify the function signature and ensure you pass the correct head pointer and node arguments to avoid removing the wrong element.
		ft_lstdrop_node(env_node, env_node, (void *)free_env_node);

@LeaYeh LeaYeh merged commit 8228ba3 into main Jul 12, 2025
41 checks passed
@github-project-automation github-project-automation bot moved this from Ready to Done in minishell (🌊rash) Jul 12, 2025
@itislu itislu deleted the fix-startup-in-non-existing-dir branch July 12, 2025 20:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working leak There are memory leaks
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

2 participants