emersion / mrsh

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

Fix segfault on `mrsh -o` #165

Closed hyphenrf closed 4 years ago

hyphenrf commented 4 years ago

addresses issue #164 (github) putting the PR here as I work on the cause

hyphenrf commented 4 years ago

I'm suspecting something is supposed to set state->frame but isn't. I've tried to set a watchpoint on frame using gdb. And I'm looking into the execution path going through shell/shell.c. From the point frame_priv hands its pub pointer to state, state->frame never changes from its initial value. my suspects are mrsh_process_args function, and I would've suspected interactive_init function but I built without readline.

What's state->frame supposed to do as opposed to just trying to access argv directly from main?

hyphenrf commented 4 years ago

ok so in builtin/set.c, precisely the set function, the reason we never get frame set to a value is because of a return here.

        switch (argv[i][1]) {
        case 'o':
            if (i + 1 == argc) {
                print_options(state);
                return 0;
            }

this seems to be the block responsible for setting up state->frame, the one being missed by our return, but I have no idea what's going on in it yet.

    if (i != argc || force_positional) {
        char *argv_0;
        if (init_args != NULL) {
            argv_0 = strdup(argv[i++]);
            init_args->command_file = argv_0;
        } else {
            argv_0 = strdup(state->frame->argv[0]);
        }
        argv_free(state->frame->argc, state->frame->argv);
        state->frame->argc = argc - i + 1;
        state->frame->argv = argv_dup(argv_0, state->frame->argc, &argv[i]);
    } else if (init_args != NULL) {
        // No args given, but we need to initialize state->argv
        state->frame->argc = 1;
        state->frame->argv = argv_dup(strdup(argv[0]), 1, argv);
    }
hyphenrf commented 4 years ago

aha this is embarrassing. first time doing this stuff on github. it says "changes requested", do i click "resolve conversation"? or should that be something the reviewer does?

Edit: I think I have to request re-review