You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by GitBox <gi...@apache.org> on 2020/08/25 14:58:31 UTC
[GitHub] [arrow] pitrou opened a new pull request #8050: ARROW-9852: [C++] Validate dictionaries fully on IPC read
pitrou opened a new pull request #8050:
URL: https://github.com/apache/arrow/pull/8050
Finally, there's no way around it. We need O(N) validation of dictionaries received over IPC, because concatenating deltas may involve arbitrary reads (for example, nested dictionaries are compared for equality when concatenating).
Found by OSS-Fuzz.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] pitrou commented on pull request #8050: ARROW-9852: [C++] Validate dictionaries fully on IPC read
Posted by GitBox <gi...@apache.org>.
pitrou commented on pull request #8050:
URL: https://github.com/apache/arrow/pull/8050#issuecomment-680887980
I think I'll revisit this after #7992.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] pitrou commented on pull request #8050: ARROW-9852: [C++] Validate dictionaries fully on IPC read
Posted by GitBox <gi...@apache.org>.
pitrou commented on pull request #8050:
URL: https://github.com/apache/arrow/pull/8050#issuecomment-680108214
Another option: only validate dictionaries when deltas actually appear, and explicitly disallow delta dictionaries on the GPU (in any case, I don't think array concatenation currently works on non-CPU arrays).
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] pitrou closed pull request #8050: ARROW-9852: [C++] Validate dictionaries fully when combining deltas
Posted by GitBox <gi...@apache.org>.
pitrou closed pull request #8050:
URL: https://github.com/apache/arrow/pull/8050
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] wesm commented on a change in pull request #8050: ARROW-9852: [C++] Validate dictionaries fully on IPC read
Posted by GitBox <gi...@apache.org>.
wesm commented on a change in pull request #8050:
URL: https://github.com/apache/arrow/pull/8050#discussion_r476727957
##########
File path: cpp/src/arrow/ipc/reader.cc
##########
@@ -717,8 +717,10 @@ Status ReadDictionary(const Buffer& metadata, DictionaryMemo* dictionary_memo,
return Status::Invalid("Dictionary record batch must only contain one field");
}
auto dictionary = batch->column(0);
- // Validate the dictionary for safe delta concatenation
- RETURN_NOT_OK(dictionary->Validate());
+ // Validate the dictionary for safe delta concatenation. Full validation
+ // is necessary for certain types (for example nested dictionaries).
+ // XXX: instead only validate when a concatenation is actually done?
+ RETURN_NOT_OK(dictionary->ValidateFull());
Review comment:
Could this be deferred until the actual time that the delta must be applied? That seems better but I'm OK with this if a stopgap is needed
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] pitrou commented on pull request #8050: ARROW-9852: [C++] Validate dictionaries fully on IPC read
Posted by GitBox <gi...@apache.org>.
pitrou commented on pull request #8050:
URL: https://github.com/apache/arrow/pull/8050#issuecomment-680077956
@wesm Need your opinion here.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] pitrou commented on pull request #8050: ARROW-9852: [C++] Validate dictionaries fully on IPC read
Posted by GitBox <gi...@apache.org>.
pitrou commented on pull request #8050:
URL: https://github.com/apache/arrow/pull/8050#issuecomment-680096981
Hmm... we must also avoid validating GPU buffers.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] pitrou commented on a change in pull request #8050: ARROW-9852: [C++] Validate dictionaries fully when combining deltas
Posted by GitBox <gi...@apache.org>.
pitrou commented on a change in pull request #8050:
URL: https://github.com/apache/arrow/pull/8050#discussion_r482853096
##########
File path: cpp/src/arrow/ipc/dictionary.cc
##########
@@ -143,10 +143,43 @@ int DictionaryFieldMapper::num_fields() const { return impl_->num_fields(); }
// DictionaryMemo implementation
struct DictionaryMemo::Impl {
- // Map of dictionary id to dictionary array
- std::unordered_map<int64_t, std::shared_ptr<ArrayData>> id_to_dictionary_;
+ // Map of dictionary id to dictionary array(s) (several in case of deltas)
+ std::unordered_map<int64_t, ArrayDataVector> id_to_dictionary_;
std::unordered_map<int64_t, std::shared_ptr<DataType>> id_to_type_;
DictionaryFieldMapper mapper_;
+
+ Result<decltype(id_to_dictionary_)::iterator> FindDictionary(int64_t id) {
+ auto it = id_to_dictionary_.find(id);
+ if (it == id_to_dictionary_.end()) {
+ return Status::KeyError("Dictionary with id ", id, " not found");
+ }
+ return it;
+ }
+
+ Result<std::shared_ptr<ArrayData>> ReifyDictionary(int64_t id, MemoryPool* pool) {
+ ARROW_ASSIGN_OR_RAISE(auto it, FindDictionary(id));
+ ArrayDataVector* data_vector = &it->second;
+
+ DCHECK(!data_vector->empty());
+ if (data_vector->size() > 1) {
+ // There are deltas, we need to concatenate them to the first dictionary.
+ ArrayVector to_combine;
+ to_combine.reserve(data_vector->size());
+ // IMPORTANT: At this point, the dictionary data may be untrusted.
+ // We need to validate it, as concatenation can crash on invalid or
+ // corrupted data. Full validation is necessary for certain types
+ // (for example nested dictionaries).
+ // XXX: this won't work if there are unresolved nested dictionaries.
Review comment:
Ideally yes, but our current infrastructure doesn't make this very easy.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] bkietz commented on a change in pull request #8050: ARROW-9852: [C++] Validate dictionaries fully when combining deltas
Posted by GitBox <gi...@apache.org>.
bkietz commented on a change in pull request #8050:
URL: https://github.com/apache/arrow/pull/8050#discussion_r482179398
##########
File path: cpp/src/arrow/ipc/dictionary.cc
##########
@@ -143,10 +143,43 @@ int DictionaryFieldMapper::num_fields() const { return impl_->num_fields(); }
// DictionaryMemo implementation
struct DictionaryMemo::Impl {
- // Map of dictionary id to dictionary array
- std::unordered_map<int64_t, std::shared_ptr<ArrayData>> id_to_dictionary_;
+ // Map of dictionary id to dictionary array(s) (several in case of deltas)
+ std::unordered_map<int64_t, ArrayDataVector> id_to_dictionary_;
std::unordered_map<int64_t, std::shared_ptr<DataType>> id_to_type_;
DictionaryFieldMapper mapper_;
+
+ Result<decltype(id_to_dictionary_)::iterator> FindDictionary(int64_t id) {
+ auto it = id_to_dictionary_.find(id);
+ if (it == id_to_dictionary_.end()) {
+ return Status::KeyError("Dictionary with id ", id, " not found");
+ }
+ return it;
+ }
+
+ Result<std::shared_ptr<ArrayData>> ReifyDictionary(int64_t id, MemoryPool* pool) {
+ ARROW_ASSIGN_OR_RAISE(auto it, FindDictionary(id));
+ ArrayDataVector* data_vector = &it->second;
+
+ DCHECK(!data_vector->empty());
+ if (data_vector->size() > 1) {
+ // There are deltas, we need to concatenate them to the first dictionary.
+ ArrayVector to_combine;
+ to_combine.reserve(data_vector->size());
+ // IMPORTANT: At this point, the dictionary data may be untrusted.
+ // We need to validate it, as concatenation can crash on invalid or
+ // corrupted data. Full validation is necessary for certain types
+ // (for example nested dictionaries).
Review comment:
IIUC it's also necessary for `string`. If not, it would probably be worthwhile to add a fast branch for dictionaries of primitives
##########
File path: cpp/src/arrow/ipc/dictionary.cc
##########
@@ -143,10 +143,43 @@ int DictionaryFieldMapper::num_fields() const { return impl_->num_fields(); }
// DictionaryMemo implementation
struct DictionaryMemo::Impl {
- // Map of dictionary id to dictionary array
- std::unordered_map<int64_t, std::shared_ptr<ArrayData>> id_to_dictionary_;
+ // Map of dictionary id to dictionary array(s) (several in case of deltas)
+ std::unordered_map<int64_t, ArrayDataVector> id_to_dictionary_;
std::unordered_map<int64_t, std::shared_ptr<DataType>> id_to_type_;
DictionaryFieldMapper mapper_;
+
+ Result<decltype(id_to_dictionary_)::iterator> FindDictionary(int64_t id) {
+ auto it = id_to_dictionary_.find(id);
+ if (it == id_to_dictionary_.end()) {
+ return Status::KeyError("Dictionary with id ", id, " not found");
+ }
+ return it;
+ }
+
+ Result<std::shared_ptr<ArrayData>> ReifyDictionary(int64_t id, MemoryPool* pool) {
+ ARROW_ASSIGN_OR_RAISE(auto it, FindDictionary(id));
+ ArrayDataVector* data_vector = &it->second;
+
+ DCHECK(!data_vector->empty());
+ if (data_vector->size() > 1) {
+ // There are deltas, we need to concatenate them to the first dictionary.
+ ArrayVector to_combine;
+ to_combine.reserve(data_vector->size());
+ // IMPORTANT: At this point, the dictionary data may be untrusted.
+ // We need to validate it, as concatenation can crash on invalid or
+ // corrupted data. Full validation is necessary for certain types
+ // (for example nested dictionaries).
+ // XXX: this won't work if there are unresolved nested dictionaries.
Review comment:
Would it be worthwhile to add a test verifying that an unresolved nested dictionary will emit an error instead of crashing?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] pitrou commented on a change in pull request #8050: ARROW-9852: [C++] Validate dictionaries fully when combining deltas
Posted by GitBox <gi...@apache.org>.
pitrou commented on a change in pull request #8050:
URL: https://github.com/apache/arrow/pull/8050#discussion_r481304054
##########
File path: cpp/src/arrow/ipc/reader.cc
##########
@@ -717,8 +717,10 @@ Status ReadDictionary(const Buffer& metadata, DictionaryMemo* dictionary_memo,
return Status::Invalid("Dictionary record batch must only contain one field");
}
auto dictionary = batch->column(0);
- // Validate the dictionary for safe delta concatenation
- RETURN_NOT_OK(dictionary->Validate());
+ // Validate the dictionary for safe delta concatenation. Full validation
+ // is necessary for certain types (for example nested dictionaries).
+ // XXX: instead only validate when a concatenation is actually done?
+ RETURN_NOT_OK(dictionary->ValidateFull());
Review comment:
Ok, it is the strategy I have finally implemented.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] github-actions[bot] commented on pull request #8050: ARROW-9852: [C++] Validate dictionaries fully on IPC read
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #8050:
URL: https://github.com/apache/arrow/pull/8050#issuecomment-680084692
https://issues.apache.org/jira/browse/ARROW-9852
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] pitrou commented on a change in pull request #8050: ARROW-9852: [C++] Validate dictionaries fully when combining deltas
Posted by GitBox <gi...@apache.org>.
pitrou commented on a change in pull request #8050:
URL: https://github.com/apache/arrow/pull/8050#discussion_r482854105
##########
File path: cpp/src/arrow/ipc/dictionary.cc
##########
@@ -143,10 +143,43 @@ int DictionaryFieldMapper::num_fields() const { return impl_->num_fields(); }
// DictionaryMemo implementation
struct DictionaryMemo::Impl {
- // Map of dictionary id to dictionary array
- std::unordered_map<int64_t, std::shared_ptr<ArrayData>> id_to_dictionary_;
+ // Map of dictionary id to dictionary array(s) (several in case of deltas)
+ std::unordered_map<int64_t, ArrayDataVector> id_to_dictionary_;
std::unordered_map<int64_t, std::shared_ptr<DataType>> id_to_type_;
DictionaryFieldMapper mapper_;
+
+ Result<decltype(id_to_dictionary_)::iterator> FindDictionary(int64_t id) {
+ auto it = id_to_dictionary_.find(id);
+ if (it == id_to_dictionary_.end()) {
+ return Status::KeyError("Dictionary with id ", id, " not found");
+ }
+ return it;
+ }
+
+ Result<std::shared_ptr<ArrayData>> ReifyDictionary(int64_t id, MemoryPool* pool) {
+ ARROW_ASSIGN_OR_RAISE(auto it, FindDictionary(id));
+ ArrayDataVector* data_vector = &it->second;
+
+ DCHECK(!data_vector->empty());
+ if (data_vector->size() > 1) {
+ // There are deltas, we need to concatenate them to the first dictionary.
+ ArrayVector to_combine;
+ to_combine.reserve(data_vector->size());
+ // IMPORTANT: At this point, the dictionary data may be untrusted.
+ // We need to validate it, as concatenation can crash on invalid or
+ // corrupted data. Full validation is necessary for certain types
+ // (for example nested dictionaries).
Review comment:
Well, primitive types have O(1) full validation, so I don't think a fast branch would change anything.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org