[PATCH v2 0/8] Add isolated mode to source builtin
Bash scripts can be tricky to get right so reusing proven solutions as shell script libraries is of immense value. However, the existing shell script sourcing mechanisms are suboptimal for this task. The source builtin uses the PATH variable for resolving file names which means they would have to be placed alongside normal executables which could cause false positives: executables and commands might be accidentally sourced instead, causing hard to debug problems. This could be fixed by overriding PATH so that it contains only library directories but this interferes with the normal execution of the sourced scripts: they are no longer able to run commands normally because the commands are not in the PATH. This is an undesirable and ultimately unnecessary limitation. This patch set adds a special operating mode to the existing source builtin to make it behave in the desired way. When source is passed the -i option which stands for "isolated", it will search for files in the directories given by the BASH_SOURCE_PATH environment variable, and only in those directories. The PATH will not be modified. A build time configurable default value is defined which includes the user's home directory in addition to system directories, enabling users to easily develop their own scripting systems. Additionally, manipulation of the BASH_SOURCE_PATH variable is prevented whenever the shell is running in restricted mode. This allows users the same control over its value as they have over the value of PATH, thereby helping to prevent unintended sourcing of files. Changes compared to v1 patch set: - Rebased on top of devel branch - Dropped library terminology - Removed long options and all related code - Made helper functions static and local - Changed default source paths to avoid clashes - Restricted source path variable Matheus Afonso Martins Moreira (8): findcmd: parameterize path variable in functions findcmd: define find_in_path_var function builtins/source: extract file executor function builtins/source: refactor file searching function builtins/source: parse the -i option builtins/source: use source path in isolated mode variables: define default BASH_SOURCE_PATH shell: restrict BASH_SOURCE_PATH when appropriate builtins/source.def | 174 +++- config-top.h| 7 ++ findcmd.c | 32 +--- findcmd.h | 1 + shell.c | 1 + variables.c | 1 + 6 files changed, 153 insertions(+), 63 deletions(-) -- 2.44.0
[PATCH v2 1/8] findcmd: parameterize path variable in functions
The PATH variable is hard coded in the user command finder function.
Transforming it into an argument allows reusing the file finding logic
with variables other than PATH, making the code more flexible.
Signed-off-by: Matheus Afonso Martins Moreira
---
findcmd.c | 25 +
1 file changed, 13 insertions(+), 12 deletions(-)
diff --git a/findcmd.c b/findcmd.c
index 73c07bd8..fbcc0451 100644
--- a/findcmd.c
+++ b/findcmd.c
@@ -52,8 +52,8 @@ extern int errno;
#endif
/* Static functions defined and used in this file. */
-static char *_find_user_command_internal (const char *, int);
-static char *find_user_command_internal (const char *, int);
+static char *_find_user_command_internal (const char *, const char *, int);
+static char *find_user_command_internal (const char *, const char *, int);
static char *find_user_command_in_path (const char *, char *, int, int *);
static char *find_in_path_element (const char *, char *, int, size_t, struct
stat *, int *);
static char *find_absolute_program (const char *, int);
@@ -246,7 +246,7 @@ executable_or_directory (const char *file)
char *
find_user_command (const char *name)
{
- return (find_user_command_internal (name, FS_EXEC_PREFERRED|FS_NODIRS));
+ return (find_user_command_internal (name, "PATH",
FS_EXEC_PREFERRED|FS_NODIRS));
}
/* Locate the file referenced by NAME, searching along the contents
@@ -257,7 +257,7 @@ find_user_command (const char *name)
char *
find_path_file (const char *name)
{
- return (find_user_command_internal (name, FS_READABLE));
+ return (find_user_command_internal (name, "PATH", FS_READABLE));
}
/* Get $PATH and normalize it. USE_TEMPENV, if non-zero, says to look in the
@@ -278,14 +278,15 @@ path_value (const char *pathvar, int use_tempenv)
}
static char *
-_find_user_command_internal (const char *name, int flags)
+_find_user_command_internal (const char *name, const char *path_var, int flags)
{
char *path_list, *cmd;
SHELL_VAR *var;
- /* Search for the value of PATH in both the temporary environments and
- in the regular list of variables. */
- path_list = path_value ("PATH", 1);
+ /* Search for the value of specified path variable
+ in both the temporary environments
+ and in the regular list of variables. */
+ path_list = path_value (path_var, 1);
if (path_list == 0)
return (savestring (name));
@@ -296,7 +297,7 @@ _find_user_command_internal (const char *name, int flags)
}
static char *
-find_user_command_internal (const char *name, int flags)
+find_user_command_internal (const char *name, const char *path_var, int flags)
{
#ifdef __WIN32__
char *res, *dotexe;
@@ -304,13 +305,13 @@ find_user_command_internal (const char *name, int flags)
dotexe = (char *)xmalloc (strlen (name) + 5);
strcpy (dotexe, name);
strcat (dotexe, ".exe");
- res = _find_user_command_internal (dotexe, flags);
+ res = _find_user_command_internal (dotexe, path_var, flags);
free (dotexe);
if (res == 0)
-res = _find_user_command_internal (name, flags);
+res = _find_user_command_internal (name, path_var, flags);
return res;
#else
- return (_find_user_command_internal (name, flags));
+ return (_find_user_command_internal (name, path_var, flags));
#endif
}
--
2.44.0
[PATCH v2 3/8] builtins/source: extract file executor function
Extract into a dedicated helper function the code which loads
the contents of a file and executes it in the current shell.
This separates this useful functionality from the path resolution
mechanism used by the source builtin.
Signed-off-by: Matheus Afonso Martins Moreira
---
builtins/source.def | 78 +
1 file changed, 44 insertions(+), 34 deletions(-)
diff --git a/builtins/source.def b/builtins/source.def
index b68d16a5..334404bd 100644
--- a/builtins/source.def
+++ b/builtins/source.def
@@ -81,6 +81,7 @@ extern int errno;
#endif /* !errno */
static void uw_maybe_pop_dollar_vars (void *);
+static int execute_file_contents (WORD_LIST *, char *, char *);
/* If non-zero, `.' uses $PATH to look up the script to be sourced. */
int source_uses_path = 1;
@@ -112,11 +113,51 @@ uw_maybe_pop_dollar_vars (void *ignore)
This cannot be done in a subshell, since things like variable assignments
take place in there. So, I open the file, place it into a large string,
close the file, and then execute the string. */
+static int
+execute_file_contents (WORD_LIST *list, char *filename, char *framename)
+{
+ int result;
+ char *debug_trap;
+
+ begin_unwind_frame (framename);
+ add_unwind_protect (xfree, filename);
+
+ if (list->next)
+{
+ push_dollar_vars ();
+ add_unwind_protect (uw_maybe_pop_dollar_vars, NULL);
+ if (debugging_mode || shell_compatibility_level <= 44)
+ init_bash_argv (); /* Initialize BASH_ARGV and BASH_ARGC */
+ remember_args (list->next, 1);
+ if (debugging_mode)
+ push_args (list->next); /* Update BASH_ARGV and BASH_ARGC */
+}
+ set_dollar_vars_unchanged ();
+
+ /* Don't inherit the DEBUG trap unless function_trace_mode (overloaded)
+ is set. XXX - should sourced files inherit the RETURN trap? Functions
+ don't. */
+ debug_trap = TRAP_STRING (DEBUG_TRAP);
+ if (debug_trap && function_trace_mode == 0)
+{
+ debug_trap = savestring (debug_trap);
+ add_unwind_protect (xfree, debug_trap);
+ add_unwind_protect (uw_maybe_set_debug_trap, debug_trap);
+ restore_default_signal (DEBUG_TRAP);
+}
+
+ result = source_file (filename, (list && list->next));
+
+ run_unwind_frame (framename);
+
+ return (result);
+}
+
+/* Read and execute commands from the file passed as argument. */
int
source_builtin (WORD_LIST *list)
{
- int result;
- char *filename, *debug_trap, *x;
+ char *filename, *x;
if (no_options (list))
return (EX_USAGE);
@@ -164,36 +205,5 @@ source_builtin (WORD_LIST *list)
filename = savestring (list->word->word);
}
- begin_unwind_frame ("source");
- add_unwind_protect (xfree, filename);
-
- if (list->next)
-{
- push_dollar_vars ();
- add_unwind_protect (uw_maybe_pop_dollar_vars, NULL);
- if (debugging_mode || shell_compatibility_level <= 44)
- init_bash_argv (); /* Initialize BASH_ARGV and BASH_ARGC */
- remember_args (list->next, 1);
- if (debugging_mode)
- push_args (list->next); /* Update BASH_ARGV and BASH_ARGC */
-}
- set_dollar_vars_unchanged ();
-
- /* Don't inherit the DEBUG trap unless function_trace_mode (overloaded)
- is set. XXX - should sourced files inherit the RETURN trap? Functions
- don't. */
- debug_trap = TRAP_STRING (DEBUG_TRAP);
- if (debug_trap && function_trace_mode == 0)
-{
- debug_trap = savestring (debug_trap);
- add_unwind_protect (xfree, debug_trap);
- add_unwind_protect (uw_maybe_set_debug_trap, debug_trap);
- restore_default_signal (DEBUG_TRAP);
-}
-
- result = source_file (filename, (list && list->next));
-
- run_unwind_frame ("source");
-
- return (result);
+ return execute_file_contents (list, filename, "source");
}
--
2.44.0
[PATCH v2 2/8] findcmd: define find_in_path_var function
It works just like the find_in_path function
but takes a PATH-like variable name instead.
This allows defining and using more PATH-like
variables much more easily.
Signed-off-by: Matheus Afonso Martins Moreira
---
findcmd.c | 7 +++
findcmd.h | 1 +
2 files changed, 8 insertions(+)
diff --git a/findcmd.c b/findcmd.c
index fbcc0451..9d64317a 100644
--- a/findcmd.c
+++ b/findcmd.c
@@ -703,3 +703,10 @@ find_in_path (const char *name, char *path_list, int flags)
{
return (find_user_command_in_path (name, path_list, flags, (int *)0));
}
+
+/* Like find_in_path but takes a PATH-like variable instead. */
+char *
+find_in_path_var (const char *name, char *path_var, int flags)
+{
+ return (_find_user_command_internal (name, path_var, flags));
+}
diff --git a/findcmd.h b/findcmd.h
index 6dbded4c..4976e422 100644
--- a/findcmd.h
+++ b/findcmd.h
@@ -34,6 +34,7 @@ extern int is_directory (const char *);
extern int executable_or_directory (const char *);
extern char *find_user_command (const char *);
extern char *find_in_path (const char *, char *, int);
+extern char *find_in_path_var (const char *, char *, int);
extern char *find_path_file (const char *);
extern char *path_value (const char *, int);
extern char *search_for_command (const char *, int);
--
2.44.0
[PATCH v2 5/8] builtins/source: parse the -i option
Passing the -i option to the source builtin
enables isolated sourcing mode which restricts
its search path to the directories defined by
the BASH_SOURCE_PATH variable. This also has
the added benefit of not touching PATH at all.
To maximize compatibility, the option will only
be recognized outside POSIXly correct mode.
Signed-off-by: Matheus Afonso Martins Moreira
---
builtins/source.def | 30 +++---
1 file changed, 27 insertions(+), 3 deletions(-)
diff --git a/builtins/source.def b/builtins/source.def
index ba5e1596..26040e70 100644
--- a/builtins/source.def
+++ b/builtins/source.def
@@ -182,10 +182,34 @@ int
source_builtin (WORD_LIST *list)
{
char *filename, *x;
+ int opt, isolated_mode = 0;
- if (no_options (list))
-return (EX_USAGE);
- list = loptend;
+ if (posixly_correct)
+{
+ /* Don't parse options in POSIX mode */
+ if (no_options (list))
+return (EX_USAGE);
+}
+ else
+{
+ reset_internal_getopt ();
+
+ while ((opt = internal_getopt (list, "i")) != -1)
+{
+ switch (opt)
+{
+case 'i':
+ isolated_mode = 1;
+ break;
+CASE_HELPOPT;
+default:
+ builtin_usage ();
+ return (EX_USAGE);
+}
+}
+
+ list = loptend;
+}
if (list == 0)
{
--
2.44.0
[PATCH v2 4/8] builtins/source: refactor file searching function
Extract the file searching algorithm of the source builtin into
a static helper function. Makes the code easier to understand
and separates the searching from the error handling logic.
Signed-off-by: Matheus Afonso Martins Moreira
---
builtins/source.def | 59 +++--
1 file changed, 36 insertions(+), 23 deletions(-)
diff --git a/builtins/source.def b/builtins/source.def
index 334404bd..ba5e1596 100644
--- a/builtins/source.def
+++ b/builtins/source.def
@@ -82,6 +82,7 @@ extern int errno;
static void uw_maybe_pop_dollar_vars (void *);
static int execute_file_contents (WORD_LIST *, char *, char *);
+static char *search_for_file (WORD_LIST *);
/* If non-zero, `.' uses $PATH to look up the script to be sourced. */
int source_uses_path = 1;
@@ -153,6 +154,29 @@ execute_file_contents (WORD_LIST *list, char *filename,
char *framename)
return (result);
}
+static char *
+search_for_file (WORD_LIST *list)
+{
+ char *filename = NULL, *x;
+
+ /* XXX -- should this be absolute_pathname? */
+ if (posixly_correct && strchr (list->word->word, '/'))
+filename = savestring (list->word->word);
+ else if (absolute_pathname (list->word->word))
+filename = savestring (list->word->word);
+ else if (source_uses_path)
+filename = find_path_file (list->word->word);
+ if (filename == 0)
+{
+ if (source_searches_cwd == 0)
+return (NULL);
+ else
+filename = savestring (list->word->word);
+}
+
+ return (filename);
+}
+
/* Read and execute commands from the file passed as argument. */
int
source_builtin (WORD_LIST *list)
@@ -178,31 +202,20 @@ source_builtin (WORD_LIST *list)
}
#endif
- filename = (char *)NULL;
- /* XXX -- should this be absolute_pathname? */
- if (posixly_correct && strchr (list->word->word, '/'))
-filename = savestring (list->word->word);
- else if (absolute_pathname (list->word->word))
-filename = savestring (list->word->word);
- else if (source_uses_path)
-filename = find_path_file (list->word->word);
+ filename = search_for_file (list);
+
if (filename == 0)
{
- if (source_searches_cwd == 0)
- {
- x = printable_filename (list->word->word, 0);
- builtin_error (_("%s: file not found"), x);
- if (x != list->word->word)
- free (x);
- if (posixly_correct && interactive_shell == 0 &&
executing_command_builtin == 0)
- {
- last_command_exit_value = EXECUTION_FAILURE;
- jump_to_top_level (EXITPROG);
- }
- return (EXECUTION_FAILURE);
- }
- else
- filename = savestring (list->word->word);
+ x = printable_filename (list->word->word, 0);
+ builtin_error (_("%s: file not found"), x);
+ if (x != list->word->word)
+free (x);
+ if (posixly_correct && interactive_shell == 0 &&
executing_command_builtin == 0)
+{
+ last_command_exit_value = EXECUTION_FAILURE;
+ jump_to_top_level (EXITPROG);
+}
+ return (EXECUTION_FAILURE);
}
return execute_file_contents (list, filename, "source");
--
2.44.0
[PATCH v2 8/8] shell: restrict BASH_SOURCE_PATH when appropriate
Make the BASH_SOURCE_PATH variable read-only and unsettable
when the shell is operating in restricted mode. This variable
should be restricted for the same reasons why PATH is restricted.
Signed-off-by: Matheus Afonso Martins Moreira
---
shell.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/shell.c b/shell.c
index 15e005d6..940de31b 100644
--- a/shell.c
+++ b/shell.c
@@ -1294,6 +1294,7 @@ maybe_make_restricted (char *name)
set_var_read_only ("ENV");
set_var_read_only ("BASH_ENV");
set_var_read_only ("HISTFILE");
+ set_var_read_only ("BASH_SOURCE_PATH");
restricted = 1;
}
return (restricted);
--
2.44.0
[PATCH v2 6/8] builtins/source: use source path in isolated mode
Behave normally when executed without any options.
Search only BASH_SOURCE_PATH for scripts to source
when isolated mode is enabled via the -i option.
Signed-off-by: Matheus Afonso Martins Moreira
---
builtins/source.def | 27 ++-
1 file changed, 26 insertions(+), 1 deletion(-)
diff --git a/builtins/source.def b/builtins/source.def
index 26040e70..7f870bfe 100644
--- a/builtins/source.def
+++ b/builtins/source.def
@@ -83,6 +83,7 @@ extern int errno;
static void uw_maybe_pop_dollar_vars (void *);
static int execute_file_contents (WORD_LIST *, char *, char *);
static char *search_for_file (WORD_LIST *);
+static char *isolated_search_for_file (WORD_LIST *);
/* If non-zero, `.' uses $PATH to look up the script to be sourced. */
int source_uses_path = 1;
@@ -177,6 +178,27 @@ search_for_file (WORD_LIST *list)
return (filename);
}
+static char *
+isolated_search_for_file (WORD_LIST *list)
+{
+ char *filename = NULL;
+
+ if (absolute_pathname (list->word->word))
+filename = savestring (list->word->word);
+ else
+filename = find_in_path_var (list->word->word, "BASH_SOURCE_PATH",
FS_READABLE);
+
+ if (filename == 0)
+{
+ if (source_searches_cwd == 0)
+return (NULL);
+ else
+filename = savestring (list->word->word);
+}
+
+ return (filename);
+}
+
/* Read and execute commands from the file passed as argument. */
int
source_builtin (WORD_LIST *list)
@@ -226,7 +248,10 @@ source_builtin (WORD_LIST *list)
}
#endif
- filename = search_for_file (list);
+ if (!isolated_mode)
+filename = search_for_file (list);
+ else
+filename = isolated_search_for_file (list);
if (filename == 0)
{
--
2.44.0
[PATCH v2 7/8] variables: define default BASH_SOURCE_PATH
Define a build time configurable default value
for the new BASH_SOURCE_PATH variable.
Look for libraries in the user's home directory
as well as the system wide directories.
Signed-off-by: Matheus Afonso Martins Moreira
---
config-top.h | 7 +++
variables.c | 1 +
2 files changed, 8 insertions(+)
diff --git a/config-top.h b/config-top.h
index b6e73c4b..65d52cb2 100644
--- a/config-top.h
+++ b/config-top.h
@@ -78,6 +78,13 @@
"/usr/local/lib/bash:/usr/lib/bash:/opt/local/lib/bash:/usr/pkg/lib/bash:/opt/pkg/lib/bash:."
#endif
+/* The default value of the BASH_SOURCE_PATH variable
+ which is used by source -l instead of PATH. */
+#ifndef DEFAULT_SOURCE_PATH
+#define DEFAULT_SOURCE_PATH \
+
"~/.local/share/bash/source:/usr/local/share/bash/source:/usr/share/bash/source"
+#endif
+
/* Default primary and secondary prompt strings. */
#define PPROMPT "\\s-\\v\\$ "
#define SPROMPT "> "
diff --git a/variables.c b/variables.c
index 84b30d93..eb363332 100644
--- a/variables.c
+++ b/variables.c
@@ -694,6 +694,7 @@ initialize_shell_variables (char **env, int privmode)
uidset ();
temp_var = set_if_not ("BASH_LOADABLES_PATH",
DEFAULT_LOADABLE_BUILTINS_PATH);
+ temp_var = set_if_not ("BASH_SOURCE_PATH", DEFAULT_SOURCE_PATH);
temp_var = find_variable ("BASH_XTRACEFD");
if (temp_var && imported_p (temp_var))
--
2.44.0
Changed behaviour of ${PARAMETER/#PATTERN/STRING} and ${PARAMETER/%PATTERN/STRING}
In 5.3-alpha, it is no longer possible to quote the special % and #
characters in a pattern replacement expansion.
$ a=1/%2/%3
$ echo "${a/\%/##}"
1/%2/%3##
$ echo "${a/\/%/##}"
1##2/%3
The second example shows that quoting still works as expected for
${PARAMETER//PATTERN/STRING}.
--
Andreas Schwab, SUSE Labs, sch...@suse.de
GPG Key fingerprint = 0196 BAD8 1CE9 1970 F4BE 1748 E4D4 88E3 0EEA B9D7
"And now for something completely different."
Re: bug in bash
On 5/12/24 10:00 AM, Oğuz wrote:
On Sun, May 12, 2024 at 4:33 PM Andreas Schwab wrote:
Since the redirection fails and the cat command is never started, bash
doesn't switch the terminal process group
It does on my computer:
554 ioctl(255, TIOCSPGRP, [554]) = 0
553 execve("/usr/bin/wc", ["wc", "-l"], 0x55706d7bac10 /* 30 vars */
554 +++ exited with 1 +++
But it doesn't change the process group ID of `wc' to its PID, so it
ends up in the foreground process group with bash.
Command substitutions and process substitutions run in the same process
group as the shell performing the word expansions. This is how ksh93 does
it as well (zsh, by contrast, performs the setpgid in the process forked
to run the process substitution).
--
``The lyf so short, the craft so long to lerne.'' - Chaucer
``Ars longa, vita brevis'' - Hippocrates
Chet Ramey, UTech, CWRUc...@case.eduhttp://tiswww.cwru.edu/~chet/
OpenPGP_signature.asc
Description: OpenPGP digital signature
Re: bug in bash
On Monday, May 13, 2024, Chet Ramey wrote: > performs the setpgid in the process forked > to run the process substitution). > Yes, this sounds like the easy way. I don't know how else to prevent the race in OP. Close stdin/stdout? Direct it from /dev/null? -- Oğuz
Re: [PATCH v2 6/8] builtins/source: use source path in isolated mode
2024年5月13日(月) 19:40 Matheus Afonso Martins Moreira
:
> diff --git a/builtins/source.def b/builtins/source.def
> index 26040e70..7f870bfe 100644
> --- a/builtins/source.def
> +++ b/builtins/source.def
> @@ -177,6 +178,27 @@ search_for_file (WORD_LIST *list)
> [...]
> +static char *
> +isolated_search_for_file (WORD_LIST *list)
> [...]
> + if (filename == 0)
> +{
> + if (source_searches_cwd == 0)
> +return (NULL);
> + else
> +filename = savestring (list->word->word);
> +}
* With this, `isolated_search_for_file()' falls back to local files in
the current working directory. I'm unsure about whether this is
intentional, but I think we should restrict the search in
BASH_SOURCE_PATH and shouldn't fall back to local files. In that case,
we need to make sure that `find_in_path_var()' returns NULL in the
case of empty BASH_SOURCE_PATH. In the current implementation,
`find_in_path_var()' seems to return `save_string (name)' for empty
BASH_SOURCE_PATH, which would induce the search in the current working
directory.
* Even if we intensionally fall back to local files, this part seems
just a repetition of `search_for_file()'. I think this common part
should be moved to `builtin_source()'.
Re: [PATCH v2 3/8] builtins/source: extract file executor function
2024年5月13日(月) 19:39 Matheus Afonso Martins Moreira : > Extract into a dedicated helper function the code which loads > the contents of a file and executes it in the current shell. > This separates this useful functionality from the path resolution > mechanism used by the source builtin. To other reviewers: I checked that this commit is still independent of the other parts, so you can skip this commit if you focus on the suggested feature.
Re: [PATCH v2 1/8] findcmd: parameterize path variable in functions
2024年5月13日(月) 19:38 Matheus Afonso Martins Moreira
:
> The PATH variable is hard coded in the user command finder function.
> Transforming it into an argument allows reusing the file finding logic
> with variables other than PATH, making the code more flexible.
I think what was suggested by Chet is to confine the changes in
builtins/source.def. The patches touch the interface of many
functions, but it seems essentially equivalent to just calling
find_in_path (list->word->word, path_value ("BASH_SOURCE_PATH", 1),
FS_READABLE);
The cases where path_value() returns NULL or an empty string would be
anyway handled by find_in_path() [more specifically by
_find_user_command_internal() and find_user_command_in_path() called
by find_in_path()]. I doubt it's worth adding those changes in
`findcmd.c'. So are the changes in [PATCH v2 2/8].
Re: Re: [PATCH v2 6/8] builtins/source: use source path in isolated mode
> I think we should restrict the search in > BASH_SOURCE_PATH and shouldn't fall back to local files. I concur but thought it'd be wise to discuss it first so I left it in. Users might expect it to respect the configuration option. I also saw that many of the PATH-like variable defaults had the current directory in them. Finally, there is precedent for this in other programming languages. > Even if we intensionally fall back to local files, this part seems > just a repetition of `search_for_file()'. I think this common part > should be moved to `builtin_source()'. I made it that way because it was explicitly requested that the POSIX mode behavior not be affected in any way. This way it's clear that there are two algorithms that the function branches into and that they are completely independent of each other. Should the current directory searching logic be eliminated, the function will be simpler. -- Matheus
Re: Re: [PATCH v2 1/8] findcmd: parameterize path variable in functions
> The patches touch the interface of many functions
I added one external function: find_in_path_var.
The other changes are all static and local.
> It seems essentially equivalent to just calling
> find_in_path (list->word->word, path_value ("BASH_SOURCE_PATH", 1),
> FS_READABLE);
It is. I'm just very averse to chaining functions like that in C
since you can't write f(g(x)) without worrying about freeing
the value returned by g. For that reason I think this is better:
find_in_path_var (list->word->word, "BASH_SOURCE_PATH", FS_READABLE);
Also, find_in_path is itself just a wrapper for a static function,
just like find_in_path_var. I followed the same approach.
> The cases where path_value() returns NULL or an empty string would be
> anyway handled by find_in_path()
With find_in_path_var there's no need to even consider that.
The knowledge of those cases has been captured by the function.
There's no need to explicitly use path_value either.
> I doubt it's worth adding those changes in
> `findcmd.c'. So are the changes in [PATCH v2 2/8].
You don't think they improve the code?
-- Matheus
proposed BASH_SOURCE_PATH
I wholeheartedly support the introduction of BASH_SOURCE_PATH, but I would
like to suggest three tweaks to its semantics.
A common pattern is to unpack a script with its associated library & config
files into a new directory, which then leaves a problem locating the
library files whose paths are only known relative to $0 (or
${BASH_SOURCE[0]}).
1. I therefore propose that where a relative path appears in
BASH_SOURCE_PATH, it should be taken as relative to the directory
containing $0 (after resolving symlinks), rather than relative to $PWD.
As an interim step until that's implemented, please ignore any relative
entries in BASH_SOURCE_PATH, so that users who really want the cwd in
BASH_SOURCE_PATH get used to writing $PWD or /proc/self/cwd instead.
2. Search BASH_SOURCE_PATH when any relative path is given, not just a path
that lacks a '/', so that libraries can be organized into subdirectories.
3. To avoid accidentally loading a script rather than a library, while
searching BASH_SOURCE_PATH, ignore any files that have exec permission,
inverting the check normally made for executables in PATH. This would keep
executables and libraries functionally separate, even if they're commingled
in the same directories.
Yes I know that some folk think it's occasionally useful to have a single
file that operates as both, but (a) new features should default to the
"safest" mode of operation, (b) this isn't python and so that's pretty rare
in practice, and (c) there was at least two work-arounds should that
behaviour actually be desired: (i) use an absolute path, or (ii) use PATH
instead of BASH_SOURCE_PATH.
4. When using "source -i", if BASH_SOURCE_PATH is unset or empty, it's
taken as equivalent to '.', so that it's useful to write "source -i
../lib/foo.bash" in a script at "$X/bin/bar" to load "$X/lib/foo.bash".
-Martin
PS: in the longer term, perhaps PATH could have similar behaviour, but
gated by a shopt or compat check.
Re: proposed BASH_SOURCE_PATH
Completely agree with your suggestions. I've proposed suggestion 3 as well in previous emails but consensus was not reached. -- Matheus
