GH-35095: [C++] Prevent write after close in arrow::ipc::IpcFormatWriter #37783

chrisjordansquire · 2023-09-18T20:36:30Z

This addresses GH-35095 by adding a flag to IpcFormatWriter to track when a writer has been closed, and check this flag before writes.

Rationale for this change

This addresses #35095 , preventing stream and file IPC writers from writing record batches once the IPC writer has been closed.

What changes are included in this PR?

Adding a flag so that an IpcFormatWriter to track when it's been closed, a check before writes in IpcFormatWriter, and two tests to confirm it works as expected.

Are these changes tested?

Yes, the changes are tested. The two tests were added, and the C++ test suite ran. No unexpected failures appeared.

Are there any user-facing changes?

Other than newly returning an invalid status when writing after close, no, there should not be any user-facing changes.

Closes: [C++] RecordBatchWriter::Close does not prevent future writes #35095

github-actions · 2023-09-18T20:37:03Z

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

kou · 2023-09-19T00:37:21Z

cpp/src/arrow/ipc/read_write_test.cc

+
+    // Write after close raises status
+    auto foo = writer_helper.WriteBatch(batch_ints);
+    // ASSERT_RAISES(Invalid, writer_helper.WriteBatch(batch_ints));


Why is this assertion commented out?

Whoops. An extremely embarrassing mistake. Thank you for catching it. It's been fixed.

kou · 2023-09-19T07:21:13Z

cpp/src/arrow/ipc/writer.cc

