[PATCH] allocate origline by ztrdup(), not by dupstring()

[PATCH] allocate origline by ztrdup(), not by dupstring()

[Jun T.]

Currently, domenuselect() allocates origline in heap by dupstring(),
but if menuselect() is called directly as a widget then the heap has
already been freed (and may have been re-used for other variables).
---
 Src/Zle/complist.c   | 6 ++++--
 Src/Zle/zle_tricky.c | 3 ++-
 2 files changed, 6 insertions(+), 3 deletions(-)

diff --git a/Src/Zle/complist.c b/Src/Zle/complist.c
index b6e7e30..8850506 100644
--- a/Src/Zle/complist.c
+++ b/Src/Zle/complist.c
@@ -2882,7 +2882,8 @@ domenuselect(Hookdef dummy, Chdata dat)
 	    brend = dupbrinfo(u->brend, &lastbrend, 0);
 	    nbrbeg = u->nbrbeg;
 	    nbrend = u->nbrend;
-	    origline = u->origline;
+	    zsfree(origline);
+	    origline = ztrdup(u->origline);
 	    origcs = u->origcs;
 	    origll = u->origll;
             strcpy(status, u->status);
@@ -3236,7 +3237,8 @@ domenuselect(Hookdef dummy, Chdata dat)
 		 * don't want that, just what the user typed,
 		 * so restore the information.
 		 */
-                origline = modeline;
+		zsfree(origline);
+		origline = ztrdup(modeline);
                 origcs = modecs;
                 origll = modell;
                 zlemetacs = 0;
diff --git a/Src/Zle/zle_tricky.c b/Src/Zle/zle_tricky.c
index 864f804..950c22f 100644
--- a/Src/Zle/zle_tricky.c
+++ b/Src/Zle/zle_tricky.c
@@ -634,7 +634,8 @@ docomplete(int lst)
     metafy_line();
 
     ocs = zlemetacs;
