Skip to content

ARROW-13655: [C++][Parquet] Disable Thrift message size protections #11123

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

Closed
pitrou wants to merge 4 commits into from
Closed

ARROW-13655: [C++][Parquet] Disable Thrift message size protections #11123

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
b328698
ARROW-13655: [C++][Parquet] Disable Thrift message size protections
pitrou Sep 9, 2021
eaad13a
Try to fix AppVeyor
pitrou Sep 9, 2021
80881db
Remove template hacks, use Thrift version extracted by CMake
pitrou Sep 9, 2021
08c61d6
Add slow large memory test
pitrou Sep 13, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions cpp/cmake_modules/ThirdpartyToolchain.cmake
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1332,6 +1332,11 @@ if(ARROW_WITH_THRIFT)
endif()
# TODO: Don't use global includes but rather target_include_directories
include_directories(SYSTEM ${THRIFT_INCLUDE_DIR})

string(REPLACE "." ";" VERSION_LIST ${THRIFT_VERSION})
list(GET VERSION_LIST 0 THRIFT_VERSION_MAJOR)
list(GET VERSION_LIST 1 THRIFT_VERSION_MINOR)
list(GET VERSION_LIST 2 THRIFT_VERSION_PATCH)
endif()

# ----------------------------------------------------------------------
Expand Down
3 changes: 3 additions & 0 deletions cpp/src/parquet/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -282,6 +282,9 @@ endif()

add_dependencies(parquet ${PARQUET_LIBRARIES} thrift::thrift)

add_definitions(-DPARQUET_THRIFT_VERSION_MAJOR=${THRIFT_VERSION_MAJOR})
add_definitions(-DPARQUET_THRIFT_VERSION_MINOR=${THRIFT_VERSION_MINOR})

# Thrift requires these definitions for some types that we use
foreach(LIB_TARGET ${PARQUET_LIBRARIES})
target_compile_definitions(${LIB_TARGET}
Expand Down
27 changes: 21 additions & 6 deletions cpp/src/parquet/thrift_internal.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,26 +20,27 @@
#include "arrow/util/windows_compatibility.h"

#include <cstdint>
#include <limits>

// Check if thrift version < 0.11.0
// or if FORCE_BOOST_SMART_PTR is defined. Ref: https://thrift.apache.org/lib/cpp
#if defined(PARQUET_THRIFT_USE_BOOST) || defined(FORCE_BOOST_SMART_PTR)
#include <boost/shared_ptr.hpp>
#else
#include <memory>
#endif
#include <sstream>
#include <string>
#include <type_traits>
#include <utility>
#include <vector>

// TCompactProtocol requires some #defines to work right.
#define SIGNED_RIGHT_SHIFT_IS 1
#define ARITHMETIC_RIGHT_SHIFT 1
#include <thrift/TApplicationException.h>
#include <thrift/protocol/TCompactProtocol.h>
#include <thrift/protocol/TDebugProtocol.h>

#include <thrift/protocol/TBinaryProtocol.h>
#include <thrift/transport/TBufferTransports.h>
#include <sstream>

#include "arrow/util/logging.h"

Expand Down Expand Up @@ -363,12 +364,26 @@ static inline format::EncryptionAlgorithm ToThrift(EncryptionAlgorithm encryptio

using ThriftBuffer = apache::thrift::transport::TMemoryBuffer;

// On Thrift 0.14.0+, we want to use TConfiguration to raise the max message size
// limit (ARROW-13655). If we wanted to protect against huge messages, we could
// do it ourselves since we know the message size up front.

inline std::shared_ptr<ThriftBuffer> CreateReadOnlyMemoryBuffer(uint8_t* buf,
uint32_t len) {
#if PARQUET_THRIFT_VERSION_MAJOR > 0 || PARQUET_THRIFT_VERSION_MINOR >= 14
auto conf = std::make_shared<apache::thrift::TConfiguration>();
conf->setMaxMessageSize(std::numeric_limits<int>::max());
return std::make_shared<ThriftBuffer>(buf, len, ThriftBuffer::OBSERVE, conf);

@edponce edponce Sep 10, 2021

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The ThriftBuffer (aka TMemoryBuffer) constructor sets the max limit to the buffer as std::numeric_limits<uint32_t>::max(). Is the intent here to use int as to cap at a lower limit?

@pitrou pitrou Sep 10, 2021

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

This is different.We're talking about maxMessageSize, not maxBufferSize.

@emkornfield emkornfield Sep 17, 2021

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I seem to recall in the past thrift wasn't good about memory bombs produced by the fuzzer is this fixed in 14?

@pitrou pitrou Sep 17, 2021

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

See a bit below. setStringSizeLimit and setContainerSizeLimit should protect us against Thrift memory bombs, IIRC. However, we don't have any protection against compression memory bombs.

#else
return std::make_shared<ThriftBuffer>(buf, len);
#endif
}

template <class T>
inline void DeserializeThriftUnencryptedMsg(const uint8_t* buf, uint32_t* len,
T* deserialized_msg) {
// Deserialize msg bytes into c++ thrift msg using memory transport.
shared_ptr<ThriftBuffer> tmem_transport(
new ThriftBuffer(const_cast<uint8_t*>(buf), *len));
auto tmem_transport = CreateReadOnlyMemoryBuffer(const_cast<uint8_t*>(buf), *len);
apache::thrift::protocol::TCompactProtocolFactoryT<ThriftBuffer> tproto_factory;
// Protect against CPU and memory bombs
tproto_factory.setStringSizeLimit(100 * 1000 * 1000);
Expand Down
26 changes: 26 additions & 0 deletions python/pyarrow/tests/parquet/test_metadata.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -496,3 +496,29 @@ def test_parquet_metadata_empty_to_dict(tempdir):
assert len(metadata_dict["row_groups"]) == 1
assert len(metadata_dict["row_groups"][0]["columns"]) == 1
assert metadata_dict["row_groups"][0]["columns"][0]["statistics"] is None


@pytest.mark.slow
@pytest.mark.large_memory
def test_metadata_exceeds_message_size():
# ARROW-13655: Thrift may enable a defaut message size that limits
# the size of Parquet metadata that can be written.
NCOLS = 1000
NREPEATS = 4000

table = pa.table({str(i): np.random.randn(10) for i in range(NCOLS)})

with pa.BufferOutputStream() as out:
pq.write_table(table, out)
buf = out.getvalue()

original_metadata = pq.read_metadata(pa.BufferReader(buf))
metadata = pq.read_metadata(pa.BufferReader(buf))
for i in range(NREPEATS):
metadata.append_row_groups(original_metadata)

@jorisvandenbossche jorisvandenbossche Sep 14, 2021

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

BTW, the reason that the reproducer is reading in the metadata twice, and then appending the one many times to the other, is because appending an object to itself is buggy -> https://issues.apache.org/jira/browse/ARROW-13654


with pa.BufferOutputStream() as out:
metadata.write_metadata_file(out)
buf = out.getvalue()

metadata = pq.read_metadata(pa.BufferReader(buf))