Support more report types in AddressSanitizerRuntime.cpp

[kubamracek]

Summary

https://reviews.llvm.org/D27012 will add more report types into ASan that will be reported via the debugging API. This patch in LLDB provides human-friendly bug descriptions.

[clayborg]

Looks fine. One question though: do we always have a shared library that contains the ASAN runtime? If so, then we can actually put the array of short to long description into the ASAN dylib and then we read this array as a global variable. This would allow you to add as many descriptions as you want directly in your ASAN code and have LLDB detect all of the new strings. Or, if there is a function you can call with the short description that returns the long description, then we can still have the ASAN code contain all of the strings.

[filcab]

I have some minor fixes I'd like to see.
If it's prefixed by "Nit: " it's a really minor one and I'm ok with it as is if that's what you prefer.

Thank you,
Filipe

source/Plugins/InstrumentationRuntime/AddressSanitizer/AddressSanitizerRuntime.cpp

220

Not necessarily recursion. There's also stack variables. I'd omit the stuff in parenthesis.

224

Nit: "Wild jump" is probably better than "wild pointer jump", no?

226

Nit: I'd probably use "Write through wild pointer ..." (same for the others)

234

Nit: Phrasing seems off. I think "Double free detected" is easier to read and more explicit.

236

Nit: Maybe "Deallocation size mismatch detected"? The user isn't deallocating a "size".

238

s/address/memory region/?

242

Maybe "Call to malloc_usable_size with not owned pointer detected"?
Unsure if it's obvious why it's "invalid". Same for the one below.

246

I think this really needs additional detail. It doesn't seem very meaningful as a sentence.
Maybe "Overlapping memory ranges in function that doesn't allow them (detected?)" or something?
My suggestion can be improved too, for sure :-)

250

Maybe "Invalid parameters to call to __sanitizer_annotate_contiguous_container" is more explicit?
Maybe also a good solution for the similar cases above.

252

"One Definition Rule violation"
It's not an initialization order problem.

[kubamracek]

Thanks for these comments, I'll have them run by more people before committing this (I'm not a native English speaker). These really need to be as user-focused as possible, so I'm really against stuff like "not owned pointer", because that's not something we should expect the user to understand. Also note that we're not using these as a replacement of full reports, we're showing them as a quick description in addition to ASan reports.

source/Plugins/InstrumentationRuntime/AddressSanitizer/AddressSanitizerRuntime.cpp

220

Multiple times have I seen that people read "stack overflow" as "stack buffer overflow" and they spend a lot of time trying to find what buffer was actually overflown... I'd like to somehow point that out. Ideas?

[filcab]

source/Plugins/InstrumentationRuntime/AddressSanitizer/AddressSanitizerRuntime.cpp

220

Maybe instead of "recursion too deep" have "stack space exhausted" or something like that?
I've seen stack overflow errors on as few as 10 (maybe even fewer) stack frames (with big objects). ASan is also more likely to make this a problem. I think seeing "recursion too deep" but having only a dozen frames is probably confusing.
Not that "stack space exhausted" is much better, but I think it's less likely to be misleading.

And yes, please ask native speakers too, as I'm not one either. :-)

[zaks.anna]

Here is another pass at this:

source/Plugins/InstrumentationRuntime/AddressSanitizer/AddressSanitizerRuntime.cpp

198

Kuba would like to drop all of these "detected" that do not add anything and are just used as filler words in all of the description messages. This will make some of them into non-complete sentences, but keeping them short is more user friendly and gets the point across just fine.

220

We could just say "Stack space exhausted" with no parentheses.

222

Drop "detected" or "Dereference of null pointer".

224

How about "Jump to a wild address". "Wild jump" sounds like a term, but I am not familiar with it.

226

"Write through", "Access through" but "Read from" (as per the English speaker on our team :)).

234

Double free is a bit of a jargon, I think it's better to be explicit:
"Deallocation of freed memory"

236

Is this always about "new" and "delete"? If so, we could try to be more explicit about it "The size of deleted object does not match the size at allocation"

"Deallocation size different than allocation size"?

238

"Deallocation of non-allocated memory"

242

"Invalid argument to malloc_usable_size"? And "Invalid argument to __sanitizer_get_allocated_size" below.

246

How about this: "Call to function disallowing overlapping memory ranges"

248

Negative size used when accessing memory

250

"Invalid arguments to __sanitizer_annotate_contiguous_container"

252

