[lldb] Reject NULL key in PythonDictionary::GetItem - #205753
Conversation
|
@llvm/pr-subscribers-lldb Author: Yao Qi (qiyao) ChangesA
Debugging the crashing lldb under lldb shows the Continuing lands on the dereference of that
This was found by Adds Full diff: https://github.com/llvm/llvm-project/pull/205753.diff 2 Files Affected:
diff --git a/lldb/source/Plugins/ScriptInterpreter/Python/PythonDataObjects.cpp b/lldb/source/Plugins/ScriptInterpreter/Python/PythonDataObjects.cpp
index 800f991680c85..fba98abf9e83b 100644
--- a/lldb/source/Plugins/ScriptInterpreter/Python/PythonDataObjects.cpp
+++ b/lldb/source/Plugins/ScriptInterpreter/Python/PythonDataObjects.cpp
@@ -697,7 +697,7 @@ PythonObject PythonDictionary::GetItemForKey(const PythonObject &key) const {
Expected<PythonObject>
PythonDictionary::GetItem(const PythonObject &key) const {
- if (!IsValid())
+ if (!IsValid() || !key.IsValid())
return nullDeref();
PyObject *o = PyDict_GetItemWithError(m_py_obj, key.get());
if (PyErr_Occurred())
diff --git a/lldb/unittests/ScriptInterpreter/Python/PythonDataObjectsTests.cpp b/lldb/unittests/ScriptInterpreter/Python/PythonDataObjectsTests.cpp
index 0d4b04b7a1284..24ed721049e67 100644
--- a/lldb/unittests/ScriptInterpreter/Python/PythonDataObjectsTests.cpp
+++ b/lldb/unittests/ScriptInterpreter/Python/PythonDataObjectsTests.cpp
@@ -490,6 +490,30 @@ TEST_F(PythonDataObjectsTest, TestPythonDictionaryValueEquality) {
EXPECT_EQ(value_1, chk_str.GetString());
}
+TEST_F(PythonDataObjectsTest, TestPythonDictionaryGetItemWithInvalidKey) {
+ // A PythonString constructed from invalid UTF-8 silently fails (the
+ // PyUnicode_FromStringAndSize call returns NULL) and leaves m_py_obj == NULL.
+ // PythonDictionary::GetItem(const PythonObject&) must not pass that NULL key
+ // through to PyDict_GetItemWithError, which would then dereference it and
+ // crash.
+ llvm::StringRef invalid_utf8("\xd0p", 2);
+ PythonString invalid_key(invalid_utf8);
+ EXPECT_FALSE(invalid_key.IsValid());
+
+ PythonDictionary dict(PyInitialValue::Empty);
+ dict.SetItemForKey(PythonString("real_key"), PythonInteger(42));
+
+ // GetItem must return an error (not crash) when given an invalid key.
+ llvm::Expected<PythonObject> item = dict.GetItem(invalid_key);
+ EXPECT_FALSE(static_cast<bool>(item));
+ llvm::consumeError(item.takeError());
+
+ // GetItemForKey wraps GetItem, consumes any error, and returns an empty
+ // PythonObject. It must not crash either.
+ PythonObject result = dict.GetItemForKey(invalid_key);
+ EXPECT_FALSE(result.IsAllocated());
+}
+
TEST_F(PythonDataObjectsTest, TestPythonDictionaryManipulation) {
// Test that manipulation of a dictionary behaves correctly when wrapped
// by a PythonDictionary.
|
|
cc: @medismailben |
|
LGTM but could you also add a python test for this ? |
By "python test", do you mean test in |
|
@medismailben , nowadays, it can't pass an invalid ute to |
A `breakpoint set -P <class>` with an invalid-UTF-8 class name crashes lldb with `EXC_BAD_ACCESS`. The class name is passed through unmodified, so the byte sequence `\xd0p` (an incomplete two-byte UTF-8 sequence) reaches the Python layer: ``` ./bin/lldb -b -o $'breakpoint set -P \xd0p -f main.cpp' ./bin/lldb ... PLEASE submit a bug report to https://github.com/llvm/llvm-project/issues/ Stack dump: llvm#4 PythonDictionary::GetItem(PythonObject const&) const llvm#6 PythonDictionary::GetItemForKey(PythonObject const&) const llvm#7 PythonObject::ResolveNameWithDictionary(StringRef, PythonDictionary const&) llvm#9 ScriptedPythonInterface::CreatePluginObject<...>(...) ``` `ResolveNameWithDictionary` builds a `PythonString` from the class name and looks it up in the module dictionary. When the name is not valid UTF-8, `PyUnicode_FromStringAndSize` returns `NULL`, so the `PythonString` is left with `m_py_obj == NULL`. `GetItemForKey` forwards that object to `GetItem`, which passed the `NULL` `PyObject *` straight to `PyDict_GetItemWithError`. Debugging the crashing lldb under lldb shows the `NULL` key arriving in `GetItem`: ``` (lldb) break set -n lldb_private::python::PythonDictionary::GetItem (lldb) run --no-use-colors -b -s /tmp/bp_cmds.txt * stop reason = breakpoint 1.1 (lldb) up (lldb) frame variable key.m_py_obj (PyObject *) key.m_py_obj = nullptr ``` Continuing lands on the dereference of that `NULL` inside CPython: ``` (lldb) continue * stop reason = EXC_BAD_ACCESS (code=1, address=0x8) frame #0: PyDict_GetItemWithError + 40 -> ldr x8, [x1, #0x8] ``` `x1` holds the key argument, which is `NULL`, so reading the type field at offset `0x8` faults at address `0x8`. `GetItem` already guards against an invalid dictionary (`!IsValid()`); extend that guard to reject an invalid key as well and return `nullDeref()`. `GetItemForKey` consumes the error and returns an empty `PythonObject`, so existing callers such as `ResolveNameWithDictionary` keep working and the breakpoint command now fails cleanly instead of crashing. This was found by `lldb-commandinterpreter-fuzzer`. Adds `PythonDataObjectsTest.TestPythonDictionaryGetItemWithInvalidKey`, which feeds an invalid-UTF-8 key to `GetItem`. Without the fix the test dereferences the `NULL` key and crashes (`SIGSEGV`).
medismailben
left a comment
medismailben
left a comment
There was a problem hiding this comment.
We shouldn't add a new method to the SBAPI just for the sake of testing. I guess the unit test is good enough.
25ecf86 to
df04765
Compare
|
The commit adding API test and changing SB API is removed. |
4 participants
A
breakpoint set -P <class>with an invalid-UTF-8 class name crasheslldb with
EXC_BAD_ACCESS. The class name is passed throughunmodified, so the byte sequence
\xd0p(an incomplete two-byte UTF-8sequence) reaches the Python layer:
ResolveNameWithDictionarybuilds aPythonStringfrom the class nameand looks it up in the module dictionary. When the name is not valid
UTF-8,
PyUnicode_FromStringAndSizereturnsNULL, so thePythonStringis left withm_py_obj == NULL.GetItemForKeyforwardsthat object to
GetItem, which passed theNULLPyObject *straightto
PyDict_GetItemWithError.Debugging the crashing lldb under lldb shows the
NULLkey arriving inGetItem:Continuing lands on the dereference of that
NULLinside CPython:x1holds the key argument, which isNULL, so reading the type fieldat offset
0x8faults at address0x8.GetItemalready guards against an invalid dictionary (!IsValid());extend that guard to reject an invalid key as well and return
nullDeref().GetItemForKeyconsumes the error and returns an emptyPythonObject, so existing callers such asResolveNameWithDictionarykeep working and the breakpoint command now fails cleanly instead of
crashing.
This was found by
lldb-commandinterpreter-fuzzer.Adds
PythonDataObjectsTest.TestPythonDictionaryGetItemWithInvalidKey,which feeds an invalid-UTF-8 key to
GetItem. Without the fix the testdereferences the
NULLkey and crashes (SIGSEGV).