Re: [PATCH v2] gdb: check for empty strings in, get_standard_cache_dir/get_standard_config_dir

[Andrew Burgess <andrew.burgess@embecosm.com>]

* Simon Marchi <simon.marchi@polymtl.ca> [2021-01-08 10:50:58 -0500]:

> On 2021-01-08 4:48 a.m., Andrew Burgess wrote:
> > I wonder if it's worth including the case where ALL of the environment
> > variables are empty as well, just to check that there's no issues with
> > GDB falling through all the checks?
> 
> I don't know, but it wasn't too difficult so I did it.
> 
> > 
> > Otherwise, LGTM.
> 
> I also thought it would be a good idea to check that the index-cache
> directory value doesn't get affected by these empty values, that we
> properly skip over them.  See updated patch below.
> 
> 
> From c010770be40c91b97be5e97ce830abea4efd8f86 Mon Sep 17 00:00:00 2001
> From: Simon Marchi <simon.marchi@polymtl.ca>
> Date: Fri, 8 Jan 2021 10:47:53 -0500
> Subject: [PATCH v2] gdb: check for empty strings in
>  get_standard_cache_dir/get_standard_config_dir
> 
> New in v2:
> 
>  - Check the value of the index-cache directory before and after.
>  - Check with all the env vars defined to empty.
> 
> As reported in PR 27157, if some environment variables read at startup
> by GDB are defined but empty, we hit the assert in gdb_abspath:
> 
>     $ XDG_CACHE_HOME= ./gdb -nx --data-directory=data-directory -q
>     AddressSanitizer:DEADLYSIGNAL
>     =================================================================
>     ==2007040==ERROR: AddressSanitizer: SEGV on unknown address 0x0000000001b0 (pc 0x5639d4aa4127 bp 0x7ffdac232c00 sp 0x7ffdac232bf0 T0)
>     ==2007040==The signal is caused by a READ memory access.
>     ==2007040==Hint: address points to the zero page.
>         #0 0x5639d4aa4126 in target_stack::top() const /home/smarchi/src/binutils-gdb/gdb/target.h:1334
>         #1 0x5639d4aa41f1 in inferior::top_target() /home/smarchi/src/binutils-gdb/gdb/inferior.h:369
>         #2 0x5639d4a70b1f in current_top_target() /home/smarchi/src/binutils-gdb/gdb/target.c:120
>         #3 0x5639d4b00591 in gdb_readline_wrapper_cleanup::gdb_readline_wrapper_cleanup() /home/smarchi/src/binutils-gdb/gdb/top.c:1046
>         #4 0x5639d4afab31 in gdb_readline_wrapper(char const*) /home/smarchi/src/binutils-gdb/gdb/top.c:1104
>         #5 0x5639d4ccce2c in defaulted_query /home/smarchi/src/binutils-gdb/gdb/utils.c:893
>         #6 0x5639d4ccd6af in query(char const*, ...) /home/smarchi/src/binutils-gdb/gdb/utils.c:985
>         #7 0x5639d4ccaec1 in internal_vproblem /home/smarchi/src/binutils-gdb/gdb/utils.c:373
>         #8 0x5639d4ccb3d1 in internal_verror(char const*, int, char const*, __va_list_tag*) /home/smarchi/src/binutils-gdb/gdb/utils.c:439
>         #9 0x5639d5151a92 in internal_error(char const*, int, char const*, ...) /home/smarchi/src/binutils-gdb/gdbsupport/errors.cc:55
>         #10 0x5639d5162ab4 in gdb_abspath(char const*) /home/smarchi/src/binutils-gdb/gdbsupport/pathstuff.cc:132
>         #11 0x5639d5162fac in get_standard_cache_dir[abi:cxx11]() /home/smarchi/src/binutils-gdb/gdbsupport/pathstuff.cc:228
>         #12 0x5639d3e76a81 in _initialize_index_cache() /home/smarchi/src/binutils-gdb/gdb/dwarf2/index-cache.c:325
>         #13 0x5639d4dbbe92 in initialize_all_files() /home/smarchi/build/binutils-gdb/gdb/init.c:321
>         #14 0x5639d4b00259 in gdb_init(char*) /home/smarchi/src/binutils-gdb/gdb/top.c:2344
>         #15 0x5639d4440715 in captured_main_1 /home/smarchi/src/binutils-gdb/gdb/main.c:950
>         #16 0x5639d444252e in captured_main /home/smarchi/src/binutils-gdb/gdb/main.c:1229
>         #17 0x5639d44425cf in gdb_main(captured_main_args*) /home/smarchi/src/binutils-gdb/gdb/main.c:1254
>         #18 0x5639d3923371 in main /home/smarchi/src/binutils-gdb/gdb/gdb.c:32
>         #19 0x7fa002d3f0b2 in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x270b2)
>         #20 0x5639d392314d in _start (/home/smarchi/build/binutils-gdb/gdb/gdb+0x4d414d)
> 
> gdb_abspath doesn't handle empty strings, so handle this case in the
> callers.  If a variable is defined but empty, I think it's reasonable in
> this case to just ignore it, as if it was not defined.
> 
> Note that this sometimes also lead to a segfault, because the failed
> assertion happens very early during startup, before things are fully
> initialized.
> 
> gdbsupport/ChangeLog:
> 
> 	PR gdb/27157
> 	* pathstuff.cc (get_standard_cache_dir, get_standard_config_dir,
> 	find_gdb_home_config_file): Add empty string check.
> 
> gdb/testsuite/ChangeLog:
> 
> 	PR gdb/27157
> 	* gdb.base/empty-host-env-vars.exp: New test.

