Skip to content
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

tentative fix to address bash problem #490 #510

Merged
merged 6 commits into from Mar 8, 2024

Conversation

ruitianzhong
Copy link
Contributor

Here, I try to address the problem in #490 with smaller change to the code.

The following command can be detected:

exec ls
if echo hello;
then echo world
fi
exec ls
exit

However, the following command have problem and it can be improved.

exec ls \ 
/usr/bin
command \
exec \
final

I will try to improve this in later time. For now, it is a draft.

Signed-off-by: ruitianzhong <ruitian-zhong@outlook.com>
@cfc4n cfc4n marked this pull request as draft March 8, 2024 00:27
@cfc4n cfc4n added enhancement New feature or request improve labels Mar 8, 2024
@sancppp
Copy link
Contributor

sancppp commented Mar 8, 2024

I think the implementation is better than mine. Perhaps I can close my PR.🥲

The advantage of my implementation is the ability to handle bash commands that fail due to closing the terminal.
For example:

> sleep 30s # a time-consuming bash command
# Then, close the terminal before the command ends

Signed-off-by: ruitianzhong <ruitian-zhong@outlook.com>
Signed-off-by: ruitianzhong <ruitian-zhong@outlook.com>
@ruitianzhong
Copy link
Contributor Author

I hook the function exit_builtin and exec_builtin in shell. I have confirmed that this two function at least have existed since 1996-08-26.

run the following command in bash source code.

git blame -L 57,67 builtins/exec.def
git blame -L 57,67 builtins/exit.def
726f6388 (Jari Aalto 1996-08-26 18:22:31 +0000 100) exec_builtin (list)
^726f6388 (Jari Aalto 1996-08-26 18:22:31 +0000 101)      WORD_LIST *list;
^726f6388 (Jari Aalto 1996-08-26 18:22:31 +0000 102) {
^726f6388 (Jari Aalto 1996-08-26 18:22:31 +0000 103)   int exit_value = EXECUTION_FAILURE;
8868edaf2 (Chet Ramey 2020-12-06 15:51:17 -0500 104)   int cleanenv, login, opt, orig_job_control;
d166f0488 (Jari Aalto 1997-06-05 14:59:13 +0000 105)   char *argv0, *command, **args, **env, *newname, *com2;
^726f6388 (Jari Aalto 1996-08-26 18:22:31 +0000 106) 
74091dd4e (Chet Ramey 2022-09-26 11:49:46 -0400 107)   cleanenv = login = orig_job_control = 0;
495aee441 (Chet Ramey 2011-11-22 19:11:26 -0500 108)   exec_argv0 = argv0 = (char *)NULL;
ccc6cda31 (Jari Aalto 1996-12-23 17:02:34 +0000 109) 
ccc6cda31 (Jari Aalto 1996-12-23 17:02:34 +0000 110)   reset_internal_getopt ();
^726f6388 (Jari Aalto 1996-08-26 18:22:31 +0000 57) exit_builtin (list)
^726f6388 (Jari Aalto 1996-08-26 18:22:31 +0000 58)      WORD_LIST *list;
^726f6388 (Jari Aalto 1996-08-26 18:22:31 +0000 59) {
a0c0a00fc (Chet Ramey 2016-09-15 16:59:08 -0400 60)   CHECK_HELPOPT (list);
a0c0a00fc (Chet Ramey 2016-09-15 16:59:08 -0400 61) 
^726f6388 (Jari Aalto 1996-08-26 18:22:31 +0000 62)   if (interactive)
^726f6388 (Jari Aalto 1996-08-26 18:22:31 +0000 63)     {
3185942a5 (Jari Aalto 2009-01-12 13:36:28 +0000 64)       fprintf (stderr, login_shell ? _("logout\n") : "exit\n");
^726f6388 (Jari Aalto 1996-08-26 18:22:31 +0000 65)       fflush (stderr);
^726f6388 (Jari Aalto 1996-08-26 18:22:31 +0000 66)     }
^726f6388 (Jari Aalto 1996-08-26 18:22:31 +0000 67) 

finally,

git log --reverse
commit 726f63884db0132f01745f1fb4465e6621088ccf
Author: Jari Aalto <jari.aalto@cante.net>
Date:   Mon Aug 26 18:22:31 1996 +0000

    Imported from ../bash-1.14.7.tar.gz.

I also provide some test case as follows:
1:

for i in {1..10}
 do
 echo hello
 done

2:

if echo hello
 then echo world
 fi

3:

exec ls

4:

exit 42
exit 1 2 # too many argument

5:

ls \
exec \
/dev

6:

exec \
echo hello world

7:

hello(){
echo hello
}
hello

8:

hello(){
echo hello
}
exit

@ruitianzhong ruitianzhong marked this pull request as ready for review March 8, 2024 07:30
user/event/event_bash.go Outdated Show resolved Hide resolved
Signed-off-by: ruitianzhong <ruitian-zhong@outlook.com>
Signed-off-by: ruitianzhong <ruitian-zhong@outlook.com>
@ruitianzhong ruitianzhong requested a review from cfc4n March 8, 2024 12:44
Copy link
Member

@cfc4n cfc4n left a comment

Choose a reason for hiding this comment

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

There is only one remaining issue with field naming, everything else is fine.

user/event/event_bash.go Outdated Show resolved Hide resolved
Signed-off-by: ruitianzhong <ruitian-zhong@outlook.com>
@ruitianzhong ruitianzhong requested a review from cfc4n March 8, 2024 14:10
@sancppp
Copy link
Contributor

sancppp commented Mar 8, 2024

I think it is more appropriate to change bashEvent.eventType from EventTypeOutput to EventTypeEventProcessor.

Consider this situation:
During the execution of a long-consuming bash command, the session in which it was located was closed. This will cause the contents in the bpf map to not be released. The root cause of this bug is that readline and execute_command are not strongly related in this case.

Bad Case:

> sleep 30s # a time-consuming bash command
# Then, close the session before the command ends

After repeating 1024 times, the bpf map event_t will reach the maximum capacity, causing eCapture to not work.
eCapture is a daemon process that runs in the background for a long time. I think this situation is possible after working for a long time.

In PR#508, the above situation will be output as a timeout and will not cause a memory leak.

Copy link
Member

@cfc4n cfc4n left a comment

Choose a reason for hiding this comment

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

LGTM, Thanks.

@cfc4n cfc4n merged commit d26ad15 into gojue:master Mar 8, 2024
6 checks passed
@ruitianzhong ruitianzhong deleted the bash-fix branch March 8, 2024 15:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request improve
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants