Crash when exporting scalar without value and getsparam fails

Crash when exporting scalar without value and getsparam fails

[Mikael Magnusson]

2086                if (!(pm->node.flags & PM_UNSET) && !pm->env && !value)

30. returned_null: getsparam returns null (checked 46 out of 57
times). [show details]

CID 439066 (#1 of 1): Dereference null return value (NULL_RETURNS)31.
dereference: Dereferencing a pointer that might be null
getsparam(pname) when calling addenv. [show details]
2087                    addenv(pm, getsparam(pname));


The issue has this comment by Oliver on it, but I couldn't find any
thread about it with a cursory search, so I'm repeating it here;

Comment:

The line of code occurs when you export an existing scalar variable
without giving a new value. It isn't easy to find ways of making
getsparam to fail but I've found one which is enough to declare this a
bug:
 unset IFS
 export IFS
memory allocation failing, such as dupstring would also do the job.
Arguably these cases need fixing but the code highlighted here should
perhaps default to adding the variable to the environment with an
empty value.


and sure enough,
zsh: segmentation fault (core dumped)  zsh -fc 'unset IFS; export IFS'

Would the non-gnu equivalent of addenv(pm, getsparam(pname) ?: ""); do
the trick here? Or should it just return with an error? I guess if we
do the former, then the environment would get out of sync with the
internal parameter value.

-- 
Mikael Magnusson

Re: Crash when exporting scalar without value and getsparam fails

[Bart Schaefer]

On Jan 9,  2:26pm, Mikael Magnusson wrote:
} bug:
}  unset IFS
}  export IFS
} 
} Would the non-gnu equivalent of addenv(pm, getsparam(pname) ?: ""); do
} the trick here? Or should it just return with an error?

I think it should do nothing to the environment and return success.

% bash -c 'unset IFS; export IFS; echo $?; printenv | grep IFS'
0
% 

However, we need an unset parameter object with the export flag set,
so that it will become both set and exported when next a value is
assigned to it.  I think that can be done but don't immediately
recall how.

Re: Crash when exporting scalar without value and getsparam fails

[Mikael Magnusson]

On Sat, Jan 10, 2015 at 7:52 AM, Bart Schaefer
<schaefer@brasslantern.com> wrote:
> On Jan 9,  2:26pm, Mikael Magnusson wrote:
> } bug:
> }  unset IFS
> }  export IFS
> }
> } Would the non-gnu equivalent of addenv(pm, getsparam(pname) ?: ""); do
> } the trick here? Or should it just return with an error?
>
> I think it should do nothing to the environment and return success.
>
> % bash -c 'unset IFS; export IFS; echo $?; printenv | grep IFS'
> 0
> %
>
> However, we need an unset parameter object with the export flag set,
> so that it will become both set and exported when next a value is
> assigned to it.  I think that can be done but don't immediately
> recall how.

Ah yes, this is what happens for other parameters in zsh too.
% unset foo
% export foo
% printenv|grep foo
% foo=3
% printenv|grep foo
foo=3

-- 
Mikael Magnusson

Re: Crash when exporting scalar without value and getsparam fails

[Mikael Magnusson]

On Sat, Jan 10, 2015 at 8:56 AM, Mikael Magnusson <mikachu@gmail.com> wrote:
> On Sat, Jan 10, 2015 at 7:52 AM, Bart Schaefer
> <schaefer@brasslantern.com> wrote:
>> On Jan 9,  2:26pm, Mikael Magnusson wrote:
>> } bug:
>> }  unset IFS
>> }  export IFS
>> }
>> } Would the non-gnu equivalent of addenv(pm, getsparam(pname) ?: ""); do
>> } the trick here? Or should it just return with an error?
>>
>> I think it should do nothing to the environment and return success.
>>
>> % bash -c 'unset IFS; export IFS; echo $?; printenv | grep IFS'
>> 0
>> %
>>
>> However, we need an unset parameter object with the export flag set,
>> so that it will become both set and exported when next a value is
>> assigned to it.  I think that can be done but don't immediately
>> recall how.
>
> Ah yes, this is what happens for other parameters in zsh too.
> % unset foo
> % export foo
> % printenv|grep foo
> % foo=3
> % printenv|grep foo
> foo=3

