This is an archive of the discontinued LLVM Phabricator instance.

[Sanitizer] Add interceptor for wctomb
ClosedPublic

Authored by labath on Mar 19 2019, 8:36 AM.

Details

Summary

This is required to avoid msan false positives for code using this
function (although generally one should avoid using this function in
favor of wcrtomb).

The interceptor and test code have been based on wcrtomb, with obvious
modifications.

Diff Detail

Repository
rL LLVM

Event Timeline

labath created this revision.Mar 19 2019, 8:36 AM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptMar 19 2019, 8:36 AM
vitalybuka added inline comments.
lib/msan/tests/msan_test.cc
2126 ↗(On Diff #191312)

Please also create sanitizer_common lit tests just to make sure that function still works

lib/sanitizer_common/sanitizer_common_interceptors.inc
3549 ↗(On Diff #191312)

I don't like that we add another FIXME here
Could you try to make something similar to https://reviews.llvm.org/rL211153

3552 ↗(On Diff #191312)

write_cnt is not needed

labath updated this revision to Diff 191484.Mar 20 2019, 6:40 AM
labath marked 3 inline comments as done.
  • add sanitizer_common test
  • fix the "write to freed memory" issue by redirecting the function call to a local buffer. To know the size of the buffer I need to allocate, I needed to add a new "platform limits" constant for the value of MB_LEN_MAX (another option would be to use some upper bound which is large enough for all reasonable platforms, such as 32).

I'm very new to the sanitizer world, so I'm not sure I'm doing everything
correctly. Please let me know if I have done something wrong.

vitalybuka added inline comments.Mar 20 2019, 1:11 PM
lib/sanitizer_common/sanitizer_common_interceptors.inc
3549 ↗(On Diff #191312)

could you avoid heap here?
with check local_dest[64] or any value large enough

vitalybuka added a comment.EditedMar 20 2019, 1:13 PM

you may add CHECK_LE(res, sizeof(local_dst))

option would be to use some upper bound which is large enough for all
reasonable platforms, such as 32).

yes, this will be better, without heap

labath updated this revision to Diff 191645.Mar 21 2019, 1:49 AM

avoid heap allocations by using a stack buffer of fixed size which "should be enough for everyone".

labath marked an inline comment as done.Mar 28 2019, 6:17 AM

Vitaly, Evgenii, do you have any more comments about this?

This revision is now accepted and ready to land.Mar 28 2019, 9:37 AM
This revision was automatically updated to reflect the committed changes.