Race condition when setting TERM{,INFO{,_DIRS}}

Race condition when setting TERM{,INFO{,_DIRS}}

[Guillaume Maudoux (Layus)]

Hi,

Half a year after submitting a patch to treat TERMINFO_DIRS like 
TERMINFO and TERM, I now see a race condition in handling these vars.

Zsh handles these variables specially because ncurses looks for them in 
the environment of the current process.
To make changes to these variables apply to zsh itself, they are 
exported to the process environment, where ncurses can find them.

But there is a possible race condition between exporting these vars 
(setenv) and refreshing the ncurses handle (getent).
In my case, when TERMINFO_DIRS is set from /etc/zshenv, the new value is 
ignored by ncurses.
The same applies to the subsequent TERM=$TERM in /etc/zshenv.

However, if I add a delay before TERM=$TERM, then the TERMINFO_DIRS 
update is seen, and the terminal is properly detected.

Could it be that there are threads involved in zsh ?
This seems really weird because the code looks sequential.

Any hint welcome.

-- Layus

Re: Race condition when setting TERM{,INFO{,_DIRS}}

[Bart Schaefer]

On Jun 27,  3:52pm, Guillaume Maudoux (Layus) wrote:
}
} In my case, when TERMINFO_DIRS is set from /etc/zshenv, the new value is 
} ignored by ncurses.
} The same applies to the subsequent TERM=$TERM in /etc/zshenv.

What does "the same" mean here?

} However, if I add a delay before TERM=$TERM, then the TERMINFO_DIRS 
} update is seen, and the terminal is properly detected.
} 
} Could it be that there are threads involved in zsh ?

There aren't threads, but there are signal handlers.  We've had issues
before where certain terminal emulators will fire signals at the process
they are controlling at, shall we say, inopportune times.

However, the most common culprit would be the WINCH (window size change)
signal, which we block before reading init scripts and don't unblock
until ZLE is started up.  And I don't know why ncurses would be doing
anything at all during /etc/zshenv, except via init_term() from those
assignments if ncurses is what provides tgetent().

Normally init_term() is called when setupvals() imports TERM from the
environment, which happens before zshenv is read.  This would also be
the case for import of TERMINFO_DIRS, although if that happens before
TERM has been imported, init_term() will do nothing.

One thing I have noticed is that if TERMINFO_DIRS is in the environment
when the shell first starts up, tgetent() looks *only* there for $TERM.
Conversely if the shell starts and then TERMINFO_DIRS is assigned later,
the default definitions are also consulted.  I don't know if that means
that TERMINFO_DIRS is ignored when a default has already been loaded.
This might differ in various tgetent() implementations.

Also, the "can't find terminal definition for ..." message is suppressed
during import from the environment.

If TERM is not in the environment 

Aside: Does it matter that terminfodirssetfn() does not check (x == 0)
except before adding to the environment?

Re: Race condition when setting TERM{,INFO{,_DIRS}}

[Guillaume Maudoux (Layus)]

Thanks Bart,

I have found the origin of this issue.
There are no threads indeed, but there is caching in ncurses for 
environment variables.

Due to a bug in ncurses' code, it failed to detect that the environment 
var changed,
and so failed to find the terminfo database.

I will send a patch upstream, with this list in cc.

Zsh could avoid that by initializing the term as late as possible.
Calling init_term before parsing zshrc, while nothing required to write 
in the terminal is premature.
Fish for example delays initializing the terminal until the first write.
Well, this would have avoided my issue, but it is not sufficient in the 
general case anyway.

Thanks again for your answer!

Regards,

-- Layus.


On 27/06/17 23:30, Bart Schaefer wrote:
> On Jun 27,  3:52pm, Guillaume Maudoux (Layus) wrote:
> }
> } In my case, when TERMINFO_DIRS is set from /etc/zshenv, the new value is
> } ignored by ncurses.
> } The same applies to the subsequent TERM=$TERM in /etc/zshenv.
>
> What does "the same" mean here?
>
> } However, if I add a delay before TERM=$TERM, then the TERMINFO_DIRS
> } update is seen, and the terminal is properly detected.
> }
> } Could it be that there are threads involved in zsh ?
>
> There aren't threads, but there are signal handlers.  We've had issues
> before where certain terminal emulators will fire signals at the process
> they are controlling at, shall we say, inopportune times.
>
> However, the most common culprit would be the WINCH (window size change)
> signal, which we block before reading init scripts and don't unblock
> until ZLE is started up.  And I don't know why ncurses would be doing
> anything at all during /etc/zshenv, except via init_term() from those
> assignments if ncurses is what provides tgetent().
>
> Normally init_term() is called when setupvals() imports TERM from the
> environment, which happens before zshenv is read.  This would also be
> the case for import of TERMINFO_DIRS, although if that happens before
> TERM has been imported, init_term() will do nothing.
>
> One thing I have noticed is that if TERMINFO_DIRS is in the environment
> when the shell first starts up, tgetent() looks *only* there for $TERM.
> Conversely if the shell starts and then TERMINFO_DIRS is assigned later,
> the default definitions are also consulted.  I don't know if that means
> that TERMINFO_DIRS is ignored when a default has already been loaded.
> This might differ in various tgetent() implementations.
>
> Also, the "can't find terminal definition for ..." message is suppressed
> during import from the environment.
>
> If TERM is not in the environment
>
> Aside: Does it matter that terminfodirssetfn() does not check (x == 0)
> except before adding to the environment?

getenv() caching bug

[Guillaume Maudoux (Layus)]

[-- Attachment #1: Type: text/plain, Size: 3721 bytes --]

Hi,

I have recently investigated a longstanding issue where zsh would not 
initialize the terminal correctly over ssh.
In our setup, TERMINFO_DIRS was required, and defined in zshrc.

Due to incorrect getenv caching, this definition was not taken into 
account, even when the terminal was reinitialized by zsh.
Zsh reinitializes the terminal after any assignment to TERM{,INFO,{_DIRS}}.

This led to the weird behavior of having the shell correctly recognised 
only when TERM* variables were assigned at least one second after the 
first TERMINFO_DIRS export in zshrc.

Caching was wrong because it did not remember the name of unset 
variables, making it impossible to detect their appearance in the 
environment in `cache_expired()`.

A patch is attached to this email.
Please tell me if it needs updates.

Regards,

-- Layus.


On 30/06/17 13:49, Guillaume Maudoux (Layus) wrote:
> Thanks Bart,
>
> I have found the origin of this issue.
> There are no threads indeed, but there is caching in ncurses for 
> environment variables.
>
> Due to a bug in ncurses' code, it failed to detect that the 
> environment var changed,
> and so failed to find the terminfo database.
>
> I will send a patch upstream, with this list in cc.
>
> Zsh could avoid that by initializing the term as late as possible.
> Calling init_term before parsing zshrc, while nothing required to 
> write in the terminal is premature.
> Fish for example delays initializing the terminal until the first write.
> Well, this would have avoided my issue, but it is not sufficient in 
> the general case anyway.
>
> Thanks again for your answer!
>
> Regards,
>
> -- Layus.
>
>
> On 27/06/17 23:30, Bart Schaefer wrote:
>> On Jun 27,  3:52pm, Guillaume Maudoux (Layus) wrote:
>> }
>> } In my case, when TERMINFO_DIRS is set from /etc/zshenv, the new 
>> value is
>> } ignored by ncurses.
>> } The same applies to the subsequent TERM=$TERM in /etc/zshenv.
>>
>> What does "the same" mean here?
>>
>> } However, if I add a delay before TERM=$TERM, then the TERMINFO_DIRS
>> } update is seen, and the terminal is properly detected.
>> }
>> } Could it be that there are threads involved in zsh ?
>>
>> There aren't threads, but there are signal handlers.  We've had issues
>> before where certain terminal emulators will fire signals at the process
>> they are controlling at, shall we say, inopportune times.
>>
>> However, the most common culprit would be the WINCH (window size change)
>> signal, which we block before reading init scripts and don't unblock
>> until ZLE is started up.  And I don't know why ncurses would be doing
>> anything at all during /etc/zshenv, except via init_term() from those
>> assignments if ncurses is what provides tgetent().
>>
>> Normally init_term() is called when setupvals() imports TERM from the
>> environment, which happens before zshenv is read.  This would also be
>> the case for import of TERMINFO_DIRS, although if that happens before
>> TERM has been imported, init_term() will do nothing.
>>
>> One thing I have noticed is that if TERMINFO_DIRS is in the environment
>> when the shell first starts up, tgetent() looks *only* there for $TERM.
>> Conversely if the shell starts and then TERMINFO_DIRS is assigned later,
>> the default definitions are also consulted.  I don't know if that means
>> that TERMINFO_DIRS is ignored when a default has already been loaded.
>> This might differ in various tgetent() implementations.
>>
>> Also, the "can't find terminal definition for ..." message is suppressed
>> during import from the environment.
>>
>> If TERM is not in the environment
>>
>> Aside: Does it matter that terminfodirssetfn() does not check (x == 0)
>> except before adding to the environment?
>


