[lldb] Fix that SBThread.GetStopDescription is returning strings with uninitialized memory at the end.

[teemperor]

Summary

SBThread.GetStopDescription is a curious API as it takes a buffer length as a parameter that specifies
how many bytes the buffer we pass has. Then we fill the buffer until the specified length (or the length
of the stop description string) and return the string length. If the buffer is a nullptr however, we instead
return how many bytes we would have written to the buffer so that the user can allocate a buffer with
the right size and pass that size to a subsequent SBThread.GetStopDescription call.

Funnily enough, it is not possible to pass a nullptr via the Python SWIG bindings, so that might be the
first API in LLDB that is not only hard to use correctly but impossible to use correctly. The only way to
call this function via Python is to throw in a large size limit that is hopefully large enough to contain the
stop description (otherwise we only get the truncated stop description).

Currently passing a size limit that is smaller than the returned stop description doesn't cause the
Python bindings to return the stop description but instead the truncated stop description + uninitialized characters
at the end of the string. The reason for this is that we return the result of snprintf from the method
which returns the amount of bytes that *would* have been written (which is larger than the buffer).
This causes our Python bindings to return a string that is as large as full stop description but the
buffer that has been filled is only as large as the passed in buffer size.

This patch fixes this issue by just recalculating the string length in our buffer instead of relying on the wrong
return value. We also have to do this in a new type map as the old type map is also used for all methods
with the given argument pair char *dst, size_t dst_len (e.g. SBProcess.GetSTDOUT`). These methods have
different semantics for these arguments and don't null-terminate the returned buffer (they instead return the
size in bytes) so we can't change the existing typemap without breaking them.

[shafik]

lldb/source/API/SBThread.cpp

337 ↗

(On Diff #235875)

The else branch feels inconsistent with the change above. Especially the + 1.

[labath]

Would it be possible to fix this problem in the swig bindings instead? (i.e. move the std::min stuff into the swig code). That way, the behavior of this function will at least match that of snprintf, which will hopefully be less surprising that what you've done here. (And it will also fix the inconsistency noticed by @shafik).

lldb/source/API/SBThread.cpp

337 ↗

(On Diff #235875)

yep

[teemperor]

• Move change to SWIG type map.
• Fixed some type in the test.

The API never emulated the snprintf behavior as it includes a NULL byte when we pass a nullptr buffer (and that's why it has the inconsistent + 1 in the else branch). We get this whole thing even more wrong as we add the NULL byte to the snprintf result in the other branch (at the end of the method). Anyway, I changed this in the type map now and just use strlen on the result string which is simpler (and doesn't rely on any of our bogus return values).

I'll make another patch where we can discuss what this function is supposed to return.

[labath]

This is fine. It also sounds like a good idea to solve the +1 vs snprintf inconsistency.

lldb/scripts/Python/python-typemaps.swig

124–125 ↗

(On Diff #236542)

This is just StringRef ref = $1, right ?

[teemperor]

• Simplify swig type map code.

[teemperor]

lldb/scripts/Python/python-typemaps.swig

124–125 ↗

(On Diff #236542)

Swig gives $1 a void* type (at least on my system), so I we still need the static_cast (but we can get rid of the rest).

[teemperor]

• Remove unintended empty line change.

[clayborg]

lldb/scripts/Python/python-typemaps.swig

124 ↗

(On Diff #236793)

This assignment looks a bit goofy IMHO. Just remove result from original code?:

llvm::StringRef ref(static_cast<char*>($1));

[teemperor]

• Just removed the result parameter.

[teemperor]

For reasons that are beyond my understanding this change seems to break SBProcess.GetSTDOUT in random tests like this:

======================================================================
ERROR: test_change_value_dwarf (TestChangeValueAPI.ChangeValueAPITestCase)
   Exercise the SBValue::SetValueFromCString API.
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/teemperor/work/ci/llvm/lldb/packages/Python/lldbsuite/test/lldbtest.py", line 1732, in test_method
    return attrvalue(self)
  File "/home/teemperor/work/ci/llvm/lldb/packages/Python/lldbsuite/test/decorators.py", line 454, in wrapper
    func(*args, **kwargs)
  File "/home/teemperor/work/ci/llvm/lldb/packages/Python/lldbsuite/test/decorators.py", line 110, in wrapper
    func(*args, **kwargs)
  File "/home/teemperor/work/ci/llvm/lldb/packages/Python/lldbsuite/test/python_api/value/change_values/TestChangeValueAPI.py", line 149, in test_change_value
    stdout = process.GetSTDOUT(1000)
  File "/home/teemperor/work/ci/build/lib/python3.8/site-packages/lldb/__init__.py", line 8020, in GetSTDOUT
    return _lldb.SBProcess_GetSTDOUT(self, dst)
SystemError: <built-in function SBProcess_GetSTDOUT> returned NULL without setting an error
Config=x86_64-/home/teemperor/work/ci/build/bin/clang-10

Anyone got a clue what is going on here? It seems like these tests only fail when run in combination with some other tests as running them alone is fine.

[teemperor]

Ah, seems like we match in the type map by function signature and GetSTDOUT matches the signature by accident....

[teemperor]

• Move to use a new typemap for GetStopDescription.

[teemperor]

I made a new typemap now that is only for GetStopDescription and it's snprintf semantics. This keeps all the other functions working. I kept the old behavior of requiring a >0 buffer size etc. in tact for now.

[labath]

Overall, I think it would be nice to introduce some consistency into the return values of functions which fill out a user-provided buffer, but the thing which convinced me that a separate typemap is needed is utf8 handling. For functions like GetStopDescription, we should hopefully be able to guarantee that the returned value is a valid utf8 string (and so we can return a python string), but for things like GetSTOUT we most certainly cannot (a python bytes object would be more suitable)...

lldb/bindings/interface/SBThread.i

~SBThread();
     SBThreadCollection
     GetStopReasonExtendedBacktraces (InstrumentationRuntimeType type);
 
 
     %feature("autodoc", "
     Pass only an (int)length and expect to get a Python string describing the
     stop reason.") GetStopDescription;
     size_t
-    GetStopDescription (char *dst, size_t dst_len);
+    GetStopDescription (char *dst_or_null, size_t dst_len);
 
     SBValue
     GetStopReturnValue ();
 
     %feature("autodoc", "
     Returns a unique thread identifier (type lldb::tid_t, typically a 64-bit type)
     for the current SBThread that will remain constant throughout the thread's
     lifetime in this process and will not be reused by another thread during this

lldb/bindings/python/python-typemaps.swig

else
     PyErr_SetString(PyExc_ValueError, "Expecting an integer");
     return nullptr;
   }
 }
 
 /* Typemap definitions to allow SWIG to properly handle char buffer. */
 
 // typemap for a char buffer
-// See also SBThread::GetStopDescription.
 %typemap(in) (char *dst, size_t dst_len) {
    if (!PyInt_Check($input)) {
        PyErr_SetString(PyExc_ValueError, "Expecting an integer");
        return NULL;
    }
    $2 = PyInt_AsLong($input);
    if ($2 <= 0) {
        PyErr_SetString(PyExc_ValueError, "Positive integer expected");
        return NULL;
    }
    $1 = (char *) malloc($2);
 }
 // SBProcess::ReadCStringFromMemory() uses a void*, but needs to be treated
 // as char data instead of byte data.
 %typemap(in) (void *char_buf, size_t size) = (char *dst, size_t dst_len);
 
 // Return the char buffer.  Discarding any previous return result
-// See also SBThread::GetStopDescription.
 %typemap(argout) (char *dst, size_t dst_len) {
    Py_XDECREF($result);   /* Blow away any previous result */
    if (result == 0) {
       PythonString string("");
       $result = string.release();
       Py_INCREF($result);
    } else {
       llvm::StringRef ref(static_cast<const char*>($1), result);
       PythonString string(ref);
       $result = string.release();
    }
    free($1);
 }
 // SBProcess::ReadCStringFromMemory() uses a void*, but needs to be treated
 // as char data instead of byte data.
 %typemap(argout) (void *char_buf, size_t size) = (char *dst, size_t dst_len);
 
 
+// typemap for handling an snprintf-like API like SBThread::GetStopDescription.
+%typemap(in) (char *dst_or_null, size_t dst_len) {
+   if (!PyInt_Check($input)) {
+       PyErr_SetString(PyExc_ValueError, "Expecting an integer");
+       return NULL;
+   }
+   $2 = PyInt_AsLong($input);
+   if ($2 <= 0) {
+       PyErr_SetString(PyExc_ValueError, "Positive integer expected");
+       return NULL;
+   }
+   $1 = (char *) malloc($2);
+}
+%typemap(argout) (char *dst_or_null, size_t dst_len) {
+   Py_XDECREF($result);   /* Blow away any previous result */
+   llvm::StringRef ref($1);
+   PythonString string(ref);
+   $result = string.release();
+   free($1);
+}
+
+
 // typemap for an outgoing buffer
 // See also SBEvent::SBEvent(uint32_t event, const char *cstr, uint32_t cstr_len).
 // Ditto for SBProcess::PutSTDIN(const char *src, size_t src_len).
 %typemap(in) (const char *cstr, uint32_t cstr_len),
              (const char *src, size_t src_len) {
    if (PythonString::Check($input)) {
       PythonString str(PyRefType::Borrowed, $input);
       $1 = (char*)str.GetString().data();

lldb/packages/Python/lldbsuite/test/python_api/thread/TestThreadAPI.py

def get_stop_description(self):
         # Launch the process, and do not stop at the entry point.
         process = target.LaunchSimple(
             None, None, self.get_process_working_directory())
 
         thread = get_stopped_thread(process, lldb.eStopReasonBreakpoint)
         self.assertTrue(
             thread.IsValid(),
             "There should be a thread stopped due to breakpoint")
-        #self.runCmd("process status")
 
-        # Due to the typemap magic (see lldb.swig), we pass in an (int)length to GetStopDescription
-        # and expect to get a Python string as the return object!
-        # The 100 is just an arbitrary number specifying the buffer size.
-        stop_description = thread.GetStopDescription(100)
-        self.expect(stop_description, exe=False,
-                    startstr='breakpoint')
+        # Get the stop reason. GetStopDescription expects that we pass in the size of the description
+        # we expect plus an additional byte for the null terminator.
+
+        # Test with a buffer that is exactly as large as the expected stop reason.
+        self.assertEqual("breakpoint 1.1", thread.GetStopDescription(len('breakpoint 1.1') + 1))
+
+        # Test some smaller buffer sizes.
+        self.assertEqual("breakpoint", thread.GetStopDescription(len('breakpoint') + 1))
+        self.assertEqual("break", thread.GetStopDescription(len('break') + 1))
+        self.assertEqual("b", thread.GetStopDescription(len('b') + 1))
+
+        # Test that we can pass in a much larger size and still get the right output.
+        self.assertEqual("breakpoint 1.1", thread.GetStopDescription(len('breakpoint 1.1') + 100))
 
     def step_out_of_malloc_into_function_b(self, exe_name):
         """Test Python SBThread.StepOut() API to step out of a malloc call where the call site is at function b()."""
         exe = self.getBuildArtifact(exe_name)
 
         target = self.dbg.CreateTarget(exe)
         self.assertTrue(target, VALID_TARGET)