So just to check, I tried both of the things anyway. With setting it
to "" on failure, it gets exported as the empty string, and this
        if (!(pm->node.flags & PM_UNSET) && !pm->env && !value) {
            void *foo = getsparam(pname);
            if (foo)
                addenv(pm, foo);
        }

results in almost the correct behaviour, except that after unsetting
the parameter and setting it again, it still gets exported. (This
doesn't happen for other parameters). Maybe bin_unset or unsetparam_pm
has some special code that needs changing too?

-- 
Mikael Magnusson

Re: Crash when exporting scalar without value and getsparam fails

[Bart Schaefer]

On Jan 10,  9:09am, Mikael Magnusson wrote:
} Subject: Re: Crash when exporting scalar without value and getsparam fails
}
} So just to check, I tried both of the things anyway. With setting it
} to "" on failure, it gets exported as the empty string, and this
}         if (!(pm->node.flags & PM_UNSET) && !pm->env && !value) {
}             void *foo = getsparam(pname);
}             if (foo)
}                 addenv(pm, foo);
}         }
} 
} results in almost the correct behaviour, except that after unsetting
} the parameter and setting it again, it still gets exported. (This
} doesn't happen for other parameters). Maybe bin_unset or unsetparam_pm
} has some special code that needs changing too?

I think the whole problem is that the PM_UNSET flag is for some reason
not set, even though the parameter is in fact unset.

This is happening at line 2075:

pm->node.flags = (pm->node.flags | (on & ~PM_READONLY)) & ~(off | PM_UNSET);

That line is never executed for e.g. "export FOO", because of this up at
line 1937:

    /*
     * We need to compare types with an existing pm if special,
     * even if that's unset
     */
    if (pm && (pm->node.flags & PM_SPECIAL))
	usepm = 1;

Consequently ...

diff --git a/Src/builtin.c b/Src/builtin.c
index 8abe728..8dee8f9 100644
--- a/Src/builtin.c
+++ b/Src/builtin.c
@@ -1935,7 +1935,7 @@ typeset_single(char *cname, char *pname, Param pm, UNUSED(int func),
      * even if that's unset
      */
     if (pm && (pm->node.flags & PM_SPECIAL))
-	usepm = 1;
+	usepm = 2;	/* indicate that we preserve the PM_UNSET flag */
 
     /*
      * Don't use an existing param if
@@ -2072,7 +2072,11 @@ typeset_single(char *cname, char *pname, Param pm, UNUSED(int func),
 		    arrfixenv(pm->node.nam, x);
 	    }
 	}
-	pm->node.flags = (pm->node.flags | (on & ~PM_READONLY)) & ~(off | PM_UNSET);
+	if (usepm == 2)		/* do not change the PM_UNSET flag */
+	    pm->node.flags = (pm->node.flags | (on & ~PM_READONLY)) & ~off;
+	else
+	    pm->node.flags = (pm->node.flags |
+			      (on & ~PM_READONLY)) & ~(off | PM_UNSET);
 	if (on & (PM_LEFT | PM_RIGHT_B | PM_RIGHT_Z)) {
 	    if (typeset_setwidth(cname, pm, ops, on, 0))
 		return NULL;

Re: Crash when exporting scalar without value and getsparam fails

[Bart Schaefer]

On Jan 10,  9:25am, Bart Schaefer wrote:
}
} @@ -1935,7 +1935,7 @@ typeset_single(char *cname, char *pname, Param pm, UNUSED(int func),
}       * even if that's unset
}       */
}      if (pm && (pm->node.flags & PM_SPECIAL))
} -	usepm = 1;
} +	usepm = 2;	/* indicate that we preserve the PM_UNSET flag */
}  
}      /*
}       * Don't use an existing param if

Upon further reflection:

diff --git a/Src/builtin.c b/Src/builtin.c
index 8dee8f9..9258ddb 100644
--- a/Src/builtin.c
+++ b/Src/builtin.c
@@ -1934,7 +1934,7 @@ typeset_single(char *cname, char *pname, Param pm, UNUSED(int func),
      * We need to compare types with an existing pm if special,
      * even if that's unset
      */
-    if (pm && (pm->node.flags & PM_SPECIAL))
+    if (!usepm && pm && (pm->node.flags & PM_SPECIAL))
 	usepm = 2;	/* indicate that we preserve the PM_UNSET flag */
 
     /*

Re: Crash when exporting scalar without value and getsparam fails

[Mikael Magnusson]

On Sat, Jan 10, 2015 at 7:28 PM, Bart Schaefer
<schaefer@brasslantern.com> wrote:
> On Jan 10,  9:25am, Bart Schaefer wrote:
> }
> } @@ -1935,7 +1935,7 @@ typeset_single(char *cname, char *pname, Param pm, UNUSED(int func),
> }       * even if that's unset
> }       */
> }      if (pm && (pm->node.flags & PM_SPECIAL))
> } -     usepm = 1;
> } +     usepm = 2;      /* indicate that we preserve the PM_UNSET flag */
> }
> }      /*
> }       * Don't use an existing param if
>
> Upon further reflection:
>
> diff --git a/Src/builtin.c b/Src/builtin.c
> index 8dee8f9..9258ddb 100644
> --- a/Src/builtin.c
> +++ b/Src/builtin.c
> @@ -1934,7 +1934,7 @@ typeset_single(char *cname, char *pname, Param pm, UNUSED(int func),
>       * We need to compare types with an existing pm if special,
>       * even if that's unset
>       */
> -    if (pm && (pm->node.flags & PM_SPECIAL))
> +    if (!usepm && pm && (pm->node.flags & PM_SPECIAL))
>         usepm = 2;      /* indicate that we preserve the PM_UNSET flag */
>
>      /*

I get this now, which I think is the same as I got for one of my two
tests earlier;
% unset IFS
% export IFS
% printenv|grep IFS
% IFS=5
% printenv|grep IFS
IFS=5
% unset IFS
% printenv|grep IFS
% IFS=5
% printenv|grep IFS # this would be empty for a normal parameter
IFS=5
% unset IFS
% printenv|grep IFS
% IFS=5
% printenv|grep IFS # and this (same case)
IFS=5

typeset +x IFS works to un-export it, even while it's unset.
(this is a new instance)
% echo ${(t)IFS}
scalar-special
% unset IFS
% echo ${(t)IFS}

% export IFS
% echo ${(t)IFS}

% printenv|grep IFS
% IFS=5
% printenv|grep IFS
IFS=5
% echo ${(t)IFS}
scalar-export-special
% typeset +x IFS
% echo ${(t)IFS}
scalar-special
% printenv|grep IFS
% export IFS
% printenv|grep IFS
IFS=5
% unset IFS
% printenv|grep IFS
% echo ${(t)IFS}

% typeset +x IFS
% echo ${(t)IFS}

% IFS=3
% printenv|grep IFS
% echo ${(t)IFS}
scalar-special


-- 
Mikael Magnusson

Re: Crash when exporting scalar without value and getsparam fails

[Bart Schaefer]

On Jan 10,  8:10pm, Mikael Magnusson wrote:
}
} % unset IFS
} % export IFS
} % IFS=5
} % unset IFS
} % printenv|grep IFS # this would be empty for a normal parameter
} IFS=5

OK, so the remaining issue is that the PM_EXPORT flag is not cleared
when a special parameter is unset.

I think this is within the definition of how a special paramter behaves;
i.e., it retains all its attributes even when unset.  In this case we
have an attribute that's not normally "part of the specialness" of the
parameter, which is also being preserved.  The same thing happens with
other flags on other specials, e.g.:

torch% print $SECONDS
762
torch% typeset -E SECONDS
torch% print $SECONDS    
7.801753190e+02
torch% unset SECONDS
torch% print $SECONDS

torch% SECONDS=763
torch% print $SECONDS
7.671235090e+02


So should e.g. stdunsetfn() and tiedarrunsetfn() clear the PM_EXPORTED
flag as well as assert the PM_UNSET flag?  Or is that going to cause a
problem elsewhere because there is a special parameter that has the
feature of always being exported?  I can't think of any, but maybe we
shouldn't rule it out ... in which case this gets really hairy, as we
will have to know, for every special, what flags should or should not
be retained across unset.

Re: Crash when exporting scalar without value and getsparam fails

[Mikael Magnusson]

On Sat, Jan 10, 2015 at 9:04 PM, Bart Schaefer
<schaefer@brasslantern.com> wrote:
> On Jan 10,  8:10pm, Mikael Magnusson wrote:
> }
> } % unset IFS
> } % export IFS
> } % IFS=5
> } % unset IFS
> } % printenv|grep IFS # this would be empty for a normal parameter
> } IFS=5
>
> OK, so the remaining issue is that the PM_EXPORT flag is not cleared
> when a special parameter is unset.
>
> I think this is within the definition of how a special paramter behaves;
> i.e., it retains all its attributes even when unset.  In this case we
> have an attribute that's not normally "part of the specialness" of the
> parameter, which is also being preserved.  The same thing happens with
> other flags on other specials, e.g.:
>
> torch% print $SECONDS
> 762
> torch% typeset -E SECONDS
> torch% print $SECONDS
> 7.801753190e+02
> torch% unset SECONDS
> torch% print $SECONDS
>
> torch% SECONDS=763
> torch% print $SECONDS
> 7.671235090e+02
>
>
> So should e.g. stdunsetfn() and tiedarrunsetfn() clear the PM_EXPORTED
> flag as well as assert the PM_UNSET flag?  Or is that going to cause a
> problem elsewhere because there is a special parameter that has the
> feature of always being exported?  I can't think of any, but maybe we
> shouldn't rule it out ... in which case this gets really hairy, as we
> will have to know, for every special, what flags should or should not
> be retained across unset.

Ah, these are good points. I don't have any especially strong feelings
about making IFS stop being exported after unset, I was just reporting
what I observed in case it indicated some further wonkiness.

-- 
Mikael Magnusson

Re: Crash when exporting scalar without value and getsparam fails

[Bart Schaefer]

On Jan 10,  8:56am, Mikael Magnusson wrote:
} Subject: Re: Crash when exporting scalar without value and getsparam fails
}
} On Sat, Jan 10, 2015 at 7:52 AM, Bart Schaefer
} <schaefer@brasslantern.com> wrote:
} > I think it should do nothing to the environment and return success.
} >
} > % bash -c 'unset IFS; export IFS; echo $?; printenv | grep IFS'
} > 0
} > %
} 
} Ah yes, this is what happens for other parameters in zsh too.

So did I somehow miss that this is specific to IFS and doesn't affect
exports in general?  Or is there some other characteristic of the
parameters that tickle this bug?

Re: Crash when exporting scalar without value and getsparam fails

[Mikael Magnusson]

On Sat, Jan 10, 2015 at 5:57 PM, Bart Schaefer
<schaefer@brasslantern.com> wrote:
> On Jan 10,  8:56am, Mikael Magnusson wrote:
> } Subject: Re: Crash when exporting scalar without value and getsparam fails
> }
> } On Sat, Jan 10, 2015 at 7:52 AM, Bart Schaefer
> } <schaefer@brasslantern.com> wrote:
> } > I think it should do nothing to the environment and return success.
> } >
> } > % bash -c 'unset IFS; export IFS; echo $?; printenv | grep IFS'
> } > 0
> } > %
> }
> } Ah yes, this is what happens for other parameters in zsh too.
>
> So did I somehow miss that this is specific to IFS and doesn't affect
> exports in general?  Or is there some other characteristic of the
> parameters that tickle this bug?

(I see that what I wrote is a bit unclear, but I'm not sure what the
'this' refers to, so I'll be specific about everything).
I don't know exactly how specific the crash is, but for most
parameters, doing an unset followed by an export does what you suggest
we should do for IFS. Eg, it isn't put into the environment, but upon
the next assignment it is set and exported into the environment
without another explicit 'export'. For IFS doing this just crashes
currently. The reason for which is that getsparam fails on IFS when it
is unset (I don't know why).

According to Oliver's original analysis, there may be other cases such
as dupstring failing (that would break everything else anyway), or
other unknown causes.

Neither of the changes I tested makes IFS behave as the generic case,
but both stop the crashing.

-- 
Mikael Magnusson