LGTM.

Thanks,
Andrew


> 
> Change-Id: I8654d8e97e74e1dff6d308c111ae4b1bbf07bef9
> ---
>  .../gdb.base/empty-host-env-vars.exp          | 72 +++++++++++++++++++
>  gdbsupport/pathstuff.cc                       | 14 ++--
>  2 files changed, 79 insertions(+), 7 deletions(-)
>  create mode 100644 gdb/testsuite/gdb.base/empty-host-env-vars.exp
> 
> diff --git a/gdb/testsuite/gdb.base/empty-host-env-vars.exp b/gdb/testsuite/gdb.base/empty-host-env-vars.exp
> new file mode 100644
> index 000000000000..bd7212e7b655
> --- /dev/null
> +++ b/gdb/testsuite/gdb.base/empty-host-env-vars.exp
> @@ -0,0 +1,72 @@
> +# Copyright 2021 Free Software Foundation, Inc.
> +
> +# This program is free software; you can redistribute it and/or modify
> +# it under the terms of the GNU General Public License as published by
> +# the Free Software Foundation; either version 3 of the License, or
> +# (at your option) any later version.
> +#
> +# This program is distributed in the hope that it will be useful,
> +# but WITHOUT ANY WARRANTY; without even the implied warranty of
> +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> +# GNU General Public License for more details.
> +#
> +# You should have received a copy of the GNU General Public License
> +# along with this program.  If not, see <http://www.gnu.org/licenses/>.
> +
> +# GDB reads some environment variables on startup, make sure it behaves
> +# correctly if these variables are defined but empty.
> +
> +set all_env_vars { HOME XDG_CACHE_HOME LOCALAPPDATA XDG_CONFIG_HOME }
> +
> +# Record the initial value of the index-cache directory.
> +clean_restart
> +set index_cache_directory ""
> +gdb_test_multiple "show index-cache directory" "" {
> +    -re -wrap "The directory of the index cache is \"(.*)\"\\." {
> +	set index_cache_directory $expect_out(1,string)
> +	set index_cache_directory [string_to_regexp $index_cache_directory]
> +	pass $gdb_test_name
> +    }
> +}
> +
> +foreach_with_prefix env_var_name $all_env_vars {
> +    # Restore the original state of the environment variable.
> +    save_vars env($env_var_name) {
> +	set env($env_var_name) {}
> +	clean_restart
> +
> +	# Verify that the empty environment variable didn't affect the
> +	# index-cache directory setting, that we still see the initial value.
> +	# "HOME" is different, because if that  one is unset, GDB isn't even
> +	# able to compute the default location.  In that case, we expect it to
> +	# be empty.
> +	if { $env_var_name == "HOME" } {
> +	    gdb_test "show index-cache directory" \
> +		"The directory of the index cache is \"\"\\."
> +	} else {
> +	    gdb_test "show index-cache directory" \
> +		"The directory of the index cache is \"$index_cache_directory\"\\."
> +	}
> +    }
> +}
> +
> +# Try the same, but with all the env vars set to an empty value at the same
> +# time.
> +with_test_prefix "all env vars" {
> +    set save_vars_arg {}
> +    foreach env_var_name $all_env_vars {
> +	lappend save_vars_arg env($env_var_name)
> +    }
> +
> +    # Restore the original state of all the environment variables.
> +    save_vars $save_vars_arg {
> +	foreach env_var_name $all_env_vars {
> +	    set env($env_var_name) {}
> +	}
> +
> +	clean_restart
> +
> +	gdb_test "show index-cache directory" \
> +	    "The directory of the index cache is \"\"\\."
> +    }
> +}
> diff --git a/gdbsupport/pathstuff.cc b/gdbsupport/pathstuff.cc
> index 51ab8c651b20..ad13900819ef 100644
> --- a/gdbsupport/pathstuff.cc
> +++ b/gdbsupport/pathstuff.cc
> @@ -222,7 +222,7 @@ get_standard_cache_dir ()
>  
>  #ifndef __APPLE__
>    const char *xdg_cache_home = getenv ("XDG_CACHE_HOME");
> -  if (xdg_cache_home != NULL)
> +  if (xdg_cache_home != NULL && xdg_cache_home[0] != '\0')
>      {
>        /* Make sure the path is absolute and tilde-expanded.  */
>        gdb::unique_xmalloc_ptr<char> abs (gdb_abspath (xdg_cache_home));
> @@ -231,7 +231,7 @@ get_standard_cache_dir ()
>  #endif
>  
>    const char *home = getenv ("HOME");
> -  if (home != NULL)
> +  if (home != NULL && home[0] != '\0')
>      {
>        /* Make sure the path is absolute and tilde-expanded.  */
>        gdb::unique_xmalloc_ptr<char> abs (gdb_abspath (home));
> @@ -240,14 +240,14 @@ get_standard_cache_dir ()
>  
>  #ifdef WIN32
>    const char *win_home = getenv ("LOCALAPPDATA");
> -  if (win_home != NULL)
> +  if (win_home != NULL && win_home[0] != '\0')
>      {
>        /* Make sure the path is absolute and tilde-expanded.  */
>        gdb::unique_xmalloc_ptr<char> abs (gdb_abspath (win_home));
>        return string_printf ("%s/gdb", abs.get ());
>      }
>  #endif
> -    
> +
>    return {};
>  }
>  
> @@ -289,7 +289,7 @@ get_standard_config_dir ()
>  
>  #ifndef __APPLE__
>    const char *xdg_config_home = getenv ("XDG_CONFIG_HOME");
> -  if (xdg_config_home != NULL)
> +  if (xdg_config_home != NULL && xdg_config_home[0] != '\0')
>      {
>        /* Make sure the path is absolute and tilde-expanded.  */
>        gdb::unique_xmalloc_ptr<char> abs (gdb_abspath (xdg_config_home));
> @@ -298,7 +298,7 @@ get_standard_config_dir ()
>  #endif
>  
>    const char *home = getenv ("HOME");
> -  if (home != NULL)
> +  if (home != NULL && home[0] != '\0')
>      {
>        /* Make sure the path is absolute and tilde-expanded.  */
>        gdb::unique_xmalloc_ptr<char> abs (gdb_abspath (home));
> @@ -340,7 +340,7 @@ find_gdb_home_config_file (const char *name, struct stat *buf)
>      }
>  
>    const char *homedir = getenv ("HOME");
> -  if (homedir != nullptr)
> +  if (homedir != nullptr && homedir[0] != '\0')
>      {
>        /* Make sure the path is absolute and tilde-expanded.  */
>        gdb::unique_xmalloc_ptr<char> abs (gdb_abspath (homedir));
> -- 
> 2.29.2
>