bashi@chromium.org, can you please take a look at this or assign it to someone who can? Thank you.

behdad@, drott@: could you triage this?

This issue is a security regression. If you are not able to fix this quickly, please revert the change that introduced it.

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot

> time error: call to function (unknown) through pointer to incorrect function type void (*)(void *)

Is there a compiler warning flag to warn when functions are cast to a different function type?  That can help me clean up harfbuzz codebase.

Nico, would you know of any such warning? Thanks.

Function pointer types are generally safe to convert between (and it has defined behavior too). What doesn't have defined behavior is a conversion between function pointers and member function pointers, see e.g. https://plus.google.com/+NicoWeberCorp/posts/12YU9rZ6bF4 for some background.

Is it understood what exactly ubsan flags here? If so, please link to that code and I can see if there's a warning to flag whatever is there. As is, I lack context to give a good answer.

We're close to promoting M58 to Beta (next week March 16th). This bug is listed as RB-Beta-blocker. Can you please take a look at this bug and resolve before Beta promotion?

This is not blocker.

behdad: Uh oh! This issue still open and hasn't been updated in the last 14 days. This is a serious vulnerability, and we want to ensure that there's progress. Could you please leave an update with the current status and any potential blockers?

If you're not the right owner for this issue, could you please remove yourself as soon as possible or help us find the right one?

If the issue is fixed or you can't reproduce it, please close the bug. If you've started working on a fix, please set the status to Started.

Thanks for your time! To disable nags, add the Disable-Nags label.

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot

Behdad, any ideas?

> Function pointer types are generally safe to convert between (and it has defined behavior too).

Indeed. That's my understanding as well.

> What doesn't have defined behavior is a conversion between function pointers and member function pointers, see e.g.

We don't use member function pointers, nor virtual methods, in HarfBuzz at all.


> Is it understood what exactly ubsan flags here? If so, please link to that code and I can see if there's a warning to flag whatever is there. As is, I lack context to give a good answer.

If you follow the first link in the report you see it.  It's a good old C idiom to call a destroy function:

../../third_party/harfbuzz-ng/src/hb-blob.cc:74:5: runtime error: call to function (unknown) through pointer to incorrect function type void (*)(void *)
(/mnt/scratch0/clusterfuzz/slave-bot/builds/chromium-browser-libfuzzer_linux-release-ubsan_ae530a86793cd6b8b56ce9af9159ac101396e802/revisions/libfuzzer-linux-release-453227/renderer_tree_fuzzer+0x37db570): note: (unknown) defined here
#0 0x37db9de in _hb_blob_destroy_user_data(hb_blob_t*) third_party/harfbuzz-ng/src/hb-blob.cc:74:5

    blob->destroy (blob->user_data); 

Ok! I think I understand now what this is flagging.  We are calling a void *(void) function as a void *(void *) function, which works fine with C calling conventions, but apparently is undefined.

I'll clean these up soon.  There's no security impact.

Humm. Umm. Ignore the last two paragraphs of my reply.  But I think I understand what's going on.  It's not the cast per se that is problematic.  What it's complaining about is calling a void *(hb_blob_t *) as a void *(void *).  While that works fine on most architectures, I can imagine it being undefined.  I'll fix.

Does this also mean that, eg., compare functions passed to bsearch() or qsort() need to match the void* prototype of their arg exactly?

Actually, before going on and fixing, I appreciate feedback on my understanding.  Thanks.

Here's the code:

https://github.com/behdad/harfbuzz/blob/740fdbcd0e6d25c1d6f12537ca2aa559650b9d52/src/hb-blob.cc#L74

where destroy is set here:

https://github.com/behdad/harfbuzz/blob/740fdbcd0e6d25c1d6f12537ca2aa559650b9d52/src/hb-blob.cc#L167

A friendly reminder that M58 Stable is launch is coming soon (less than 2 weeks)! Your bug is labelled as Stable ReleaseBlock, pls make sure to land the fix and get it merged into the release branch ASAP so it gets enough baking time in Beta (before Stable promotion). Thank you!

Reducing priority and removing RB-S based on discussion above.

behdad: Uh oh! This issue still open and hasn't been updated in the last 14 days. This is a serious vulnerability, and we want to ensure that there's progress. Could you please leave an update with the current status and any potential blockers?

If you're not the right owner for this issue, could you please remove yourself as soon as possible or help us find the right one?

If the issue is fixed or you can't reproduce it, please close the bug. If you've started working on a fix, please set the status to Started.

Thanks for your time! To disable nags, add the Disable-Nags label.

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot

I need help, ie. answers to my comments 19..21 before I can do anything.

This reproduces easily given the instructions in comment 0. After playing with it a bit, here's the situation in simplified. Harfbuzz can have generic data (as void*) in an hb_blob, and a generic function to destroy said data, as a pointer to a void(void*). Harfbuzz then uses this structure to store a parent hb_blob in that generic data blob, and passes the hb_blob destruction function for the generic destruction function. In reduced:

