Fix handling of backslashes in Args parsing

[labath]

Summary

Presently Args::SetCommandString allows quotes to be escaped with backslash. However, the
backslash itself is not removed from the argument, nor there is a way to escape the backslash
itself. This leads to surprising results:

"a b" c" -> 'a b', 'c' # Here we actually have an unterminated quote, but that is ignored
"a b\" c" -> 'a b\" c' # We try to escape the quote. That works but the backslash is not removed.
"a b\\" c" -> 'a b\\" c' # Escaping the backslash has no effect.

This change removes the possibility to escape the terminating quotes, making every quote run from
one quote character to the next one. The contents for the quote are always taken literally, just
like the single quotes in a posix shell. The only way to insert a literal quote character is to
close the quote and then escape it with a backslash or to use a different kind of quotes.

To summarize:
"a b" c" -> 'a b', 'c' # unterminated quotes are still ignored
"a b"\"" c" -> 'a b" c' # first way to insert a literal quote
'a b" c' -> 'a b" c' # second way to insert a literal quote
"a b\" c -> 'a b\', c # backslash has no effect, we still get two arguments
"a b\\" c -> 'a b\\', c # same here

This change also removes some dead quote-handling code. This code (branching on the value of
quote_char) was never executed since all quote handling was concentrated in the case '"': block,
and the value was always set to 0 after exiting the block.

[zturner]

Can you add a test with the examples you summarized? What does a\b generate
with this change?

[labath]

The handling of the backslash+random character sequence is unchanged. If the character is not recognised, the backslash is not removed
\b -> "\b"

k_escapable_characters defines which characters have the backslash removed. Currently it is: space, tab, backslash, single and double quote.

I will add some tests tomorrow.

[zturner]

Thanks. If you're going to add tests anyway, may I suggest one more for the
case just mentioned (backslash + letter).

Is SBArgs exposed through python? If not it might need to be a gtest

[clayborg]

You can always test this by just launching a process that prints out the arguments and parse the STDOUT to verify they got parsed correctly.

(lldb) process launch -- "a b\" c"

Then verify you got 2 args, where arg[1] == 'a b" c'

[labath]

Greg, a quick question as your example seems to indicate the opposite behavior of what I propose here:
a) current code: "a b\" c" -> 'a b\" c'
b) my patch: "a b\" c" -> 'a b\' 'c' (aka bash single quote)
c) your example: "a b\" c" -> 'a b" c' (aka bash double quote)

Which behavior do we want? I am fine with (b) or (c), but I found (a) confusing, hence the patch. We could even go more bash-like and have " work like (b) and ' like (c).

Or do you not care? :)

[clayborg]

sh-3.2$ ./a.out "a b\" c"

---

Arguments

argv[ 0] = './a.out'
argv[ 1] = 'a b" c'

So "/bin/sh" on MacOSX seems to think case C is the correct one. Note the single quotes are not part of the argument, they are just used to dump the arguments in case they have spaces.

[labath]

I have taken your last comment as an expression of desire to do shell like quoting (which I
preferred anyway), so I have made a new version of the patch which tries to emulate that. In
double quotes, backslash can be used to escape things and is removed from the argument. In single quotes, the backslash is not special and is preserved. This has the added benefit that copy pasting program arguments from shell to "process launch" will most likely "just work".

I have also taken the liberty to modernize some of the code with StringRefs.

[labath]

Added some comments to helper functions.

[clayborg]

Remove the function:

void
Args::SetCommandString (const char *command, size_t len)

now that we have a llvm::StringRef based one.

include/lldb/Interpreter/Args.h

119–121

You can remove this function now that we have a llvm::StringRef version.

source/Interpreter/Args.cpp

138–180

Remove this function now that we have a llvm::StringRef version.

[labath]

Remove the const char * + length variants of SetCommandString and Args constructor in favor of
the StringRef versions.

include/lldb/Interpreter/Args.h

 // C Includes
 // C++ Includes
 #include <list>
 #include <string>
 #include <vector>
 #include <utility>
 
 // Other libraries and framework includes
