1909944 - UndefinedBehaviorSanitizer undefined-behavior .../mozilla/extensions/spellcheck/hunspell/glue/mozHunspellRLBoxSandbox.cpp:12:9

Status: ASSIGNED

(Core :: Spelling checker, defect, P5)

Description

[ISHIKAWA, Chiaki]

Attachment: UBSAN undefined-behavior error from comm/mail/base/test/widgets/browser_formPickers.js

Here is a UBSAN error reported while I executed mochitest using a SANITIZED version of C-C TB under local LINUX PC.

/NEW-SSD/NREF-COMM-CENTRAL/mozilla/extensions/spellcheck/hunspell/glue/mozHunspellRLBoxSandbox.cpp:12:9: runtime error: call to function unsigned int rlbox::rlbox_noop_sandbox::callback_trampoline<0u, unsigned int, void*>(void*) through pointer to incorrect function type 'unsigned int (*)(const char *)'
47:07.24 GECKO(37140) /NEW-SSD/ASAN-OBJ-DIR/objdir-tb3/dist/include/mozilla/rlbox/rlbox_noop_sandbox.hpp:69: note: unsigned int rlbox::rlbox_noop_sandbox::callback_trampoline<0u, unsigned int, void*>(void*) defined here
47:07.44 GECKO(37140)	  #0 0x7f3f47d35d81 in FileMgr::FileMgr(char const*, char const*) /NEW-SSD/NREF-COMM-CENTRAL/mozilla/extensions/spellcheck/hunspell/glue/mozHunspellRLBoxSandbox.cpp:12:9
47:07.44 GECKO(37140)	  #1 0x7f3f47d35d81 in HashMgr::load_config(char const*, char const*) /NEW-SSD/NREF-COMM-CENTRAL/mozilla/extensions/spellcheck/hunspell/src/hashmgr.cxx:930:25
47:07.45 GECKO(37140)	  #2 0x7f3f47d34a1a in HashMgr::HashMgr(char const*, char const*, char const*) /NEW-SSD/NREF-COMM-CENTRAL/mozilla/extensions/spellcheck/hunspell/src/hashmgr.cxx:101:3
47:07.45 GECKO(37140)	  #3 0x7f3f47d41974 in HunspellImpl::HunspellImpl(char const*, char const*, char const*) /NEW-SSD/NREF-COMM-CENTRAL/mozilla/extensions/spellcheck/hunspell/src/hunspell.cxx:179:25

The full log is attached.
There are about a dozen similar errors from the same line of code in the full mochitest log and I quote only the first instance.

Comment 1

[ISHIKAWA, Chiaki]

C-C TB requires a patch to eliminate other UBSAN issue.
It is likely that SANITIZED version of C-C TB will fail before it proceeds to report the erorr I report here.

https://bugzilla.mozilla.org/show_bug.cgi?id=1852662

Comment 2

[Andrew Sutherland [:asuth] (he/him)]

As I understand what's happening, rlbox creates a trampoline where the function pointer arg is void* instead of const char* for the call here which might be an intentional thing?.

Deian, you are listed as the author of the hunspell rlboxing code in the log and are listed in the peers list as active and :tjr is currently not around / able to be needinfo-ed. Do you know if this is expected behavior? I see rlbox has an issue about testing with ubsan that is open, so it would not be surprising if this is expected-ish. Thanks!

Comment 3

[Deian Stefan]

