This is an automated email from the ASF dual-hosted git repository.
wasphin pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/brpc.git
The following commit(s) were added to refs/heads/master by this push:
new bac1b3b2 Reject malformed mcpack input instead of CHECK-fatal in
parser (#3576)
bac1b3b2 is described below
commit bac1b3b2be6f84dbe86fbb7b3c8760e9655b70f3
Author: Weibing Wang <[email protected]>
AuthorDate: Mon Oct 5 23:18:04 2026 +0800
Reject malformed mcpack input instead of CHECK-fatal in parser (#3576)
* Reject malformed mcpack input instead of CHECK-fatal in parser
The mcpack2pb parser used CHECK(false) to handle malformed input in
unbox(), the object/array/isoarray iterators and the value conversion
functions. CHECK(false) logs at FATAL level and aborts the process when
-crash_on_fatal_log is on, so a single malformed nshead+mcpack request
could terminate a server and drop all in-flight requests (CWE-617).
Replace the wire-controlled CHECK(false) sites with LOG(ERROR) plus the
existing error propagation (set_bad()/return 0, which the generated
parsing code checks), and mark the stream bad in unbox() and the
truncated as_string()/as_binary() paths so failed parses are reported
by stream()->good() as before. The serializer and code-generator CHECKs
are untouched since they are not driven by network input.
* Avoid wire-controlled eager resize in as_string/as_binary
Follow-up to the CHECK(false) hardening: the claimed value size of
string/binary fields comes from the wire and can be close to UINT32_MAX.
resize(size) before reading allocates that amount up front, so a tiny
request could trigger an uncaught std::bad_alloc/std::length_error and
still terminate the process.
Read in bounded chunks and grow the output only for bytes that are
actually present; a short read marks the stream bad as before.
* Reject truncated primitive reads in cut_packed_pod
The value-returning InputStream::cut_packed_pod<T>() left its result
uninitialized when cutn() read fewer than sizeof(T) bytes, which is
reachable when the enclosing object declares enough bytes but the real
input ends inside a primitive value. The indeterminate value was then
consumed or logged (UB), and the parse continued as if the input were
complete.
Zero-initialize the result, return a zero value and mark the stream bad
on a short read, so the generated code fails the parse. The pointer
version is unchanged: its callers already check the returned size.
* Validate the trailing NUL when reading mcpack strings
as_string() skipped the trailing byte with popn(1), which returns 0 on
an exhausted stream and leaves it good. A string whose content was
present but whose stream ended exactly before the required trailing
'\0' (or ended with a non-NUL byte) was therefore accepted as valid.
Read the terminator and reject the input when it is missing or not a
NUL, marking the stream bad and clearing the output.
* Avoid double copy of valid string/binary payload
cut_bytes_to_string() copied every byte twice on valid input: once from
the input into a stack buffer and once into the destination string. Grow
the destination by at most one 8 KiB chunk at a time and cut directly
into it, restoring the single-copy behavior of the original resize+cutn
path while keeping the bounded allocation.
---------
Co-authored-by: brpc-oncall <brpc-oncall@localhost>
---
src/mcpack2pb/parser-inl.h | 24 ++-
src/mcpack2pb/parser.cpp | 151 ++++++++++------
test/brpc_mcpack2pb_unittest.cpp | 376 +++++++++++++++++++++++++++++++++++++++
3 files changed, 485 insertions(+), 66 deletions(-)
diff --git a/src/mcpack2pb/parser-inl.h b/src/mcpack2pb/parser-inl.h
index 611d2266..030597ab 100644
--- a/src/mcpack2pb/parser-inl.h
+++ b/src/mcpack2pb/parser-inl.h
@@ -82,7 +82,10 @@ inline size_t InputStream::cut_packed_pod(T* packed_pod) {
template <typename T>
inline T InputStream::cut_packed_pod() {
- T packed_pod;
+ // Zero-initialize so a short read never returns an indeterminate value
+ // (reading one is UB), and mark the stream bad so the truncated input
+ // fails the parse instead of being consumed as a garbage value.
+ T packed_pod = T();
if (_size >= (int)sizeof(T)) {
packed_pod = *(T*)_data;
_data = (const char*)_data + sizeof(T);
@@ -90,7 +93,10 @@ inline T InputStream::cut_packed_pod() {
_popped_bytes += sizeof(T);
return packed_pod;
}
- cutn(&packed_pod, sizeof(T));
+ if (cutn(&packed_pod, sizeof(T)) != sizeof(T)) {
+ set_bad();
+ return T();
+ }
return packed_pod;
}
@@ -189,12 +195,12 @@ inline void ArrayIterator::init(InputStream* stream,
size_t size, size_t depth)
_expected_popped_bytes = _stream->popped_bytes() + sizeof(ItemsHead);
_expected_popped_end = _stream->popped_bytes() + size;
if (size < sizeof(ItemsHead)) {
- CHECK(false) << "buffer(size=" << size << ") is not enough";
+ LOG(ERROR) << "buffer(size=" << size << ") is not enough";
return set_bad();
}
ItemsHead items_head;
if (_stream->cut_packed_pod(&items_head) != sizeof(ItemsHead)) {
- CHECK(false) << "buffer(size=" << size << ") is not enough";
+ LOG(ERROR) << "buffer(size=" << size << ") is not enough";
return set_bad();
}
_item_count = items_head.item_count;
@@ -218,25 +224,25 @@ inline void ISOArrayIterator::init(InputStream* stream,
size_t size) {
_item_count = 0;
_left_item_count = 0;
if (size < sizeof(IsoItemsHead)) {
- CHECK(false) << "Not enough data";
+ LOG(ERROR) << "Not enough data";
return set_bad();
}
IsoItemsHead items_head;
if (_stream->cut_packed_pod(&items_head) != sizeof(IsoItemsHead)) {
- CHECK(false) << "Not enough data";
+ LOG(ERROR) << "Not enough data";
return set_bad();
}
_item_type = (PrimitiveFieldType)items_head.type;
_item_size = get_primitive_type_size(_item_type);
if (!_item_size) {
- CHECK(false) << "type=" << type2str(_item_type)
+ LOG(ERROR) << "type=" << type2str(_item_type)
<< " in primitive isoarray is not primitive";
return set_bad();
}
const size_t items_full_size = size - sizeof(IsoItemsHead);
_item_count = items_full_size / _item_size;
if (_item_count * _item_size != items_full_size) {
- CHECK(false) << "inconsistent item_count(" << _item_count
+ LOG(ERROR) << "inconsistent item_count(" << _item_count
<< ") and value_size(" << items_full_size
<< "), item_size=" << _item_size;
return set_bad();
@@ -261,7 +267,7 @@ inline void ISOArrayIterator::operator++() {
_buf_index = 0;
if (_stream->cutn(_item_buf, _buf_count * _item_size) !=
_buf_count * _item_size) {
- CHECK(false) << "Not enough data";
+ LOG(ERROR) << "Not enough data";
return set_bad();
}
_left_item_count -= _buf_count;
diff --git a/src/mcpack2pb/parser.cpp b/src/mcpack2pb/parser.cpp
index 9b8c964b..5cfb2bd9 100644
--- a/src/mcpack2pb/parser.cpp
+++ b/src/mcpack2pb/parser.cpp
@@ -89,13 +89,13 @@ void ObjectIterator::operator++() {
// skipping untouched values is acceptible.
_stream->popn(_current_field.value.size());
} else if (_stream->popped_bytes() < _expected_popped_bytes) {
- CHECK(false) << "value of name=" << _current_field.name
- << " is not fully consumed, expected="
- << _expected_popped_bytes << " actually="
- << _stream->popped_bytes();
+ LOG(ERROR) << "value of name=" << _current_field.name
+ << " is not fully consumed, expected="
+ << _expected_popped_bytes << " actually="
+ << _stream->popped_bytes();
return set_bad();
} else {
- CHECK(false) << "Over popped in value of name=" <<
_current_field.name
+ LOG(ERROR) << "Over popped in value of name=" <<
_current_field.name
<< " expected=" << _expected_popped_bytes << "
actually="
<< _stream->popped_bytes();
return set_bad();
@@ -109,7 +109,7 @@ void ObjectIterator::operator++() {
FieldFixedHead head;
if (_stream->cut_packed_pod(&head) != sizeof(FieldFixedHead) ||
left_size() < head.full_size()) {
- CHECK(false) << "buffer(size=" << left_size() << ") is not enough";
+ LOG(ERROR) << "buffer(size=" << left_size() << ") is not enough";
return set_bad();
}
_expected_popped_bytes = _stream->popped_bytes() + head.full_size()
@@ -127,7 +127,7 @@ void ObjectIterator::operator++() {
FieldShortHead head;
if (_stream->cut_packed_pod(&head) != sizeof(FieldShortHead) ||
left_size() < head.full_size()) {
- CHECK(false) << "buffer(size=" << left_size() << ") is not enough";
+ LOG(ERROR) << "buffer(size=" << left_size() << ") is not enough";
return set_bad();
}
_expected_popped_bytes = _stream->popped_bytes() + head.full_size()
@@ -148,7 +148,7 @@ void ObjectIterator::operator++() {
FieldLongHead head;
if (_stream->cut_packed_pod(&head) != sizeof(FieldLongHead) ||
left_size() < head.full_size()) {
- CHECK(false) << "buffer(size=" << left_size() << ") is not enough";
+ LOG(ERROR) << "buffer(size=" << left_size() << ") is not enough";
return set_bad();
}
_expected_popped_bytes = _stream->popped_bytes() + head.full_size()
@@ -173,12 +173,12 @@ void ArrayIterator::operator++() {
// skipping untouched values is acceptible.
_stream->popn(_current_field.size());
} else if (_stream->popped_bytes() < _expected_popped_bytes) {
- CHECK(false) << "previous value is not fully consumed, expected="
+ LOG(ERROR) << "previous value is not fully consumed, expected="
<< _expected_popped_bytes << " actually="
<< _stream->popped_bytes();
return set_bad();
} else {
- CHECK(false) << "Over popped in previous value, expected="
+ LOG(ERROR) << "Over popped in previous value, expected="
<< _expected_popped_bytes << " actually="
<< _stream->popped_bytes();
return set_bad();
@@ -192,7 +192,7 @@ void ArrayIterator::operator++() {
FieldFixedHead head;
if (_stream->cut_packed_pod(&head) != sizeof(FieldFixedHead) ||
left_size() < head.full_size()) {
- CHECK(false) << "buffer(size=" << left_size() << ") is not enough";
+ LOG(ERROR) << "buffer(size=" << left_size() << ") is not enough";
return set_bad();
}
_expected_popped_bytes = _stream->popped_bytes() + head.full_size()
@@ -211,7 +211,7 @@ void ArrayIterator::operator++() {
FieldShortHead head;
if (_stream->cut_packed_pod(&head) != sizeof(FieldShortHead) ||
left_size() < head.full_size()) {
- CHECK(false) << "buffer(size=" << left_size() << ") is not enough";
+ LOG(ERROR) << "buffer(size=" << left_size() << ") is not enough";
return set_bad();
}
_expected_popped_bytes = _stream->popped_bytes() + head.full_size()
@@ -232,7 +232,7 @@ void ArrayIterator::operator++() {
FieldLongHead head;
if (_stream->cut_packed_pod(&head) != sizeof(FieldLongHead) ||
left_size() < head.full_size()) {
- CHECK(false) << "buffer(size=" << left_size() << ") is not enough";
+ LOG(ERROR) << "buffer(size=" << left_size() << ") is not enough";
return set_bad();
}
_expected_popped_bytes = _stream->popped_bytes() + head.full_size()
@@ -252,20 +252,27 @@ void ArrayIterator::operator++() {
size_t unbox(InputStream* stream) {
FieldLongHead head;
+ // Note that these are wire-controlled inputs, so reject (set_bad +
+ // return 0, which the generated code checks) rather than CHECK-fatal,
+ // which would let a single malformed request terminate the process.
if (stream->cut_packed_pod(&head) != sizeof(FieldLongHead)) {
- CHECK(false) << "Input buffer is not enough";
+ LOG(ERROR) << "Input buffer is not enough";
+ stream->set_bad();
return 0;
}
if (head.type() != FIELD_OBJECT) {
- CHECK(false) << "type=" << type2str(head.type()) << " is not object";
+ LOG(ERROR) << "type=" << type2str(head.type()) << " is not object";
+ stream->set_bad();
return 0;
}
if (!(head.type() & FIELD_NON_DELETED_MASK)) {
- CHECK(false) << "The item is deleted";
+ LOG(ERROR) << "The item is deleted";
+ stream->set_bad();
return 0;
}
if (head.name_size() != 0) {
- CHECK(false) << "The object should not have name";
+ LOG(ERROR) << "The object should not have name";
+ stream->set_bad();
return 0;
}
return head.value_size();
@@ -299,24 +306,24 @@ int64_t UnparsedValue::as_int64(const char* var) {
if (value <= (uint64_t)std::numeric_limits<int64_t>::max()) {
return (int64_t)value;
}
- CHECK(false) << "uint64=" << value << " to " << var << " overflows";
+ LOG(ERROR) << "uint64=" << value << " to " << var << " overflows";
_stream->set_bad();
return std::numeric_limits<int64_t>::max();
}
case PRIMITIVE_FIELD_BOOL:
return _stream->cut_packed_pod<bool>();
case PRIMITIVE_FIELD_FLOAT:
- CHECK(false) << "Can't set float=" << _stream->cut_packed_pod<float>()
+ LOG(ERROR) << "Can't set float=" << _stream->cut_packed_pod<float>()
<< " to " << var;
_stream->set_bad();
return 0;
case PRIMITIVE_FIELD_DOUBLE:
- CHECK(false) << "Can't set double=" <<
_stream->cut_packed_pod<double>()
+ LOG(ERROR) << "Can't set double=" << _stream->cut_packed_pod<double>()
<< " to " << var;
_stream->set_bad();
return 0;
}
- CHECK(false) << "Can't set type=" << type2str(_type) << " to " << var;
+ LOG(ERROR) << "Can't set type=" << type2str(_type) << " to " << var;
_stream->set_bad();
return 0;
}
@@ -328,7 +335,7 @@ uint64_t UnparsedValue::as_uint64(const char* var) {
if (value >= 0) {
return (uint64_t)value;
}
- CHECK(false) << "Can't set int8=" << value << " to " << var;
+ LOG(ERROR) << "Can't set int8=" << value << " to " << var;
_stream->set_bad();
return 0;
}
@@ -337,7 +344,7 @@ uint64_t UnparsedValue::as_uint64(const char* var) {
if (value >= 0) {
return (uint64_t)value;
}
- CHECK(false) << "Can't set int16=" << value << " to " << var;
+ LOG(ERROR) << "Can't set int16=" << value << " to " << var;
_stream->set_bad();
return 0;
}
@@ -346,7 +353,7 @@ uint64_t UnparsedValue::as_uint64(const char* var) {
if (value >= 0) {
return (uint64_t)value;
}
- CHECK(false) << "Can't set int32=" << value << " to " << var;
+ LOG(ERROR) << "Can't set int32=" << value << " to " << var;
_stream->set_bad();
return 0;
}
@@ -355,7 +362,7 @@ uint64_t UnparsedValue::as_uint64(const char* var) {
if (value >= 0) {
return (uint64_t)value;
}
- CHECK(false) << "Can't set int64=" << value << " to " << var;
+ LOG(ERROR) << "Can't set int64=" << value << " to " << var;
_stream->set_bad();
return 0;
}
@@ -370,17 +377,17 @@ uint64_t UnparsedValue::as_uint64(const char* var) {
case PRIMITIVE_FIELD_BOOL:
return _stream->cut_packed_pod<bool>();
case PRIMITIVE_FIELD_FLOAT:
- CHECK(false) << "Can't set float=" << _stream->cut_packed_pod<float>()
+ LOG(ERROR) << "Can't set float=" << _stream->cut_packed_pod<float>()
<< " to " << var;
_stream->set_bad();
return 0;
case PRIMITIVE_FIELD_DOUBLE:
- CHECK(false) << "Can't set double=" <<
_stream->cut_packed_pod<double>()
+ LOG(ERROR) << "Can't set double=" << _stream->cut_packed_pod<double>()
<< " to " << var;
_stream->set_bad();
return 0;
}
- CHECK(false) << "Can't set type=" << type2str(_type) << " to " << var;
+ LOG(ERROR) << "Can't set type=" << type2str(_type) << " to " << var;
_stream->set_bad();
return 0;
}
@@ -396,11 +403,11 @@ int32_t UnparsedValue::as_int32(const char* var) {
case PRIMITIVE_FIELD_INT64: {
const int64_t value = _stream->cut_packed_pod<int64_t>();
if (value > std::numeric_limits<int32_t>::max()) {
- CHECK(false) << "int64=" << value << " to " << var << " overflows";
+ LOG(ERROR) << "int64=" << value << " to " << var << " overflows";
_stream->set_bad();
return std::numeric_limits<int32_t>::max();
} else if (value < std::numeric_limits<int32_t>::min()) {
- CHECK(false) << "int64=" << value << " to " << var << "
underflows";
+ LOG(ERROR) << "int64=" << value << " to " << var << " underflows";
_stream->set_bad();
return std::numeric_limits<int32_t>::min();
}
@@ -415,7 +422,7 @@ int32_t UnparsedValue::as_int32(const char* var) {
if (value <= (uint32_t)std::numeric_limits<int32_t>::max()) {
return (int32_t)value;
}
- CHECK(false) << "uint32=" << value << " to " << var << " overflows";
+ LOG(ERROR) << "uint32=" << value << " to " << var << " overflows";
_stream->set_bad();
return std::numeric_limits<int32_t>::max();
}
@@ -424,24 +431,24 @@ int32_t UnparsedValue::as_int32(const char* var) {
if (value <= (uint64_t)std::numeric_limits<int32_t>::max()) {
return (int32_t)value;
}
- CHECK(false) << "uint64=" << value << " to " << var << " overflows";
+ LOG(ERROR) << "uint64=" << value << " to " << var << " overflows";
_stream->set_bad();
return std::numeric_limits<int32_t>::max();
}
case PRIMITIVE_FIELD_BOOL:
return _stream->cut_packed_pod<bool>();
case PRIMITIVE_FIELD_FLOAT:
- CHECK(false) << "Can't set float=" << _stream->cut_packed_pod<float>()
+ LOG(ERROR) << "Can't set float=" << _stream->cut_packed_pod<float>()
<< " to " << var;
_stream->set_bad();
return 0;
case PRIMITIVE_FIELD_DOUBLE:
- CHECK(false) << "Can't set double=" <<
_stream->cut_packed_pod<double>()
+ LOG(ERROR) << "Can't set double=" << _stream->cut_packed_pod<double>()
<< " to " << var;
_stream->set_bad();
return 0;
}
- CHECK(false) << "Can't set type=" << type2str(_type) << " to " << var;
+ LOG(ERROR) << "Can't set type=" << type2str(_type) << " to " << var;
_stream->set_bad();
return 0;
}
@@ -453,7 +460,7 @@ uint32_t UnparsedValue::as_uint32(const char* var) {
if (value >= 0) {
return (uint32_t)value;
}
- CHECK(false) << "Can't set int8=" << value << " to " << var;
+ LOG(ERROR) << "Can't set int8=" << value << " to " << var;
_stream->set_bad();
return 0;
}
@@ -462,7 +469,7 @@ uint32_t UnparsedValue::as_uint32(const char* var) {
if (value >= 0) {
return (uint32_t)value;
}
- CHECK(false) << "Can't set int16=" << value << " to " << var;
+ LOG(ERROR) << "Can't set int16=" << value << " to " << var;
_stream->set_bad();
return 0;
}
@@ -471,7 +478,7 @@ uint32_t UnparsedValue::as_uint32(const char* var) {
if (value >= 0) {
return (uint32_t)value;
}
- CHECK(false) << "Can't set int32=" << value << " to " << var;
+ LOG(ERROR) << "Can't set int32=" << value << " to " << var;
_stream->set_bad();
return 0;
}
@@ -481,7 +488,7 @@ uint32_t UnparsedValue::as_uint32(const char* var) {
value <= (int64_t)std::numeric_limits<uint32_t>::max()) {
return (uint32_t)value;
}
- CHECK(false) << "Can't set int64=" << value << " to " << var;
+ LOG(ERROR) << "Can't set int64=" << value << " to " << var;
_stream->set_bad();
return 0;
}
@@ -496,24 +503,24 @@ uint32_t UnparsedValue::as_uint32(const char* var) {
if (value <= std::numeric_limits<uint32_t>::max()) {
return (uint32_t)value;
}
- CHECK(false) << "uint64=" << value << " to " << var << " overflows";
+ LOG(ERROR) << "uint64=" << value << " to " << var << " overflows";
_stream->set_bad();
return std::numeric_limits<uint32_t>::max();
}
case PRIMITIVE_FIELD_BOOL:
return _stream->cut_packed_pod<bool>();
case PRIMITIVE_FIELD_FLOAT:
- CHECK(false) << "Can't set float=" << _stream->cut_packed_pod<float>()
+ LOG(ERROR) << "Can't set float=" << _stream->cut_packed_pod<float>()
<< " to " << var;
_stream->set_bad();
return 0;
case PRIMITIVE_FIELD_DOUBLE:
- CHECK(false) << "Can't set double=" <<
_stream->cut_packed_pod<double>()
+ LOG(ERROR) << "Can't set double=" << _stream->cut_packed_pod<double>()
<< " to " << var;
_stream->set_bad();
return 0;
}
- CHECK(false) << "Can't set type=" << type2str(_type) << " to " << var;
+ LOG(ERROR) << "Can't set type=" << type2str(_type) << " to " << var;
_stream->set_bad();
return 0;
}
@@ -539,17 +546,17 @@ bool UnparsedValue::as_bool(const char* var) {
case PRIMITIVE_FIELD_BOOL:
return _stream->cut_packed_pod<bool>();
case PRIMITIVE_FIELD_FLOAT:
- CHECK(false) << "Can't set float=" << _stream->cut_packed_pod<float>()
+ LOG(ERROR) << "Can't set float=" << _stream->cut_packed_pod<float>()
<< " to " << var;
_stream->set_bad();
return false;
case PRIMITIVE_FIELD_DOUBLE:
- CHECK(false) << "Can't set double=" <<
_stream->cut_packed_pod<double>()
+ LOG(ERROR) << "Can't set double=" << _stream->cut_packed_pod<double>()
<< " to " << var;
_stream->set_bad();
return false;
}
- CHECK(false) << "Can't set type=" << type2str(_type) << " to " << var;
+ LOG(ERROR) << "Can't set type=" << type2str(_type) << " to " << var;
_stream->set_bad();
return false;
}
@@ -560,7 +567,7 @@ float UnparsedValue::as_float(const char* var) {
} else if (_type == FIELD_FLOAT) {
return _stream->cut_packed_pod<float>();
}
- CHECK(false) << "Can't set type=" << type2str(_type) << " to " << var;
+ LOG(ERROR) << "Can't set type=" << type2str(_type) << " to " << var;
_stream->set_bad();
return 0;
}
@@ -571,11 +578,38 @@ double UnparsedValue::as_double(const char* var) {
} else if (_type == FIELD_FLOAT) {
return _stream->cut_packed_pod<float>();
}
- CHECK(false) << "Can't set type=" << type2str(_type) << " to " << var;
+ LOG(ERROR) << "Can't set type=" << type2str(_type) << " to " << var;
_stream->set_bad();
return 0;
}
+// Copy `size' bytes from `stream' into `out'. `size' is wire-controlled and
+// may far exceed the bytes actually present, so grow `out' by at most one
+// chunk at a time and cut directly into it (single copy): an eager
+// resize(size) could throw an uncaught std::bad_alloc/std::length_error on
+// attacker-crafted sizes and terminate the process. Returns false and marks
+// the stream bad on a short read.
+static bool cut_bytes_to_string(InputStream* stream, std::string* out,
+ size_t size, const char* var) {
+ out->clear();
+ const size_t kChunkSize = 8192;
+ size_t total = 0;
+ while (total < size) {
+ const size_t left = size - total;
+ const size_t chunk = left < kChunkSize ? left : kChunkSize;
+ out->resize(total + chunk);
+ const size_t n = stream->cutn(&(*out)[total], chunk);
+ if (n != chunk) {
+ LOG(ERROR) << "Not enough data for " << var;
+ stream->set_bad();
+ out->clear();
+ return false;
+ }
+ total += n;
+ }
+ return true;
+}
+
void UnparsedValue::as_string(std::string* out, const char* var) {
if (_size < 1) {
// A string field must contain at least the trailing '\0'.
@@ -586,12 +620,19 @@ void UnparsedValue::as_string(std::string* out, const
char* var) {
_stream->set_bad();
return;
}
- out->resize(_size - 1);
- if (_stream->cutn(&(*out)[0], _size - 1) != _size - 1) {
- CHECK(false) << "Not enough data for " << var;
+ if (!cut_bytes_to_string(_stream, out, _size - 1, var)) {
+ return;
+ }
+ // The string must end with a trailing '\0'. Read and validate it,
+ // otherwise a stream that ends exactly before the terminator accepts a
+ // truncated string with the stream still good.
+ char terminator = 0;
+ if (_stream->cutn(&terminator, 1) != 1 || terminator != '\0') {
+ LOG(ERROR) << "Invalid string terminator for " << var;
+ _stream->set_bad();
+ out->clear();
return;
}
- _stream->popn(1);
}
std::string UnparsedValue::as_string(const char* var) {
@@ -601,11 +642,7 @@ std::string UnparsedValue::as_string(const char* var) {
}
void UnparsedValue::as_binary(std::string* out, const char* var) {
- out->resize(_size);
- if (_stream->cutn(&(*out)[0], _size) != _size) {
- CHECK(false) << "Not enough data for " << var;
- return;
- }
+ cut_bytes_to_string(_stream, out, _size, var);
}
std::string UnparsedValue::as_binary(const char* var) {
diff --git a/test/brpc_mcpack2pb_unittest.cpp b/test/brpc_mcpack2pb_unittest.cpp
index 3ca56d56..b40a30c4 100644
--- a/test/brpc_mcpack2pb_unittest.cpp
+++ b/test/brpc_mcpack2pb_unittest.cpp
@@ -334,4 +334,380 @@ TEST(Mcpack2pbParserTest, EmptyObjectStillParses) {
EXPECT_TRUE(stream.good());
}
+TEST(Mcpack2pbParserTest, UnboxTruncatedBufferIsRejected) {
+ // A body shorter than the 6-byte FieldLongHead must be rejected by
+ // unbox() instead of hitting a fatal CHECK (which aborts the process
+ // when -crash_on_fatal_log is on).
+ const unsigned char data[] = { 0xff, 0x01, 0x02 };
+ butil::IOBuf body;
+ body.append(data, sizeof(data));
+
+ butil::IOBufAsZeroCopyInputStream zc_stream(body);
+ mcpack2pb::InputStream stream(&zc_stream);
+ EXPECT_EQ(0u, mcpack2pb::unbox(&stream));
+ EXPECT_FALSE(stream.good());
+}
+
+TEST(Mcpack2pbParserTest, UnboxNonObjectTypeIsRejected) {
+ // The top-level value must be an object; here it is an int8.
+ const unsigned char data[] = {
+ 0x11, 0x00, 0x05, 0x00, 0x00, 0x00, // FieldLongHead, type=int8
+ };
+ butil::IOBuf body;
+ body.append(data, sizeof(data));
+
+ butil::IOBufAsZeroCopyInputStream zc_stream(body);
+ mcpack2pb::InputStream stream(&zc_stream);
+ EXPECT_EQ(0u, mcpack2pb::unbox(&stream));
+ EXPECT_FALSE(stream.good());
+}
+
+TEST(Mcpack2pbParserTest, UnboxNamedTopLevelObjectIsRejected) {
+ // The wrapping object must be unnamed.
+ const unsigned char data[] = {
+ 0x10, 0x03, 0x05, 0x00, 0x00, 0x00, // FieldLongHead, name_size=3
+ 0x61, 0x62, 0x00, // name "ab"
+ };
+ butil::IOBuf body;
+ body.append(data, sizeof(data));
+
+ butil::IOBufAsZeroCopyInputStream zc_stream(body);
+ mcpack2pb::InputStream stream(&zc_stream);
+ EXPECT_EQ(0u, mcpack2pb::unbox(&stream));
+ EXPECT_FALSE(stream.good());
+}
+
+TEST(Mcpack2pbParserTest, ObjectFieldBeyondBufferIsRejected) {
+ // An object field whose head claims value_size=100 while the buffer
+ // holds nothing must set the iterator bad instead of hitting a fatal
+ // CHECK.
+ const unsigned char data[] = {
+ 0x10, 0x00, 0x0c, 0x00, 0x00, 0x00, // top object, value_size=12
+ 0x01, 0x00, 0x00, 0x00, // item_count=1
+ 0x10, 0x02, 0x64, 0x00, 0x00, 0x00, // field "a", value_size=100
+ 0x61, 0x00,
+ };
+ butil::IOBuf body;
+ body.append(data, sizeof(data));
+
+ butil::IOBufAsZeroCopyInputStream zc_stream(body);
+ mcpack2pb::InputStream stream(&zc_stream);
+ ASSERT_EQ(12u, mcpack2pb::unbox(&stream));
+ mcpack2pb::ObjectIterator it(&stream, body.size() - stream.popped_bytes());
+ EXPECT_TRUE(it == NULL);
+ EXPECT_FALSE(stream.good());
+}
+
+TEST(Mcpack2pbParserTest, StringFieldTruncatedDataIsRejected) {
+ // The top-level head claims value_size=1000 but the stream holds far
+ // less, and the string field claims value_size=80 with a single byte of
+ // data. as_string() must fail cleanly (mark the stream bad) instead of
+ // hitting a fatal CHECK, and advancing the iterator afterwards must not
+ // abort either.
+ const unsigned char data[] = {
+ 0x10, 0x00, 0xe8, 0x03, 0x00, 0x00, // top object, value_size=1000
+ 0x01, 0x00, 0x00, 0x00, // item_count=1
+ 0x50, 0x04, 0x50, 0x00, 0x00, 0x00, // string field "msg", size=80
+ 0x6d, 0x73, 0x67, 0x00,
+ 0x61, // only 1 byte of the string
+ };
+ butil::IOBuf body;
+ body.append(data, sizeof(data));
+
+ butil::IOBufAsZeroCopyInputStream zc_stream(body);
+ mcpack2pb::InputStream stream(&zc_stream);
+ ASSERT_EQ(1000u, mcpack2pb::unbox(&stream));
+ // The generated code trusts the value size claimed by unbox().
+ mcpack2pb::ObjectIterator it(&stream, 1000);
+ ASSERT_TRUE(it != NULL);
+ ASSERT_EQ(mcpack2pb::FIELD_STRING, it->value.type());
+ std::string value;
+ it->value.as_string(&value, "msg");
+ EXPECT_FALSE(stream.good());
+ // Advancing past the unconsumed truncated value must not abort.
+ ++it;
+ EXPECT_TRUE(it == NULL);
+}
+
+TEST(Mcpack2pbParserTest, ArrayPayloadSmallerThanHeaderIsRejected) {
+ // An array field whose declared value_size=2 is smaller than the 4-byte
+ // ItemsHead must set the iterator bad instead of hitting a fatal CHECK.
+ const unsigned char data[] = {
+ 0x10, 0x00, 0x0e, 0x00, 0x00, 0x00, // top object, value_size=14
+ 0x01, 0x00, 0x00, 0x00, // item_count=1
+ 0x20, 0x02, 0x02, 0x00, 0x00, 0x00, // array field "a", value_size=2
+ 0x61, 0x00,
+ 0x00, 0x00,
+ };
+ butil::IOBuf body;
+ body.append(data, sizeof(data));
+
+ butil::IOBufAsZeroCopyInputStream zc_stream(body);
+ mcpack2pb::InputStream stream(&zc_stream);
+ ASSERT_EQ(14u, mcpack2pb::unbox(&stream));
+ mcpack2pb::ObjectIterator it(&stream, body.size() - stream.popped_bytes());
+ ASSERT_TRUE(it != NULL);
+ ASSERT_EQ(mcpack2pb::FIELD_ARRAY, it->value.type());
+ mcpack2pb::ArrayIterator it2(it->value);
+ EXPECT_TRUE(it2 == NULL);
+ EXPECT_FALSE(stream.good());
+}
+
+TEST(Mcpack2pbParserTest, ArrayItemBeyondBufferIsRejected) {
+ // An array item whose head claims value_size=100 while nothing follows
+ // must set the iterator bad instead of hitting a fatal CHECK.
+ const unsigned char data[] = {
+ 0x10, 0x00, 0x16, 0x00, 0x00, 0x00, // top object, value_size=22
+ 0x01, 0x00, 0x00, 0x00, // item_count=1
+ 0x20, 0x02, 0x0a, 0x00, 0x00, 0x00, // array field "a", value_size=10
+ 0x61, 0x00,
+ 0x01, 0x00, 0x00, 0x00, // item_count=1
+ 0x10, 0x00, 0x64, 0x00, 0x00, 0x00, // item object, value_size=100
+ };
+ butil::IOBuf body;
+ body.append(data, sizeof(data));
+
+ butil::IOBufAsZeroCopyInputStream zc_stream(body);
+ mcpack2pb::InputStream stream(&zc_stream);
+ ASSERT_EQ(22u, mcpack2pb::unbox(&stream));
+ mcpack2pb::ObjectIterator it(&stream, body.size() - stream.popped_bytes());
+ ASSERT_TRUE(it != NULL);
+ ASSERT_EQ(mcpack2pb::FIELD_ARRAY, it->value.type());
+ mcpack2pb::ArrayIterator it2(it->value);
+ EXPECT_TRUE(it2 == NULL);
+ EXPECT_FALSE(stream.good());
+}
+
+TEST(Mcpack2pbParserTest, ISOArrayNonPrimitiveTypeIsRejected) {
+ // An isomorphic array whose element type byte is FIELD_OBJECT (not a
+ // primitive type) must set the iterator bad instead of hitting a fatal
+ // CHECK.
+ const unsigned char data[] = {
+ 0x10, 0x00, 0x0d, 0x00, 0x00, 0x00, // top object, value_size=13
+ 0x01, 0x00, 0x00, 0x00, // item_count=1
+ 0x30, 0x02, 0x01, 0x00, 0x00, 0x00, // isoarray field "a", size=1
+ 0x61, 0x00,
+ 0x10, // element type = FIELD_OBJECT
+ };
+ butil::IOBuf body;
+ body.append(data, sizeof(data));
+
+ butil::IOBufAsZeroCopyInputStream zc_stream(body);
+ mcpack2pb::InputStream stream(&zc_stream);
+ ASSERT_EQ(13u, mcpack2pb::unbox(&stream));
+ mcpack2pb::ObjectIterator it(&stream, body.size() - stream.popped_bytes());
+ ASSERT_TRUE(it != NULL);
+ ASSERT_EQ(mcpack2pb::FIELD_ISOARRAY, it->value.type());
+ mcpack2pb::ISOArrayIterator it2(it->value);
+ EXPECT_TRUE(it2 == NULL);
+ EXPECT_FALSE(stream.good());
+}
+
+TEST(Mcpack2pbParserTest, ISOArrayInconsistentItemSizeIsRejected) {
+ // An int32 isomorphic array whose payload (5 bytes) is not a multiple
+ // of the element size (4) must set the iterator bad instead of hitting
+ // a fatal CHECK.
+ const unsigned char data[] = {
+ 0x10, 0x00, 0x12, 0x00, 0x00, 0x00, // top object, value_size=18
+ 0x01, 0x00, 0x00, 0x00, // item_count=1
+ 0x30, 0x02, 0x06, 0x00, 0x00, 0x00, // isoarray field "a", size=6
+ 0x61, 0x00,
+ 0x14, // element type = int32
+ 0x01, 0x02, 0x03, 0x04, 0x05, // 5 bytes, not a multiple of 4
+ };
+ butil::IOBuf body;
+ body.append(data, sizeof(data));
+
+ butil::IOBufAsZeroCopyInputStream zc_stream(body);
+ mcpack2pb::InputStream stream(&zc_stream);
+ ASSERT_EQ(18u, mcpack2pb::unbox(&stream));
+ mcpack2pb::ObjectIterator it(&stream, body.size() - stream.popped_bytes());
+ ASSERT_TRUE(it != NULL);
+ ASSERT_EQ(mcpack2pb::FIELD_ISOARRAY, it->value.type());
+ mcpack2pb::ISOArrayIterator it2(it->value);
+ EXPECT_TRUE(it2 == NULL);
+ EXPECT_FALSE(stream.good());
+}
+
+TEST(Mcpack2pbParserTest, FloatFieldReadAsIntegerIsRejected) {
+ // Reading a float field as an integer is a type mismatch caused by
+ // input data: it must mark the stream bad and return 0 instead of
+ // hitting a fatal CHECK.
+ const unsigned char data[] = {
+ 0x10, 0x00, 0x0c, 0x00, 0x00, 0x00, // top object, value_size=12
+ 0x01, 0x00, 0x00, 0x00, // item_count=1
+ 0x44, 0x02, // fixed head: float field "a"
+ 0x61, 0x00,
+ 0x00, 0x00, 0x80, 0x3f, // 1.0f
+ };
+ butil::IOBuf body;
+ body.append(data, sizeof(data));
+
+ butil::IOBufAsZeroCopyInputStream zc_stream(body);
+ mcpack2pb::InputStream stream(&zc_stream);
+ ASSERT_EQ(12u, mcpack2pb::unbox(&stream));
+ mcpack2pb::ObjectIterator it(&stream, body.size() - stream.popped_bytes());
+ ASSERT_TRUE(it != NULL);
+ ASSERT_EQ(mcpack2pb::FIELD_FLOAT, it->value.type());
+ EXPECT_EQ("a", it->name.as_string());
+ EXPECT_EQ(0, it->value.as_int64("a"));
+ EXPECT_FALSE(stream.good());
+}
+
+TEST(Mcpack2pbParserTest, StringFieldHugeClaimedSizeDoesNotThrow) {
+ // The string field claims a ~4GB value_size while the
+ // buffer holds a single byte. as_string() must not eagerly resize to
+ // the claimed size (an uncaught std::bad_alloc/std::length_error would
+ // terminate the process) and must reject the input cleanly.
+ const unsigned char data[] = {
+ 0x10, 0x00, 0xff, 0xff, 0xff, 0xff, // top object,
value_size=UINT32_MAX
+ 0x01, 0x00, 0x00, 0x00, // item_count=1
+ 0x50, 0x04, 0xe0, 0xff, 0xff, 0xff, // string field "msg", size=~4GB
+ 0x6d, 0x73, 0x67, 0x00,
+ 0x61, // only 1 byte of the string
+ };
+ butil::IOBuf body;
+ body.append(data, sizeof(data));
+
+ butil::IOBufAsZeroCopyInputStream zc_stream(body);
+ mcpack2pb::InputStream stream(&zc_stream);
+ ASSERT_EQ(0xffffffffu, mcpack2pb::unbox(&stream));
+ // The generated code trusts the value size claimed by unbox().
+ mcpack2pb::ObjectIterator it(&stream, 0xffffffffu);
+ ASSERT_TRUE(it != NULL);
+ ASSERT_EQ(mcpack2pb::FIELD_STRING, it->value.type());
+ std::string value;
+ it->value.as_string(&value, "msg");
+ EXPECT_TRUE(value.empty());
+ EXPECT_FALSE(stream.good());
+}
+
+TEST(Mcpack2pbParserTest, BinaryFieldHugeClaimedSizeDoesNotThrow) {
+ // Same as above for a binary field claiming a ~4GB size: it must not be
fed
+ // into an eager resize() before the data is proven to exist.
+ const unsigned char data[] = {
+ 0x10, 0x00, 0xff, 0xff, 0xff, 0xff, // top object,
value_size=UINT32_MAX
+ 0x01, 0x00, 0x00, 0x00, // item_count=1
+ 0x60, 0x02, 0xe0, 0xff, 0xff, 0xff, // binary field "a", size=~4GB
+ 0x61, 0x00,
+ 0x62, // only 1 byte of data
+ };
+ butil::IOBuf body;
+ body.append(data, sizeof(data));
+
+ butil::IOBufAsZeroCopyInputStream zc_stream(body);
+ mcpack2pb::InputStream stream(&zc_stream);
+ ASSERT_EQ(0xffffffffu, mcpack2pb::unbox(&stream));
+ mcpack2pb::ObjectIterator it(&stream, 0xffffffffu);
+ ASSERT_TRUE(it != NULL);
+ ASSERT_EQ(mcpack2pb::FIELD_BINARY, it->value.type());
+ std::string value;
+ it->value.as_binary(&value, "a");
+ EXPECT_TRUE(value.empty());
+ EXPECT_FALSE(stream.good());
+}
+
+TEST(Mcpack2pbParserTest, Int32FieldTruncatedPayloadIsRejected) {
+ // The top-level head claims value_size=64 but the stream ends in the
+ // middle of the int32 field value. The truncated read must mark the
+ // stream bad (so the generated code fails the parse) and must not
+ // return an indeterminate value.
+ const unsigned char data[] = {
+ 0x10, 0x00, 0x40, 0x00, 0x00, 0x00, // top object, value_size=64
+ 0x01, 0x00, 0x00, 0x00, // item_count=1
+ 0x14, 0x02, // fixed head: int32 field "a"
+ 0x61, 0x00,
+ 0x2a, // only 1 byte of the 4-byte value
+ };
+ butil::IOBuf body;
+ body.append(data, sizeof(data));
+
+ butil::IOBufAsZeroCopyInputStream zc_stream(body);
+ mcpack2pb::InputStream stream(&zc_stream);
+ ASSERT_EQ(64u, mcpack2pb::unbox(&stream));
+ // The generated code trusts the value size claimed by unbox().
+ mcpack2pb::ObjectIterator it(&stream, 64);
+ ASSERT_TRUE(it != NULL);
+ ASSERT_EQ(mcpack2pb::FIELD_INT32, it->value.type());
+ EXPECT_EQ(0, it->value.as_int32("a"));
+ EXPECT_FALSE(stream.good());
+}
+
+TEST(Mcpack2pbParserTest, FloatFieldTruncatedPayloadIsRejected) {
+ // Same truncation reached through a type-mismatch log path: as_int64()
+ // on a float field reads the 4-byte float payload, which must be
+ // zero-initialized on the short read (logging an indeterminate float is
+ // UB) and must mark the stream bad.
+ const unsigned char data[] = {
+ 0x10, 0x00, 0x40, 0x00, 0x00, 0x00, // top object, value_size=64
+ 0x01, 0x00, 0x00, 0x00, // item_count=1
+ 0x44, 0x02, // fixed head: float field "a"
+ 0x61, 0x00,
+ 0x2a, // only 1 byte of the 4-byte value
+ };
+ butil::IOBuf body;
+ body.append(data, sizeof(data));
+
+ butil::IOBufAsZeroCopyInputStream zc_stream(body);
+ mcpack2pb::InputStream stream(&zc_stream);
+ ASSERT_EQ(64u, mcpack2pb::unbox(&stream));
+ mcpack2pb::ObjectIterator it(&stream, 64);
+ ASSERT_TRUE(it != NULL);
+ ASSERT_EQ(mcpack2pb::FIELD_FLOAT, it->value.type());
+ EXPECT_EQ(0, it->value.as_int64("a"));
+ EXPECT_FALSE(stream.good());
+}
+
+TEST(Mcpack2pbParserTest, StringFieldMissingTerminatorIsRejected) {
+ // The string content ("a") is fully present but the stream ends exactly
+ // before the required trailing '\0'. The terminator must be read and
+ // validated instead of being skipped with popn(1) (which returns 0 on
+ // an exhausted stream and leaves it good, accepting a truncated string).
+ const unsigned char data[] = {
+ 0x10, 0x00, 0x14, 0x00, 0x00, 0x00, // top object, value_size=20
+ 0x01, 0x00, 0x00, 0x00, // item_count=1
+ 0x50, 0x02, 0x02, 0x00, 0x00, 0x00, // string field "a", size=2
+ 0x61, 0x00,
+ 0x61, // content "a", no terminator
+ };
+ butil::IOBuf body;
+ body.append(data, sizeof(data));
+
+ butil::IOBufAsZeroCopyInputStream zc_stream(body);
+ mcpack2pb::InputStream stream(&zc_stream);
+ ASSERT_EQ(20u, mcpack2pb::unbox(&stream));
+ mcpack2pb::ObjectIterator it(&stream, 20);
+ ASSERT_TRUE(it != NULL);
+ ASSERT_EQ(mcpack2pb::FIELD_STRING, it->value.type());
+ std::string value;
+ it->value.as_string(&value, "a");
+ EXPECT_TRUE(value.empty());
+ EXPECT_FALSE(stream.good());
+}
+
+TEST(Mcpack2pbParserTest, StringFieldNonNulTerminatorIsRejected) {
+ // A string whose trailing byte is not '\0' is malformed and must be
+ // rejected as well.
+ const unsigned char data[] = {
+ 0x10, 0x00, 0x14, 0x00, 0x00, 0x00, // top object, value_size=20
+ 0x01, 0x00, 0x00, 0x00, // item_count=1
+ 0x50, 0x02, 0x02, 0x00, 0x00, 0x00, // string field "a", size=2
+ 0x61, 0x00,
+ 0x61, 0x62, // "a" with 'b' as terminator
+ };
+ butil::IOBuf body;
+ body.append(data, sizeof(data));
+
+ butil::IOBufAsZeroCopyInputStream zc_stream(body);
+ mcpack2pb::InputStream stream(&zc_stream);
+ ASSERT_EQ(20u, mcpack2pb::unbox(&stream));
+ mcpack2pb::ObjectIterator it(&stream, 20);
+ ASSERT_TRUE(it != NULL);
+ ASSERT_EQ(mcpack2pb::FIELD_STRING, it->value.type());
+ std::string value;
+ it->value.as_string(&value, "a");
+ EXPECT_TRUE(value.empty());
+ EXPECT_FALSE(stream.good());
+}
+
} // namespace
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
