1
0
Fork 0
mirror of git://git.code.sf.net/p/cdesktopenv/code synced 2025-03-09 15:50:02 +00:00

Robustify ${.sh.level} scope switching (re: 69d37d5e, e1c41bb2)

Switching the function scope to a parent scope by assigning to
.sh.level (SH_LEVELNOD) leaves the shell in an inconsistent state,
causing invalid-free and/or use-after-free bugs. The intention of
.sh.level was always to temporarily switch scopes inside a DEBUG
trap, so this commit minimises the pitfalls and instability by
imposing some sensible limitations:
1. .sh.level is now a read-only variable except while executing a
   DEBUG trap;
2. while it's writeable, attempts to unset .sh.level or to change
   its attributes are ignored;
3. attempts to set a discipline function for .sh.level are ignored;
4. it is an error to set a level < 0 or > the current scope.

Even more crashing bugs are fixed by simplifiying the handling and
initialisation of .sh.level and by exempting it completely from
virtual subshell scoping (to which it's irrelevant).

TODO: one thing remains: scope corruption and use-after-free happen
when using the '.' command inside a DEBUG trap with ${.sh.level}
changed. Behaviour same as before this commit. To be investigated.

All changed files:
- Consistently use the int16_t type for level values as that is the
  type of its non-pointer storage in SH_LEVELNOD.
- Update .sh.level by using an update_sh_level() macro that assigns
  directly to the node value, then restores the scope if needed.
- To eliminate implicit typecasts, use the same int16_t type (the
  type used by short ints such as SH_LEVELNOD) for all variables
  containing a function and/or dot script level.

src/cmd/ksh93/include/variables.h:
- Add update_sh_level() macro.

src/cmd/ksh93/include/name.h,
src/cmd/ksh93/sh/macro.c:
- Add a nv_nonptr() macro that checks attributes for a non-pointer
  value -- currently only signed or unsigned short integer value,
  accessed via the 's' member of 'union Value' (e.g. np->nvalue.s).
- nv_isnull(): To avoid undefined behaviour, check for attributes
  indicating a non-pointer value before accessing the nvalue.cp
  pointer (re: 5aba0c72).
- varsub(): In the set/unset check, remove the now-redundant
  exception for SH_LEVELNOD.

src/cmd/ksh93/data/variables.c,
src/cmd/ksh93/sh/init.c:
- shtab_variables[]: Make .sh.level a read-only short integer.
- sh_inittree(): To avoid undefined behaviour, do not assign to the
  'union Value' char pointer if the attribute indicates a non-
  pointer short integer value. Instead, the table value is ignored.

src/cmd/ksh93/sh/subshell.c: sh_assignok():
- Never create a subshell scope for SH_LEVELNOD.

src/cmd/ksh93/sh/xec.c:
- Get rid of 'struct Level' and its maxlevel member. This was only
  used in put_level() to check for an out of range assignment, but
  this can be trivially done by checking sh.fn_depth+sh.dot_depth.
- This in turn allows further simplification that reduces init for
  .sh.level to a single nv_disc() call in sh_debug(), so get rid of
  init_level().
- put_level(): Throw a "level out of range" error if assigned a
  wrong level.
- sh_debug():
  - Turn off the NV_RDONLY (read-only) attribute for SH_LEVELNOD
    while executing the DEBUG trap.
  - Restore the current scope when trap execution is finished.
- sh_funct(): Remove all .sh.level handling. POSIX functions (and
  dot scripts) already handle it in b_dot_cmd(), so sh_funct(),
  which is used by both, is the wrong place to do it.
- sh_funscope(): Update .sh.level for ksh syntax functions here
  instead. Also, do not bother to initialise its discipline here,
  as it can now only be changed in a DEBUG trap.

src/cmd/ksh93/bltins/typeset.c: setall():
- When it's not read-only, ignore all attribute changes for
  .sh.level, as changing the attributes would crash the shell.

src/cmd/ksh93/sh/nvdisc.c: nv_setdisc():
- Ignore all attempts to set a discipline function for .sh.level,
  as doing this would crash the shell.

src/cmd/ksh93/bltins/misc.c: b_dot_cmd():
- Bug fix: also update .sh.level when quitting a dot script.

src/cmd/ksh93/sh/name.c:
- _nv_unset():
  - To avoid an inconsistent state, ignore all attempts to unset
    .sh.level.
  - To avoid undefined behaviour, do not zero np->nvalue.cp if
    attributes for np indicate a non-pointer value (the actual bit
    value of a null pointer is not defined by the standard, so
    there is no guarantee that zeroing .cp will zero .s).
- sh_setscope(): For consistency, always set error_info.id (the
  command name for error messages) to the new scope's cmdname.
  Previously this was only done for two calls of this function.
- nv_name(): Fix a crashing bug by checking that np->nvname is a
  non-null pointer before dereferencing it.

src/cmd/ksh93/include/nval.h:
- The NV_UINT16P macro (which is unsigned NV_INT16P) had a typo in
  it, which went unnoticed for many years because it's not directly
  used (though its bit flags are set and used indirectly). Let's
  fix it anyway and keep it for completeness' sake.
This commit is contained in:
Martijn Dekker 2022-07-12 17:48:44 +02:00
parent 3ce064bbba
commit ffee9100d5
19 changed files with 134 additions and 110 deletions

View file

@ -192,10 +192,11 @@ struct Namval
#define NV_PUBLIC (~(NV_NOSCOPE|NV_ASSIGN|NV_IDENT|NV_VARNAME|NV_NOADD))
/* numeric types */
/* NV_INT16 and NV_UINT16 store values directly in the node; all the others use pointers */
#define NV_INT16P (NV_LJUST|NV_SHORT|NV_INTEGER)
#define NV_INT16 (NV_SHORT|NV_INTEGER)
#define NV_UINT16P (NV_LJUST|NV_UNSIGN|NV_SHORT|NV_INTEGER)
#define NV_UINT16 (NV_UNSIGN|NV_SHORT|NV_INTEGER)
#define NV_UINT16P (NV_LJUSTNV_UNSIGN|NV_SHORT|NV_INTEGER)
#define NV_INT32 (NV_INTEGER)
#define NV_UNT32 (NV_UNSIGN|NV_INTEGER)
#define NV_INT64 (NV_LONG|NV_INTEGER)