[Lldb-commits] [PATCH] D73860: [lldb/StringPrinter] Avoid reading garbage in uninitialized strings

Wed Feb 12 11:01:14 PST 2020

teemperor accepted this revision.
teemperor added a comment.
This revision is now accepted and ready to land.

In D73860#1872746 <https://reviews.llvm.org/D73860#1872746>, @vsk wrote:

> Ping, @teemperor do you have any concerns about this one?

Didn't see Jonas comment when he accepted, sorry. LGTM.

================
Comment at: lldb/packages/Python/lldbsuite/test/functionalities/data-formatter/data-formatter-stl/libcxx/string/main.cpp:29
+    if (sizeof(std::string) == sizeof(garbage_string_sso))
+      memcpy((void *)&garbage1, &garbage_string_sso, sizeof(std::string));
+    if (sizeof(std::string) == sizeof(garbage_string_long))
----------------
teemperor wrote:
> shafik wrote:
> > vsk wrote:
> > > shafik wrote:
> > > > While I get what you are doing here, we know he structure of libc++ SSO implementation and we are manually building a corrupt one, this is fragile to changes in the implementation. 
> > > > 
> > > > I don't have an immediate suggestion for an alternative approach but if we stick with this we should stick a big comment explaining this, perhaps laying out the assumptions of the internal layout we are assuming and maybe some sanity checks maybe using `offsetof` to verify fields exist and are where we expect them to be.
> > > I don't see how this is fragile. The structure of libc++'s SSO implementation is ABI, and is unlikely to change (esp. not in a way that turns either one of the garbage strings into a valid string). I've left comments explaining what's wrong with both of the garbage strings, but can leave a pointer to https://joellaity.com/2020/01/31/string.html for more info?
> > Sure, that note would be fine.
> Can you instead do a `#if _LIBCPP_ABI_VERSION == 1` and have the #else as an #error that this test needs updating. We don't support any other libc++ ABI beside 1 in LLDB but if we ever do then this should not silently pass.
Actually the #if *and* a static_assert comparing the size would be best IMHO.

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D73860/new/

https://reviews.llvm.org/D73860