Add checks for ValueObjectSP in Cocoa summary providers

[shafik]

Summary

We saw a crash recently (rdar://problem/65276489) that looks related to we had good ValueObjectSP for some in Cocoa.cpp e.g. NSBundleSummaryProvider(...) summary providers. This adds checks before we use them when calling NSStringSummaryProvider.

[jingham]

The !text test is correct, since you intend to pass *text in as a ValueObject &. But I wouldn't add the GetValueAsUnsigned check, that seems confusing. The NSStringSummaryProvider is returning a bool to tell you whether it succeeded or not, so it seems odd to pre-judge one of it's error states.

[shafik]

I was testing out how NSStringSummaryProvider deals w/ NULL using NSString *foo = nullptr and we filter out NULL values in ValueObjectPrinter::GetValueSummaryError(...) :

if (IsNil())
      summary.assign("nil");

That should mean that the text->GetValueAsUnsigned(0) == 0 is not necessary.

[shafik]

Removing text->GetValueAsUnsigned(0) == 0 check.

[davide]

why? Do you have a testcase?

[shafik]

In D84272#2168172, @davide wrote:

why? Do you have a testcase?

There are already tests but we have a crash report w/o a reproducer, these are obviously correct checks.

[teemperor]

I think Davide's point is that it's not clear what motivated this change and why this doesn't have a regression test. I'm also a bit confused by the description ("We saw a crash recently that looks related to we had good ValueObjectSP for some Cocoa summary providers."). If there isn't a way to test it, then maybe a sentence about why and some background information would be helpful (backtrace and radar numbers are usually fine).

Anyway, from what I can understand in the context is that we saw crashes that looks like we accessed a ValueObject nullptr from these function. And I think this change itself is fine as that's apparently how GetSyntheticChildAtOffset communicates errors.

I'm just a bit lost how we can get into this code path and I assume that's also the reason why there is no test. valobj appears to be valid, otherwise we wouldn't have gotten so far into this function. So GetSyntheticChildAtOffset returns a nullptr in this situation only if either the CompilerType we pass in is invalid or we can't complete the decl behind it. But the type we pass in is BasicTypeObjCID and I can't see a situation where we could end up not figuring out the size of plain id (the decl behind that is to my knowledge a builtin decl).

[shafik]

I updated to include the radar number.

So we have a crash in NSStringSummaryProvider(...) when calling valobj.GetProcessSP() and we have what looks like a nullptr e.g. 0x0000000000000068

In this specific case we are coming from NSBundleSummaryProvider(...) and so we have some situation where text is not valid but we don't have enough information to reproduce it.

We are doing a very similar check in NSURLSummaryProvider(...) and so this is just a defensive check in the rest of the function to mirror that one.

[teemperor]

I'm still not sure how this can happen or how this could be tested, but this patch seems reasonable. Also it seems we usually do nullptr checks on GetSyntheticChildAtOffset results, so at least it would be consistent if we do it here too. So LGTM.

The patch description is a bit confusing so maybe clarify that before landing (I'm not sure what "that looks related to we had good ValueObjectSP for some Cocoa summary providers" means).

lldb/source/Plugins/Language/ObjC/Cocoa.cpp

bool lldb_private::formatters::NSBundleSummaryProvider(
 
   if (class_name == "NSBundle") {
     uint64_t offset = 5 * ptr_size;
     ValueObjectSP text(valobj.GetSyntheticChildAtOffset(
         offset,
         valobj.GetCompilerType().GetBasicTypeFromAST(lldb::eBasicTypeObjCID),
         true));
 
+    if (!text)
+      return false;
+
     StreamString summary_stream;
     bool was_nsstring_ok =
         NSStringSummaryProvider(*text, summary_stream, options);
     if (was_nsstring_ok && summary_stream.GetSize() > 0) {
       stream.Printf("%s", summary_stream.GetData());
       return true;
     }
   }
bool lldb_private::formatters::NSTimeZoneSummaryProvider(
 
   if (class_name.empty())
     return false;
 
   if (class_name == "__NSTimeZone") {
     uint64_t offset = ptr_size;
     ValueObjectSP text(valobj.GetSyntheticChildAtOffset(
         offset, valobj.GetCompilerType(), true));
+
+    if (!text)
+      return false;
+
     StreamString summary_stream;
     bool was_nsstring_ok =
         NSStringSummaryProvider(*text, summary_stream, options);
     if (was_nsstring_ok && summary_stream.GetSize() > 0) {
       stream.Printf("%s", summary_stream.GetData());
       return true;
     }
   }
bool lldb_private::formatters::NSNotificationSummaryProvider(
 
   if (class_name.empty())
     return false;
 
   if (class_name == "NSConcreteNotification") {
     uint64_t offset = ptr_size;
     ValueObjectSP text(valobj.GetSyntheticChildAtOffset(
         offset, valobj.GetCompilerType(), true));
+
+    if (!text)
+      return false;
+
     StreamString summary_stream;
     bool was_nsstring_ok =
         NSStringSummaryProvider(*text, summary_stream, options);
     if (was_nsstring_ok && summary_stream.GetSize() > 0) {
       stream.Printf("%s", summary_stream.GetData());
       return true;
     }
   }