+#include "llvm/ADT/StringRef.h"
 // Project includes
 #include "lldb/lldb-private-types.h"
 #include "lldb/lldb-types.h"
 #include "lldb/Core/Error.h"
 #include "lldb/Host/OptionParser.h"
 
 namespace lldb_private {
 
public:
 
     //------------------------------------------------------------------
     /// Construct with an option command string.
     ///
     /// @param[in] command
     ///     A NULL terminated command that will be copied and split up
     ///     into arguments.
     ///
-    /// @see Args::SetCommandString(const char *)
+    /// @see Args::SetCommandString(llvm::StringRef)
     //------------------------------------------------------------------
-    Args (const char *command = NULL);
-
-    Args (const char *command, size_t len);
+    Args (llvm::StringRef command = llvm::StringRef());
 
     Args (const Args &rhs);
     
     const Args &
     operator= (const Args &rhs);
 
     //------------------------------------------------------------------
     /// Destructor.
public:
 
     //------------------------------------------------------------------
     /// Sets the command string contained by this object.
     ///
     /// The command string will be copied and split up into arguments
     /// that can be accessed via the accessor functions.
     ///
     /// @param[in] command
-    ///     A NULL terminated command that will be copied and split up
+    ///     A command StringRef that will be copied and split up
     ///     into arguments.
     ///
     /// @see Args::GetArgumentCount() const
     /// @see Args::GetArgumentAtIndex (size_t) const
     /// @see Args::GetArgumentVector ()
     /// @see Args::Shift ()
     /// @see Args::Unshift (const char *)
     //------------------------------------------------------------------
     void
-    SetCommandString (const char *command);
-
-    void
-    SetCommandString (const char *command, size_t len);
+    SetCommandString (llvm::StringRef command);
 
     bool
     GetCommandString (std::string &command) const;
 
     bool
     GetQuotedCommandString (std::string &command) const;
 
     //------------------------------------------------------------------
     /// Gets the number of arguments left in this command object.
protected:
     arg_cstr_collection m_argv; ///< The current argument vector.
     arg_quote_char_collection m_args_quote_char;
 
     void
     UpdateArgsAfterOptionParsing ();
 
     void
     UpdateArgvFromArgs ();
+
+    llvm::StringRef
+    ParseSingleArgument (llvm::StringRef command);
 };
 
 } // namespace lldb_private
 
 #endif  // liblldb_Command_h_

source/Commands/CommandObjectExpression.cpp

if (command[0] == '-')
                     break;
                 }
             }
             s = end_options;
         }
 
         if (end_options)
         {
-            Args args (command, end_options - command);
+            Args args (llvm::StringRef(command, end_options - command));
             if (!ParseOptions (args, result))
                 return false;
             
             Error error (m_option_group.NotifyOptionParsingFinished());
             if (error.Fail())
             {
                 result.AppendError (error.AsCString());
                 result.SetStatus (eReturnStatusFailed);

source/Commands/CommandObjectPlatform.cpp

DoExecute (const char *raw_command_line, CommandReturnObject &result)
                         break;
                     }
                 }
                 s = end_options;
             }
             
             if (end_options)
             {
-                Args args (raw_command_line, end_options - raw_command_line);
+                Args args (llvm::StringRef(raw_command_line, end_options - raw_command_line));
                 if (!ParseOptions (args, result))
                     return false;
             }
         }
         
         if (expr == NULL)
             expr = raw_command_line;
         

source/Commands/CommandObjectWatchpoint.cpp

