[lldb/DataFormatter] Check for overflow when finding NSDate epoch

[vsk]

Summary

Fixes UBSan-reported issues where the date value inside of an
uninitialized NSDate overflows the 64-bit epoch.

rdar://61774575

[davide]

Looking, as I touched NSDate last year.

[davide]

lldb/include/lldb/DataFormatters/Mock.h

2–9

Neat way to test.

lldb/source/Plugins/Language/ObjC/Cocoa.cpp

801–804

I think you can drop this comment [you just moved it, but it feels irrelevant at this point because there's only one vendor].

[JDevlieghere]

Davide already covered what I was going to say :-)

[vsk]

Thanks!

lldb/source/Plugins/Language/ObjC/Cocoa.cpp

801–804

I'll fix this before committing.

[labath]

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.

lldb/unittests/DataFormatter/MockTests.cpp

27

EXPECT_EQ(formatDateValue(...), std::string(...)) would be better. That way we'll get a test failure if the function returns None (instead of an assertion failure due to "dereferencing" an empty optional. Optional<string> and string are comparable and pretty-print correctly. The only reason this does not work out-of-the-box is because the Optional's operator== does not accept implicit conversions due to it being a template.

30

Isn't this actually std::numeric_limits<time_t>::min() (and UB due to singed wraparound) ? Did you want to convert to double before doing the +1 ?

[vsk]

@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.

lldb/unittests/DataFormatter/MockTests.cpp

30

Yes, thank you! It looks like Eric caught this before I did.

[omjavaid]

This patch breaks lldb unit tests on lldb-arm-ubuntu buildbot.

http://lab.llvm.org:8014/builders/lldb-arm-ubuntu/builds/1697

[labath]

In D80150#2045364, @vsk wrote:

@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.

Cool. Thanks.

lldb/unittests/DataFormatter/MockTests.cpp

30

Actually, thinking about that further, (for 64-bit time_ts), double(numeric_limits<time_t>::max()) is exactly the same value as double(numeric_limits<time_t>::max())+1.0 because double doesn't have enough bits to represent the value precisely. So, I have a feeling these checks are still not testing the exact thing you want to test (though I'm not sure what that is exactly).

[MForster]

In D80150#2045632, @omjavaid wrote:

This patch breaks lldb unit tests on lldb-arm-ubuntu buildbot.

http://lab.llvm.org:8014/builders/lldb-arm-ubuntu/builds/1697

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.

[MForster]

lldb/source/Plugins/Language/ObjC/Cocoa.cpp

797

This is the line where the "Illegal instruction" happens. Actually here, exactly (as there has been a follow-up change by @echristo)

[MForster]

Reverts of a few follow-up changes were necessary as well:

• 79fcd35c688b94becadd162a6c37e9b95df5c084
• 23f29b2fcc5668c51f15809067a1c3503b422c64
• f67f9e86e86e68d4cbc05ef8d6ffd0cb33246d45
• 0f1195a58648998d21bcfa1575a78a4706eaa52c
• 8214eff467f583309e9fbb971862d3c1cdcc65e4

[vsk]

lldb/unittests/DataFormatter/MockTests.cpp

30

Hm, that's right. What I'm trying to do is look for float-cast UB in two places: first, when we initially convert the double "date_value" to time_t, and second, when we add the OSXEpoch to that time_t.

I'll take a fresh cut at this. For the first conversion, I think I can rely on implementation-defined behavior from std::llrint to do the conversion without crashing LLDB due to a floating-point exception. For the second, we have checkedAdd from llvm, but the tricky part is testing it: to do that, I'm crafting a special time_t that should trigger overflow, and asserting that a round-trip conversion to/from double doesn't break the property.

[vsk]

Apologies for all the breakage. I think I've identified the issues in the
initial patch which caused breakage on the bots, and which caused the tests to
not target what they meant to test.

I've reworked this patch to use std::llrint, instead of casting the max
time_t value to double as part of an overflow check. The cast to double
was not precise, and could crash on some machines (presumably due to a floating
point exception?).

For the tests, I've added asserts to make sure that the specially-crafted
values that are meant to trigger clamping behavior in std::llrint, or an
overflow in llvm::checkedAdd, actually do satisfy those conditions.

[vsk]

Fix an ASSERT_LE that was actually meant to be an ASSERT_GE.

[labath]

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.

lldb/source/Plugins/Language/ObjC/Cocoa.cpp

794

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.

lldb/unittests/Language/AppleObjC/UtilitiesTests.cpp

43 ↗

