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

nv_disc(NV_LAST) loses trailing shell context from discipline stack #276

Merged
merged 1 commit into from
Apr 15, 2021

Conversation

citrus-it
Copy link

This is the underlying cause for the issue worked around in
3654ee73c03fb622dc878d6d

Each environment variable (name/value pair) has a linked list of disciplines
attached to it, and at the end of that list there is optionally a shell context
pointer. For example, for the EDITOR variable:

> ::bp libshell.so.1`put_ed
> ::run
$
$ EDITOR=vim
> ::stack ! head -1
libshell.so.1`put_ed+0x14(e06208, e01c58, 0, dced90)
> e06208::print Namval_t
{
    nvname = 0xfffffbffeec40a0e "EDITOR"
    nvfun = 0xdced90
    nvalue = 0
}
> e06208::print Namval_t nvfun | ::print Namfun_t
{
    disc = libshell.so.1`EDITOR_disc
    next = libshell.so.1`sh+0x710
}

Here, the EDITOR Namval_t has a discipline stack containing
EDITOR_disc and &Shell_t.nvfun.

The problem arises when a new discipline is pushed onto the stack, such as when
using typeset -u to add an upper-case translation discipline.

$ typeset -u EDITOR
> e06208::print Namval_t
{
    nvname = 0xfffffbffeec40a0e "EDITOR"
    nvfun = 0xdced90
    nvalue = 0xe0fdb0 "vim"
}
> e06208::print Namval_t nvfun | ::print Namfun_t
{
    disc = libshell.so.1`EDITOR_disc
    next = 0xdc27a0
}
> e06208::print Namval_t nvfun | ::print Namfun_t next | ::print Namfun_t
{
    disc = libshell.so.1`TRANS_disc
    next = 0
}

TRANS_disc has been pushed onto the end of the discipline stack, but the
shell handle has been lost.

With this change, the attributes and variables tests pass (this is on
illumos where this change originates).

This is the underlying cause for the issue worked around in
  ksh93@3654ee73c03fb622dc878d6d

Each environment variable (name/value pair) has a linked list of disciplines
attached to it, and at the end of that list there is optionally a shell context
pointer. For example, for the EDITOR variable:

	> ::bp libshell.so.1`put_ed
	> ::run
	$
	$ EDITOR=vim
	> ::stack ! head -1
	libshell.so.1`put_ed+0x14(e06208, e01c58, 0, dced90)
	> e06208::print Namval_t
	{
	    nvname = 0xfffffbffeec40a0e "EDITOR"
	    nvfun = 0xdced90
	    nvalue = 0
	}
	> e06208::print Namval_t nvfun | ::print Namfun_t
	{
	    disc = libshell.so.1`EDITOR_disc
	    next = libshell.so.1`sh+0x710
	}

Here, the EDITOR Namval_t has a discipline stack containing
EDITOR_disc and &Shell_t.nvfun.

The problem arises when a new discipline is pushed onto the stack, such as when
using typeset -u to add an upper-case translation discipline.

    $ typeset -u EDITOR
    > e06208::print Namval_t
    {
        nvname = 0xfffffbffeec40a0e "EDITOR"
        nvfun = 0xdced90
        nvalue = 0xe0fdb0 "vim"
    }
    > e06208::print Namval_t nvfun | ::print Namfun_t
    {
        disc = libshell.so.1`EDITOR_disc
        next = 0xdc27a0
    }
    > e06208::print Namval_t nvfun | ::print Namfun_t next | ::print Namfun_t
    {
        disc = libshell.so.1`TRANS_disc
        next = 0
    }

TRANS_disc has been pushed onto the end of the discipline stack, but the
shell handle has been lost.

With this change, the attributes and variables tests pass (this is on
illumos where this change originates).
@McDutchie
Copy link

McDutchie commented Apr 15, 2021

Welcome and thanks for the fix. Looks good to me. I can confirm that this fixes the bug worked around in 3654ee7 even after reverting that commit. Not that we need to revert it -- in fact I plan at some point to detransition the code from passing around so many shp pointers, as there is only one static sh struct that they all point to.

On a related note, it would be nice to prevent further fragmentation of the ksh landscape. Would you consider switching to 93u+m as the upstream for illumos? If there are things that make that undesirable, what are they? Perhaps we can fix them. We're close to releasing the first 93u+m beta, so now or soon would be a good time to make any required changes.

@McDutchie McDutchie merged commit 2fdf394 into ksh93:master Apr 15, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants