Skip to content

GH-41044: [C++] formatting.h: Make sure space is allocated for the 'Z' when formatting timestamps #41045

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
felipecrv merged 3 commits into from
Apr 6, 2024
Merged

GH-41044: [C++] formatting.h: Make sure space is allocated for the 'Z' when formatting timestamps #41045

felipecrv merged 3 commits into from
Apr 6, 2024

Conversation

@felipecrv
Copy link
Contributor

@felipecrv felipecrv commented Apr 5, 2024 •
edited by github-actions bot
Loading

What changes are included in this PR?

A test that reproduces an issue found by the fuzzer and a fix for it.

Are these changes tested?

  • A test
  • Comments clarifying somethings around formatting.h
  • Increasing the size of the local buffer used to format timestamps

The issue was introduced only recently (unreleased): #39272

felipecrv added 3 commits April 5, 2024 15:20
@felipecrv
formatting.h: Clarify meaning of cursor in FormatOneChar
e9b04fe
@felipecrv
formatting_util_test.cc: Add a test case that uses all the bytes in f…
87c2d49 
…ormatting buffer

With ASAN, this reproduces the issue.

    [ RUN      ] Formatting.Timestamp
    =================================================================
    ==4191383==ERROR: AddressSanitizer: stack-buffer-overflow on address 0x7fff1804c48f at pc 0x5608edffe39d bp 0x7fff1804c110 sp 0x7fff1804c108
    WRITE of size 1 at 0x7fff1804c48f thread T0
        #0 0x5608edffe39c in arrow::internal::detail::FormatOneChar(char, char**) /home/felipeo/code/arrow/cpp/src/arrow/util/formatting.h:132:67
        apache#1 0x5608ee035c00 in arrow::internal::detail::FormatYYYY_MM_DD(arrow_vendored::date::year_month_day, char**) /home/felipeo/code/arrow/cpp/src/arrow/util/formatting.h:351:5
        apache#2 0x5608ee05e8a0 in decltype(std::declval<arrow::StringAppender&>()(std::basic_string_view<char, std::char_traits<char> >{})) arrow::internal::StringFormatter<arrow::TimestampType, void>::operator()<std::chrono::duration<long, std::ratio<1l, 1000l> >, arrow::StringAppender&>(std::chrono::duration<long, std::ratio<1l, 1000l> >, long, arrow::StringAppender&) /home/felipeo/code/arrow/cpp/src/arrow/util/formatting.h
    :521:5
        apache#3 0x5608ee05d60f in decltype(std::declval<arrow::internal::StringFormatter<arrow::TimestampType, void>&>()(std::chrono::duration<long, std::ratio<1l, 1l> >{}, std::declval<long&>(), std::declval<arrow::StringAppender&>())) arrow::util::VisitDuration<arrow::internal::StringFormatter<arrow::TimestampType, void>&, long&, arrow::StringAppender&>(arrow::TimeUnit::type, arrow::internal::StringFormatter<arrow::Timestam
    pType, void>&, long&, arrow::StringAppender&) /home/felipeo/code/arrow/cpp/src/arrow/util/time.h:60:14
        apache#4 0x5608ee05d122 in decltype(std::declval<arrow::StringAppender&>()(std::basic_string_view<char, std::char_traits<char> >{})) arrow::internal::StringFormatter<arrow::TimestampType, void>::operator()<arrow::StringAppender&>(long, arrow::StringAppender&) /home/felipeo/code/arrow/cpp/src/arrow/util/formatting.h:527:12
        apache#5 0x5608edfeffb3 in void arrow::AssertFormatting<arrow::internal::StringFormatter<arrow::TimestampType, void>, long>(arrow::internal::StringFormatter<arrow::TimestampType, void>&, long, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&) /home/felipeo/code/arrow/cpp/src/arrow/util/formatting_util_test.cc:52:3
        apache#6 0x5608edfece95 in arrow::Formatting_Timestamp_Test::TestBody() /home/felipeo/code/arrow/cpp/src/arrow/util/formatting_util_test.cc:540:5
        apache#7 0x7fd95d7901de in void testing::internal::HandleExceptionsInMethodIfSupported<testing::Test, void>(testing::Test*, void (testing::Test::*)(), char const*) (/home/felipeo/code/arrow/cpp/ninja/debug/libarrow_testing.so.1600+0xd901de) (BuildId: dd9af0bafdb1786050262e8f6002568a9f08ecf6)
        apache#8 0x7fd95d784905 in testing::Test::Run() (/home/felipeo/code/arrow/cpp/ninja/debug/libarrow_testing.so.1600+0xd84905) (BuildId: dd9af0bafdb1786050262e8f6002568a9f08ecf6)
        apache#9 0x7fd95d784a84 in testing::TestInfo::Run() (/home/felipeo/code/arrow/cpp/ninja/debug/libarrow_testing.so.1600+0xd84a84) (BuildId: dd9af0bafdb1786050262e8f6002568a9f08ecf6)
        apache#10 0x7fd95d785038 in testing::TestSuite::Run() (/home/felipeo/code/arrow/cpp/ninja/debug/libarrow_testing.so.1600+0xd85038) (BuildId: dd9af0bafdb1786050262e8f6002568a9f08ecf6)
        apache#11 0x7fd95d78573e in testing::internal::UnitTestImpl::RunAllTests() (/home/felipeo/code/arrow/cpp/ninja/debug/libarrow_testing.so.1600+0xd8573e) (BuildId: dd9af0bafdb1786050262e8f6002568a9f08ecf6)
        apache#12 0x7fd95d7907a6 in bool testing::internal::HandleExceptionsInMethodIfSupported<testing::internal::UnitTestImpl, bool>(testing::internal::UnitTestImpl*, bool (testing::internal::UnitTestImpl::*)(), char const*) (/home/felipeo/code/arrow/cpp/ninja/debug/libarrow_testing.so.1600+0xd907a6) (BuildId: dd9af0bafdb1786050262e8f6002568a9f08ecf6)
        apache#13 0x7fd95d784b4b in testing::UnitTest::Run() (/home/felipeo/code/arrow/cpp/ninja/debug/libarrow_testing.so.1600+0xd84b4b) (BuildId: dd9af0bafdb1786050262e8f6002568a9f08ecf6)
        apache#14 0x5608ee54506d in RUN_ALL_TESTS() /usr/include/gtest/gtest.h:2490:46
        apache#15 0x5608ee544fb9 in main /home/felipeo/code/arrow/cpp/src/arrow/util/logging_test.cc:129:10
        apache#16 0x7fd93de29d8f in __libc_start_call_main csu/../sysdeps/nptl/libc_start_call_main.h:58:16
        apache#17 0x7fd93de29e3f in __libc_start_main csu/../csu/libc-start.c:392:3
        apache#18 0x5608ed7b4f64 in _start (/home/felipeo/code/arrow/cpp/ninja/debug/arrow-utility-test+0x1224f64) (BuildId: 81cfdc36b7a960a7249ecd5884beaa869140ab89)

    Address 0x7fff1804c48f is located in stack of thread T0 at offset 399 in frame
        #0 0x5608ee05dc9f in decltype(std::declval<arrow::StringAppender&>()(std::basic_string_view<char, std::char_traits<char> >{})) arrow::internal::StringFormatter<arrow::TimestampType, void>::operator()<std::chrono::duration<long, std::ratio<1l, 1000l> >, arrow::StringAppender&>(std::chrono::duration<long, std::ratio<1l, 1000l> >, long, arrow::StringAppender&) /home/felipeo/code/arrow/cpp/src/arrow/util/formatting.h
    :486
@felipecrv
formatting.h: Make sure space is allocated for the 'Z' in formatted t…
d3afed7 
…imestamp
@github-actions
Copy link

github-actions bot commented Apr 5, 2024

⚠️ GitHub issue #41044 has been automatically assigned in GitHub to PR creator.

kou
kou approved these changes Apr 5, 2024
View reviewed changes
Copy link
Member

@kou kou left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1

@github-actions github-actions bot added awaiting merge Awaiting merge and removed awaiting committer review Awaiting committer review labels Apr 5, 2024
@felipecrv felipecrv merged commit 6aa3321 into apache:main Apr 6, 2024
@felipecrv felipecrv removed the awaiting merge Awaiting merge label Apr 6, 2024
@felipecrv felipecrv mentioned this pull request Apr 6, 2024
Closed
@felipecrv felipecrv deleted the timestamp_formatting branch April 6, 2024 02:08
@pitrou
Copy link
Member

pitrou commented Apr 6, 2024

Thanks @felipecrv . The regression file must also be added to https://github.com/apache/arrow-testing/tree/master/data (which will test it in the ASAN UBSAN build).

@conbench-apache-arrow
Copy link

conbench-apache-arrow bot commented Apr 6, 2024

After merging your PR, Conbench analyzed the 7 benchmarking runs that have been run so far on merge-commit 6aa3321.