(On Diff #265830)

What about infinities and NaNs? (I can believe this will do the right thing for the former, but I have no clue what will happen in the NaN case).

lldb/unittests/Language/CMakeLists.txt

4–6 ↗

(On Diff #265830)

I don't think the distinction between AppleObjC and ObjC is very clear, but if the code under test lives in ObjC, I think it makes sense to call the test folder the same way.

lldb/include/lldb/DataFormatters/Mock.h

+//===-- Mock.h --------------------------------------------------*- C++ -*-===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+#ifndef LLDB_DATAFORMATTERS_MOCK_H
+#define LLDB_DATAFORMATTERS_MOCK_H
+
+namespace lldb_private {
+
+class Stream;
+
+namespace formatters {
+namespace NSDate {
+
+/// Format the date_value field of a NSDate.
+bool FormatDateValue(double date_value, Stream &stream);
+
+} // namespace NSDate
+} // namespace formatters
+} // namespace lldb_private
+
+#endif // LLDB_DATAFORMATTERS_MOCK_H

lldb/source/Plugins/Language/ObjC/Cocoa.cpp

 //===-- Cocoa.cpp ---------------------------------------------------------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 #include "Cocoa.h"
 
 #include "Plugins/TypeSystem/Clang/TypeSystemClang.h"
 #include "lldb/Core/Mangled.h"
 #include "lldb/Core/ValueObject.h"
 #include "lldb/Core/ValueObjectConstResult.h"
 #include "lldb/DataFormatters/FormattersHelpers.h"
+#include "lldb/DataFormatters/Mock.h"
 #include "lldb/DataFormatters/StringPrinter.h"
 #include "lldb/DataFormatters/TypeSummary.h"
 #include "lldb/Host/Time.h"
 #include "lldb/Target/Language.h"
 #include "lldb/Target/Process.h"
 #include "lldb/Target/ProcessStructReader.h"
 #include "lldb/Target/Target.h"
 #include "lldb/Utility/DataBufferHeap.h"
 #include "lldb/Utility/Endian.h"
 #include "lldb/Utility/Status.h"
 #include "lldb/Utility/Stream.h"
 
 #include "llvm/ADT/APInt.h"
 #include "llvm/ADT/bit.h"
+#include "llvm/Support/CheckedArithmetic.h"
 
 #include "Plugins/LanguageRuntime/ObjC/AppleObjCRuntime/AppleObjCRuntime.h"
 
 #include "NSString.h"
 
 using namespace lldb;
 using namespace lldb_private;
 using namespace lldb_private::formatters;
static double decodeTaggedTimeInterval(uint64_t encodedTimeInterval) {
   DoubleBits decodedBits;
   decodedBits.sign = encodedBits.sign;
   decodedBits.fraction = encodedBits.fraction;
   decodedBits.exponent = decodeExponent(encodedBits.exponent);
 
   return llvm::bit_cast<double>(decodedBits);
 }
 
+bool lldb_private::formatters::NSDate::FormatDateValue(double date_value,
+                                                       Stream &stream) {
+  if (date_value == -63114076800) {
+    stream.Printf("0001-12-30 00:00:00 +0000");
+    return true;
+  }
+
+  if (date_value > std::numeric_limits<time_t>::max() ||
+      date_value < std::numeric_limits<time_t>::min())
+    return false;
+
+  time_t epoch = GetOSXEpoch();
+  if (auto osx_epoch = llvm::checkedAdd(epoch, (time_t)date_value))
+    epoch = *osx_epoch;
+  else
+    return false;
+  tm *tm_date = gmtime(&epoch);
+  if (!tm_date)
+    return false;
+  std::string buffer(1024, 0);
+  if (strftime(&buffer[0], 1023, "%Z", tm_date) == 0)
+    return false;
+  stream.Printf("%04d-%02d-%02d %02d:%02d:%02d %s", tm_date->tm_year + 1900,
+                tm_date->tm_mon + 1, tm_date->tm_mday, tm_date->tm_hour,
+                tm_date->tm_min, tm_date->tm_sec, buffer.c_str());
+  return true;
+}
+
 bool lldb_private::formatters::NSDateSummaryProvider(
     ValueObject &valobj, Stream &stream, const TypeSummaryOptions &options) {
   ProcessSP process_sp = valobj.GetProcessSP();
   if (!process_sp)
     return false;
 
   ObjCLanguageRuntime *runtime = ObjCLanguageRuntime::Get(*process_sp);
 
if (class_name.IsEmpty())
     return false;
 
   uint64_t info_bits = 0, value_bits = 0;
   if ((class_name == g_NSDate) || (class_name == g___NSDate) ||
       (class_name == g___NSTaggedDate)) {
     if (descriptor->GetTaggedPointerInfo(&info_bits, &value_bits)) {
       date_value_bits = ((value_bits << 8) | (info_bits << 4));
       memcpy(&date_value, &date_value_bits, sizeof(date_value_bits));
+
+      // Accomodate the __NSTaggedDate format introduced in Foundation 1600.
+      if (class_name == g___NSTaggedDate) {
+        auto *apple_runtime = llvm::dyn_cast_or_null<AppleObjCRuntime>(
+            ObjCLanguageRuntime::Get(*process_sp));
+        if (!apple_runtime)
+          return false;
+        if (apple_runtime->GetFoundationVersion() >= 1600)
+          date_value = decodeTaggedTimeInterval(value_bits << 4);
+      }
     } else {
       llvm::Triple triple(
           process_sp->GetTarget().GetArchitecture().GetTriple());
       uint32_t delta =
           (triple.isWatchOS() && triple.isWatchABI()) ? 8 : ptr_size;
       Status error;
       date_value_bits = process_sp->ReadUnsignedIntegerFromMemory(
           valobj_addr + delta, 8, 0, error);
       memcpy(&date_value, &date_value_bits, sizeof(date_value_bits));
       if (error.Fail())
         return false;
     }
   } else if (class_name == g_NSCalendarDate) {
     Status error;
     date_value_bits = process_sp->ReadUnsignedIntegerFromMemory(
         valobj_addr + 2 * ptr_size, 8, 0, error);
     memcpy(&date_value, &date_value_bits, sizeof(date_value_bits));
     if (error.Fail())
       return false;
   } else
     return false;
 
-  if (date_value == -63114076800) {
-    stream.Printf("0001-12-30 00:00:00 +0000");
-    return true;
-  }
-
-  // Accomodate for the __NSTaggedDate format introduced in Foundation 1600.
-  if (class_name == g___NSTaggedDate) {
-    auto *runtime = llvm::dyn_cast_or_null<AppleObjCRuntime>(
-        ObjCLanguageRuntime::Get(*process_sp));
-    if (runtime && runtime->GetFoundationVersion() >= 1600)
-      date_value = decodeTaggedTimeInterval(value_bits << 4);
-  }
-
-  // this snippet of code assumes that time_t == seconds since Jan-1-1970 this
-  // is generally true and POSIXly happy, but might break if a library vendor
-  // decides to get creative
-  time_t epoch = GetOSXEpoch();
-  epoch = epoch + (time_t)date_value;
-  tm *tm_date = gmtime(&epoch);
-  if (!tm_date)
-    return false;
-  std::string buffer(1024, 0);
-  if (strftime(&buffer[0], 1023, "%Z", tm_date) == 0)
-    return false;
-  stream.Printf("%04d-%02d-%02d %02d:%02d:%02d %s", tm_date->tm_year + 1900,
-                tm_date->tm_mon + 1, tm_date->tm_mday, tm_date->tm_hour,
-                tm_date->tm_min, tm_date->tm_sec, buffer.c_str());
-  return true;
+  return NSDate::FormatDateValue(date_value, stream);
 }
 
 bool lldb_private::formatters::ObjCClassSummaryProvider(
     ValueObject &valobj, Stream &stream, const TypeSummaryOptions &options) {
   ProcessSP process_sp = valobj.GetProcessSP();
   if (!process_sp)
     return false;
 

lldb/unittests/DataFormatter/CMakeLists.txt

 add_lldb_unittest(LLDBFormatterTests
   FormatManagerTests.cpp
+  MockTests.cpp
   StringPrinterTests.cpp
 
   LINK_LIBS
     lldbCore
     lldbInterpreter
     lldbSymbol
     lldbTarget
     lldbUtility
 
   LINK_COMPONENTS
     Support
   )

lldb/unittests/DataFormatter/MockTests.cpp

+//===-- MockTests.cpp -----------------------------------------------------===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+#include "lldb/DataFormatters/Mock.h"
+#include "lldb/Utility/StreamString.h"
+#include "llvm/ADT/Optional.h"
+#include "gtest/gtest.h"
+#include <string>
+
+using namespace lldb_private;
+
+// Try to format the date_value field of a NSDate.
+static llvm::Optional<std::string> formatDateValue(double date_value) {
+  StreamString s;
+  bool succcess = formatters::NSDate::FormatDateValue(date_value, s);
+  if (succcess)
+    return std::string(s.GetData());
+  return llvm::None;
+}
+
+TEST(DataFormatterMockTest, NSDate) {
+  EXPECT_EQ(*formatDateValue(-63114076800), "0001-12-30 00:00:00 +0000");
+
+  // Can't convert the date_value to a time_t.
+  EXPECT_EQ(formatDateValue(std::numeric_limits<time_t>::max() + 1),
+            llvm::None);
+  EXPECT_EQ(formatDateValue(std::numeric_limits<time_t>::min() - 1),
+            llvm::None);
+
+  // Can't add the macOS epoch to the converted date_value (the add overflows).
+  EXPECT_EQ(formatDateValue(std::numeric_limits<time_t>::max()), llvm::None);
+  EXPECT_EQ(formatDateValue(std::numeric_limits<time_t>::min()), llvm::None);
+
+  EXPECT_EQ(*formatDateValue(0), "2001-01-01 00:00:00 UTC");
+}