DoExecute (const char *raw_command, CommandReturnObject &result)
                         break;
                     }
                 }
                 s = end_options;
             }
             
             if (end_options)
             {
-                Args args (raw_command, end_options - raw_command);
+                Args args (llvm::StringRef(raw_command, end_options - raw_command));
                 if (!ParseOptions (args, result))
                     return false;
                 
                 Error error (m_option_group.NotifyOptionParsingFinished());
                 if (error.Fail())
                 {
                     result.AppendError (error.AsCString());
                     result.SetStatus (eReturnStatusFailed);

source/Interpreter/Args.cpp

 #include "lldb/Target/Target.h"
 
 using namespace lldb;
 using namespace lldb_private;
 
 //----------------------------------------------------------------------
 // Args constructor
 //----------------------------------------------------------------------
-Args::Args (const char *command) :
+Args::Args (llvm::StringRef command) :
     m_args(),
     m_argv(),
     m_args_quote_char()
 {
-    if (command)
-        SetCommandString (command);
 }
 
 
-Args::Args (const char *command, size_t len) :
-    m_args(),
-    m_argv(),
-    m_args_quote_char()
-{
-    if (command && len)
-        SetCommandString (command, len);
-}
-
 //----------------------------------------------------------------------
 // We have to be very careful on the copy constructor of this class
 // to make sure we copy all of the string values, but we can't copy the
 // rhs.m_argv into m_argv since it will point to the "const char *" c 
 // strings in rhs.m_args. We need to copy the string list and update our
 // own m_argv appropriately. 
 //----------------------------------------------------------------------
 Args::Args (const Args &rhs) :
for (size_t i = 0; i < argc; ++i)
             command.append (m_argv[i]);
             command.append (1, quote_char);
         }
         else
             command.append (m_argv[i]);
     }
     return argc > 0;
 }
 