There was 1 benchmark result with an error:

There were no benchmark performance regressions. 🎉

The full Conbench report has more details. It also includes information about 1 possible false positive for unstable benchmarks that are known to sometimes produce them.

@AlenkaF
Copy link
Member

AlenkaF commented Apr 6, 2024

Thank you for fixing this @felipecrv!

felipecrv added a commit to felipecrv/arrow-testing that referenced this pull request Apr 9, 2024
@felipecrv
GH-41044: [C++] Add bug repro file that triggers bug in formatting.h
cfdbea9 
Bug was fixed here apache/arrow#41045
@felipecrv felipecrv mentioned this pull request Apr 9, 2024
Merged
@felipecrv
Copy link
Contributor Author

felipecrv commented Apr 9, 2024

Thanks @felipecrv . The regression file must also be added to https://github.com/apache/arrow-testing/tree/master/data (which will test it in the ASAN UBSAN build).

Done here apache/arrow-testing#100

@pitrou is arrow-testing bumped automatically or I should do it as well once the PR above is merged?

@pitrou
Copy link
Member

pitrou commented Apr 10, 2024

It's a regular git submodule, so you should open a PR here to bump it.

pitrou pushed a commit to apache/arrow-testing that referenced this pull request Apr 10, 2024
@felipecrv
GH-41044: [C++] Add bug repro file that triggers bug in formatting.h (#…
735ae71 
…100)

Bug was fixed here apache/arrow#41045
@felipecrv
Copy link
Contributor Author

felipecrv commented Apr 11, 2024

It's a regular git submodule, so you should open a PR here to bump it.

#41139

vibhatha pushed a commit to vibhatha/arrow that referenced this pull request May 25, 2024
@felipecrv @vibhatha
apacheGH-41044: [C++] formatting.h: Make sure space is allocated for …
11918b4 
…the 'Z' when formatting timestamps (apache#41045)

### What changes are included in this PR?

A test that reproduces an issue found by the fuzzer and a fix for it.

### Are these changes tested?

 - A test
 - Comments clarifying somethings around `formatting.h`
 - Increasing the size of the local buffer used to format timestamps

The issue was introduced only recently (unreleased): apache#39272
* GitHub Issue: apache#41044

Authored-by: Felipe Oliveira Carvalho <felipekde@gmail.com>
Signed-off-by: Felipe Oliveira Carvalho <felipekde@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@kou kou kou approved these changes

@zeroshade zeroshade zeroshade approved these changes

@pitrou pitrou Awaiting requested review from pitrou

@AlenkaF AlenkaF Awaiting requested review from AlenkaF

@jorisvandenbossche jorisvandenbossche Awaiting requested review from jorisvandenbossche

Assignees

No one assigned

Labels

Component: C++

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@felipecrv @pitrou @AlenkaF @kou @zeroshade