Re: PR/45269 CVS commit: src/bin/sh

[David Laight <david%l8s.co.uk@localhost>]

The following reply was made to PR bin/45269; it has been noted by GNATS.

From: David Laight <david%l8s.co.uk@localhost>
To: gnats%netbsd.org@localhost, Andreas Gustafsson <gson%gson.org@localhost>
Cc: 
Subject: Re: PR/45269 CVS commit: src/bin/sh
Date: Sat, 3 Sep 2011 23:08:59 +0100

 On Tue, Aug 23, 2011 at 01:20:03PM +0000, David Laight wrote:
 >  
 >  Either I'm going blind or something else is wrong.
 >  I don't believe the netbsd code in rev 1.84 is wrong.
 >  The code prior to rev 1.68 was wrong, the fix from freebsd
 >  might have been removing the old version of that code.
 
 Ok, I think this is what happens - but I've not managed to generate
 a short script that exhibits the failure.
 
 1) expbackq() is called with 'expdest' pointing to the start of
    a chunk of stack (and not the first chunk).
    This also means it is the start of a token.
 
 2) grabstackstr() doesn't need to update the pointers - so expdest
    is still at the start of the stack.
 
 3) evalbackcmd() starts building a string on the stack (for something)
    and exceeds the blcok size - so calls growstackblock()
 
 4) in growstackblock() the condition:
        if (stacknxt == stackp->space && stackp != &stackbase) {
    is true. So the memory containing the string is realloc()ed.
 
 All is now lost. The 'dest' pointer within expbackq() - the original
 'expdest' now points into freed memory.
 
 This all means I'm not sure how it works as well as it does!
 
 In any case this has nothing to do with how the trailing newlines are
 removed. For instance, if 'qouted' is non-zero the 'recordregion()'
 call will also go wrong.
 
 I think the simplest correct fix is to pass a byte count to ungrabstackstr()
 and return the new write pointer.
 
        David
 
 -- 
 David Laight: david%l8s.co.uk@localhost