Re: Segfault on =() expansion

["Mikael Magnusson" <mikachu@xxxxxxxxx>]

2008/9/1 Peter Stephenson <p.w.stephenson@xxxxxxxxxxxx>:
> On Sun, 31 Aug 2008 10:39:35 +0200
> "Mikael Magnusson" <mikachu@xxxxxxxxx> wrote:
>> I could reproduce this on all my remote ssh shells, ranging from 4.2.3
>> to my local current cvs.
>>
>> % a==(echo foo); : & cat $a
>> [1] 14645
>> foo
>> %
>> [1]  + done       :
>> % a==(echo foo); : & cat $a
>> zsh: segmentation fault  zsh -f
>
> This was quite involved (yes, I know, just for once...)  Working backwards,

I am somehow not overly surprised by this :).

> 1. In the second assignment, the list node's data was NULL.  This shouldn't
>   happen, so caused the crash when processing it as a string.
> 2. The data was NULL because =(...) expansion failed.
> 3. That failed because "thisjob" was -1.
> 4. thisjob was -1 for two reasons.
>  a. It didn't get set for the current job because the current command was
>     parsed as "simple", so it went through execsimple(), missing out a
>     lot of steps including all job handling.
>  b. In this case, the execution of the background job had reset thisjob
>     to -1.  That's why it didn't happen the first time.
> 5. The assignment was parsed as simple because all assignments are.
>   "Simple" means everything happens entirely in the shell and won't
>   use job handling:  this isn't true here because process substitutions
>   need to have a job to attach the temporary file to.
>
> The fixes are:
> to 5: assignments with process substitutions can't be simple.
> to 2: on failure of process substitutions (and others) set the data to
> an empty string.
>
> (I wonder, actually, if 4b. needs unfixing, i.e. thisjob should be made to
> default to -1.)
>
> This will cause the "cat" in the test to give an error.  This is the
> correct behaviour: the file generated by the =(...) substition is a
> temporary file (this is clearly documented) that only lasts for the
> length of the current command, which is just the assignment.  So
> actually process substitutions aren't useful for assignments, which is
> probably why this hasn't shown up before.

Right, that's what i wanted to confirm just seconds before finding the bug,
I was quite surprised to find the temp file hadn't been deleted, though in
my case it was sort of what i wanted :). (what i want can also be achieved
with a shell function, i'm just very lazy).

> I thought about being safe and detecting $(...)'s, too, but that caused this:
>
>  myfd=99
>  (echo >&$myfd) 2>msg
>  bad_fd_msg="${$(<msg)##*:}"
>
> to return a non-zero status.  That's because when assignments are
> handled by execcmd() rather than execsimple() we don't set the status
> for the command, so it retains the previous status.  I've fixed that,
> but in the end I decided just to restrict the fix to 5. to process
> substitution since we'd know by now if it was necessary for command
> substitution.

-- 
Mikael Magnusson