Project

General

Profile

Actions

Bug #13718

closed

ksh: segfault on typeset -l/-u against special/read-only variables

Added by Andy Fiddaman 24 days ago. Updated 3 days ago.

Status:
Closed
Priority:
Normal
Assignee:
Category:
cmd - userland programs
Start date:
Due date:
% Done:

100%

Estimated time:
Difficulty:
Medium
Tags:
Gerrit CR:

Description

See https://github.com/ksh93/ksh/commit/3654ee73c03fb622dc878d6d2adb54dff13e0624

The following analysis is copied from there:

When using typeset -l or -u on a variable that cannot be changed
when the shell is in restricted mode, ksh crashed.

This fixed is inspired by this Red Hat fix, which is incomplete:
https://src.fedoraproject.org/rpms/ksh/blob/642af4d6/f/ksh-20120801-tpstl.patch

The crash was caused by the nv_shell() function. It walks though a
discipline function tree to get the pointer to the interpreter
associated with it. Evidently, the problem is that some pointer in
that walk is not set correctly for all special variables.

Thing is, ksh only has one shell language interpreter, and only one
global data structure (called 'sh') to keep its main state[*]. Yet,
the code is full of 'shp' pointers to that structure. Most (not
all) functions pass that pointer around to each other, accessing
that struct indirectly, ostensibly to account for the non-existent
possibility that there might be more than one interpreter state.
The "why" of that is an interesting cause for speculation that I
may get to sometime. For now, it is enough to know that, in the
code as it is, it matters not one iota what pointer to the shell
interpreter state is used; they all point to the same thing (unless
it's broken, as in this bug).

So, rather than fixing nv_shell() and/or associated pointer
assignments, this commit simply removes it, and replaces it with
calls to sh_getinterp(), which always returns a pointer to sh (see
init.c, where that function is defined as literally 'return &sh').

[*] Defined in shell.h, with the _SH_PRIVATE part in defs.h
Actions #1

Updated by Andy Fiddaman 24 days ago

A trivial reproducer is:

af@build:~$ typeset -u ENV
zsh: segmentation fault (core dumped)  ksh
Actions #2

Updated by Andy Fiddaman 23 days ago

Not being entirely happy with the fix in the mentioned commit, I looked into this.
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, if one looks at the EDITOR variable (some fields removed in the following for brevity)

> ::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
}
> e06208::print Namval_t nvfun | ::print Namfun_t next | ::print Namfun_t disc
disc = 0

The trailing list element, the libshell.so.1`sh+0x710 does not have a disc.

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
}

The trailing shell context pointer has been lost. This turns out to be a bug in the nv_disc() function, described as:

/*
 * push, pop, clne, or reorder disciplines onto node <np>
 * mode can be one of
 *    NV_FIRST:  Move or push <fp> to top of the stack or delete top
 *    NV_LAST:   Move or push <fp> to bottom of stack or delete last
 *    NV_POP:    Delete <fp> from top of the stack
 *    NV_CLONE:  Replace fp with a copy created my malloc() and return it
 */
Namfun_t *nv_disc(register Namval_t *np, register Namfun_t* fp, int mode)

When called with NV_LAST, any trailing shell context pointer is dropped from the end of the list.

Actions #3

Updated by Andy Fiddaman 23 days ago

  • Subject changed from ksh segfault on typeset -l/-u against special/read-only variables to ksh: segfault on typeset -l/-u against special/read-only variables
Actions #4

Updated by Electric Monk 23 days ago

  • Gerrit CR set to 1418
Actions #5

Updated by Andy Fiddaman 22 days ago

PR also opened upstream at https://github.com/ksh93/ksh/pull/276

Actions #6

Updated by Andy Fiddaman 3 days ago

I've tested that the original crashes no longer occur, and that the new tests added as part of this change pass successfully.
I've also had the patched ksh running on an OmniOS system for several weeks and noticed no problems running nightly builds etc.

Actions #7

Updated by Electric Monk 3 days ago

  • Status changed from In Progress to Closed
  • % Done changed from 0 to 100

git commit 5b8dd251e631f58e50129eeb313228a0646df3fe

commit  5b8dd251e631f58e50129eeb313228a0646df3fe
Author: Andy Fiddaman <omnios@citrus-it.co.uk>
Date:   2021-05-04T18:50:34.000Z

    13718 ksh: segfault on typeset -l/-u against special/read-only variables
    Portions contributed by:  Martijn Dekker <martijn@inlv.org>
    Reviewed by: Robert Mustacchi <rm@fingolfin.org>
    Approved by: Dan McDonald <danmcd@joyent.com>

Actions

Also available in: Atom PDF