-void
-Args::SetCommandString (const char *command, size_t len)
+// A helper function for argument parsing.
+// Parses the initial part of the first argument using normal double quote rules:
+// backslash escapes the double quote and itself. The parsed string is appended to the second
+// argument. The function returns the unparsed portion of the string, starting at the closing
+// quote.
+static llvm::StringRef
+ParseDoubleQuotes(llvm::StringRef quoted, std::string &result)
 {
-    // Use std::string to make sure we get a NULL terminated string we can use
-    // as "command" could point to a string within a large string....
-    std::string null_terminated_command(command, len);
-    SetCommandString(null_terminated_command.c_str());
-}
-
-void
-Args::SetCommandString (const char *command)
+    // Inside double quotes, '\' and '"' are special.
+    static const char *k_escapable_characters = "\"\\";
+    while (true)
     {
-    m_args.clear();
-    m_argv.clear();
-    m_args_quote_char.clear();
+        // Skip over over regular characters and append them.
+        size_t regular = quoted.find_first_of(k_escapable_characters);
+        result += quoted.substr(0, regular);
+        quoted = quoted.substr(regular);
 
-    if (command && command[0])
+        // If we have reached the end of string or the closing quote, we're done.
+        if (quoted.empty() || quoted.front() == '"')
+            break;
+
+        // We have found a backslash.
+        quoted = quoted.drop_front();
+
+        if (quoted.empty())
         {
-        static const char *k_space_separators = " \t";
-        static const char *k_escapable_characters = " \t\\'\"";
-        const char *arg_end = nullptr;
-        const char *arg_pos;
-        for (arg_pos = command;
-             arg_pos && arg_pos[0];
-             arg_pos = arg_end)
-        {
-            // Skip any leading space separators
-            const char *arg_start = ::strspn (arg_pos, k_space_separators) + arg_pos;
-            
-            // If there were only space separators to the end of the line, then
-            // we're done.
-            if (*arg_start == '\0')
-                break;
+            // A lone backslash at the end of string, let's just append it.
+            result += '\\';
+        }
+
+        // If the character after the backslash is not a whitelisted escapable character, we
+        // leave the character sequence untouched.
+        if (strchr(k_escapable_characters, quoted.front()) == nullptr)
+            result += '\\';
+
+        result += quoted.front();
+        quoted = quoted.drop_front();
+    }
 
-            // Arguments can be split into multiple discontiguous pieces,
-            // for example:
-            //  "Hello ""World"
-            // this would result in a single argument "Hello World" (without/
-            // the quotes) since the quotes would be removed and there is 
-            // not space between the strings. So we need to keep track of the
-            // current start of each argument piece in "arg_piece_start"
-            const char *arg_piece_start = arg_start;
-            arg_pos = arg_piece_start;
+    return quoted;
+}
+
+// A helper function for SetCommandString.
+// Parses a single argument from the command string, processing quotes and backslashes in a
+// shell-like manner. The parsed argument is appended to the m_args array. The function returns
+// the unparsed portion of the string, starting at the first unqouted, unescaped whitespace
+// character.
+llvm::StringRef
+Args::ParseSingleArgument(llvm::StringRef command)
+{
+    // Argument can be split into multiple discontiguous pieces,
+    // the quotes) since the quotes would be removed and there is
+    // not space between the strings.
 
-            std::string arg;
-            // Since we can have multiple quotes that form a single command
-            // in a command like: "Hello "world'!' (which will make a single
-            // argument "Hello world!") we remember the first quote character
-            // we encounter and use that for the quote character.
-            char first_quote_char = '\0';
-            char quote_char = '\0';
-            bool arg_complete = false;
+
 
-            do
-            {
-                arg_end = ::strcspn (arg_pos, k_escapable_characters) + arg_pos;
+    bool arg_complete = false;
+        // Skip over over regular characters and append them.
+        size_t regular = command.find_first_of(" \t\"'`\\");
+        arg += command.substr(0, regular);
+        command = command.substr(regular);
 
-                switch (arg_end[0])
-                {
-                default:
-                    assert (!"Unhandled case statement, we must handle this...");
-                    break;
+        if (command.empty())
 
-                case '\0':
-                    // End of C string
-                    if (arg_piece_start && arg_piece_start[0])
-                        arg.append (arg_piece_start);
-                    arg_complete = true;
-                    break;
-                case '\\':
-                    // Backslash character
-                    switch (arg_end[1])
-                    {
-                        case '\0':
-                            arg.append (arg_piece_start);
-                            ++arg_end;
-                            arg_complete = true;
-                            break;
+        char special = command.front();
+        command = command.drop_front();
+        switch (special)
+        {
+            if (command.empty())
+                arg += '\\';
+            }
 
-                        default:
-                            // Only consider this two-character sequence an escape sequence if we're unquoted and
-                            // the character after the backslash is a whitelisted escapable character.  Otherwise
-                            // leave the character sequence untouched.
-                            if (quote_char == '\0' && (nullptr != strchr(k_escapable_characters, arg_end[1])))
-                            {
-                                arg.append (arg_piece_start, arg_end - arg_piece_start);
-                                arg.append (arg_end + 1, 1);
-                                arg_pos = arg_end + 2;
-                                arg_piece_start = arg_pos;
-                            }
-                            else
-                                arg_pos = arg_end + 2;
-                            break;
-                    }
-                    break;
-                case '"':
-                case '\'':
-                case '`':
-                    // Quote characters 
-                    if (quote_char)
-                    {
-                        // We found a quote character while inside a quoted
-                        // character argument. If it matches our current quote
-                        // character, this ends the effect of the quotes. If it
-                        // doesn't we ignore it.
-                        if (quote_char == arg_end[0])
-                        {
-                            arg.append (arg_piece_start, arg_end - arg_piece_start);
-                            // Clear the quote character and let parsing
-                            // continue (we need to watch for things like:
-                            // "Hello ""World"
-                            // "Hello "World
-                            // "Hello "'World'
-                            // All of which will result in a single argument "Hello World"
-                            quote_char = '\0'; // Note that we are no longer inside quotes
-                            arg_pos = arg_end + 1; // Skip the quote character
-                            arg_piece_start = arg_pos; // Note we are starting from later in the string
-                        }
-                        else
-                        {
-                            // different quote, skip it and keep going
-                            arg_pos = arg_end + 1;
-                        }
-                    }
-                    else
-                    {
-                        // We found the start of a quote scope.
-                        // Make sure there isn't a string that precedes
-                        // the start of a quote scope like:
-                        // Hello" World"
-                        // If so, then add the "Hello" to the arg
-                        if (arg_end > arg_piece_start)
-                            arg.append (arg_piece_start, arg_end - arg_piece_start);
-                            
-                        // Enter into a quote scope
-                        quote_char = arg_end[0];
-                        
-                        if (first_quote_char == '\0')
-                            first_quote_char = quote_char;
+            // If the character after the backslash is not a whitelisted escapable character, we
+            static const char *k_escapable_characters = " \t\\'\"`";
+            if (strchr(k_escapable_characters, command.front()) == nullptr)
+                arg += '\\';
+
+            arg += command.front();
+            command.drop_front();
+
+
+        case ' ':
+        case '\t':
+            // We are not inside any quotes, we just found a space after an
+            // argument. We are done.
+            arg_complete = true;
+
+                first_quote_char = special;
 
-                        arg_pos = arg_end;
-                        ++arg_pos;                 // Skip the quote character
-                        arg_piece_start = arg_pos; // Note we are starting from later in the string
-                        
-                        // Skip till the next quote character
-                        const char *end_quote = ::strchr (arg_piece_start, quote_char);
-                        while (end_quote && end_quote[-1] == '\\')
-                        {
-                            // Don't skip the quote character if it is 
-                            // preceded by a '\' character
-                            end_quote = ::strchr (end_quote + 1, quote_char);
-                        }
-                        
-                        if (end_quote)
-                        {
-                            if (end_quote > arg_piece_start)
-                                arg.append (arg_piece_start, end_quote - arg_piece_start);
-
-                            // If the next character is a space or the end of 
-                            // string, this argument is complete...
-                            if (end_quote[1] == ' ' || end_quote[1] == '\t' || end_quote[1] == '\0')
-                            {
-                                arg_complete = true;
-                                arg_end = end_quote + 1;
-                            }
-                            else
-                            {
-                                arg_pos = end_quote + 1;
-                                arg_piece_start = arg_pos;
-                            }
-                            quote_char = '\0';
-                        }
-                        else
-                        {
-                            // Consume the rest of the string as there was no terminating quote
-                            arg.append(arg_piece_start);
-                            arg_end = arg_piece_start + strlen(arg_piece_start);
-                            arg_complete = true;
-                        }
-                    }
-                    break;
+            if (special == '"')
+                command = ParseDoubleQuotes(command, arg);
+                // For single quotes, we simply skip ahead to the matching quote character
+                // (or the end of the string).
+                size_t quoted = command.find(special);
+                arg += command.substr(0, quoted);
+                command = command.substr(quoted);
 
-                case ' ':
-                case '\t':
-                    if (quote_char)
-                    {
-                        // We are currently processing a quoted character and found
-                        // a space character, skip any spaces and keep trying to find
-                        // the end of the argument. 
-                        arg_pos = ::strspn (arg_end, k_space_separators) + arg_end;
-                    }
-                    else
-                    {
-                        // We are not inside any quotes, we just found a space after an
-                        // argument
-                        if (arg_end > arg_piece_start)
-                            arg.append (arg_piece_start, arg_end - arg_piece_start);
-                        arg_complete = true;
-                    }
-                    break;
-                }
-            } while (!arg_complete);
+            // If we found a closing quote, skip it.
+            if (! command.empty())
+                command = command.drop_front();
+
 
-            m_args.push_back(arg);
-            m_args_quote_char.push_back (first_quote_char);
-        }
-        UpdateArgvFromArgs();
+    return command;
+
+void
+Args::SetCommandString (llvm::StringRef command)
+{
+    m_args.clear();
+    m_argv.clear();
+    m_args_quote_char.clear();
+
+    static const char *k_space_separators = " \t";
+    command = command.ltrim(k_space_separators);
+    while (!command.empty())
+    {
+        command = ParseSingleArgument(command);
+        command = command.ltrim(k_space_separators);
     }
+
+    UpdateArgvFromArgs();
 }
 
 void
 Args::UpdateArgsAfterOptionParsing()
 {
     // Now m_argv might be out of date with m_args, so we need to fix that
     arg_cstr_collection::const_iterator argv_pos, argv_end = m_argv.end();
     arg_sstr_collection::iterator args_pos;

source/Interpreter/CommandInterpreter.cpp

 
 CommandObject *
 CommandInterpreter::BuildAliasResult (const char *alias_name, 
                                       std::string &raw_input_string, 
                                       std::string &alias_result,
                                       CommandReturnObject &result)
 {
     CommandObject *alias_cmd_obj = nullptr;
-    Args cmd_args (raw_input_string.c_str());
+    Args cmd_args (raw_input_string);
     alias_cmd_obj = GetCommandObject (alias_name);
     StreamString result_str;
     
     if (alias_cmd_obj)
     {
         std::string alias_name_str = alias_name;
         if ((cmd_args.GetArgumentCount() == 0)
             || (alias_name_str.compare (cmd_args.GetArgumentAtIndex(0)) != 0))
CommandInterpreter::HandleCompletion (const char *current_line,
                                       const char *last_char,
                                       int match_start_point,
                                       int max_return_elements,
                                       StringList &matches)
 {
     // We parse the argument up to the cursor, so the last argument in parsed_line is
     // the one containing the cursor, and the cursor is after the last character.
 
-    Args parsed_line(current_line, last_char - current_line);
-    Args partial_parsed_line(current_line, cursor - current_line);
+    Args parsed_line(llvm::StringRef(current_line, last_char - current_line));
+    Args partial_parsed_line(llvm::StringRef(current_line, cursor - current_line));
 
     // Don't complete comments, and if the line we are completing is just the history repeat character, 
     // substitute the appropriate history line.
     const char *first_arg = parsed_line.GetArgumentAtIndex(0);
     if (first_arg)
     {
         if (first_arg[0] == m_comment_char)
             return 0;

test/settings/quoting/Makefile

+LEVEL = ../../make
+
+C_SOURCES := main.c
+
+include $(LEVEL)/Makefile.rules

test/settings/quoting/TestQuoting.py

+"""
+Test quoting of arguments to lldb commands
+"""
+
+import os, time, re
+import unittest2
+import lldb
+from lldbtest import *
+
+class SettingsCommandTestCase(TestBase):
+
+    mydir = TestBase.compute_mydir(__file__)
+
+    @classmethod
+    def classCleanup(cls):
+        """Cleanup the test byproducts."""
+        cls.RemoveTempFile("stdout.txt")
+
+    def test_no_quote(self):
+        self.do_test_args("a b c", "a\0b\0c\0")
+
+    def test_single_quote(self):
+        self.do_test_args("'a b c'", "a b c\0")
+
+    def test_double_quote(self):
+        self.do_test_args('"a b c"', "a b c\0")
+
+    def test_double_quote(self):
+        self.do_test_args('"a b c"', 'a b c\0')
+
+    def test_single_quote_escape(self):
+        self.do_test_args("'a b\\' c", "a b\\\0c\0")
+
+    def test_double_quote_escape(self):
+        self.do_test_args('"a b\\" c"', 'a b" c\0')
+
+    def test_double_quote_escape(self):
+        self.do_test_args('"a b\\" c"', 'a b" c\0')
+
+    def test_double_quote_escape2(self):
+        self.do_test_args('"a b\\\\" c', 'a b\\\0c\0')
+
+    def test_double_quote_escape2(self):
+        self.do_test_args('"a b\\\\" c', 'a b\\\0c\0')
+
+    def test_single_in_double(self):
+        self.do_test_args('"a\'b"', "a'b\0")
+
+    def test_double_in_single(self):
+        self.do_test_args("'a\"b'", 'a"b\0')
+
+    def test_combined(self):
+        self.do_test_args('"a b"c\'d e\'', 'a bcd e\0')
+
+    def test_bare_single(self):
+        self.do_test_args("a\\'b", "a'b\0")
+
+    def test_bare_double(self):
+        self.do_test_args('a\\"b', 'a"b\0')
+
+    def do_test_args(self, args_in, args_out):
+        """Test argument parsing. Run the program with args_in. The program dumps its arguments
+        to stdout. Compare the stdout with args_out."""
+        self.buildDefault()
+
+        exe = os.path.join(os.getcwd(), "a.out")
+        self.runCmd("file " + exe, CURRENT_EXECUTABLE_SET)
+
+        self.runCmd("process launch -o stdout.txt -- " + args_in)
+
+        with open('stdout.txt', 'r') as f:
+            output = f.read()
+
+        self.RemoveTempFile("stdout.txt")
+
+        self.assertEqual(output, args_out)
+
+if __name__ == '__main__':
+    import atexit
+    lldb.SBDebugger.Initialize()
+    atexit.register(lambda: lldb.SBDebugger.Terminate())
+    unittest2.main()

test/settings/quoting/main.c

+#include <stdio.h>
+#include <string.h>
+
+/* This program writes its arguments (separated by '\0') to stdout. */
+int
+main(int argc, char const *argv[])
+{
+    int i;
+    for (i = 1; i < argc; ++i)
+        fwrite(argv[i], strlen(argv[i])+1, 1, stdout);
+
+    return 0;
+}