PATCH: Fix use-after-free for print -zf and print -sf

PATCH: Fix use-after-free for print -zf and print -sf

[Mikael Magnusson]

% print -zf hi
 mem.c:1525: BUG: attempt to free already free storage                                        
% XY<81>^A

This is only visible (for me) with --enable-zsh-mem, but the bug is
always there. As the error message says, we do a double free. The reason
is that while we fflush the open_memstream'd fout, which updates buf,
we then after copying the pointer do fclose, which updates the pointer
again and frees the old memory area. Boom.

I don't know what the nicest way to handle this is. We could do what
this patch does, or we could copy the contents of buf after flushing,
but that would cause two extra pointless copies compared to this patch.

We could also set fout = stdout to avoid changing the final if, but I
recall someone complained about the extra fflush of stdout on my other
patch a while back.

---
 Src/builtin.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/Src/builtin.c b/Src/builtin.c
index 19155fd..e0f6a5c 100644
--- a/Src/builtin.c
+++ b/Src/builtin.c
@@ -4526,7 +4526,8 @@ bin_print(char *name, char **args, Options ops, int func)
     if (OPT_ISSET(ops,'z') || OPT_ISSET(ops,'s')) {
 #ifdef HAVE_OPEN_MEMSTREAM
 	putc(0, fout);
-	fflush(fout);
+	fclose(fout);
+	fout = NULL;
 #else
 	rewind(fout);
 	buf = (char *)zalloc(count + 1);
@@ -4548,7 +4549,7 @@ bin_print(char *name, char **args, Options ops, int func)
     }
 
     /* Testing EBADF special-cases >&- redirections */
