Fixes UBSan-reported issues where the date value inside of an
uninitialized NSDate overflows the 64-bit epoch.
rdar://61774575
Differential D80150
[lldb/DataFormatter] Check for overflow when finding NSDate epoch vsk on May 18 2020, 12:13 PM. Authored by
Details Fixes UBSan-reported issues where the date value inside of an rdar://61774575
Diff Detail
Event TimelineComment Actions Thanks!
Comment Actions While I think the idea in this patch is good, the actual implementation seems problematic to me, on several levels. The name Mock.h is overly generic, and inappropriate. According to wikipedia, mock (object)s are "simulated objects that mimic the behavior of real objects in controlled ways, most often as part of a software testing initiative". This file misses that mark completely, because it is not simulating anything -- it *is* the real thing, production code. It's true that the main reason it is even declared in a header is testing, but that does not make it a "mock". A mock "nsdate formatter" would be if we had the ability to, in a test, replace the implementation of the "convert an nsdate value to a string". Such a thing might potentially be useful as a way to test some higher level data formatter functionality, but it is definitely not what is happening here. The second issue is that the placement of the file in lldb/DataFormatter breaks encapsulation. The generic data formatter code should have no knowledge of a particular language (runtime) implementation details. That might be excused if this particular functionality was useful for more than one language, but I don't see any evidence that this is the case. What's worse, by putting the declaration into lldb/DataFormatter, but having the implementation in Language/ObjC you're making it very easy to unknowingly depend on a plugin. Last, but not least, it is against the llvm library layering rules. (The paths there are specific to the llvm subtree, but it's clear the intent is for the header/interface and the implementation to live together.) TL;DR: The fix is quite simple: move lldb/DataFormatter/Mock.h to Plugins/Language/ObjC and rename it to something less misleading (Utilities.h ?) And move the test to unittests/Language/ObjC.
Comment Actions @labath Agreed on all points, I've addressed the feedback in 82dbf4aca84 by moving "DataFormatters/Mock.h" to "Plugins/Language/ObjC/Utilities.h", and adding a separate LanguageObjCTests unit test.
Comment Actions This patch breaks lldb unit tests on lldb-arm-ubuntu buildbot. http://lab.llvm.org:8014/builders/lldb-arm-ubuntu/builds/1697 Comment Actions Cool. Thanks.
Comment Actions It also breaks our internal build bots with an "Illegal instruction". Here is the stacktrace: 0 <redacted>libsupport.so 0x00007f8c5f6d87ba llvm::sys::PrintStackTrace(llvm::raw_ostream&) + 58 1 <redacted>libsupport.so 0x00007f8c5f6d8c79 2 <redacted>libsupport.so 0x00007f8c5f6d71bb llvm::sys::RunSignalHandlers() + 123 3 <redacted>libsupport.so 0x00007f8c5f6d92fb 4 libpthread.so.0 0x00007f8c5e7509a0 5 <redacted>libcore.so 0x00007f8c764820b1 lldb_private::formatters::NSDate::FormatDateValue(double, lldb_private::Stream&) + 129 6 <redacted>unittests_Sliblanguage_Utests.so 0x00007f8c771c29b5 7 <redacted>unittests_Sliblanguage_Utests.so 0x00007f8c771c24d8 DataFormatterMockTest_NSDate_Test::TestBody() + 248 8 <redacted>libgtest.so 0x00007f8c5f85ce44 void testing::internal::HandleSehExceptionsInMethodIfSupported<testing::Test, void>(testing::Test*, void (testing::Test::*)(), char const*) + 132 9 <redacted>libgtest.so 0x00007f8c5f84b532 void testing::internal::HandleExceptionsInMethodIfSupported<testing::Test, void>(testing::Test*, void (testing::Test::*)(), char const*) + 114 10 <redacted>libgtest.so 0x00007f8c5f840866 testing::Test::Run() + 182 11 <redacted>libgtest.so 0x00007f8c5f840eb5 testing::TestInfo::Run() + 213 12 <redacted>libgtest.so 0x00007f8c5f8413e4 testing::TestCase::Run() + 228 13 <redacted>libgtest.so 0x00007f8c5f8461db testing::internal::UnitTestImpl::RunAllTests() + 731 14 <redacted>libgtest.so 0x00007f8c5f861a24 bool testing::internal::HandleSehExceptionsInMethodIfSupported<testing::internal::UnitTestImpl, bool>(testing::internal::UnitTestImpl*, bool (testing::internal::UnitTestImpl::*)(), char const*) + 132 15 <redacted>libgtest.so 0x00007f8c5f84cf42 bool testing::internal::HandleExceptionsInMethodIfSupported<testing::internal::UnitTestImpl, bool>(testing::internal::UnitTestImpl*, bool (testing::internal::UnitTestImpl::*)(), char const*) + 114 16 <redacted>libgtest.so 0x00007f8c5f845eda testing::UnitTest::Run() + 186 17 <redacted>libgtest_Umain.so 0x00007f8c5f8b7ce1 RUN_ALL_TESTS() + 17 18 <redacted>libgtest_Umain.so 0x00007f8c5f8b7c6a main + 186 19 libc.so.6 0x00007f8c5e5bebbd __libc_start_main + 253 20 language_tests 0x0000556b0f40dd09 Illegal instruction @gribozavr2 is preparing a revert of this commit and the two follow-up changes.
Comment Actions Apologies for all the breakage. I think I've identified the issues in the I've reworked this patch to use std::llrint, instead of casting the max For the tests, I've added asserts to make sure that the specially-crafted Comment Actions I don't know how widespread the llrint behavior is. Clamping to the largest/smallest int seems reasonable, so it might be reasonable to depend on that -- though I don't know what will that do for NaNs... I actually don't think the previous approach was wrong. IIUC, it only became a problem when Eric added an explicit int cast to the input value -- it should have been done the other way around, casting the integer to the float type. Then we'd just need to be careful in the test to select a number that is sufficiently bigger than UINT64_MAX, so that the difference is observable after conversion to floating point. In a way, I think that the real problem here is that we're using host floating point arithmetic on numbers that are: a) untrusted; b) come from a system which has potentially different floating point encoding/semantics. A more principled approach would be to do it like the compiler, and work with APFloats with semantics explicitly configured from based on the target.
|
I think that changing !_WIN32 to APPLE is a step in the wrong direction. Ideally, this function should work the same everywhere. The way to achieve that would be to have a host function which selects between gmtime and _mkgmtime depending on the platform. But failing that, I don't see a reason to restrict the scope of this further.