[-- Attachment #2: getenv_caching.patch --]
[-- Type: text/x-patch, Size: 1561 bytes --]

diff --git a/ncurses/tinfo/db_iterator.c b/ncurses/tinfo/db_iterator.c
index 94a4082047..0549dae224 100644
--- a/ncurses/tinfo/db_iterator.c
+++ b/ncurses/tinfo/db_iterator.c
@@ -92,33 +92,33 @@ check_existence(const char *name, struct stat *sb)
  * Store the latest value of an environment variable in my_vars[] so we can
  * detect if one changes, invalidating the cached search-list.
  */
 static bool
 update_getenv(const char *name, DBDIRS which)
 {
     bool result = FALSE;
 
     if (which < dbdLAST) {
 	char *value;
+	char *cached_value = my_vars[which].value;
+	bool same_value;
 
-	if ((value = getenv(name)) == 0 || (value = strdup(value)) == 0) {
-	    ;
-	} else if (my_vars[which].name == 0 || strcmp(my_vars[which].name, name)) {
-	    FreeIfNeeded(my_vars[which].value);
-	    my_vars[which].name = name;
-	    my_vars[which].value = value;
-	    result = TRUE;
-	} else if ((my_vars[which].value != 0) ^ (value != 0)) {
-	    FreeIfNeeded(my_vars[which].value);
-	    my_vars[which].value = value;
-	    result = TRUE;
-	} else if (value != 0 && strcmp(value, my_vars[which].value)) {
+	if ((value = getenv(name)) != 0) {
+	    value = strdup(value);
+	}
+	same_value = (value == 0 && cached_value == 0)
+	    || (value != 0 && cached_value != 0 && strcmp(value, cached_value) == 0);
+
+	// Update var name for later checks
+	my_vars[which].name = name;
+
+	if (!same_value) {
 	    FreeIfNeeded(my_vars[which].value);
 	    my_vars[which].value = value;
 	    result = TRUE;
 	} else {
 	    free(value);
 	}
     }
     return result;
 }
 

Re: getenv() caching bug

[Thomas Dickey]

[-- Attachment #1: Type: text/plain, Size: 490 bytes --]

On Fri, Jun 30, 2017 at 02:05:44PM +0200, Guillaume Maudoux (Layus) wrote:
> Hi,
> 
> I have recently investigated a longstanding issue where zsh would
> not initialize the terminal correctly over ssh.
> In our setup, TERMINFO_DIRS was required, and defined in zshrc.
> 
> Due to incorrect getenv caching, this definition was not taken into

thanks (will review/etc).

-- 
Thomas E. Dickey <dickey@invisible-island.net>
http://invisible-island.net
ftp://invisible-island.net

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

Re: Race condition when setting TERM{,INFO{,_DIRS}}

[Mikael Magnusson]

On Fri, Jun 30, 2017 at 1:49 PM, Guillaume Maudoux (Layus)
<layus.on@gmail.com> wrote:
> Thanks Bart,
>
> I have found the origin of this issue.
> There are no threads indeed, but there is caching in ncurses for environment
> variables.
>
> Due to a bug in ncurses' code, it failed to detect that the environment var
> changed,
> and so failed to find the terminfo database.
>
> I will send a patch upstream, with this list in cc.
>
> Zsh could avoid that by initializing the term as late as possible.
> Calling init_term before parsing zshrc, while nothing required to write in
> the terminal is premature.
> Fish for example delays initializing the terminal until the first write.
> Well, this would have avoided my issue, but it is not sufficient in the
> general case anyway.

Plenty of people (including me) write to the terminal from zshrc and
other initialization files.

-- 
Mikael Magnusson

Re: Race condition when setting TERM{,INFO{,_DIRS}}

[Guillaume Maudoux (Layus)]

On 30/06/17 14:34, Mikael Magnusson wrote:
> On Fri, Jun 30, 2017 at 1:49 PM, Guillaume Maudoux (Layus)
> <layus.on@gmail.com> wrote:
>> Thanks Bart,
>>
>> I have found the origin of this issue.
>> There are no threads indeed, but there is caching in ncurses for environment
>> variables.
>>
>> Due to a bug in ncurses' code, it failed to detect that the environment var
>> changed,
>> and so failed to find the terminfo database.
>>
>> I will send a patch upstream, with this list in cc.
>>
>> Zsh could avoid that by initializing the term as late as possible.
>> Calling init_term before parsing zshrc, while nothing required to write in
>> the terminal is premature.
>> Fish for example delays initializing the terminal until the first write.
>> Well, this would have avoided my issue, but it is not sufficient in the
>> general case anyway.
> Plenty of people (including me) write to the terminal from zshrc and
> other initialization files.
>

Well, I should have said that differently: "The reason why the same 
issue did not appear with other shells is because they postpone term 
initialization for as long as possible"; therefore not triggering the 
caching issue when used in our setup.

It is indeed not a correct nor desirable fix.