[libcxx-commits] [PATCH] D57761: [libc++] Avoid UB in the no-exceptions mode in a few places

[Louis Dionne via Phabricator via libcxx-commits]

ldionne marked 12 inline comments as done.
ldionne added a comment.

Here's the justification for changes in this patch.



================
Comment at: libcxx/include/unordered_map:1606
     if (__i == end())
-        throw out_of_range("unordered_map::at: key not found");
-#endif  // _LIBCPP_NO_EXCEPTIONS
+        __throw_out_of_range("unordered_map::at: key not found");
     return __i->second;
----------------
Justification: If we don't `abort()`, we dereference `__i` below, which is a past-the-end iterator.


================
Comment at: libcxx/include/unordered_map:1616
     if (__i == end())
-        throw out_of_range("unordered_map::at: key not found");
-#endif  // _LIBCPP_NO_EXCEPTIONS
+        __throw_out_of_range("unordered_map::at: key not found");
     return __i->second;
----------------
Ditto.


================
Comment at: libcxx/src/hash.cpp:179
     // Check for overflow
     __check_for_overflow(n);
     // Start searching list of potential primes: L * k0 + indices[in]
----------------
I guess nothing good happens if we keep going after `n` has overflown, but TBH I haven't done a deep analysis of this one.


================
Comment at: libcxx/src/ios.cpp:271
     if (((state | (__rdbuf_ ? goodbit : badbit)) & __exceptions_) != 0)
-        throw failure("ios_base::clear");
-#endif  // _LIBCPP_NO_EXCEPTIONS
+        __throw_failure("ios_base::clear");
 }
----------------
Actually, looking at this again, it seems like we probably don't want to abort here, since the stream will report the error with the error flags. So I should not change this one -- @mclow.lists does that seem right to you?


================
Comment at: libcxx/src/ios.cpp:312
         if (!new_callbacks)
-            throw bad_alloc();
-#endif  // _LIBCPP_NO_EXCEPTIONS
+            __throw_bad_alloc();
 
----------------
Justification: If we don't `abort()` when `new_callbacks` is null, we assign `new_callbacks` to `__fn_` on line 343 and  we then access `__fn[__event_size]` on line 350, which is UB.


================
Comment at: libcxx/src/ios.cpp:317
         if (!new_ints)
-            throw bad_alloc();
-#endif  // _LIBCPP_NO_EXCEPTIONS
+            __throw_bad_alloc();
     }
----------------
Ditto.


================
Comment at: libcxx/src/ios.cpp:324
         if (!new_longs)
-            throw bad_alloc();
-#endif  // _LIBCPP_NO_EXCEPTIONS
+            __throw_bad_alloc();
     }
----------------
Ditto.


================
Comment at: libcxx/src/ios.cpp:331
         if (!new_pointers)
-            throw bad_alloc();
-#endif  // _LIBCPP_NO_EXCEPTIONS
+            __throw_bad_alloc();
     }
----------------
Ditto.


================
Comment at: libcxx/src/ios.cpp:350
     {
         __fn_[__event_size_] = rhs.__fn_[__event_size_];
         __index_[__event_size_] = rhs.__index_[__event_size_];
----------------
UB here if `new_callbacks` is null and we don't `abort()`.


================
Comment at: libcxx/src/locale.cpp:472
     if (!has_facet(id))
-        throw bad_cast();
-#endif  // _LIBCPP_NO_EXCEPTIONS
+        __throw_bad_cast();
     return facets_[static_cast<size_t>(id)];
----------------
Justification: If we don't `abort()`, we will access `facets_` at an out-of-bounds index `id`. The check `has_facet(id)` roughly checks whether `id` is in bound for `facets_`.


================
Comment at: libcxx/src/locale.cpp:539
     : __locale_(name ? new __imp(name)
-                     : throw runtime_error("locale constructed with null"))
-#else  // _LIBCPP_NO_EXCEPTIONS
-    : __locale_(new __imp(name))
-#endif
+                     : (__throw_runtime_error("locale constructed with null"), (__imp*)0))
 {
----------------
Justification: If we don't `abort()`, we create the `std::string` member `name_` inside `__imp` from the `name` parameter (which is null). It's UB to construct a `std::string` from a null `char const*`.


================
Comment at: libcxx/src/locale.cpp:552
     : __locale_(name ? new __imp(*other.__locale_, name, c)
-                     : throw runtime_error("locale constructed with null"))
-#else  // _LIBCPP_NO_EXCEPTIONS
-    : __locale_(new __imp(*other.__locale_, name, c))
-#endif
+                     : (__throw_runtime_error("locale constructed with null"), (__imp*)0))
 {
----------------
Ditto.


Repository:
  rCXX libc++

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D57761/new/

https://reviews.llvm.org/D57761