Fix segfault resulting from empty print prompt

[xiaobai]

Summary

I have found a way to segfault lldb in 7 keystrokes! Steps to reproduce:

1. Launch lldb
2. Type print and hit enter. lldb will now prompt you to type a list of expressions, followed by an empty line.
3. Hit enter, indicating the end of your input.
4. Segfault!

After some investigation, I've found the issue in Host/common/Editline.cpp.
Editline::MoveCursor() relies on m_input_lines not being empty when the to
argument is CursorPosition::BlockEnd. This scenario, as far as I can tell,
occurs in one specific instance: In Editline::EndOrAddLineCommand() when the
list of lines being processed contains exactly one string (""). Meeting this
condition is fairly simple, I have posted steps to reproduce above.

I see two options: check if the state of m_input_lines is valid while inside
Editline::MoveCursor(), or validate the state of m_input_lines before calling
Editline::MoveCursor(). I have chosen to do the latter, for these 2 reason:

1. This happens in one spot in under very specific conditions. Check for it when it could occur, not every time you call Editline::MoveCursor().
2. I'm not sure how Editline::MoveCursor() should behave when m_input_lines is empty, nor am I sure if it should be called. I have roughly 4-5 hours experience with the code in Editline.cpp over the course of about 2 days, so

I'm treating this as a learning opportunity. :)

Let me know what you think and/or if you want more context. Thanks!

[krytarowski]

I cannot reproduce it locally.

$ lldb 
(lldb) print
Enter expressions, then terminate with an empty line to evaluate:
(lldb) 
Enter expressions, then terminate with an empty line to evaluate:
(lldb)

Steps:

1. start lldb
2. "print"<enter>
3. <enter>
4. <enter>

NetBSD 7.99.70 amd64 with our basesystem editline(3).

[labath]

Thanks for the patch Alex.

After looking around the code a bit (I'm quite new to that area as well), I think a better approach would be to fix MoveCursor to handle this situation gracefully. If you look at what this code does in the "normal" case, you'll see that it deletes the "n: " prompt on the last empty line. Your fix would bypass that and leave a dangling "1:" on the screen (which is not that bad really, but let's be consistent). If you look closely you'll see that the exact column we move the cursor to does not matter here (we immediately print \n), but we should still move the cursor one line up. Probably moving it to column 1 would be sufficient in this case.

You don't have worry about performance of this part of the code too much, as nothing it does will be slower than your hands. Readability is more important here.

A good way to "enhance" :) your learning experience would be to also create a test for this bug. I think that you will have to go for a pexpect-style test to reproduce this. TestConvenienceVariables.py would be a good candidate to model the test on. Let me know if you have any questions.

I cannot reproduce it locally.

Well.. I guess that's how undefined behavior works in practice. :)

[xiaobai]

@krytarowski: Thanks for checking! I can set up a NetBSD environment sometime in the next few days to see what's going on. While it might not be an issue on this platform, I think it's an issue that MoveCursor() accesses m_input_lines[m_input_lines.size() - 1] without verifying that m_input_lines.size() > 0 explicitly.

@labath: After reading over what you said, I believe I understand what you're saying. An older version of lldb (I have checked with the default version provided on MacOS Sierra 10.12.3) will leave the dangling "1: ", but I agree that consistency will be better than going back to the old behavior. I'll change my patch and add a test in the next day or two. Thank you so much for your feedback! :)

[xiaobai]

Updating per @labath's suggestions

[xiaobai]

Over the weekend I dug into this further. It looks like the environment I found this bug in didn't have C++11 support and was using std::string::length(), which tries to perform a memory read where it's not supposed to, resulting in a segfault.
On my friend's Ubuntu server, it used std::basic_string::length() (since I had C++11 support). By sheer luck, std::basic_string::length() read a memory location that was indeed mapped but contained unrelated data. This isn't a breaking behavior since we use the result mod 80 to calculate toColumn, and immediately after the MoveCursor call we fprintf("\n"). I still think this is an issue.

As for the pexpect test, I tried to make it as simple as possible while maintaining the general style/feel of the test you suggested I look at. It passes when I run it with dotest.py, but I'm not sure if it should do more. I'm interested to see what you think.

[labath]

It's definitely still a bug worth fixing, we cannot rely on undefined behavior like that.

Thank you very much for adding the test case. Looking at the test suite again, I think I've found a better place for the test. Could you put the test in test/testcases/expression_command/multiline/TestMultilineExpressions.py. You don't even have to create a new file, just add a new function (test_empty_list ?) to the existing class.

Other than that, I think this is great.

[xiaobai]

Moving test per @labath's suggestion

[labath]

Perfect, thank you.

Do you have commit access?

[xiaobai]

I do not have commit access. I would appreciate it if you could commit it for me.

[labath]

Committed as 302225. I've fixed the indentation in your test, and added a couple of decorators to match other pexpect tests. Thanks for the patch!

lldb/trunk/packages/Python/lldbsuite/test/expression_command/multiline/TestMultilineExpressions.py

 """Test multiline expressions."""
 
 from __future__ import print_function
 
 import os
 import lldb
 from lldbsuite.test.decorators import *
 from lldbsuite.test.lldbtest import *
 from lldbsuite.test import lldbutil
 
 
 class MultilineExpressionsTestCase(TestBase):
 
     mydir = TestBase.compute_mydir(__file__)
+    NO_DEBUG_INFO_TESTCASE = True
 
     def setUp(self):
         # Call super's setUp().
         TestBase.setUp(self)
         # Find the line number to break on inside main.cpp.
         self.line = line_number('main.c', 'break')
 
     @skipIfRemote
def test_with_run_commands(self):
 
         child.sendline('3')
         child.expect_exact('3:')
 
         child.sendline('')
         child.expect_exact(prompt)
         self.expect(child.before, exe=False,
                     patterns=['= 5'])
+
+    @skipIfRemote
+    @expectedFailureAll(
+        oslist=["windows"],
+        bugnumber="llvm.org/pr22274: need a pexpect replacement for windows")
+    def test_empty_list(self):
+        """Test printing an empty list of expressions"""
+        import pexpect
+        prompt = "(lldb) "
+
+        # So that the child gets torn down after the test
+        self.child = pexpect.spawn(
+                "%s %s" %
+                (lldbtest_config.lldbExec, self.lldbOption))
+        child = self.child
+
+        # Turn on logging for what the child sends back.
+        if self.TraceOn():
+            child.logfile_read = sys.stdout
+
+        # We expect a prompt, then send "print" to start a list of expressions,
+        # then an empty line. We expect a prompt back.
+        child.expect_exact(prompt)
+        child.sendline("print")
+        child.expect_exact('1:')
+        child.sendline("")
+        child.expect_exact(prompt)

lldb/trunk/source/Host/common/Editline.cpp

fprintf(m_output_file,
             std::abs(toLine - fromLine));
   }
 
   // Determine target column
   int toColumn = 1;
   if (to == CursorLocation::EditingCursor) {
     toColumn =
         editline_cursor_position - (editline_cursor_row * m_terminal_width) + 1;
-  } else if (to == CursorLocation::BlockEnd) {
+  } else if (to == CursorLocation::BlockEnd && !m_input_lines.empty()) {
     toColumn =
         ((m_input_lines[m_input_lines.size() - 1].length() + GetPromptWidth()) %
          80) +
         1;
   }
   fprintf(m_output_file, ANSI_SET_COLUMN_N, toColumn);
 }