-    origline = dupstring(zlemetaline);
+    zsfree(origline);
+    origline = ztrdup(zlemetaline);
     origcs = zlemetacs;
     origll = zlemetall;
     if (!isfirstln && (chline != NULL || zle_chline != NULL)) {
-- 
1.9.3 (Apple Git-50)

Re: [PATCH] allocate origline by ztrdup(), not by dupstring()

[Jun T.]

2015/01/07 01:37, Jun T. <takimoto-j@kba.biglobe.ne.jp> wrote:

> Currently, domenuselect() allocates origline in heap by dupstring(),

Sorry, I meant "docomplete() allocates ...".

Jun

Re: [PATCH] allocate origline by ztrdup(), not by dupstring()

[Oliver Kiddle]

On 7 Jan, "Jun T." wrote:
> Currently, domenuselect() allocates origline in heap by dupstring(),
> but if menuselect() is called directly as a widget then the heap has
> already been freed (and may have been re-used for other variables).

I noticed problems which I traced back to this change.

> diff --git a/Src/Zle/complist.c b/Src/Zle/complist.c
> @@ -2882,7 +2882,8 @@ domenuselect(Hookdef dummy, Chdata dat)
> -	    origline = u->origline;
> +	    zsfree(origline);
> +	    origline = ztrdup(u->origline);

This part is run when pressing backspace, it seems to always be the case
that origline == u->origline before this block runs. So it is copying freed
memory. I tried the obvious change of checking that condition first but
that seems to result in deleted characters being treated as if they were
part of the original string before completion. It is also calling zsfree
on origline before ther very first time that it is initialised.

I think it might be easier to approach this by revisiting the original
problem. I've tried binding a widget direct to menuselect but haven't
been able to get anything odd to happen.

origline is initialised in docomplete. pushheap() is done later in
do_completion(). That would seem not to be a problem for a later
menu-select widget accessing origline. I am, however, suspicious of the
line which does origline = modeline because the value of modeline does,
I think, come from dupstring() while completion is active. Setting a
breakpoint at that line, I've only managed to trigger it when origline
and modeline are already equal (in strcmp terms) so I'm not sure of the
situation that code is there to handle. Any ideas?

Oliver

Re: [PATCH] allocate origline by ztrdup(), not by dupstring()

[Jun T.]

2015/06/17 01:54, Oliver Kiddle <okiddle@yahoo.co.uk> wrote:

> it seems to always be the case
> that origline == u->origline before this block runs. So it is copying freed
> memory. I tried the obvious change of checking that condition first but
> that seems to result in deleted characters being treated as if they were
> part of the original string before completion.

Yes, my patch is obviously wrong here. I guess the problem is that, near
line 2683 of complist.c, the value of origline is saved in s->origline,
but the memory pointed to by origline will be zsfree'ed while s->origline
is still pointing to the memory.

> It is also calling zsfree
> on origline before ther very first time that it is initialised.

Initially origline is NULL, and zsfree() will do nothing for that case.


> I think it might be easier to approach this by revisiting the original
> problem. I've tried binding a widget direct to menuselect but haven't
> been able to get anything odd to happen.

I can't remember exactly what was the problem I was trying to solve.
But if I checkout the commit b11f7a7e3028420d027e71713d5ec1cae0451fbb
(i.e., revert my patch), then I have the following problem (Mac OS X 10.9.5):

$ ~/test/bin/zsh -f             # zsh-5.0.7-194-gb11f7a7
% modload zsh/complist
% autoload -U compinit
% compinit
% bindkey '^K' menu-select
% bindkey -M menuselect '^E' vi-insert
% mkdir tmp && cd tmp
% touch foo bar boo
% ls <TAB><TAB><ctrl-K><ctrl-E>

and the command line is not updated correctly.

In gdb attached to the above zsh, after 'ls <TAB><TAB>',

(gdb) print origline
$12 = 0x7fc412802870 "ls "
(gdb) print *(char*)0x7fc412802873    # 0x7fc412802870 + 3
$13 = 0 '\0'

then I set a watchpoint at 0x7fc412802873 as

(gdb) watch *(char*)0x7fc412802873
(gdb) cont

and hit <ctrl-K> in the zsh gives:

Hardware watchpoint 4: *(char *) 140480100706419   # = 0x7fc412802873

Old value = 0 '\0'
New value = 98 'b'
0x00007fff8bd9e042 in _platform_memmove$VARIANT$Nehalem ()
(gdb) print origline
$4 = 0x7fc412802870 "ls bo"          # address has not changed
(gdb) bt
#0  0x00007fff8bd9e042 in _platform_memmove$VARIANT$Nehalem ()
#1  0x00007fff908577e3 in stpcpy ()
#2  0x00007fff908c5a44 in __strcpy_chk ()
#3  0x000000010975ec49 in dupstring (s=0x7fc411c29ba0 "ls bar") at string.c:40
#4  0x00000001099246e4 in domenuselect (dummy=0x0, dat=0x0) at complist.c:2551
#5  0x0000000109922f78 in menuselect (args=0x1098bae60) at complist.c:3428
#6  0x0000000109899b36 in completecall (args=0x1098bae60) at zle_tricky.c:208
#7  0x0000000109883919 in execzlefunc (func=0x7fc411c04d20, args=0x1098bae60, set_bindk=0) at zle_main.c:1345
#8  0x0000000109883454 in zlecore () at zle_main.c:1066
#9  0x00000001098844ba in zleread (lp=0x109797ad0, rp=0x0, flags=3, context=0, init=0x1098b23c8 "zle-line-init", finish=0x1098b23d6 "zle-line-finish") at zle_main.c:1253
#10 0x00000001098851bb in zle_main_entry (cmd=1, ap=0x7fff5653c690) at zle_main.c:1914
...

So the memory pointed to by origline (0x7fc412802870 ...) seems to be
reused by dupstring(). Does this mean that the heap in which origline
is allocated is popped somewhere?

I'm quite busy now and may not have time to go into any more detail
at least for a few days. Sorry.

Jun

Re: [PATCH] allocate origline by ztrdup(), not by dupstring()

[Jun T.]

2015/06/17 01:54, Oliver Kiddle <okiddle@yahoo.co.uk> wrote:

> This part is run when pressing backspace, it seems to always be the case
> that origline == u->origline before this block runs. So it is copying freed
> memory.

Backspace (in the interactive mode) seems to work as expected with the
following patch. Does this look reasonable?


diff --git a/Src/Zle/complist.c b/Src/Zle/complist.c
index 0f73181..ccee9a7 100644
--- a/Src/Zle/complist.c
+++ b/Src/Zle/complist.c
@@ -2655,7 +2655,7 @@ domenuselect(Hookdef dummy, Chdata dat)
 	    s->nbrbeg = nbrbeg;
 	    s->nbrend = nbrend;
 	    s->nmatches = nmatches;
-	    s->origline = origline;
+	    s->origline = dupstring(origline);
 	    s->origcs = origcs;
 	    s->origll = origll;
             s->status = dupstring(status);
@@ -2786,7 +2786,7 @@ domenuselect(Hookdef dummy, Chdata dat)
 	    s->nbrbeg = nbrbeg;
 	    s->nbrend = nbrend;
 	    s->nmatches = nmatches;
-	    s->origline = origline;
+	    s->origline = dupstring(origline);
 	    s->origcs = origcs;
 	    s->origll = origll;
             s->status = dupstring(status);

Re: [PATCH] allocate origline by ztrdup(), not by dupstring()

[Peter Stephenson]

On Sun, 21 Jun 2015 20:53:49 +0900
Jun T. <takimoto-j@kba.biglobe.ne.jp> wrote:
> 2015/06/17 01:54, Oliver Kiddle <okiddle@yahoo.co.uk> wrote:
> 
> > This part is run when pressing backspace, it seems to always be the case
> > that origline == u->origline before this block runs. So it is copying freed
> > memory.
> 
> Backspace (in the interactive mode) seems to work as expected with the
> following patch. Does this look reasonable?

I certainly can't see how this could have bad effects.

pws