You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by "pitrou (via GitHub)" <gi...@apache.org> on 2025/11/12 12:24:09 UTC
[PR] GH-48105: [C++][Parquet][IPC] Cap allocated memory when fuzzing [arrow]
pitrou opened a new pull request, #48108:
URL: https://github.com/apache/arrow/pull/48108
Thanks for opening a pull request!
If this is your first pull request you can find detailed information on how to contribute here:
* [New Contributor's Guide](https://arrow.apache.org/docs/dev/developers/guide/step_by_step/pr_lifecycle.html#reviews-and-merge-of-the-pull-request)
* [Contributing Overview](https://arrow.apache.org/docs/dev/developers/overview.html)
Please remove this line and the above text before creating your pull request.
### Rationale for this change
### What changes are included in this PR?
### Are these changes tested?
### Are there any user-facing changes?
**This PR includes breaking changes to public APIs.** (If there are any breaking changes to public APIs, please explain which changes are breaking. If not, you can remove this.)
**This PR contains a "Critical Fix".** (If the changes fix either (a) a security vulnerability, (b) a bug that caused incorrect or invalid data to be produced, or (c) a bug that causes a crash (even when the API contract is upheld), please provide explanation. If not, you can remove this.)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-48105: [C++][Parquet][IPC] Cap allocated memory when fuzzing [arrow]
Posted by "zanmato1984 (via GitHub)" <gi...@apache.org>.
zanmato1984 commented on code in PR #48108:
URL: https://github.com/apache/arrow/pull/48108#discussion_r2522582980
##########
cpp/src/arrow/memory_pool.h:
##########
@@ -245,6 +246,64 @@ class ARROW_EXPORT ProxyMemoryPool : public MemoryPool {
std::unique_ptr<ProxyMemoryPoolImpl> impl_;
};
+/// EXPERIMENTAL MemoryPool wrapper with an upper limit
+class ARROW_EXPORT CappedMemoryPool : public MemoryPool {
+ public:
+ CappedMemoryPool(MemoryPool* wrapped_pool, int64_t bytes_allocated_limit)
+ : wrapped_(wrapped_pool), bytes_allocated_limit_(bytes_allocated_limit) {}
+
+ using MemoryPool::Allocate;
+ using MemoryPool::Reallocate;
+
+ Status Allocate(int64_t size, int64_t alignment, uint8_t** out) override {
+ const auto attempted = size + wrapped_->bytes_allocated();
+ if (ARROW_PREDICT_FALSE(attempted > bytes_allocated_limit_)) {
+ return OutOfMemory(attempted);
+ }
Review Comment:
Thanks for the information. Then I think this attempt is well justified even if it's still racy. I will later review the rest of the pr.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-48105: [C++][Parquet][IPC] Cap allocated memory when fuzzing [arrow]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #48108:
URL: https://github.com/apache/arrow/pull/48108#issuecomment-3521721355
Revision: c63e380aa5b35dc054c4d99320373561e112e176
Submitted crossbow builds: [ursacomputing/crossbow @ actions-a5b0deeb96](https://github.com/ursacomputing/crossbow/branches/all?query=actions-a5b0deeb96)
|Task|Status|
|----|------|
|example-cpp-minimal-build-static|[](https://github.com/ursacomputing/crossbow/actions/runs/19297625081/job/55183444479)|
|example-cpp-minimal-build-static-system-dependency|[](https://github.com/ursacomputing/crossbow/actions/runs/19297625702/job/55183446173)|
|example-cpp-tutorial|[](https://github.com/ursacomputing/crossbow/actions/runs/19297624815/job/55183443438)|
|test-build-cpp-fuzz|[](https://github.com/ursacomputing/crossbow/actions/runs/19297625691/job/55183446178)|
|test-conda-cpp|[](https://github.com/ursacomputing/crossbow/actions/runs/19297624838/job/55183443649)|
|test-conda-cpp-valgrind|[](https://github.com/ursacomputing/crossbow/actions/runs/19297626210/job/55183447896)|
|test-cuda-cpp-ubuntu-22.04-cuda-11.7.1|[](https://github.com/ursacomputing/crossbow/actions/runs/19297625355/job/55183445260)|
|test-debian-12-cpp-amd64|[](https://github.com/ursacomputing/crossbow/actions/runs/19297625219/job/55183445286)|
|test-debian-12-cpp-i386|[](https://github.com/ursacomputing/crossbow/actions/runs/19297624868/job/55183443714)|
|test-fedora-42-cpp|[](https://github.com/ursacomputing/crossbow/actions/runs/19297624678/job/55183443092)|
|test-ubuntu-22.04-cpp|[](https://github.com/ursacomputing/crossbow/actions/runs/19297625563/job/55183445804)|
|test-ubuntu-22.04-cpp-20|[](https://github.com/ursacomputing/crossbow/actions/runs/19297625396/job/55183445309)|
|test-ubuntu-22.04-cpp-bundled|[](https://github.com/ursacomputing/crossbow/actions/runs/19297625582/job/55183445814)|
|test-ubuntu-22.04-cpp-emscripten|[](https://github.com/ursacomputing/crossbow/actions/runs/19297625088/job/55183444435)|
|test-ubuntu-22.04-cpp-no-threading|[](https://github.com/ursacomputing/crossbow/actions/runs/19297624751/job/55183443353)|
|test-ubuntu-24.04-cpp|[](https://github.com/ursacomputing/crossbow/actions/runs/19297625510/job/55183445720)|
|test-ubuntu-24.04-cpp-bundled-offline|[](https://github.com/ursacomputing/crossbow/actions/runs/19297625583/job/55183445871)|
|test-ubuntu-24.04-cpp-gcc-13-bundled|[](https://github.com/ursacomputing/crossbow/actions/runs/19297624680/job/55183443298)|
|test-ubuntu-24.04-cpp-gcc-14|[](https://github.com/ursacomputing/crossbow/actions/runs/19297625057/job/55183444347)|
|test-ubuntu-24.04-cpp-minimal-with-formats|[](https://github.com/ursacomputing/crossbow/actions/runs/19297625871/job/55183446928)|
|test-ubuntu-24.04-cpp-thread-sanitizer|[](https://github.com/ursacomputing/crossbow/actions/runs/19297625036/job/55183444220)|
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-48105: [C++][Parquet][IPC] Cap allocated memory when fuzzing [arrow]
Posted by "pitrou (via GitHub)" <gi...@apache.org>.
pitrou commented on PR #48108:
URL: https://github.com/apache/arrow/pull/48108#issuecomment-3522948458
@github-actions crossbow submit -g cpp
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-48105: [C++][Parquet][IPC] Cap allocated memory when fuzzing [arrow]
Posted by "conbench-apache-arrow[bot] (via GitHub)" <gi...@apache.org>.
conbench-apache-arrow[bot] commented on PR #48108:
URL: https://github.com/apache/arrow/pull/48108#issuecomment-3536811218
After merging your PR, Conbench analyzed the 4 benchmarking runs that have been run so far on merge-commit 7c3d4867e40dd0100542247a61cb83520369b2d4.
There were no benchmark performance regressions. 🎉
The [full Conbench report](https://github.com/apache/arrow/runs/55492871917) has more details. It also includes information about 2 possible false positives for unstable benchmarks that are known to sometimes produce them.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-48105: [C++][Parquet][IPC] Cap allocated memory when fuzzing [arrow]
Posted by "pitrou (via GitHub)" <gi...@apache.org>.
pitrou commented on PR #48108:
URL: https://github.com/apache/arrow/pull/48108#issuecomment-3536233455
@github-actions crossbow submit -g cpp
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-48105: [C++][Parquet][IPC] Cap allocated memory when fuzzing [arrow]
Posted by "wgtmac (via GitHub)" <gi...@apache.org>.
wgtmac commented on code in PR #48108:
URL: https://github.com/apache/arrow/pull/48108#discussion_r2521233595
##########
cpp/src/arrow/memory_pool.h:
##########
@@ -245,6 +246,64 @@ class ARROW_EXPORT ProxyMemoryPool : public MemoryPool {
std::unique_ptr<ProxyMemoryPoolImpl> impl_;
};
+/// EXPERIMENTAL MemoryPool wrapper with an upper limit
+class ARROW_EXPORT CappedMemoryPool : public MemoryPool {
+ public:
+ CappedMemoryPool(MemoryPool* wrapped_pool, int64_t bytes_allocated_limit)
+ : wrapped_(wrapped_pool), bytes_allocated_limit_(bytes_allocated_limit) {}
+
+ using MemoryPool::Allocate;
+ using MemoryPool::Reallocate;
+
+ Status Allocate(int64_t size, int64_t alignment, uint8_t** out) override {
+ const auto attempted = size + wrapped_->bytes_allocated();
+ if (ARROW_PREDICT_FALSE(attempted > bytes_allocated_limit_)) {
+ return OutOfMemory(attempted);
Review Comment:
I like the idea to cap the memory allocation. We have seen memory issues while reading large Parquet files in a multi-tenant environment. At least it can help protect the stability.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-48105: [C++][Parquet][IPC] Cap allocated memory when fuzzing [arrow]
Posted by "pitrou (via GitHub)" <gi...@apache.org>.
pitrou commented on code in PR #48108:
URL: https://github.com/apache/arrow/pull/48108#discussion_r2522542457
##########
cpp/src/arrow/memory_pool.h:
##########
@@ -245,6 +246,64 @@ class ARROW_EXPORT ProxyMemoryPool : public MemoryPool {
std::unique_ptr<ProxyMemoryPoolImpl> impl_;
};
+/// EXPERIMENTAL MemoryPool wrapper with an upper limit
+class ARROW_EXPORT CappedMemoryPool : public MemoryPool {
+ public:
+ CappedMemoryPool(MemoryPool* wrapped_pool, int64_t bytes_allocated_limit)
+ : wrapped_(wrapped_pool), bytes_allocated_limit_(bytes_allocated_limit) {}
+
+ using MemoryPool::Allocate;
+ using MemoryPool::Reallocate;
+
+ Status Allocate(int64_t size, int64_t alignment, uint8_t** out) override {
+ const auto attempted = size + wrapped_->bytes_allocated();
+ if (ARROW_PREDICT_FALSE(attempted > bytes_allocated_limit_)) {
+ return OutOfMemory(attempted);
+ }
Review Comment:
Fuzzing itself is not multi-threaded, but our fuzz targets use default settings for Parquet reading, so there is probably some multi-threading going on. That said, the likelihood of a race condition in this particular place is rather low IMHO.
We have a couple fuzz files that can trigger OOM on the fuzzer, and this PR successfully prevents OOM on these files (at least when tested locally).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-48105: [C++][Parquet][IPC] Cap allocated memory when fuzzing [arrow]
Posted by "pitrou (via GitHub)" <gi...@apache.org>.
pitrou commented on code in PR #48108:
URL: https://github.com/apache/arrow/pull/48108#discussion_r2526614764
##########
cpp/src/arrow/util/fuzz_internal.cc:
##########
@@ -0,0 +1,39 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+#include "arrow/util/fuzz_internal.h"
+
+#include "arrow/memory_pool.h"
+#include "arrow/status.h"
+#include "arrow/util/logging_internal.h"
+
+namespace arrow::internal {
+
+MemoryPool* fuzzing_memory_pool() {
+ static auto pool = std::make_shared<::arrow::CappedMemoryPool>(
+ ::arrow::default_memory_pool(), /*bytes_allocated_limit=*/kFuzzingMemoryLimit);
Review Comment:
I don't think that's useful for now. On fuzzing builds, we only enable the system allocator, as it's the most likely to be correctly instrumented by ASAN.
(it is possible to build mimalloc with ASAN tracking enabled, but I'm not sure what that would bring us over simply using the system allocator)
##########
cpp/src/arrow/util/fuzz_internal.h:
##########
@@ -0,0 +1,37 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+#pragma once
+
+#include <cstdint>
+
+#include "arrow/type_fwd.h"
+#include "arrow/util/macros.h"
+
+namespace arrow::internal {
+
+// The default rss_limit_mb on OSS-Fuzz is 2560 MB and we want to fail allocations
+// before that limit is reached, otherwise the fuzz target gets killed (GH-48105).
+constexpr int64_t kFuzzingMemoryLimit = 2200LL * 1000 * 1000;
+
+/// Return a memory pool that will not allocate more than kFuzzingMemoryLimit bytes.
+ARROW_EXPORT MemoryPool* fuzzing_memory_pool();
+
+// Optionally log the outcome of fuzzing an input
+ARROW_EXPORT void NoteFuzzStatus(const Status&, const uint8_t* data, int64_t size);
Review Comment:
Probably, yes :-)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-48105: [C++][Parquet][IPC] Cap allocated memory when fuzzing [arrow]
Posted by "zanmato1984 (via GitHub)" <gi...@apache.org>.
zanmato1984 commented on code in PR #48108:
URL: https://github.com/apache/arrow/pull/48108#discussion_r2524866619
##########
cpp/src/arrow/util/fuzz_internal.h:
##########
@@ -0,0 +1,37 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+#pragma once
+
+#include <cstdint>
+
+#include "arrow/type_fwd.h"
+#include "arrow/util/macros.h"
+
+namespace arrow::internal {
+
+// The default rss_limit_mb on OSS-Fuzz is 2560 MB and we want to fail allocations
+// before that limit is reached, otherwise the fuzz target gets killed (GH-48105).
+constexpr int64_t kFuzzingMemoryLimit = 2200LL * 1000 * 1000;
+
+/// Return a memory pool that will not allocate more than kFuzzingMemoryLimit bytes.
+ARROW_EXPORT MemoryPool* fuzzing_memory_pool();
+
+// Optionally log the outcome of fuzzing an input
+ARROW_EXPORT void NoteFuzzStatus(const Status&, const uint8_t* data, int64_t size);
Review Comment:
Do you think a name like `LogFuzzStatus` can be more straightforward?
##########
cpp/src/arrow/util/fuzz_internal.cc:
##########
@@ -0,0 +1,39 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+#include "arrow/util/fuzz_internal.h"
+
+#include "arrow/memory_pool.h"
+#include "arrow/status.h"
+#include "arrow/util/logging_internal.h"
+
+namespace arrow::internal {
+
+MemoryPool* fuzzing_memory_pool() {
+ static auto pool = std::make_shared<::arrow::CappedMemoryPool>(
+ ::arrow::default_memory_pool(), /*bytes_allocated_limit=*/kFuzzingMemoryLimit);
Review Comment:
Do we want to expose the wrapped memory pool as a parameter and default it to `::arrow::default_memory_pool()`?
##########
cpp/src/arrow/memory_pool.h:
##########
@@ -245,6 +246,73 @@ class ARROW_EXPORT ProxyMemoryPool : public MemoryPool {
std::unique_ptr<ProxyMemoryPoolImpl> impl_;
};
+/// EXPERIMENTAL MemoryPool wrapper with an upper limit
+///
+/// Checking for limits is not done in a fully thread-safe way, therefore
+/// multi-threaded allocations might be able to go successfully above the
+/// configured limit.
+class ARROW_EXPORT CappedMemoryPool : public MemoryPool {
+ public:
+ CappedMemoryPool(MemoryPool* wrapped_pool, int64_t bytes_allocated_limit)
+ : wrapped_(wrapped_pool), bytes_allocated_limit_(bytes_allocated_limit) {}
+
+ using MemoryPool::Allocate;
+ using MemoryPool::Reallocate;
+
+ Status Allocate(int64_t size, int64_t alignment, uint8_t** out) override {
+ // XXX Another thread may allocate memory between the limit check and
+ // the `Allocate` call. It is possible for the two allocations to be successful
+ // while going above the limit.
+ // Solving this issue would require refactoring the `MemoryPool` implementation
+ // to delegate the limit check to `MemoryPoolStats`.
+ const auto attempted = size + wrapped_->bytes_allocated();
+ if (ARROW_PREDICT_FALSE(attempted > bytes_allocated_limit_)) {
+ return OutOfMemory(attempted);
+ }
+ return wrapped_->Allocate(size, alignment, out);
+ }
+
+ Status Reallocate(int64_t old_size, int64_t new_size, int64_t alignment,
+ uint8_t** ptr) override {
+ const auto attempted = new_size - old_size + wrapped_->bytes_allocated();
+ if (ARROW_PREDICT_FALSE(attempted > bytes_allocated_limit_)) {
+ return OutOfMemory(attempted);
+ }
+ return wrapped_->Reallocate(old_size, new_size, alignment, ptr);
+ }
+
+ void Free(uint8_t* buffer, int64_t size, int64_t alignment) override {
+ return wrapped_->Free(buffer, size, alignment);
+ }
+
+ void ReleaseUnused() override { wrapped_->ReleaseUnused(); }
+
+ void PrintStats() override { wrapped_->PrintStats(); }
+
+ int64_t bytes_allocated() const override { return wrapped_->bytes_allocated(); }
+
+ int64_t max_memory() const override { return wrapped_->max_memory(); }
+
+ int64_t total_bytes_allocated() const override {
+ return wrapped_->total_bytes_allocated();
+ }
+
+ int64_t num_allocations() const override { return wrapped_->num_allocations(); }
+
+ std::string backend_name() const override { return wrapped_->backend_name(); }
+
+ private:
+ Status OutOfMemory(int64_t value) {
+ return Status::OutOfMemory(
+ "MemoryPool bytes_allocated cap exceeded: "
+ "limit=",
+ bytes_allocated_limit_, ", attempted=", value);
+ }
+
+ MemoryPool* wrapped_;
+ int64_t bytes_allocated_limit_;
Review Comment:
Maybe this can be `const`?
##########
cpp/src/arrow/util/fuzz_internal.cc:
##########
@@ -0,0 +1,39 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+#include "arrow/util/fuzz_internal.h"
+
+#include "arrow/memory_pool.h"
+#include "arrow/status.h"
+#include "arrow/util/logging_internal.h"
+
+namespace arrow::internal {
+
+MemoryPool* fuzzing_memory_pool() {
+ static auto pool = std::make_shared<::arrow::CappedMemoryPool>(
+ ::arrow::default_memory_pool(), /*bytes_allocated_limit=*/kFuzzingMemoryLimit);
+ return pool.get();
+}
+
+void NoteFuzzStatus(const Status& st, const uint8_t* data, int64_t size) {
+ if (st.IsOutOfMemory()) {
+ ARROW_LOG(WARNING) << "Fuzzing input with size=" << size
Review Comment:
If this is for general purpose of status logging, maybe we can simply do:
```
if (!st.ok()) {
ARROW_LOG(WARNING) << "Fuzzing input with size=" << size << " hit: " << st.ToString();
}
```
Because the information of OOM (or any other non-OK status) is already encoded within the `st`.
##########
cpp/src/arrow/memory_pool.h:
##########
@@ -245,6 +246,73 @@ class ARROW_EXPORT ProxyMemoryPool : public MemoryPool {
std::unique_ptr<ProxyMemoryPoolImpl> impl_;
};
+/// EXPERIMENTAL MemoryPool wrapper with an upper limit
+///
+/// Checking for limits is not done in a fully thread-safe way, therefore
+/// multi-threaded allocations might be able to go successfully above the
+/// configured limit.
+class ARROW_EXPORT CappedMemoryPool : public MemoryPool {
+ public:
+ CappedMemoryPool(MemoryPool* wrapped_pool, int64_t bytes_allocated_limit)
+ : wrapped_(wrapped_pool), bytes_allocated_limit_(bytes_allocated_limit) {}
+
+ using MemoryPool::Allocate;
+ using MemoryPool::Reallocate;
Review Comment:
Are these necessary?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-48105: [C++][Parquet][IPC] Cap allocated memory when fuzzing [arrow]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #48108:
URL: https://github.com/apache/arrow/pull/48108#issuecomment-3522959666
Revision: ff9469085fbd1d086e4e31a1807d48f26e0dbf37
Submitted crossbow builds: [ursacomputing/crossbow @ actions-8fa1900da3](https://github.com/ursacomputing/crossbow/branches/all?query=actions-8fa1900da3)
|Task|Status|
|----|------|
|example-cpp-minimal-build-static|[](https://github.com/ursacomputing/crossbow/actions/runs/19305320470/job/55211098663)|
|example-cpp-minimal-build-static-system-dependency|[](https://github.com/ursacomputing/crossbow/actions/runs/19305319876/job/55211094180)|
|example-cpp-tutorial|[](https://github.com/ursacomputing/crossbow/actions/runs/19305320134/job/55211095184)|
|test-build-cpp-fuzz|[](https://github.com/ursacomputing/crossbow/actions/runs/19305320405/job/55211097152)|
|test-conda-cpp|[](https://github.com/ursacomputing/crossbow/actions/runs/19305319668/job/55211093551)|
|test-conda-cpp-valgrind|[](https://github.com/ursacomputing/crossbow/actions/runs/19305319812/job/55211096986)|
|test-cuda-cpp-ubuntu-22.04-cuda-11.7.1|[](https://github.com/ursacomputing/crossbow/actions/runs/19305320054/job/55211094693)|
|test-debian-12-cpp-amd64|[](https://github.com/ursacomputing/crossbow/actions/runs/19305320511/job/55211098025)|
|test-debian-12-cpp-i386|[](https://github.com/ursacomputing/crossbow/actions/runs/19305320718/job/55211098001)|
|test-fedora-42-cpp|[](https://github.com/ursacomputing/crossbow/actions/runs/19305319709/job/55211093463)|
|test-ubuntu-22.04-cpp|[](https://github.com/ursacomputing/crossbow/actions/runs/19305320540/job/55211098676)|
|test-ubuntu-22.04-cpp-20|[](https://github.com/ursacomputing/crossbow/actions/runs/19305320587/job/55211102249)|
|test-ubuntu-22.04-cpp-bundled|[](https://github.com/ursacomputing/crossbow/actions/runs/19305320565/job/55211098363)|
|test-ubuntu-22.04-cpp-emscripten|[](https://github.com/ursacomputing/crossbow/actions/runs/19305320075/job/55211094804)|
|test-ubuntu-22.04-cpp-no-threading|[](https://github.com/ursacomputing/crossbow/actions/runs/19305320049/job/55211096076)|
|test-ubuntu-24.04-cpp|[](https://github.com/ursacomputing/crossbow/actions/runs/19305320284/job/55211096651)|
|test-ubuntu-24.04-cpp-bundled-offline|[](https://github.com/ursacomputing/crossbow/actions/runs/19305320576/job/55211097756)|
|test-ubuntu-24.04-cpp-gcc-13-bundled|[](https://github.com/ursacomputing/crossbow/actions/runs/19305320953/job/55211098554)|
|test-ubuntu-24.04-cpp-gcc-14|[](https://github.com/ursacomputing/crossbow/actions/runs/19305321637/job/55211111784)|
|test-ubuntu-24.04-cpp-minimal-with-formats|[](https://github.com/ursacomputing/crossbow/actions/runs/19305320143/job/55211095073)|
|test-ubuntu-24.04-cpp-thread-sanitizer|[](https://github.com/ursacomputing/crossbow/actions/runs/19305319697/job/55211093410)|
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-48105: [C++][Parquet][IPC] Cap allocated memory when fuzzing [arrow]
Posted by "pitrou (via GitHub)" <gi...@apache.org>.
pitrou commented on PR #48108:
URL: https://github.com/apache/arrow/pull/48108#issuecomment-3521711289
@github-actions crossbow submit -g cpp
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-48105: [C++][Parquet][IPC] Cap allocated memory when fuzzing [arrow]
Posted by "zanmato1984 (via GitHub)" <gi...@apache.org>.
zanmato1984 commented on code in PR #48108:
URL: https://github.com/apache/arrow/pull/48108#discussion_r2528761987
##########
cpp/src/arrow/memory_pool.h:
##########
@@ -245,6 +246,73 @@ class ARROW_EXPORT ProxyMemoryPool : public MemoryPool {
std::unique_ptr<ProxyMemoryPoolImpl> impl_;
};
+/// EXPERIMENTAL MemoryPool wrapper with an upper limit
+///
+/// Checking for limits is not done in a fully thread-safe way, therefore
+/// multi-threaded allocations might be able to go successfully above the
+/// configured limit.
+class ARROW_EXPORT CappedMemoryPool : public MemoryPool {
+ public:
+ CappedMemoryPool(MemoryPool* wrapped_pool, int64_t bytes_allocated_limit)
+ : wrapped_(wrapped_pool), bytes_allocated_limit_(bytes_allocated_limit) {}
+
+ using MemoryPool::Allocate;
+ using MemoryPool::Reallocate;
Review Comment:
I think they are necessary for `ProxyMemoryPool` only because there is a test `ProxyMemoryPool.Logging` invoking the other overloads directly on a `ProxyMemoryPool` type (i.e., non-OO-ly). But OK, not a big deal if we keep them.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-48105: [C++][Parquet][IPC] Cap allocated memory when fuzzing [arrow]
Posted by "pitrou (via GitHub)" <gi...@apache.org>.
pitrou commented on code in PR #48108:
URL: https://github.com/apache/arrow/pull/48108#discussion_r2526618822
##########
cpp/src/arrow/util/fuzz_internal.cc:
##########
@@ -0,0 +1,39 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+#include "arrow/util/fuzz_internal.h"
+
+#include "arrow/memory_pool.h"
+#include "arrow/status.h"
+#include "arrow/util/logging_internal.h"
+
+namespace arrow::internal {
+
+MemoryPool* fuzzing_memory_pool() {
+ static auto pool = std::make_shared<::arrow::CappedMemoryPool>(
+ ::arrow::default_memory_pool(), /*bytes_allocated_limit=*/kFuzzingMemoryLimit);
+ return pool.get();
+}
+
+void NoteFuzzStatus(const Status& st, const uint8_t* data, int64_t size) {
+ if (st.IsOutOfMemory()) {
+ ARROW_LOG(WARNING) << "Fuzzing input with size=" << size
Review Comment:
Most fuzz inputs will be invalid and therefore produce error outcomes. This would mean a hundred error logs per second. I'm only logging the out-of-memory conditions because they're supposed to be much rarer.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-48105: [C++][Parquet][IPC] Cap allocated memory when fuzzing [arrow]
Posted by "zanmato1984 (via GitHub)" <gi...@apache.org>.
zanmato1984 commented on code in PR #48108:
URL: https://github.com/apache/arrow/pull/48108#discussion_r2522512656
##########
cpp/src/arrow/memory_pool.h:
##########
@@ -245,6 +246,64 @@ class ARROW_EXPORT ProxyMemoryPool : public MemoryPool {
std::unique_ptr<ProxyMemoryPoolImpl> impl_;
};
+/// EXPERIMENTAL MemoryPool wrapper with an upper limit
+class ARROW_EXPORT CappedMemoryPool : public MemoryPool {
+ public:
+ CappedMemoryPool(MemoryPool* wrapped_pool, int64_t bytes_allocated_limit)
+ : wrapped_(wrapped_pool), bytes_allocated_limit_(bytes_allocated_limit) {}
+
+ using MemoryPool::Allocate;
+ using MemoryPool::Reallocate;
+
+ Status Allocate(int64_t size, int64_t alignment, uint8_t** out) override {
+ const auto attempted = size + wrapped_->bytes_allocated();
+ if (ARROW_PREDICT_FALSE(attempted > bytes_allocated_limit_)) {
+ return OutOfMemory(attempted);
+ }
Review Comment:
Do we know if fuzz-oss tests use multi-threading and, if yes, how effective this new memory pool would avoid the tests being brutally killed because of OOM?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-48105: [C++][Parquet][IPC] Cap allocated memory when fuzzing [arrow]
Posted by "zanmato1984 (via GitHub)" <gi...@apache.org>.
zanmato1984 commented on code in PR #48108:
URL: https://github.com/apache/arrow/pull/48108#discussion_r2528763122
##########
cpp/src/arrow/util/fuzz_internal.cc:
##########
@@ -0,0 +1,39 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+#include "arrow/util/fuzz_internal.h"
+
+#include "arrow/memory_pool.h"
+#include "arrow/status.h"
+#include "arrow/util/logging_internal.h"
+
+namespace arrow::internal {
+
+MemoryPool* fuzzing_memory_pool() {
+ static auto pool = std::make_shared<::arrow::CappedMemoryPool>(
+ ::arrow::default_memory_pool(), /*bytes_allocated_limit=*/kFuzzingMemoryLimit);
Review Comment:
OK. Thanks.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-48105: [C++][Parquet][IPC] Cap allocated memory when fuzzing [arrow]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #48108:
URL: https://github.com/apache/arrow/pull/48108#issuecomment-3536235300
Revision: 0b0f8f928b07c6f34fe53e58524d17b1a1934929
Submitted crossbow builds: [ursacomputing/crossbow @ actions-562ff7b1ac](https://github.com/ursacomputing/crossbow/branches/all?query=actions-562ff7b1ac)
|Task|Status|
|----|------|
|example-cpp-minimal-build-static|[](https://github.com/ursacomputing/crossbow/actions/runs/19387574191/job/55476679142)|
|example-cpp-minimal-build-static-system-dependency|[](https://github.com/ursacomputing/crossbow/actions/runs/19387574383/job/55476679591)|
|example-cpp-tutorial|[](https://github.com/ursacomputing/crossbow/actions/runs/19387574269/job/55476679286)|
|test-build-cpp-fuzz|[](https://github.com/ursacomputing/crossbow/actions/runs/19387574000/job/55476678873)|
|test-conda-cpp|[](https://github.com/ursacomputing/crossbow/actions/runs/19387574388/job/55476679613)|
|test-conda-cpp-valgrind|[](https://github.com/ursacomputing/crossbow/actions/runs/19387574149/job/55476679075)|
|test-cuda-cpp-ubuntu-22.04-cuda-11.7.1|[](https://github.com/ursacomputing/crossbow/actions/runs/19387574211/job/55476679191)|
|test-debian-12-cpp-amd64|[](https://github.com/ursacomputing/crossbow/actions/runs/19387574316/job/55476679413)|
|test-debian-12-cpp-i386|[](https://github.com/ursacomputing/crossbow/actions/runs/19387574070/job/55476678966)|
|test-fedora-42-cpp|[](https://github.com/ursacomputing/crossbow/actions/runs/19387574001/job/55476678864)|
|test-ubuntu-22.04-cpp|[](https://github.com/ursacomputing/crossbow/actions/runs/19387574350/job/55476679638)|
|test-ubuntu-22.04-cpp-20|[](https://github.com/ursacomputing/crossbow/actions/runs/19387574125/job/55476679037)|
|test-ubuntu-22.04-cpp-bundled|[](https://github.com/ursacomputing/crossbow/actions/runs/19387574196/job/55476679162)|
|test-ubuntu-22.04-cpp-emscripten|[](https://github.com/ursacomputing/crossbow/actions/runs/19387574094/job/55476678988)|
|test-ubuntu-22.04-cpp-no-threading|[](https://github.com/ursacomputing/crossbow/actions/runs/19387574065/job/55476679003)|
|test-ubuntu-24.04-cpp|[](https://github.com/ursacomputing/crossbow/actions/runs/19387574024/job/55476678875)|
|test-ubuntu-24.04-cpp-bundled-offline|[](https://github.com/ursacomputing/crossbow/actions/runs/19387574241/job/55476679199)|
|test-ubuntu-24.04-cpp-gcc-13-bundled|[](https://github.com/ursacomputing/crossbow/actions/runs/19387574360/job/55476679508)|
|test-ubuntu-24.04-cpp-gcc-14|[](https://github.com/ursacomputing/crossbow/actions/runs/19387574406/job/55476679675)|
|test-ubuntu-24.04-cpp-minimal-with-formats|[](https://github.com/ursacomputing/crossbow/actions/runs/19387574143/job/55476679046)|
|test-ubuntu-24.04-cpp-thread-sanitizer|[](https://github.com/ursacomputing/crossbow/actions/runs/19387574337/job/55476679468)|
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-48105: [C++][Parquet][IPC] Cap allocated memory when fuzzing [arrow]
Posted by "zanmato1984 (via GitHub)" <gi...@apache.org>.
zanmato1984 commented on code in PR #48108:
URL: https://github.com/apache/arrow/pull/48108#discussion_r2528770055
##########
cpp/src/arrow/util/fuzz_internal.cc:
##########
@@ -0,0 +1,39 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+#include "arrow/util/fuzz_internal.h"
+
+#include "arrow/memory_pool.h"
+#include "arrow/status.h"
+#include "arrow/util/logging_internal.h"
+
+namespace arrow::internal {
+
+MemoryPool* fuzzing_memory_pool() {
+ static auto pool = std::make_shared<::arrow::CappedMemoryPool>(
+ ::arrow::default_memory_pool(), /*bytes_allocated_limit=*/kFuzzingMemoryLimit);
+ return pool.get();
+}
+
+void NoteFuzzStatus(const Status& st, const uint8_t* data, int64_t size) {
+ if (st.IsOutOfMemory()) {
+ ARROW_LOG(WARNING) << "Fuzzing input with size=" << size
Review Comment:
Ah, OK. Thanks for the information. I think we can comment a bit on this situation to avoid future readers' curiosity.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-48105: [C++][Parquet][IPC] Cap allocated memory when fuzzing [arrow]
Posted by "zanmato1984 (via GitHub)" <gi...@apache.org>.
zanmato1984 commented on code in PR #48108:
URL: https://github.com/apache/arrow/pull/48108#discussion_r2519586626
##########
cpp/src/arrow/memory_pool.h:
##########
@@ -245,6 +246,64 @@ class ARROW_EXPORT ProxyMemoryPool : public MemoryPool {
std::unique_ptr<ProxyMemoryPoolImpl> impl_;
};
+/// EXPERIMENTAL MemoryPool wrapper with an upper limit
+class ARROW_EXPORT CappedMemoryPool : public MemoryPool {
+ public:
+ CappedMemoryPool(MemoryPool* wrapped_pool, int64_t bytes_allocated_limit)
+ : wrapped_(wrapped_pool), bytes_allocated_limit_(bytes_allocated_limit) {}
+
+ using MemoryPool::Allocate;
+ using MemoryPool::Reallocate;
+
+ Status Allocate(int64_t size, int64_t alignment, uint8_t** out) override {
+ const auto attempted = size + wrapped_->bytes_allocated();
+ if (ARROW_PREDICT_FALSE(attempted > bytes_allocated_limit_)) {
+ return OutOfMemory(attempted);
+ }
Review Comment:
Do we want to take multi-threading into consideration? I'm not sure if it is the case for fuzz-oss. If so, we may need to have some synchronization on the allocated bytes incrementing.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-48105: [C++][Parquet][IPC] Cap allocated memory when fuzzing [arrow]
Posted by "pitrou (via GitHub)" <gi...@apache.org>.
pitrou commented on PR #48108:
URL: https://github.com/apache/arrow/pull/48108#issuecomment-3531794718
> I see outstanding fuzz targets namely `arrow/csv/fuzz.cc`, `arrow/ipc/tensor_stream_fuzz.cc` and `parquet/arrow/fuzz.cc`. Do we want to instrument those as well?
Those are just the top-level "main" files for fuzzing, but they call the functions that are modified in this PR.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-48105: [C++][Parquet][IPC] Cap allocated memory when fuzzing [arrow]
Posted by "pitrou (via GitHub)" <gi...@apache.org>.
pitrou merged PR #48108:
URL: https://github.com/apache/arrow/pull/48108
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-48105: [C++][Parquet][IPC] Cap allocated memory when fuzzing [arrow]
Posted by "pitrou (via GitHub)" <gi...@apache.org>.
pitrou commented on code in PR #48108:
URL: https://github.com/apache/arrow/pull/48108#discussion_r2526600506
##########
cpp/src/arrow/memory_pool.h:
##########
@@ -245,6 +246,73 @@ class ARROW_EXPORT ProxyMemoryPool : public MemoryPool {
std::unique_ptr<ProxyMemoryPoolImpl> impl_;
};
+/// EXPERIMENTAL MemoryPool wrapper with an upper limit
+///
+/// Checking for limits is not done in a fully thread-safe way, therefore
+/// multi-threaded allocations might be able to go successfully above the
+/// configured limit.
+class ARROW_EXPORT CappedMemoryPool : public MemoryPool {
+ public:
+ CappedMemoryPool(MemoryPool* wrapped_pool, int64_t bytes_allocated_limit)
+ : wrapped_(wrapped_pool), bytes_allocated_limit_(bytes_allocated_limit) {}
+
+ using MemoryPool::Allocate;
+ using MemoryPool::Reallocate;
Review Comment:
At least they were when `ProxyMemoryPool` was written :-)
I think they're necessary to inherit the other Allocate/Reallocate overloads while overriding the two below.
##########
cpp/src/arrow/memory_pool.h:
##########
@@ -245,6 +246,73 @@ class ARROW_EXPORT ProxyMemoryPool : public MemoryPool {
std::unique_ptr<ProxyMemoryPoolImpl> impl_;
};
+/// EXPERIMENTAL MemoryPool wrapper with an upper limit
+///
+/// Checking for limits is not done in a fully thread-safe way, therefore
+/// multi-threaded allocations might be able to go successfully above the
+/// configured limit.
+class ARROW_EXPORT CappedMemoryPool : public MemoryPool {
+ public:
+ CappedMemoryPool(MemoryPool* wrapped_pool, int64_t bytes_allocated_limit)
+ : wrapped_(wrapped_pool), bytes_allocated_limit_(bytes_allocated_limit) {}
+
+ using MemoryPool::Allocate;
+ using MemoryPool::Reallocate;
+
+ Status Allocate(int64_t size, int64_t alignment, uint8_t** out) override {
+ // XXX Another thread may allocate memory between the limit check and
+ // the `Allocate` call. It is possible for the two allocations to be successful
+ // while going above the limit.
+ // Solving this issue would require refactoring the `MemoryPool` implementation
+ // to delegate the limit check to `MemoryPoolStats`.
+ const auto attempted = size + wrapped_->bytes_allocated();
+ if (ARROW_PREDICT_FALSE(attempted > bytes_allocated_limit_)) {
+ return OutOfMemory(attempted);
+ }
+ return wrapped_->Allocate(size, alignment, out);
+ }
+
+ Status Reallocate(int64_t old_size, int64_t new_size, int64_t alignment,
+ uint8_t** ptr) override {
+ const auto attempted = new_size - old_size + wrapped_->bytes_allocated();
+ if (ARROW_PREDICT_FALSE(attempted > bytes_allocated_limit_)) {
+ return OutOfMemory(attempted);
+ }
+ return wrapped_->Reallocate(old_size, new_size, alignment, ptr);
+ }
+
+ void Free(uint8_t* buffer, int64_t size, int64_t alignment) override {
+ return wrapped_->Free(buffer, size, alignment);
+ }
+
+ void ReleaseUnused() override { wrapped_->ReleaseUnused(); }
+
+ void PrintStats() override { wrapped_->PrintStats(); }
+
+ int64_t bytes_allocated() const override { return wrapped_->bytes_allocated(); }
+
+ int64_t max_memory() const override { return wrapped_->max_memory(); }
+
+ int64_t total_bytes_allocated() const override {
+ return wrapped_->total_bytes_allocated();
+ }
+
+ int64_t num_allocations() const override { return wrapped_->num_allocations(); }
+
+ std::string backend_name() const override { return wrapped_->backend_name(); }
+
+ private:
+ Status OutOfMemory(int64_t value) {
+ return Status::OutOfMemory(
+ "MemoryPool bytes_allocated cap exceeded: "
+ "limit=",
+ bytes_allocated_limit_, ", attempted=", value);
+ }
+
+ MemoryPool* wrapped_;
+ int64_t bytes_allocated_limit_;
Review Comment:
I'll give it a try indeed.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-48105: [C++][Parquet][IPC] Cap allocated memory when fuzzing [arrow]
Posted by "pitrou (via GitHub)" <gi...@apache.org>.
pitrou commented on code in PR #48108:
URL: https://github.com/apache/arrow/pull/48108#discussion_r2522280467
##########
cpp/src/arrow/memory_pool.h:
##########
@@ -245,6 +246,64 @@ class ARROW_EXPORT ProxyMemoryPool : public MemoryPool {
std::unique_ptr<ProxyMemoryPoolImpl> impl_;
};
+/// EXPERIMENTAL MemoryPool wrapper with an upper limit
+class ARROW_EXPORT CappedMemoryPool : public MemoryPool {
+ public:
+ CappedMemoryPool(MemoryPool* wrapped_pool, int64_t bytes_allocated_limit)
+ : wrapped_(wrapped_pool), bytes_allocated_limit_(bytes_allocated_limit) {}
+
+ using MemoryPool::Allocate;
+ using MemoryPool::Reallocate;
+
+ Status Allocate(int64_t size, int64_t alignment, uint8_t** out) override {
+ const auto attempted = size + wrapped_->bytes_allocated();
+ if (ARROW_PREDICT_FALSE(attempted > bytes_allocated_limit_)) {
+ return OutOfMemory(attempted);
+ }
Review Comment:
The MemoryPool stats (`bytes_allocated`) use atomic ops. But it's true that the check as written does not fully guarantee that the stat will stay below the limit.
I honestly don't know how to solve this in a simple and efficient way, but I should add a comment about the issue.
##########
cpp/src/arrow/memory_pool.h:
##########
@@ -245,6 +246,64 @@ class ARROW_EXPORT ProxyMemoryPool : public MemoryPool {
std::unique_ptr<ProxyMemoryPoolImpl> impl_;
};
+/// EXPERIMENTAL MemoryPool wrapper with an upper limit
+class ARROW_EXPORT CappedMemoryPool : public MemoryPool {
+ public:
+ CappedMemoryPool(MemoryPool* wrapped_pool, int64_t bytes_allocated_limit)
+ : wrapped_(wrapped_pool), bytes_allocated_limit_(bytes_allocated_limit) {}
+
+ using MemoryPool::Allocate;
+ using MemoryPool::Reallocate;
+
+ Status Allocate(int64_t size, int64_t alignment, uint8_t** out) override {
+ const auto attempted = size + wrapped_->bytes_allocated();
+ if (ARROW_PREDICT_FALSE(attempted > bytes_allocated_limit_)) {
+ return OutOfMemory(attempted);
Review Comment:
With the limitation that it probably won't account for all memory allocations (for example STL structures used to represent metadata).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org