emersion / mrsh

A minimal POSIX shell
MIT License
489 stars 35 forks source link

`mrsh -o` ends with a segfault #164

Closed hyphenrf closed 3 years ago

hyphenrf commented 4 years ago
; mrsh -o
set +o allexport
set +o notify
set +o noclobber
set +o errexit
set +o noglob
set +o monitor
set +o noexec
set +o ignoreeof
set +o nolog
set +o vi
set +o nounset
set +o verbose
set +o xtrace
Segmentation fault
hyphenrf commented 4 years ago

idk why argv is nulled (gdb says so) but anyway a sanity check seems harmless until the issue is handled on a deeper level by someone who knows better C than I do (lol)

From 094db405f7c854aac61241b04799fa18c0d04fd4 Mon Sep 17 00:00:00 2001
From: Haz <hyphens@pm.me>
Date: Mon, 15 Jun 2020 20:58:37 +0200
Subject: [PATCH] fix segfault on `mrsh -o`

---
 main.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/main.c b/main.c
index 0f433d2..9555081 100644
--- a/main.c
+++ b/main.c
@@ -64,6 +64,7 @@ int main(int argc, char *argv[]) {

    if (!(state->options & MRSH_OPT_NOEXEC)) {
        // If argv[0] begins with `-`, it's a login shell
+       if (state->frame->argv)
        if (state->frame->argv[0][0] == '-') {
            mrsh_source_profile(state);
        }
-- 
2.27.0
hyphenrf commented 4 years ago

idk if this tiny bug is worthy of a whole PR though (I read the contributing section)

jubalh commented 4 years ago

Why wouldn't it be worth a PR? What would be the downside of it?

hyphenrf commented 4 years ago

it's a one line patch and it's ad-hoc. idk where we're trying to read/write from an unallocated address or if that's the underlying problem even. I imagine those are things that warrant a proper PR

jubalh commented 4 years ago

If you would have formatted the patch properly its not one line. And it would be easier/quicker for a maintainer to hit merge than to do the changes himself or copy paste your patch.

hyphenrf commented 4 years ago

I'm gonna try to figure out the underlying cause and submit a proper patch. This might be good experience for me too!