Is it possible to use something similar to what clang uses, for example, "external variable %0 defined in multiple translation units" (from ./include//clang/Basic/DiagnosticASTKinds.td)?

Ex: "Symbol defined in multiple translation units"

254

Comparison or arithmetic on pointers from different memory regions

[filcab]

I'd mostly use Anna's suggestions. Kuba: Can you remove the "detected" from the messages and refresh the patch, just so we can have one last look? Otherwise, if you think it's good enough, commit with these changes and we can do post-commit review.

source/Plugins/InstrumentationRuntime/AddressSanitizer/AddressSanitizerRuntime.cpp

198

Agree.

220

Yes.

222

I'd go for the second option.

224

"Jump to non-executable address" (basically, it's either non-readable (or unmapped) or non-executable. But I don't think we need to have those distinctions in this error message)? If not, I'm fine with Anna's suggestion too.

226

Heh. Right.

234

Good.

236

"... different from..."?
Otherwise, I like it.

[kubamracek]

Updating the patch.

[kubamracek]

In D27017#604265, @clayborg wrote:

Looks fine. One question though: do we always have a shared library that contains the ASAN runtime? If so, then we can actually put the array of short to long description into the ASAN dylib and then we read this array as a global variable. This would allow you to add as many descriptions as you want directly in your ASAN code and have LLDB detect all of the new strings. Or, if there is a function you can call with the short description that returns the long description, then we can still have the ASAN code contain all of the strings.

We do have the dylib with the runtime any time this code is run. However, I don't think these should live in the ASan runtime, because they should be considered as more high-level user-visible strings. The ASan runtime itself produces detailed technical reports that don't use the wording used here. For example, the console report will just say "Invalid pointer pair" assuming the user knows what that means. When using LLDB (and/or Xcode) we should provide less technical descriptions.

Anyway, I think this asks for a longer discussion. Are you okay with leaving these strings here for now?

[zaks.anna]

Looks great to me. Thank you Kuba!!!

[filcab]

LGTM

(I commented on a minor nit. It might just be me, so feel free to keep the current wording if you feel it's preferred)

[kubamracek]

In D27017#611894, @filcab wrote:

LGTM

(I commented on a minor nit. It might just be me, so feel free to keep the current wording if you feel it's preferred)

I'm not seeing this comment. Can you post it again?

[kubamracek]

Thanks for reviewing this!

source/Plugins/InstrumentationRuntime/AddressSanitizer/AddressSanitizerRuntime.cpp

 
 std::string
 AddressSanitizerRuntime::FormatDescription(StructuredData::ObjectSP report) {
   std::string description = report->GetAsDictionary()
                                 ->GetValueForKey("description")
                                 ->GetAsString()
                                 ->GetValue();
   if (description == "heap-use-after-free") {
     return "Use of deallocated memory detected";
   } else if (description == "heap-buffer-overflow") {
     return "Heap buffer overflow detected";
   } else if (description == "stack-buffer-underflow") {
     return "Stack buffer underflow detected";
   } else if (description == "initialization-order-fiasco") {
     return "Initialization order problem detected";
   } else if (description == "stack-buffer-overflow") {
     return "Stack buffer overflow detected";
   } else if (description == "stack-use-after-return") {
     return "Use of returned stack memory detected";
   } else if (description == "use-after-poison") {
     return "Use of poisoned memory detected";
   } else if (description == "container-overflow") {
     return "Container overflow detected";
   } else if (description == "stack-use-after-scope") {
     return "Use of out-of-scope stack memory detected";
   } else if (description == "global-buffer-overflow") {
     return "Global buffer overflow detected";
   } else if (description == "unknown-crash") {
     return "Invalid memory access detected";
+  } else if (description == "stack-overflow") {
+    return "Stack overflow detected (recursion too deep)";
+  } else if (description == "null-deref") {
+    return "Null pointer dereference detected";
+  } else if (description == "wild-jump") {
+    return "Wild pointer jump detected";
+  } else if (description == "wild-addr-write") {
+    return "Write to a wild pointer detected";
+  } else if (description == "wild-addr-read") {
+    return "Read from a wild pointer detected";
+  } else if (description == "wild-addr") {
+    return "Access to a wild pointer detected";
+  } else if (description == "signal") {
+    return "Deadly signal detected";
+  } else if (description == "double-free") {
+    return "Attempt to deallocate a freed object detected";
+  } else if (description == "new-delete-type-mismatch") {
+    return "Deallocation of a wrong size detected";
+  } else if (description == "bad-free") {
+    return "Attempt to free a non-allocated address detected";
+  } else if (description == "alloc-dealloc-mismatch") {
+    return "Mismatch between allocation and deallocation APIs detected";
+  } else if (description == "bad-malloc_usable_size") {
+    return "Invalid call to malloc_usable_size detected";
+  } else if (description == "bad-__sanitizer_get_allocated_size") {
+    return "Invalid call to __sanitizer_get_allocated_size detected";
+  } else if (description == "param-overlap") {
+    return "Overlapping memory ranges detected";
+  } else if (description == "negative-size-param") {
+    return "Negative size memory access detected";
+  } else if (description == "bad-__sanitizer_annotate_contiguous_container") {
+    return "Invalid call to __sanitizer_annotate_contiguous_container detected";
+  } else if (description == "odr-violation") {
+    return "Initialization order problem detected";
+  } else if (description == "invalid-pointer-pair") {
+    return "Invalid pointer pair in arithmetic operation detected";
   }
 
   // for unknown report codes just show the code
-  return description;
+  return "AddressSanitizer detected: " + description;
 }
 
 bool AddressSanitizerRuntime::NotifyBreakpointHit(
     void *baton, StoppointCallbackContext *context, user_id_t break_id,
     user_id_t break_loc_id) {
   assert(baton && "null baton");
   if (!baton)
     return false;