getenv() caching bug

["Guillaume Maudoux (Layus)" <layus.on@xxxxxxxxx>]

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?

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;
 }