3 2

[PATCH] Flesh out $ENV implementation

Details
Message ID
<20190228154806.4848-1-sir@cmpwn.com>
DKIM signature
permerror
Download raw message
Patch: +19 -7
Perform parameter expansion and print an error if it's not an absolute
path.
---
 shell/entry.c | 26 +++++++++++++++++++-------
 1 file changed, 19 insertions(+), 7 deletions(-)

diff --git a/shell/entry.c b/shell/entry.c
index e3c0aa3..3afebbe 100644
--- a/shell/entry.c
+++ b/shell/entry.c
@@ -1,4 +1,5 @@
 #define _POSIX_C_SOURCE 200809L
+#define _XOPEN_SOURCE
 #include <mrsh/builtin.h>
 #include <mrsh/entry.h>
 #include <mrsh/shell.h>
@@ -10,8 +11,8 @@
 #include <unistd.h>
 #include "builtin.h"
 
-static char *expand_ps(struct mrsh_state *state, const char *ps1) {
-	struct mrsh_parser *parser = mrsh_parser_with_data(ps1, strlen(ps1));
+static char *expand_parameter(struct mrsh_state *state, const char *src) {
+	struct mrsh_parser *parser = mrsh_parser_with_data(src, strlen(src));
 	struct mrsh_word *word = mrsh_parse_word(parser);
 	mrsh_parser_destroy(parser);
 	if (word == NULL) {
@@ -25,7 +26,7 @@ char *mrsh_get_ps1(struct mrsh_state *state, int next_history_id) {
 	// TODO: Replace ! with next history ID
 	const char *ps1 = mrsh_env_get(state, "PS1", NULL);
 	if (ps1 != NULL) {
-		return expand_ps(state, ps1);
+		return expand_parameter(state, ps1);
 	}
 	char *p = malloc(3);
 	sprintf(p, "%s", getuid() ? "$ " : "# ");
@@ -36,7 +37,7 @@ char *mrsh_get_ps2(struct mrsh_state *state) {
 	// TODO: Replace ! with next history ID
 	const char *ps2 = mrsh_env_get(state, "PS2", NULL);
 	if (ps2 != NULL) {
-		return expand_ps(state, ps2);
+		return expand_parameter(state, ps2);
 	}
 	return strdup("> ");
 }
@@ -44,7 +45,7 @@ char *mrsh_get_ps2(struct mrsh_state *state) {
 char *mrsh_get_ps4(struct mrsh_state *state) {
 	const char *ps4 = mrsh_env_get(state, "PS4", NULL);
 	if (ps4 != NULL) {
-		return expand_ps(state, ps4);
+		return expand_parameter(state, ps4);
 	}
 	return strdup("+ ");
 }
@@ -109,6 +110,17 @@ void mrsh_source_env(struct mrsh_state *state) {
 	if (getuid() != geteuid() || getgid() != getegid()) {
 		return;
 	}
-	// TODO: parameter expansion
-	source_file(state, path);
+	path = expand_parameter(state, path);
+	char *real = realpath(path, NULL);
+	if (strcmp(path, real) != 0) {
+		fprintf(stderr, "Error: $ENV is not an absolute path; "
+				"this is undefined behavior.\n");
+		fprintf(stderr, "Continuing without sourcing it.\n");
+		free(path);
+		free(real);
+		return;
+	}
+	source_file(state, real);
+	free(path);
+	free(real);
 }
-- 
2.20.1
Details
Message ID
<guIXGZYiDHTsqgvMSsra36p_bUxOeXwA7OHl8Q6EYUa3LR9crNssnFqBG84_9t8hMvvzhFom5ZSOGCBA9DIeBnQbMu_EN7Qcyy8Jb45jgzA=@emersion.fr>
In-Reply-To
<20190228154806.4848-1-sir@cmpwn.com> (view parent)
DKIM signature
permerror
Download raw message
> -static char *expand_ps(struct mrsh_state *state, const char *ps1) {
> -struct mrsh_parser *parser = mrsh_parser_with_data(ps1, strlen(ps1));
> +static char *expand_parameter(struct mrsh_state *state, const char *src) {
> +struct mrsh_parser *parser = mrsh_parser_with_data(src, strlen(src));

This function doesn't only expand parameters, it also performs command
substitution and arithmetic expansion. For $ENV, we probably want to
add a function to transform a `mrsh_word_parameter` into a
`mrsh_word_string` instead, and stringify other words as-is. The logic
for parameter expansion is already implemented in word.c (see
`parameter_get_value`), we just need to extract it.

(Full spec for reference:

> This variable, when and only when an interactive shell is invoked,
> shall be subjected to parameter expansion (see Parameter Expansion)
> by the shell and the resulting value shall be used as a pathname of a
> file containing shell commands to execute in the current environment.
> The file need not be executable. If the expanded value of ENV is not
> an absolute pathname, the results are unspecified. ENV shall be
> ignored if the user's real and effective user IDs or real and
> effective group IDs are different.

It would be nice to at least add a TODO for the user IDs part.)
Details
Message ID
<20190302170715.GA3930@cirno.my.domain>
In-Reply-To
<guIXGZYiDHTsqgvMSsra36p_bUxOeXwA7OHl8Q6EYUa3LR9crNssnFqBG84_9t8hMvvzhFom5ZSOGCBA9DIeBnQbMu_EN7Qcyy8Jb45jgzA=@emersion.fr> (view parent)
DKIM signature
permerror
Download raw message
On 2019-03-01 , Simon Ser wrote:
> This function doesn't only expand parameters, it also performs command
> substitution and arithmetic expansion. For $ENV, we probably want to
> add a function to transform a `mrsh_word_parameter` into a
> `mrsh_word_string` instead, and stringify other words as-is. The logic
> for parameter expansion is already implemented in word.c (see
> `parameter_get_value`), we just need to extract it.

According to the spec, the same applies to all of these variables,
including e.g. PS1. Command expansion in PS1 is important for many
people, for example those who want to include their git repo's status in
their PS1, or in my case I have a little command which displays the cwd
in a different format.

Since this only affects the interactive shell, and since word expansion
is a superset of parameter expansion, I suggest we expand on the
standard in this case for the sake of a comfortable interactive shell
experience.

> It would be nice to at least add a TODO for the user IDs part.)

The code already handles the uid/euid issue.
Details
Message ID
<LhDPhkTK-VQUYSUh-DTgoz6AYcg8QMjQRUeXlwbCXdV5RsADCkEJdf4wheADVXpZYOfB4lZXrrvVIwtdsuwxB0dSwc8q8vn04D7jboMYYAA=@emersion.fr>
In-Reply-To
<20190302170715.GA3930@cirno.my.domain> (view parent)
DKIM signature
permerror
Download raw message
On Saturday, March 2, 2019 6:07 PM, Drew DeVault <sir@cmpwn.com> wrote:
> According to the spec, the same applies to all of these variables,
> including e.g. PS1. Command expansion in PS1 is important for many
> people, for example those who want to include their git repo's status in
> their PS1, or in my case I have a little command which displays the cwd
> in a different format.
>
> Since this only affects the interactive shell, and since word expansion
> is a superset of parameter expansion, I suggest we expand on the
> standard in this case for the sake of a comfortable interactive shell
> experience.

You're right. This seems reasonable.

The patch doesn't apply anymore even with a 3-way merge, can you rebase
it?

While you're at it, can you remove the _POSIX_C_SOURCE definition?
When _XOPEN_SOURCE is defined, it also implicitly defines
_POSIX_C_SOURCE. Explicitly defining both makes it so it's not clear
which version of the spec to use, and leads to bugs where an old
version is picked up on some platforms.

> > It would be nice to at least add a TODO for the user IDs part.)
>
> The code already handles the uid/euid issue.

Indeed, my bad.