util: Avoid potential uninitialized read in FormatISO8601DateTime(int64_t) by checking gmtime_s/gmtime_r return value #18162

pull practicalswift wants to merge 1 commits into bitcoin:master from practicalswift:FormatISO8601DateTime-unitialized-read changing 2 files +10 −6
  1. practicalswift commented at 7:55 pm on February 16, 2020: contributor

    Avoid potential uninitialized read in FormatISO8601DateTime(int64_t) by checking gmtime_s/gmtime_r return value.

    Before this patch FormatISO8601DateTime(67768036191676800) resulted in:

     0==5930== Conditional jump or move depends on uninitialised value(s)
 1==5930==    at 0x4F44C0A: std::ostreambuf_iterator<char, std::char_traits<char> > std::num_put<char, std::ostreambuf_iterator<char, std::char_traits<char> > >::_M_insert_int<long>(std::ostreambuf_iterator<char, std::char_traits<char> >, std::ios_base&, char, long) const (in /usr/lib/x86_64-linux-gnu/libstdc++.so.6.0.25)
 2==5930==    by 0x4F511A4: std::ostream& std::ostream::_M_insert<long>(long) (in /usr/lib/x86_64-linux-gnu/libstdc++.so.6.0.25)
 3==5930==    by 0x4037C3: void tinyformat::formatValue<int>(std::ostream&, char const*, char const*, int, int const&) (tinyformat.h:358)
 4==5930==    by 0x403725: void tinyformat::detail::FormatArg::formatImpl<int>(std::ostream&, char const*, char const*, int, void const*) (tinyformat.h:543)
 5==5930==    by 0x402E02: tinyformat::detail::FormatArg::format(std::ostream&, char const*, char const*, int) const (tinyformat.h:528)
 6==5930==    by 0x401B16: tinyformat::detail::formatImpl(std::ostream&, char const*, tinyformat::detail::FormatArg const*, int) (tinyformat.h:907)
 7==5930==    by 0x4017AE: tinyformat::vformat(std::ostream&, char const*, tinyformat::FormatList const&) (tinyformat.h:1054)
 8==5930==    by 0x401765: void tinyformat::format<int, int, int, int, int, int>(std::ostream&, char const*, int const&, int const&, int const&, int const&, int const&, int const&) (tinyformat.h:1064)
 9==5930==    by 0x401656: std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > tinyformat::format<int, int, int, int, int, int>(char const*, int const&, int const&, int const&, int const&, int const&, int const&) (tinyformat.h:1073)
10==5930==    by 0x4014CC: FormatISO8601DateTime[abi:cxx11](long) (…)

    The same goes for other very large positive and negative arguments.

    Fix by simply checking the gmtime_s/gmtime_r return value :)

  2. practicalswift renamed this:
    util: Avoid potential uninitialized read in FormatISO8601DateTime(int64_t n) by checking gmtime_s/gmtime_r return value
    util: Avoid potential uninitialized read in FormatISO8601DateTime(int64_t) by checking gmtime_s/gmtime_r return value
    on Feb 16, 2020
  3. fanquake added the label Utils/log/libs on Feb 16, 2020
  4. theStack changes_requested
  5. theStack commented at 9:38 pm on February 16, 2020: member
    gmtime_s doesn’t return a pointer, but errno_t (see https://docs.microsoft.com/en-us/cpp/c-runtime-library/reference/gmtime-s-gmtime32-s-gmtime64-s?view=vs-2019), hence the condition != 0 rather than == nullptr is needed in those cases to detect an error. See AppVeyor build failure.
  6. fanquake added the label Waiting for author on Feb 17, 2020
  7. practicalswift force-pushed on Feb 17, 2020
  8. fanquake removed the label Waiting for author on Feb 17, 2020
  9. practicalswift commented at 7:19 am on February 17, 2020: contributor
    @theStack Oh, thanks for notifying! :) Now addressed . Please re-review updated version :)
  10. Empact commented at 7:19 am on February 17, 2020: member
    The gmtime_s return value seems to be a windows-specific situation: https://en.cppreference.com/w/c/chrono/gmtime
  11. practicalswift commented at 7:22 am on February 17, 2020: contributor

    @Empact

    The gmtime_s return value seems to be a windows-specific situation: https://en.cppreference.com/w/c/chrono/gmtime

    Weird: “The implementation of gmtime_s in Microsoft CRT is incompatible with the C standard since it has reversed parameter order.” :)

    Luckily we use gmtime_s only in the case of _MSC_VER.

  12. fanquake commented at 9:11 am on February 17, 2020: member

    The unit tests are not passing:

    0test/util_tests.cpp(155): error: in "util_tests/util_FormatParseISO8601DateTime": check FormatISO8601DateTime(std::numeric_limits<int64_t>::min()) == "" has failed [1970-01-01T00:00:00Z != ]
