[LLDB] Fix 'std::out_of_range' crashing bug when file name completion using file path.

[HirokiImai]

Summary

When I run a lldb command that uses filename completion, if I enter a string that is not only a filename but also a string with a non-file name string added, such as "./" that is relative path string , it will crash as soon as I press the [Tab] key.
For example, debugging an executable file named "hello" that is compiled from a file named "hello.c" , and I’ll put a breakpoint on line 3 of hello.c.

$ lldb ./hello
(lldb) breakpoint set --file hello.c --line 3

This is not a problem, but if I set "--file ./hello." and then press [Tab] key to complete file name, lldb crashes.

$ lldb ./hello
(lldb) breakpoint set --file ./hello.terminate called after throwing an instance of 'std::out_of_range'
  what():  basic_string::substr: __pos (which is 8) > this->size() (which is 7)
PLEASE submit a bug report to https://bugs.llvm.org/ and include the crash backtrace.
Stack dump:
0.	Program arguments: lldb-12 ./hello
Stack dump without symbol names (ensure you have llvm-symbolizer in your PATH or set the environment var `LLVM_SYMBOLIZER_PATH` to point to it):
/lib/x86_64-linux-gnu/libLLVM-12.so.1(_ZN4llvm3sys15PrintStackTraceERNS_11raw_ostreamEi+0x23)[0x7f172281de53]
/lib/x86_64-linux-gnu/libLLVM-12.so.1(_ZN4llvm3sys17RunSignalHandlersEv+0x50)[0x7f172281c170]
/lib/x86_64-linux-gnu/libLLVM-12.so.1(+0xbd94bf)[0x7f172281e4bf]
/lib/x86_64-linux-gnu/libpthread.so.0(+0x153c0)[0x7f172b08a3c0]
/lib/x86_64-linux-gnu/libc.so.6(gsignal+0xcb)[0x7f172174b18b]
/lib/x86_64-linux-gnu/libc.so.6(abort+0x12b)[0x7f172172a859]
/lib/x86_64-linux-gnu/libstdc++.so.6(+0x9e911)[0x7f1721b01911]
/lib/x86_64-linux-gnu/libstdc++.so.6(+0xaa38c)[0x7f1721b0d38c]
/lib/x86_64-linux-gnu/libstdc++.so.6(+0xaa3f7)[0x7f1721b0d3f7]
/lib/x86_64-linux-gnu/libstdc++.so.6(+0xaa6a9)[0x7f1721b0d6a9]
/lib/x86_64-linux-gnu/libstdc++.so.6(+0xa13ab)[0x7f1721b043ab]
/lib/x86_64-linux-gnu/liblldb-12.so.1(+0x63cbb3)[0x7f172a67bbb3]
/lib/x86_64-linux-gnu/liblldb-12.so.1(+0x63fa59)[0x7f172a67ea59]
/lib/x86_64-linux-gnu/libedit.so.2(el_wgets+0x102)[0x7f1721112d42]
/lib/x86_64-linux-gnu/liblldb-12.so.1(+0x63ee36)[0x7f172a67de36]
/lib/x86_64-linux-gnu/liblldb-12.so.1(+0x5b9a5b)[0x7f172a5f8a5b]
/lib/x86_64-linux-gnu/liblldb-12.so.1(+0x5babfe)[0x7f172a5f9bfe]
/lib/x86_64-linux-gnu/liblldb-12.so.1(+0x59f254)[0x7f172a5de254]
/lib/x86_64-linux-gnu/liblldb-12.so.1(+0x66446d)[0x7f172a6a346d]
/lib/x86_64-linux-gnu/liblldb-12.so.1(_ZN4lldb10SBDebugger21RunCommandInterpreterEbb+0xe9)[0x7f172a2be949]
lldb-12[0x406e5a]
lldb-12[0x408826]
/lib/x86_64-linux-gnu/libc.so.6(__libc_start_main+0xf3)[0x7f172172c0b3]
lldb-12[0x40435e]
Aborted (core dumped)

The crash was caused because substr() (in lldb/source/Host/common/Editline.cpp) cut out string which size is user's input string from the completion string.

I modified the code that erase the user's intput string from current line and then add the completion string.

[HirokiImai]

Apply clang-format

[JDevlieghere]

Thanks for the patch. Can you please add a regression test that covers the previously crashing behavior? I think something like TestIOHandlerCompletion.py might serve as inspiration.

[teemperor]

Thanks for the patch! As Jonas pointed out, tests are usually expected for these kind of patches. But the IOHandler logic has some really quirky way to write tests, so just went ahead and wrote one that you can just apply on top:

