[FIX] Fix startup in non existing directory#392
Conversation
|
source/shell/env_list.c
Outdated
| 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.
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.
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.
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_listininit_shellto avoid early return before signal setup. - Refactored environment list setup to handle special variables (
PWD,OLDPWD) viahandle_pwdandhandle_oldpwdhelpers. - Enhanced default PWD initialization to report
getcwderrors (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_nodeincheck_special_env_varsis ambiguous; consider renaming toenv_list_nodeor 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
getcwdfailures (ENOMEM vs other errors) inadd_default_pwd_env_nodeintroduces new error-reporting branches; consider adding unit tests for PWD initialization whengetcwdfails to validate behavior.
if (!value)
source/shell/env_list.c:92
ft_lstdrop_nodeis 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:
PWDenv 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.