[PATCH] Fortify zrealloc append to arrays

[PATCH] Fortify zrealloc append to arrays

[Sebastian Gniazdowski]

[-- Attachment #1: Type: text/plain, Size: 645 bytes --]

Hello,
one user of my project reports crash with message about realloc(), when pasting:

$ openssl req -new -newkey rsa:4096 > regisrealloc(): invalid old size
Connection to localhost closed.

I looked at my code that introduced realloc() to array appends. It
seems that its correctness is guarded by this: before patch, old
pointer (old array) was subject to arrsetfn, which does freearray().
So if string can be freed, it for sure can be realloc()-ed.

That said I have a patch that checks if old pointer isn't nullarray
(static variable) and has the standard getter. A fortification, to
sleep better.

-- 
Best regards,
Sebastian Gniazdowski

[-- Attachment #2: append_nular_fortify.diff.txt --]
[-- Type: text/plain, Size: 1117 bytes --]

diff --git a/Src/params.c b/Src/params.c
index f130934..95272b7 100644
--- a/Src/params.c
+++ b/Src/params.c
@@ -150,6 +150,8 @@ rprompt_indent_unsetfn(Param pm, int exp);
 
 /* Standard methods for get/set/unset pointers in parameters */
 
+static char *nullarray = NULL;
+
 /**/
 mod_export const struct gsu_scalar stdscalar_gsu =
 { strgetfn, strsetfn, stdunsetfn };
@@ -2803,7 +2805,8 @@ setarrvalue(Value v, char **val)
             if (post_assignment_length > pre_assignment_length &&
                     pre_assignment_length <= v->start &&
                     pre_assignment_length > 0 &&
-                    v->pm->gsu.a->setfn == arrsetfn)
+                    v->pm->gsu.a->setfn == arrsetfn && v->pm->gsu.a->getfn == arrgetfn &&
+                    old != &nullarray)
             {
                 p = new = (char **) zrealloc(old, sizeof(char *)
                                            * (post_assignment_length + 1));
@@ -3788,8 +3791,6 @@ strsetfn(Param pm, char *x)
 
 /* Function to get value of an array parameter */
 
-static char *nullarray = NULL;
-
 /**/
 char **
 arrgetfn(Param pm)

Re: [PATCH] Fortify zrealloc append to arrays

[Sebastian Gniazdowski]

I luckily obtained segfaults and coredumps. Ran -O0 zsh, waited for
segfault, and here is backtrace:

(lldb) bt
* thread #1, stop reason = signal SIGSTOP
  * frame #0: 0x0000000103f81e18 zle.so`getkeybuf(w=0) at zle_keymap.c:1679
    frame #1: 0x0000000103f81b26
zle.so`getkeymapcmd(km=0x00007faa2801d800, funcp=0x00007ffeebdeaab0,
strp=0x00007ffeebdeaa98) at zle_keymap.c:1587
    frame #2: 0x0000000103f81f73 zle.so`getkeycmd at zle_keymap.c:1705
    frame #3: 0x0000000103f84b7c zle.so`zlecore at zle_main.c:1129
    frame #4: 0x0000000103f85575 zle.so`zleread(lp=0x0000000103ee2bf8,
rp=0x0000000103ee2c38, flags=3, context=0, init="zle-line-init",
finish="zle-line-finish") at zle_main.c:1352
    frame #5: 0x0000000103f863c3 zle.so`zle_main_entry(cmd=1,
ap=0x00007ffeebdeb090) at zle_main.c:2109
    frame #6: 0x0000000103e60f67 zsh`zleentry(cmd=1) at init.c:1602
    frame #7: 0x0000000103e6265d zsh`inputline at input.c:295
    frame #8: 0x0000000103e62255 zsh`ingetc at input.c:228
    frame #9: 0x0000000103e541bd zsh`ihgetc at hist.c:407
    frame #10: 0x0000000103e6bd36 zsh`gettok at lex.c:611

I've listed source and obtained:

frame #0: 0x0000000103f81e18 zle.so`getkeybuf(w=0) at zle_keymap.c:1679
   1676        int c = getbyte((long)w, NULL);
   1677
   1678        if(c < 0)
-> 1679        return EOF;
   1680        addkeybuf(c);
   1681        return c;
   1682    }

Where could be segfault hiding? I know one thing. If I comment out
following line with <():

                         exec {PCFD}< <(echo ${sysparams[pid]})

I get no segfault nor other (Ctrl-C) problems. Interior of <( ) can be
different from above, the same happens.

On 17 June 2018 at 16:39, Sebastian Gniazdowski <sgniazdowski@gmail.com> wrote:
> Hello,
> one user of my project reports crash with message about realloc(), when pasting:
>
> $ openssl req -new -newkey rsa:4096 > regisrealloc(): invalid old size
> Connection to localhost closed.
>
> I looked at my code that introduced realloc() to array appends. It
> seems that its correctness is guarded by this: before patch, old
> pointer (old array) was subject to arrsetfn, which does freearray().
> So if string can be freed, it for sure can be realloc()-ed.
>
> That said I have a patch that checks if old pointer isn't nullarray
> (static variable) and has the standard getter. A fortification, to
> sleep better.
>
> --
> Best regards,
> Sebastian Gniazdowski