[lldb] Fix crash in "help memory read"

[DavidSpickett]

Summary

When a command option does not have a short version
(e.g. -f for --file), we use an arbitrary value in the
short_option field to mark it as invalid.
(though this value is unqiue to be used later for other
things)

We check that this short option is valid to print using
llvm::isPrint. This implicitly casts our int to char,
meaning we check the last char of any short_option value.

Since the arbitrary value we chose for these options is
some shortened hex version of the name, this returned true
even for invalid values.

Since llvm::isPrint returns true we later call std::islower
and/or std::isupper on the short_option value. (the int)

Calling these functions with something that cannot be validly
converted to unsigned char is undefined. Somehow we got/get
away with this but for me compiling with g++-9 I got a crash
for "help memory read".

The other command that uses this is "target variable" but that
didn't crash for unknown reasons.

Checking that short_option can fit into an unsigned char before
we call llvm::isPrint means we will not attempt to call islower/upper
on these options since we have no reason to print them.

This also fixes bogus short options being shown for "memory read"
and target variable.

For "target variable", before:

-e <filename> ( --file <filename> )
-b <filename> ( --shlib <filename> )

After:

--file <filename>
--shlib <filename>

(note that the bogus short options are just the bottom byte of our
arbitrary short_option value)

[DavidSpickett]

UBSAN might have caught this if we had a test that took this path but I need to confirm that. If so I could add some more as a smoke test of sorts, or just call help on every command if that's not going to take an age.

See lldb/source/Interpreter/OptionGroupOutputFile.cpp and lldb/source/Commands/CommandObjectTarget.cpp for two uses of these arbitrary short_option values. The calls to islower/upper are in lldb/source/Interpreter/Options.cpp Options::GenerateOptionUsage.

[labath]

lldb/include/lldb/Utility/OptionDefinition.h

52

Something like llvm::is(U)Int<CHAR_BIT>(short_option) would better convey the intention.

[teemperor]

From what I understand this means D91378 is no longer necessary. thanks!

lldb/include/lldb/Utility/OptionDefinition.h

 //===-- OptionDefinition.h --------------------------------------*- C++ -*-===//
 //
 // 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
 //
 //===----------------------------------------------------------------------===//
 
 #ifndef LLDB_UTILITY_OPTIONDEFINITION_H
 #define LLDB_UTILITY_OPTIONDEFINITION_H
 
 #include "lldb/lldb-enumerations.h"
 #include "lldb/lldb-private-types.h"
 #include "llvm/ADT/StringExtras.h"
+#include <climits>
 #include <cstdint>
 
 namespace lldb_private {
 struct OptionDefinition {
   /// Used to mark options that can be used together.  If
   /// `(1 << n & usage_mask) != 0` then this option belongs to option set n.
   uint32_t usage_mask;
   /// This option is required (in the current usage level).
struct OptionDefinition {
   lldb::CommandArgumentType argument_type;
   /// Full text explaining what this options does and what (if any) argument to
   /// pass it.
   const char *usage_text;
 
   /// Whether this has a short option character.
   bool HasShortOption() const {
     // See the short_option documentation for more.
-    return llvm::isPrint(short_option);
+    // Must not have any bits set outside of unsigned char range
+    return !(short_option & ~((1 << CHAR_BIT) - 1)) &&
+           llvm::isPrint(short_option);
   }
 };
 } // namespace lldb_private
 
 #endif // LLDB_UTILITY_OPTIONDEFINITION_H

lldb/test/API/commands/help/TestHelp.py

class HelpCommandTestCase(TestBase):
 
     @no_debug_info_test
     def test_help_should_not_crash_lldb(self):
         """Command 'help disasm' should not crash lldb."""
         self.runCmd("help disasm", check=False)
         self.runCmd("help unsigned-integer")
 
     @no_debug_info_test
+    def test_help_memory_read_should_not_crash_lldb(self):
+        """Command 'help memory read' should not crash lldb."""
+        self.runCmd("help memory read", check=False)
+
+    @no_debug_info_test
     def test_help_should_not_hang_emacsshell(self):
         """Command 'settings set term-width 0' should not hang the help command."""
         self.expect(
             "settings set term-width 0",
             COMMAND_FAILED_AS_EXPECTED,
             error=True,
             substrs=['error: 0 is out of range, valid values must be between'])
         # self.runCmd("settings set term-width 0")