From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail.efficios.com (mail.efficios.com [167.114.26.124]) by sourceware.org (Postfix) with ESMTPS id 868413858C83 for ; Tue, 19 Jul 2022 18:55:09 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 868413858C83 Received: from localhost (localhost [127.0.0.1]) by mail.efficios.com (Postfix) with ESMTP id 2FF68430E81; Tue, 19 Jul 2022 14:55:09 -0400 (EDT) Received: from mail.efficios.com ([127.0.0.1]) by localhost (mail03.efficios.com [127.0.0.1]) (amavisd-new, port 10032) with ESMTP id uGqmT_eLpQAD; Tue, 19 Jul 2022 14:55:08 -0400 (EDT) Received: from localhost (localhost [127.0.0.1]) by mail.efficios.com (Postfix) with ESMTP id 9069F430996; Tue, 19 Jul 2022 14:55:08 -0400 (EDT) DKIM-Filter: OpenDKIM Filter v2.10.3 mail.efficios.com 9069F430996 X-Virus-Scanned: amavisd-new at efficios.com Received: from mail.efficios.com ([127.0.0.1]) by localhost (mail03.efficios.com [127.0.0.1]) (amavisd-new, port 10026) with ESMTP id KmBm-GbFb3xK; Tue, 19 Jul 2022 14:55:08 -0400 (EDT) Received: from [10.0.0.11] (192-222-157-6.qc.cable.ebox.net [192.222.157.6]) by mail.efficios.com (Postfix) with ESMTPSA id 63CDB430995; Tue, 19 Jul 2022 14:55:08 -0400 (EDT) Message-ID: Date: Tue, 19 Jul 2022 14:55:07 -0400 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101 Thunderbird/91.11.0 Subject: Re: [PATCH] gdbsupport: change path_join parameter to std::vector Content-Language: en-US To: Bruno Larsen , Simon Marchi , gdb-patches@sourceware.org References: <20220719142741.3307396-1-simon.marchi@polymtl.ca> <45e2fd55-0ef7-5275-9b42-7b8480d84328@redhat.com> From: Simon Marchi In-Reply-To: <45e2fd55-0ef7-5275-9b42-7b8480d84328@redhat.com> Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit X-Spam-Status: No, score=-3039.5 required=5.0 tests=BAYES_00, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, GIT_PATCH_0, NICE_REPLY_A, SPF_HELO_NONE, SPF_PASS, TXREP, WEIRD_PORT autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on server2.sourceware.org X-BeenThere: gdb-patches@sourceware.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Gdb-patches mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 19 Jul 2022 18:55:11 -0000 On 2022-07-19 11:10, Bruno Larsen wrote: > > On 7/19/22 11:27, Simon Marchi via Gdb-patches wrote: >> From: Simon Marchi >> >> When a GDB built with -D_GLIBCXX_DEBUG=1 reads a binary with a single >> character name, we hit this assertion failure: >> >> $ ./gdb -q --data-directory=data-directory -nx ./x >> /usr/include/c++/12.1.0/string_view:239: constexpr const std::basic_string_view<_CharT, _Traits>::value_type& std::basic_string_view<_CharT, _Traits>::operator[](size_type) const [with _CharT = char; _Traits = std::char_traits; const_reference = const char&; size_type = long unsigned int]: Assertion '__pos < this->_M_len' failed. >> >> The backtrace: >> >> #3 0x00007ffff6c0f002 in std::__glibcxx_assert_fail (file=, line=, function=, condition=) at /usr/src/debug/gcc/libstdc++-v3/src/c++11/debug.cc:60 >> #4 0x000055555da8a864 in std::basic_string_view >::operator[] (this=0x7fffffffcc30, __pos=1) at /usr/include/c++/12.1.0/string_view:239 >> #5 0x00005555609dcb88 in path_join[abi:cxx11](gdb::array_view > const>) (paths=...) at /home/simark/src/binutils-gdb/gdbsupport/pathstuff.cc:203 >> #6 0x000055555e0443f4 in path_join () at /home/simark/src/binutils-gdb/gdb/../gdbsupport/pathstuff.h:84 >> #7 0x00005555609dc336 in gdb_realpath_keepfile[abi:cxx11](char const*) (filename=0x6060000a8d40 "/home/simark/build/binutils-gdb-one-target/gdb/./x") at /home/simark/src/binutils-gdb/gdbsupport/pathstuff.cc:122 >> #8 0x000055555ebd2794 in exec_file_attach (filename=0x7fffffffe0f9 "./x", from_tty=1) at /home/simark/src/binutils-gdb/gdb/exec.c:471 >> #9 0x000055555f2b3fb0 in catch_command_errors (command=0x55555ebd1ab6 , arg=0x7fffffffe0f9 "./x", from_tty=1, do_bp_actions=false) at /home/simark/src/binutils-gdb/gdb/main.c:513 >> #10 0x000055555f2b7e11 in captured_main_1 (context=0x7fffffffdb60) at /home/simark/src/binutils-gdb/gdb/main.c:1209 >> #11 0x000055555f2b9144 in captured_main (data=0x7fffffffdb60) at /home/simark/src/binutils-gdb/gdb/main.c:1319 >> #12 0x000055555f2b9226 in gdb_main (args=0x7fffffffdb60) at /home/simark/src/binutils-gdb/gdb/main.c:1344 >> #13 0x000055555d938c5e in main (argc=5, argv=0x7fffffffdcf8) at /home/simark/src/binutils-gdb/gdb/gdb.c:32 >> >> The problem is this line in path_join: >> >> gdb_assert (strlen (path) == 0 || !IS_ABSOLUTE_PATH (path)); >> >> ... where `path` is "x". IS_ABSOLUTE_PATH eventually calls >> HAS_DRIVE_SPEC_1: >> >> #define HAS_DRIVE_SPEC_1(dos_based, f) \ >> ((f)[0] && ((f)[1] == ':') && (dos_based)) >> >> This macro accesses indices 0 and 1 of the input string. However, `f` >> is a string_view of length 1, so it's incorrect to try to access index >> 1. We know that the string_view's underlying object is a null-terminated >> string, so in practice there's no harm. But as far as the string_view >> is concerned, index 1 is considered out of bounds. >> >> This patch makes the easy fix, that is to change the path_join parameter >> from a vector of to a vector of `const char *`. Another solution would >> be to introduce a non-standard gdb::cstring_view class, which would be a >> view over a null-terminated string. With that class, it would be >> correct to access index 1, it would yield the NUL character. If there >> is interest in having this class (it has been mentioned a few times in >> the past) I can do it and use it here. >> >> This was found by running tests such as gdb.ada/arrayidx.exp, which >> produce 1-char long filenames, so adding a new test is not necessary. >> >> Change-Id: Ia41a16c7243614636b18754fd98a41860756f7af >> --- >> gdbsupport/pathstuff.cc | 8 ++++---- >> gdbsupport/pathstuff.h | 8 ++++---- >> 2 files changed, 8 insertions(+), 8 deletions(-) >> >> diff --git a/gdbsupport/pathstuff.cc b/gdbsupport/pathstuff.cc >> index af10c6ebd2e8..390f10b1b5e4 100644 >> --- a/gdbsupport/pathstuff.cc >> +++ b/gdbsupport/pathstuff.cc >> @@ -191,21 +191,21 @@ child_path (const char *parent, const char *child) >> /* See gdbsupport/pathstuff.h. */ >> std::string >> -path_join (gdb::array_view paths) >> +path_join (gdb::array_view paths) >> { >> std::string ret; >> for (int i = 0; i < paths.size (); ++i) >> { >> - const gdb::string_view path = paths[i]; >> + const char *path = paths[i]; >> if (i > 0) >> - gdb_assert (path.empty () || !IS_ABSOLUTE_PATH (path)); >> + gdb_assert (strlen (path) == 0 || !IS_ABSOLUTE_PATH (path)); >> if (!ret.empty () && !IS_DIR_SEPARATOR (ret.back ())) >> ret += '/'; >> - ret.append (path.begin (), path.end ()); >> + ret.append (path); >> } >> return ret; >> diff --git a/gdbsupport/pathstuff.h b/gdbsupport/pathstuff.h >> index d01db89e085f..aa7b0ffa2fbd 100644 >> --- a/gdbsupport/pathstuff.h >> +++ b/gdbsupport/pathstuff.h >> @@ -67,7 +67,7 @@ extern const char *child_path (const char *parent, const char *child); >> The first element can be absolute or relative. All the others must be >> relative. */ >> -extern std::string path_join (gdb::array_view paths); >> +extern std::string path_join (gdb::array_view paths); >> /* Same as the above, but accept paths as distinct parameters. */ >> @@ -78,10 +78,10 @@ path_join (Args... paths) >> /* It doesn't make sense to join less than two paths. */ >> gdb_static_assert (sizeof... (Args) >= 2); >> - std::array views >> - { gdb::string_view (paths)... }; >> + std::array views >> + { paths... }; > > Very minor nit, this array used to be called "views" because it held string_views, the name > should probably be changed, maybe path_array or something? Good idea, I made the change locally. Simon