1test/util_tests.cpp(156): error: in "util_tests/util_FormatParseISO8601DateTime": check FormatISO8601DateTime(std::numeric_limits<int64_t>::max()) == "" has failed [1969-12-31T23:59:59Z != ]
2test/util_tests.cpp(157): error: in "util_tests/util_FormatParseISO8601DateTime": check FormatISO8601Date(std::numeric_limits<int64_t>::min()) == "" has failed [1970-01-01 != ]
3test/util_tests.cpp(158): error: in "util_tests/util_FormatParseISO8601DateTime": check FormatISO8601Date(std::numeric_limits<int64_t>::max()) == "" has failed [1969-12-31 != ]
  13. DrahtBot commented at 12:18 pm on February 17, 2020: member

    The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

    Conflicts

    No conflicts as of last run.

  14. practicalswift force-pushed on Feb 17, 2020
  15. theStack approved
  17. practicalswift commented at 7:58 pm on February 17, 2020: contributor
    Added a valgrind CI job that would have caught this: see PR #18166 (ci: Run fuzz testing test cases (bitcoin-core/qa-assets) under valgrind to catch memory errors).
  18. elichai approved
  19. elichai commented at 4:09 pm on February 19, 2020: contributor
    Nice find! I’d refactor this out to a function that returns Optional<struct tm> but this can be done in a future PR ACK 6558ec35654d1a9990dcb534b18b5c88c5a4e165
  20. MarcoFalke referenced this in commit eddcbfb109 on Feb 19, 2020
  21. util: Avoid potential uninitialized read in FormatISO8601DateTime(int64_t nTime) by checking gmtime_s/gmtime_r return value 12a2f37718
  22. practicalswift force-pushed on Feb 19, 2020
  23. practicalswift commented at 10:42 pm on February 19, 2020: contributor
    Rebased and removed --exclude integer,parse_iso8601 which is not needed after the merge of this PR.
  24. sidhujag referenced this in commit 0d366a0195 on Feb 22, 2020
  25. elichai approved
  26. elichai commented at 1:16 pm on February 24, 2020: contributor
    re ACK 12a2f377185a413b740460db36812de22ee2e041
  27. MarcoFalke commented at 4:17 pm on February 24, 2020: member
    ACK 12a2f377185a413b740460db36812de22ee2e041
  29. fanquake merged this on Feb 25, 2020
  30. fanquake closed this on Feb 25, 2020
  31. sidhujag referenced this in commit a015603757 on Feb 25, 2020
  32. MarkLTZ referenced this in commit ba69387ff1 on Apr 19, 2020
  33. MarkLTZ referenced this in commit e274ffaec3 on Apr 19, 2020
  34. MarcoFalke referenced this in commit ead6d686eb on Jun 25, 2020
  35. jasonbcox referenced this in commit 3bf07d187a on Nov 1, 2020
  36. sidhujag referenced this in commit f98402e2f7 on Nov 10, 2020
  37. sidhujag referenced this in commit 98c1a0a402 on Nov 10, 2020
  38. practicalswift deleted the branch on Apr 10, 2021
  39. random-zebra referenced this in commit 14060430d9 on May 5, 2021
  40. DrahtBot locked this on Aug 18, 2022


practicalswift theStack Empact fanquake DrahtBot elichai MarcoFalke

Labels
Utils/log/libs

github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2024-12-22 06:12 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me