-
Notifications
You must be signed in to change notification settings - Fork 2
[FIX] Fix startup in non existing directory #392
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
source/shell/env_list.c
Outdated
@@ -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); |
There was a problem hiding this comment.
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.
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); |
There was a problem hiding this comment.
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
b2fa705
to
002e7ec
Compare
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".
002e7ec
to
11f2640
Compare
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`.
There was a problem hiding this 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
ininit_shell
to avoid early return before signal setup. - Refactored environment list setup to handle special variables (
PWD
,OLDPWD
) viahandle_pwd
andhandle_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
incheck_special_env_vars
is ambiguous; consider renaming toenv_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) inadd_default_pwd_env_node
introduces new error-reporting branches; consider adding unit tests for PWD initialization whengetcwd
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);
Test case:
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.