(In reply to Andrew Sutherland [:asuth] (he/him) from comment #2)

As I understand what's happening, rlbox creates a trampoline where the function pointer arg is void* instead of const char* for the call here which might be an intentional thing?.

Yep this is exactly right!

Do you know if this is expected behavior? I see rlbox has an issue about testing with ubsan that is open, so it would not be surprising if this is expected-ish. Thanks!

Yep this is expected and in rlbox v2 the plan is to not rely on this casting (and make ubsan happy),.

Comment 4

[Shravan Narayan]

All of the above is right. Just to add some more detail, RLBox in essence hides the fact that it basically has to (in spirit) do the equivalent of dlsym to lookup function pointers in sandboxed code prior to executing it, i.e., get a function pointer whose type is generically stored as a void* and cast it to its target type prior to using it. Unfortunately, C++ doesn't really have a great way of doing this that is technically UB-free; in practice, this doesn't matter as compilers allow this as they basically have to support patterns like dlsym. The stack overflow discussion here captures the rough idea

https://stackoverflow.com/questions/1096341/function-pointers-casting-in-c

Comment 5

[Andrew Sutherland [:asuth] (he/him)]

Thank you both for the context and clarifications!

Comment 6

[ISHIKAWA, Chiaki]

Fair enough.

Does anyone know how I can whitelist this particular undefined behavior so that the
UBSAN version of C-C TB does not stop upon seeing this UBSAN event?
I have a feeling that there are a few more hidden UBSAN events because C-C TB does not proceed beyond this.

After I created a patch for Bug 1852662,
I already found a few UBSAN issues by running mochitest and xpcshell-test using UBSAN version of C-C TB.
I think if I can proceed past this UBSAN trap reported here, I may find a few more.

TIA

Comment 7

[Shravan Narayan]

Does anyone know how I can whitelist this particular undefined behavior so that the
UBSAN version of C-C TB does not stop upon seeing this UBSAN event?

I haven't tried this myself, so not sure if this works, but maybe you can just put an ubsan ignore attribute on the rlbox function that performs this cast

Comment 8

[ISHIKAWA, Chiaki]

Attachment: WIP: Bug 1909944 - Ignoring known UBSAN behavior. r=deian,shravanrn

Comment 9

[ISHIKAWA, Chiaki]

(In reply to Shravan Narayan from comment #7)

Does anyone know how I can whitelist this particular undefined behavior so that the
UBSAN version of C-C TB does not stop upon seeing this UBSAN event?

I haven't tried this myself, so not sure if this works, but maybe you can just put an ubsan ignore attribute on the rlbox function that performs this cast

Thank you for the comment. Before I read your comment, I embarked on setting ubsan ignore attribute on problematic functions, and
I ended up applying the attribute on three different functions since I encountered two more UBSANs in successes once I eliminated the first one reported here.

History that led to the patch submitted.

(1) #if defined(clang)
attribute((no_sanitize("undefined")))
#endif
FileMgr::FileMgr(const char* aFilename, const char* aKey) : mFd(0) {
// The key is not used in firefox
// (void*) casting is to shut up UBSAN runtime check.
mFd = moz_glue_hunspell_create_filemgr(aFilename);
}

Applied the above.
Then I hit the second UBSAN.

(2)
/NEW-SSD/NREF-COMM-CENTRAL/mozilla/extensions/spellcheck/hunspell/glue/mozHunspellRLBoxSandbox.cpp:21:13: runtime error: call to function bool rlbox::rlbox_noop_sandbox::callback_trampoline<1u, bool, unsigned int, void*>(unsigned int, void*) through pointer to incorrect function type 'bool (*)(unsigned int, char )'
00:33.65 GECKO(1332839) /NEW-SSD/ASAN-OBJ-DIR/objdir-tb3/dist/include/mozilla/rlbox/rlbox_noop_sandbox.hpp:69: note: bool rlbox::rlbox_noop_sandbox::callback_trampoline<1u, bool, unsigned int, void>(unsigned int, void) defined here
00:33.84 GECKO(1332839)

Here is the function.

bool FileMgr::getline(std::string& aResult) {
char* line = nullptr;
bool ok = moz_glue_hunspell_get_line(mFd, &line);
if (ok && line) {
aResult = line;
free(line);
}
return ok;
}

Applied ignore ubsan attribute to the above.

The I got another ubsan runtime error.

(3)

00:33.41 GECKO(1344682) /NEW-SSD/NREF-COMM-CENTRAL/mozilla/extensions/spellcheck/hunspell/src/csutil.cxx:2284:10: runtime error: call to function void* rlbox::rlbox_noop_sandbox::callback_trampoline<6u, void*, void*>(void*) through pointer to incorrect function type 'cs_info ()(const char )'
00:33.41 GECKO(1344682) /NEW-SSD/ASAN-OBJ-DIR/objdir-tb3/dist/include/mozilla/rlbox/rlbox_noop_sandbox.hpp:69: note: void rlbox::rlbox_noop_sandbox::callback_trampoline<6u, void*, void*>(void*) defined here
00:33.59 GECKO(1344682) #0 0x7ff203d0bf7d in get_current_cs(std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char>> const&) /NEW-SSD/NREF-COMM-CENTRAL/mozilla/extensions/spellcheck/hunspell/src/csutil.cxx:2284:10
00:33.59 GECKO(1344682) #1 0x7ff203d0bf7d in HashMgr::load_config(char const*, char const*) /NEW-SSD/NREF-COMM-CENTRAL/mozilla/extensions/spellcheck/hunspell/src/hashmgr.cxx
00:33.59 GECKO(1344682) #2 0x7ff203d0ac0a in HashMgr::HashMgr(char const*, char const*, char const*) /NEW-SSD/NREF-COMM-CENTRAL/mozilla/extensions/spellcheck/hunspell/src/hashmgr.cxx:101:3
00:33.59 GECKO(1344682) #3 0x7ff203d17ed4 in HunspellImpl::HunspellImpl(char const*, char const*, char const*) /NEW-SSD/NREF-COMM-CENTRAL/mozilla/extensions/spellcheck/hunspell/src/hunspell.cxx:179:25
00:33.59 GECKO(1344682) #4 0x7ff203d37e14 in Hunspell_create /NEW-SSD/NREF-COMM-CENTRAL/mozilla/extensions/spellcheck/hunspell/src/hunspell.cxx:2164:43
00:33.59 GECKO(1344682) #5 0x7ff203c6e9d7 in auto rlbox::rlbox_noop_sandbox::impl_invoke_with_func_ptr<Hunhandle* (char const*, char const*), void* (void*, void*), void*, void*>(void* ()(void, void*), void*&&, void*&&) /NEW-SSD/ASAN-OBJ-DIR/objdir-tb3/dist/include/mozilla/rlbox/rlbox_noop_sandbox.hpp:187:12

So I applied the ignore ubsan attribute to the following function.

struct cs_info* get_current_cs(const std::string& es) {
return moz_hunspell_GetCurrentCS(es.c_str());
}

Now all is well.

I did not apply the ignore ubsan attribute to a generic function because it seems that ubsan did not like to meddle with
polymorphic function, but I may be wrong.

I wish SANITIZER has a better documentation. I hoped I could define a whitelist in a text file and pass it to
runtime WITHOUT re-compling. But to no avail.
By running SANITIZEd version of C-C TB with the followng

UBSAN_OPTIONS=report_error_type=1

I learned that |function-type-mismatch| was reported.
But trying to suppress this using runtime suppression method mentioned in the following, never seemed to work.
Either there is a dichotomy of available documentation and the implementation, or the implementation is simply broken since there are many posts regarding the failure to make runtime suppression work.

The patch I posted is what I am using now.
C-C TB now produces one more known UBSAN error in M-C code. It happens a couple of times depending on the timing (obviously it is caused by a race condition of a sort), and may mask other UBSAN errors possibly, but who knows.

Comment 10

[ISHIKAWA, Chiaki]

M-C FF compilation with the patch for this bugzilla entry is in the following.
https://treeherder.mozilla.org/jobs?repo=try&revision=1fc73e1e1222c9597694bca98e17f738dfe594ff
FF builds and executes fine, I think.

Locally, I test the patch and sanitized C-C TB compiled with clang works fine.
I also checked GCC compilation just in case. It builds.

So there are no easy-to-spot issues with the patch as far as I can tell.

Comment 11

[Andrew Sutherland [:asuth] (he/him)]

Moving bug 1766721 to see also since the patch that's up for review will mitigate the problem here and the patch doesn't depend on bug 1766721 being fixed. (When bug 1766721 is fixed, the proposed mitigation patch here can be removed.)