thakis@thakis:~/src/chrome/src$ cat test.cc
struct S { int i; };
void process_s(S* ps) { ps->i = 4; }

int main() {
   S s = {};

   void* generic_pointer = &s;
   void (*generic_fun)(void*) = (void(*)(void*))&process_s;

   generic_fun(generic_pointer);
}
thakis@thakis:~/src/chrome/src$ third_party/llvm-build/Release+Asserts/bin/clang++ -o foo test.cc -fsanitize=undefined
thakis@thakis:~/src/chrome/src$ ./foo
test.cc:10:4: runtime error: call to function process_s(S*) through pointer to incorrect function type 'void (*)(void *)'
(/usr/local/google/home/thakis/src/chrome/src/foo+0x423ac0): note: process_s(S*) defined here

I think the problem here is that hb_blob_destroy(hb_blob_t*) takes an hb_blob_t*, not a void*, but it's called through a function pointer taking void*. So the callee has a guarantee (parameter has type hb_blob_t*) that isn't actually guaranteed by the caller.

One possible fix would be to have a

void hb_blob_destroy_through_void(void* hopefully_hb_blob) {
  // This function assumes that the passed-in type is an hb_blob_t,
  // but it can't verify that. Be careful when you use it.
  hb_blob_destroy((hb_blob_t*)hopefully_hb_blob);
}

and then pass that instead of hb_blob_destroy in https://github.com/behdad/harfbuzz/blob/740fdbcd0e6d25c1d6f12537ca2aa559650b9d52/src/hb-blob.cc#L167 (then you won't need the cast there).

+krasin since he's dealt with this type of thing often and can tell me if there's a more elegant way.

The suggestion from thakis@ is good to follow. I don't have a better suggestions anyway.

Thanks.  Yes, I figured that adding a wrapper will probably fix it.  But that doesn't explain why I have not received more of these reports.  AFAIU it's a common idiom in C to do what I'm doing.  For example, I'm doing the same when calling qsort() and bsearch() library functions...

Anyway, I'll go ahead and add wrappers, because I've also got reports that that's needed for emscripten to work correctly.

I'm still interested to know whether this is actually undefined by the standards.

> For example, I'm doing the same when calling qsort() and bsearch() library functions...

UBSan is a compiler instrumentation added by the compiler at compile time. qsort and bsearch are in the C library and not compiled as part of Chrome's build process, which is probably why we don't notice that.

As for standard, here's from the C standard, C++ is probably similar (but maybe more restrictive):

Annex J lists undefined behaviors, it contains "A pointer is used to call a function whose type is not compatible with the pointed-to type (6.3.2.3)."

That section reads: (6.3.2.3p8): """A pointer to a function of one type may be converted to a pointer to a function of another type and back again; the result shall compare equal to the original pointer. If a converted pointer is used to call a function whose type is not compatible with the pointed-to type, the behavior is undefined."""

Now, when are function types compatible? 6.7.5.3p15: """[...]Moreover, the parameter type lists, if both are present, shall agree in the number of
parameters and in use of the ellipsis terminator; corresponding parameters shall have compatible types."""

The parameters are type pointer-to-something, when are pointer types compatible? 6.7.5.1p2: """For two pointer types to be compatible, both shall be identically qualified and both shall be pointers to compatible types."""

And types void and hb_blob_t aren't compatible. So this does indeed look like undefined behavior per standard.

Thank you very much!  I'll go ahead and fix them all.

I'm tracking this upstream:
https://github.com/behdad/harfbuzz/issues/474

Can we make this public? There's no security implications.

Sure, let's.

ClusterFuzz has detected this issue as fixed in range 489276:489324.

Detailed report: https://clusterfuzz.com/testcase?key=6172238601781248

Fuzzer: libFuzzer_renderer_tree_fuzzer
Job Type: libfuzzer_chrome_ubsan
Platform Id: linux

Crash Type: Incorrect-function-pointer-type
Crash Address: 
Crash State:
  _hb_blob_destroy_user_data
  hb_blob_destroy
  hb_face_t::get_upem
  
Sanitizer: undefined (UBSAN)

Recommended Security Severity: Medium

Regressed: https://clusterfuzz.com/revisions?job=libfuzzer_chrome_ubsan&range=453205:453227
Fixed: https://clusterfuzz.com/revisions?job=libfuzzer_chrome_ubsan&range=489276:489324

Reproducer Testcase: https://clusterfuzz.com/download?testcase_id=6172238601781248


See https://chromium.googlesource.com/chromium/src/+/master/testing/libfuzzer/reproducing.md for more information.

If you suspect that the result above is incorrect, try re-doing that job on the test case report page.

ClusterFuzz testcase 6172238601781248 is verified as fixed, so closing issue as verified.

If this is incorrect, please add ClusterFuzz-Wrong label and re-open the issue.

This bug has been closed for more than 14 weeks. Removing security view restrictions.

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot