Help with diagnosing a tricky bug

[Jeffrey Yu]

Hi Chrome developers,

I could use an extra pair of eyes from some C++ exports in helping determine the cause of a bug that resulted in some user-visible errors.

This is the original CL: https://crrev.com/c/7363419, which was intended to refactor some string handling code to use string_view, in order to avoid unnecessary string copies. It was auto-reverted after automated detection determined it to be the source of a crash in https://g-issues.chromium.org/issues/474893357, but not before it made it into a release M145, which is believed to be the cause of a second and different bug: https://g-issues.chromium.org/issues/483672730. In the second bug, users view an incorrectly displayed domain string (example screenshot: https://www.reddit.com/r/chrome/comments/1r1p1mu/very_weird_problem_and_i_do_not_know_how_fix_it/) 

I was able to track down the cause of the crash that caused first bug, and have verified this by adding unit tests: https://crrev.com/c/7663088. 

The logic in chrome/browser/extensions/api/settings_private/prefs_util.cc was updated from using a string to a string_view to take advantage of the API change in the change described above.

Before

std::string string_value = value->GetString(); if (IsPrefTypeURL(pref_name)) {   GURL fixed = url_formatter::FixupURL(string_value, std::string());   if (fixed.is_valid()) {     string_value = fixed.spec();   } else {     string_value = std::string();   } } pref_service->SetString(pref_name, string_value);

After (changes bolded)

std::string_view string_value = value->GetString(); if (IsPrefTypeURL(pref_name)) {   GURL fixed = url_formatter::FixupURL(string_value, "");   if (fixed.is_valid()) {     string_value = fixed.spec();   } else {     string_value = "";   } } pref_service->SetString(pref_name, string_value);

In the changed code, `string_value` was changed to a string_view instead of a string. In one branch of the code, it gets assigned to `fixed.spec()`. However, since `fixed` is a temporary object that leaves scope at the end of the if-statement, the resulting string_view becomes invalid as its internal pointer points to a deallocated object. 

As `prefs_util.cc` previously had no unit test coverage, the potential issue wasn't caught by unit tests. 

However, I haven't been able to recreate the second bug. I initially suspected that it might have been due to the changes I made to the string editing logic in components/url_formatter/url_fixer.cc, but I added unit tests (https://crrev.com/c/7644611) and was unable to verify this.

I don't think the change above would have resulted in the garbage characters, as the string leaving scope would leave the string_view with an invalid pointer, which should result in a seg fault and a crash instead when it gets dereferenced.

So I could get some extra pairs of eyes to look over the CL and see if there's any other places that could have caused the bug, or if this invalid string_view usage indeed was the cause.

[David Baron]

My intuition would be that "garbage characters" would be the normal failure mode for the buggy code you've pointed to, and that a crash would require some sort of hardening being done in the interaction between the allocator and the OS.  That's based on my (unverified) assumption that std::string_view is implemented as either a pair of pointers, or a pointer and a length, so it just has pointers to the character data, and reading characters from a std::string_view just has a single dereference of the pointer(s) it was constructed with.  So I would think freeing the buffer containing that character data would normally return the memory to the allocator but not (barring some sort of hardening) return it to the OS that would make accesses to it crash.  And I don't immediately see anything that causes an additional dereference beyond just reading characters from a previously-valid pointer.  I'm not familiar with what hardening techniques are enabled on what platforms, or how to detect signs of them in crash reports -- someone more familiar with this might be able to confirm.  But after looking at this for a few minutes I think it's plausible that both bugs could have come from the error you've already found.

-David

--
You received this message because you are subscribed to the Google Groups "cxx" group.
To unsubscribe from this group and stop receiving emails from it, send an email to cxx+uns...@chromium.org.
To view this discussion visit https://groups.google.com/a/chromium.org/d/msgid/cxx/CAD4jpnOQ0yOB6Rm%2Bjve3X06esM0DTFNF2JC5pKN5zn0%3D6fwGCw%40mail.gmail.com.