diff --git a/lldb/test/API/iohandler/completion/Makefile b/lldb/test/API/iohandler/completion/Makefile
new file mode 100644
index 000000000000..10495940055b
--- /dev/null
+++ b/lldb/test/API/iohandler/completion/Makefile
@@ -0,0 +1,3 @@
+C_SOURCES := main.c
+
+include Makefile.rules
diff --git a/lldb/test/API/iohandler/completion/TestIOHandlerCompletion.py b/lldb/test/API/iohandler/completion/TestIOHandlerCompletion.py
index 2f49b810fb4c..42cc00b4f9e8 100644
--- a/lldb/test/API/iohandler/completion/TestIOHandlerCompletion.py
+++ b/lldb/test/API/iohandler/completion/TestIOHandlerCompletion.py
@@ -20,7 +20,8 @@ class IOHandlerCompletionTest(PExpectTest):
     @expectedFailureAll(oslist=['freebsd'], bugnumber='llvm.org/pr49408')
     @skipIf(oslist=["linux"], archs=["arm", "aarch64"])
     def test_completion(self):
-        self.launch(dimensions=(100,500))
+        self.build()
+        self.launch(dimensions=(100,500), executable=self.getBuildArtifact("a.out"))
 
         # Start tab completion, go to the next page and then display all with 'a'.
         self.child.send("\t\ta")
@@ -51,6 +52,11 @@ class IOHandlerCompletionTest(PExpectTest):
         self.child.send("\n")
         self.expect_prompt()
 
+        # Complete a file path.
+        # FIXME: This should complete to './main.c' and not 'main.c'
+        self.child.send("breakpoint set --file ./main\t")
+        self.child.expect_exact("main.c")
+        self.child.send("\n")
+        self.expect_prompt()
+
         # Start tab completion and abort showing more commands with 'n'.
         self.child.send("\t")
         self.child.expect_exact("More (Y/n/a)")

FWIW, I think this is just fixing the symptom but not the cause of the underlying LLDB bug. The file completion logic completes ./hello to hello.c instead of ./hello.c. It also completes /hello to hello.c apparently. This doesn't seem right. I think fixing this crash is fine, but we probably should also add an assert somewhere that the completion of a token is actually completing the provided token (and not something else). If people want to rewrite the current token they should do it explicitly via a different (new) completion mode. But this seems out-of-scope for this fix. So LGTM with the test applied on top.

[HirokiImai]

Add a regression test

[HirokiImai]

Thanks for the review!
I didn't know how to write tests very well, so the review was very helpful for me.
I found the LLVM project to be beautiful in its componentization. I'll learn more about it.

[teemperor]

In D108817#2972047, @HirokiImai wrote:

Thanks for the review!
I didn't know how to write tests very well, so the review was very helpful for me.
I found the LLVM project to be beautiful in its componentization. I'll learn more about it.

You're welcome and thanks for the patch! I'll merge this for you.

Small note: The test here is not how tests are usually written. The test here has to spawn a virtual terminal to test the command line interface (editline) code, so that's why it uses the special PExpectTest test class. You might want to check out tests that were recently added to the lldb/test/API folder to see how LLDB tests usually look like.

lldb/source/Host/common/Editline.cpp

 //===-- Editline.cpp ------------------------------------------------------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 #include <climits>
 #include <iomanip>
 
 #include "lldb/Host/Editline.h"
 
 #include "lldb/Host/ConnectionFileDescriptor.h"
 #include "lldb/Host/FileSystem.h"
 #include "lldb/Host/Host.h"
 #include "lldb/Utility/CompletionRequest.h"
 #include "lldb/Utility/FileSpec.h"
 #include "lldb/Utility/LLDBAssert.h"
 #include "lldb/Utility/SelectHelper.h"
unsigned char Editline::TabCommand(int ch) {
   if (results.size() == 0)
     return CC_ERROR;
 
   if (results.size() == 1) {
     CompletionResult::Completion completion = results.front();
     switch (completion.GetMode()) {
     case CompletionMode::Normal: {
       std::string to_add = completion.GetCompletion();
-      to_add = to_add.substr(request.GetCursorArgumentPrefix().size());
       // Terminate the current argument with a quote if it started with a quote.
       if (!request.GetParsedLine().empty() && request.GetParsedArg().IsQuoted())
         to_add.push_back(request.GetParsedArg().GetQuoteChar());
       to_add.push_back(' ');
+    el_deletestr(m_editline, request.GetCursorArgumentPrefix().size());
       el_insertstr(m_editline, to_add.c_str());
       // Clear all the autosuggestion parts if the only single space can be completed.
       if (to_add == " ")
         return CC_REDISPLAY;
       return CC_REFRESH;
     }
     case CompletionMode::Partial: {
       std::string to_add = completion.GetCompletion();