@@ -1070,6 +1070,9 @@ class ARROW_EXPORT IpcFormatWriter : public RecordBatchWriter {
  Status WriteRecordBatch(
      const RecordBatch& batch,
      const std::shared_ptr<const KeyValueMetadata>& custom_metadata) override {
+    if (closed_) {
+      return Status::Invalid("Destination already closed");
+    }


Do we need similar check in WriteTable too?

Good question.
It appears that WriteTable ultimately calls WriteRecordBatch. This appears to be documented as well, in that the documentation for RecordBatchWriter::WriteTable is that a a table is written by creating a sequence of record batches. So testing WriteTable doesn't seem to be necessary.

Thanks. I see.

kou

+1

cpp/src/arrow/ipc/read_write_test.cc

kou · 2023-09-19T23:46:53Z

cpp/src/arrow/ipc/writer.cc

@@ -1070,6 +1070,9 @@ class ARROW_EXPORT IpcFormatWriter : public RecordBatchWriter {
  Status WriteRecordBatch(
      const RecordBatch& batch,
      const std::shared_ptr<const KeyValueMetadata>& custom_metadata) override {
+    if (closed_) {
+      return Status::Invalid("Destination already closed");
+    }


Thanks. I see.

github-actions · 2023-09-19T23:48:47Z

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

kou · 2023-09-20T02:20:14Z

Could you check the "C++ / AMD64 Ubuntu 22.04 C++ ASAN UBSAN" failure?

https://github.com/apache/arrow/actions/runs/6242111241/job/16945464958?pr=37783#step:6:3569

==15144==ERROR: LeakSanitizer: detected memory leaks

Indirect leak of 512 byte(s) in 1 object(s) allocated from:
...

chrisjordansquire · 2023-09-20T13:32:25Z

Yep, taking a look. Rebasing on main first, in case it was fixed elsewhere.

This addresses apacheGH-35095 by adding a flag to IpcFormatWriter to track when a writer has been closed, and check this flag before writes.

kou · 2023-09-21T03:24:58Z

Oh, the failure has gone.
It'll not be related to this change.
I'll merge this.

conbench-apache-arrow · 2023-09-21T14:04:46Z

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

There were no benchmark performance regressions. 🎉

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

…matWriter (apache#37783) This addresses apacheGH-35095 by adding a flag to IpcFormatWriter to track when a writer has been closed, and check this flag before writes. ### Rationale for this change This addresses apache#35095 , preventing stream and file IPC writers from writing record batches once the IPC writer has been closed. ### What changes are included in this PR? Adding a flag so that an IpcFormatWriter to track when it's been closed, a check before writes in IpcFormatWriter, and two tests to confirm it works as expected. ### Are these changes tested? Yes, the changes are tested. The two tests were added, and the C++ test suite ran. No unexpected failures appeared. ### Are there any user-facing changes? Other than newly returning an invalid status when writing after close, no, there should not be any user-facing changes. * Closes: apache#35095 Lead-authored-by: Chris Jordan-Squire <[email protected]> Co-authored-by: Sutou Kouhei <[email protected]> Signed-off-by: Sutou Kouhei <[email protected]>

github-actions bot added Component: C++ awaiting review Awaiting review labels Sep 18, 2023

chrisjordansquire mentioned this pull request Sep 18, 2023

[C++] RecordBatchWriter::Close does not prevent future writes #35095

Closed

kou reviewed Sep 19, 2023

View reviewed changes

github-actions bot added awaiting changes Awaiting changes awaiting change review Awaiting change review and removed awaiting review Awaiting review awaiting changes Awaiting changes labels Sep 19, 2023

kou reviewed Sep 19, 2023

View reviewed changes

github-actions bot added awaiting changes Awaiting changes and removed awaiting change review Awaiting change review labels Sep 19, 2023

kou changed the title ~~GH-35095: [C++] Prevent write after close~~ GH-35095: [C++] Prevent write after close in arrow::ipc::IpcFormatWriter Sep 19, 2023

kou approved these changes Sep 19, 2023

View reviewed changes

github-actions bot added awaiting change review Awaiting change review and removed awaiting changes Awaiting changes labels Sep 19, 2023

chrisjordansquire force-pushed the GH-35095-write-record-batch-after-close-is-error branch from c1754fe to 85f3916 Compare September 20, 2023 13:31

chrisjordansquire and others added 3 commits September 20, 2023 16:13

apacheGH-35095: Prevent write after close

5f967c8

This addresses apacheGH-35095 by adding a flag to IpcFormatWriter to track when a writer has been closed, and check this flag before writes.

Uncomment mistakely commented out line

ee470e9

Use auto

ae6e0fa

chrisjordansquire force-pushed the GH-35095-write-record-batch-after-close-is-error branch from 85f3916 to ae6e0fa Compare September 20, 2023 20:13

kou merged commit 6cd34f3 into apache:main Sep 21, 2023
30 of 33 checks passed

kou removed the awaiting change review Awaiting change review label Sep 21, 2023

chrisjordansquire deleted the GH-35095-write-record-batch-after-close-is-error branch September 21, 2023 14:51

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

GH-35095: [C++] Prevent write after close in arrow::ipc::IpcFormatWriter #37783

GH-35095: [C++] Prevent write after close in arrow::ipc::IpcFormatWriter #37783

chrisjordansquire commented Sep 18, 2023 •

edited by github-actions bot

Loading

github-actions bot commented Sep 18, 2023

kou Sep 19, 2023

chrisjordansquire Sep 19, 2023

kou Sep 19, 2023

chrisjordansquire Sep 19, 2023

kou Sep 19, 2023

kou left a comment

kou Sep 19, 2023

github-actions bot commented Sep 19, 2023

kou commented Sep 20, 2023

chrisjordansquire commented Sep 20, 2023

kou commented Sep 21, 2023

conbench-apache-arrow bot commented Sep 21, 2023

GH-35095: [C++] Prevent write after close in arrow::ipc::IpcFormatWriter #37783

GH-35095: [C++] Prevent write after close in arrow::ipc::IpcFormatWriter #37783

Conversation

chrisjordansquire commented Sep 18, 2023 • edited by github-actions bot Loading

Rationale for this change

What changes are included in this PR?

Are these changes tested?

Are there any user-facing changes?

github-actions bot commented Sep 18, 2023

kou Sep 19, 2023

Choose a reason for hiding this comment

chrisjordansquire Sep 19, 2023

Choose a reason for hiding this comment

kou Sep 19, 2023

Choose a reason for hiding this comment

chrisjordansquire Sep 19, 2023

Choose a reason for hiding this comment

kou Sep 19, 2023

Choose a reason for hiding this comment

kou left a comment

Choose a reason for hiding this comment

kou Sep 19, 2023

Choose a reason for hiding this comment

github-actions bot commented Sep 19, 2023

kou commented Sep 20, 2023

chrisjordansquire commented Sep 20, 2023

kou commented Sep 21, 2023

conbench-apache-arrow bot commented Sep 21, 2023

chrisjordansquire commented Sep 18, 2023 •

edited by github-actions bot

Loading