-    if ((fout != stdout) ? (fclose(fout) != 0) :
+    if (fout && (fout != stdout) ? (fclose(fout) != 0) :
 	(fflush(fout) != 0 && errno != EBADF)) {
 	zwarnnam(name, "write error: %e", errno);
 	ret = 1;
-- 
2.2.0.GIT

Re: PATCH: Fix use-after-free for print -zf and print -sf

[Mikael Magnusson]

On Tue, Feb 10, 2015 at 8:12 AM, Mikael Magnusson <mikachu@gmail.com> wrote:
>      /* Testing EBADF special-cases >&- redirections */
> -    if ((fout != stdout) ? (fclose(fout) != 0) :
> +    if (fout && (fout != stdout) ? (fclose(fout) != 0) :

Do I need an extra set of parens here? C precedence rules are fun.

-- 
Mikael Magnusson

Re: PATCH: Fix use-after-free for print -zf and print -sf

[Peter Stephenson]

On Tue, 10 Feb 2015 08:16:56 +0100
Mikael Magnusson <mikachu@gmail.com> wrote:
> On Tue, Feb 10, 2015 at 8:12 AM, Mikael Magnusson <mikachu@gmail.com> wrote:
> >      /* Testing EBADF special-cases >&- redirections */
> > -    if ((fout != stdout) ? (fclose(fout) != 0) :
> > +    if (fout && (fout != stdout) ? (fclose(fout) != 0) :
> 
> Do I need an extra set of parens here? C precedence rules are fun.
> 

https://en.wikipedia.org/wiki/Operators_in_C_and_C%2B%2B#Operator_precedence

"&&" is higher precedence than && so this looks OK.

I suppose it's at the point where a lot of people would decide it needed
expanding into something clearer, but it's a reasonable extension of
what was there before.  Can't see a problem with it.

pws

Re: PATCH: Fix use-after-free for print -zf and print -sf

[Mikael Magnusson]

On Tue, Feb 10, 2015 at 10:37 AM, Peter Stephenson
<p.stephenson@samsung.com> wrote:
> On Tue, 10 Feb 2015 08:16:56 +0100
> Mikael Magnusson <mikachu@gmail.com> wrote:
>> On Tue, Feb 10, 2015 at 8:12 AM, Mikael Magnusson <mikachu@gmail.com> wrote:
>> >      /* Testing EBADF special-cases >&- redirections */
>> > -    if ((fout != stdout) ? (fclose(fout) != 0) :
>> > +    if (fout && (fout != stdout) ? (fclose(fout) != 0) :
>>
>> Do I need an extra set of parens here? C precedence rules are fun.
>>
>
> https://en.wikipedia.org/wiki/Operators_in_C_and_C%2B%2B#Operator_precedence
>
> "&&" is higher precedence than ?: so this looks OK.
>
> I suppose it's at the point where a lot of people would decide it needed
> expanding into something clearer, but it's a reasonable extension of
> what was there before.  Can't see a problem with it.

Oops, actually I think I do need it, since I want fout being NULL to
short circuit the whole if statement to false, and currently it's
interpreted as if ((fout && (fout != stdout)) ? .. : ..), resulting in
a call to fflush(NULL) which flushes all open output buffers. I should
probably have quoted both lines of the if. :)

-- 
Mikael Magnusson

Re: PATCH: Fix use-after-free for print -zf and print -sf

[Peter Stephenson]

On Tue, 10 Feb 2015 12:13:12 +0100
Mikael Magnusson <mikachu@gmail.com> wrote:
> Oops, actually I think I do need it, since I want fout being NULL to
> short circuit the whole if statement to false, and currently it's
> interpreted as if ((fout && (fout != stdout)) ? .. : ..), resulting in
> a call to fflush(NULL) which flushes all open output buffers. I should
> probably have quoted both lines of the if. :)

Yes, I missed that... that's kind of why I talked about rewriting it...

pws

Re: PATCH: Fix use-after-free for print -zf and print -sf

[Mikael Magnusson]

On Tue, Feb 10, 2015 at 12:18 PM, Peter Stephenson
<p.stephenson@samsung.com> wrote:
> On Tue, 10 Feb 2015 12:13:12 +0100
> Mikael Magnusson <mikachu@gmail.com> wrote:
>> Oops, actually I think I do need it, since I want fout being NULL to
>> short circuit the whole if statement to false, and currently it's
>> interpreted as if ((fout && (fout != stdout)) ? .. : ..), resulting in
>> a call to fflush(NULL) which flushes all open output buffers. I should
>> probably have quoted both lines of the if. :)
>
> Yes, I missed that... that's kind of why I talked about rewriting it...
>
> pws

-    /* Testing EBADF special-cases >&- redirections */
-    if (fout && ((fout != stdout) ? (fclose(fout) != 0) :
-       (fflush(fout) != 0 && errno != EBADF))) {
-       zwarnnam(name, "write error: %e", errno);
-       ret = 1;
-    }
+#ifdef HAVE_OPEN_MEMSTREAM
+    if (fout)
+#endif
+       /* Testing EBADF special-cases >&- redirections */
+       if ((fout != stdout) ? (fclose(fout) != 0) :
+           (fflush(fout) != 0 && errno != EBADF)) {
+           zwarnnam(name, "write error: %e", errno);
+           ret = 1;
+       }
     return ret;
 }

This would make it very clear that fout can only be NULL in one very
particular case, and also leaves the main EBADF thing the same as the
other places in the function... (sorry for bikeshedding so much over
this). Which of these do you prefer?

-- 
Mikael Magnusson

Re: PATCH: Fix use-after-free for print -zf and print -sf

[Peter Stephenson]

On Tue, 10 Feb 2015 12:32:52 +0100
Mikael Magnusson <mikachu@gmail.com> wrote:
> +#ifdef HAVE_OPEN_MEMSTREAM
> +    if (fout)
> +#endif
> +       /* Testing EBADF special-cases >&- redirections */
> +       if ((fout != stdout) ? (fclose(fout) != 0) :
> +           (fflush(fout) != 0 && errno != EBADF)) {
> +           zwarnnam(name, "write error: %e", errno);
> +           ret = 1;
> +       }

This looks fine although based on experience I'd tend to write it as...

#ifdef HAVE_OPEN_MEMSTREAM
    if (fout)
#endif
    {
       /* Testing EBADF special-cases >&- redirections */
       if ((fout != stdout) ? (fclose(fout) != 0) :
           (fflush(fout) != 0 && errno != EBADF)) {
           zwarnnam(name, "write error: %e", errno);
           ret = 1;
       }
    }

for clarity.  The braces are harmless without HAVE_OPEN_MEMSTREAM and
explain the indentation --- otherwise you may well find lint or Coverity
wailing they don't understand the indentation.

(... anyone remember lint ...?)

pws