Skip to content

Commit

Permalink
[chttp2] Backport to 1.55.x: Fix some fuzzer found bugs (#33016)
Browse files Browse the repository at this point in the history 
Backport #33005

Co-authored-by: Craig Tiller <ctiller@google.com>
  • Loading branch information
@veblush @ctiller
veblush and ctiller committed May 4, 2023
1 parent bf90257 commit 5d3e6e3
Show file tree
Hide file tree
Showing 42 changed files with 1,834 additions and 526 deletions.
20 changes: 12 additions & 8 deletions src/core/ext/transport/chttp2/transport/bin_encoder.cc
View file
Expand Up @@ -149,7 +149,8 @@ static void enc_flush_some(huff_out* out) {
}
}

static void enc_add2(huff_out* out, uint8_t a, uint8_t b) {
static void enc_add2(huff_out* out, uint8_t a, uint8_t b, uint32_t* wire_size) {
*wire_size += 2;
b64_huff_sym sa = huff_alphabet[a];
b64_huff_sym sb = huff_alphabet[b];
out->temp = (out->temp << (sa.length + sb.length)) |
Expand All @@ -159,15 +160,16 @@ static void enc_add2(huff_out* out, uint8_t a, uint8_t b) {
enc_flush_some(out);
}

static void enc_add1(huff_out* out, uint8_t a) {
static void enc_add1(huff_out* out, uint8_t a, uint32_t* wire_size) {
*wire_size += 1;
b64_huff_sym sa = huff_alphabet[a];
out->temp = (out->temp << sa.length) | sa.bits;
out->temp_length += sa.length;
enc_flush_some(out);
}

grpc_slice grpc_chttp2_base64_encode_and_huffman_compress(
const grpc_slice& input) {
const grpc_slice& input, uint32_t* wire_size) {
size_t input_length = GRPC_SLICE_LENGTH(input);
size_t input_triplets = input_length / 3;
size_t tail_case = input_length % 3;
Expand All @@ -183,16 +185,17 @@ grpc_slice grpc_chttp2_base64_encode_and_huffman_compress(
out.temp = 0;
out.temp_length = 0;
out.out = start_out;
*wire_size = 0;

// encode full triplets
for (i = 0; i < input_triplets; i++) {
const uint8_t low_to_high = static_cast<uint8_t>((in[0] & 0x3) << 4);
const uint8_t high_to_low = in[1] >> 4;
enc_add2(&out, in[0] >> 2, low_to_high | high_to_low);
enc_add2(&out, in[0] >> 2, low_to_high | high_to_low, wire_size);

const uint8_t a = static_cast<uint8_t>((in[1] & 0xf) << 2);
const uint8_t b = (in[2] >> 6);
enc_add2(&out, a | b, in[2] & 0x3f);
enc_add2(&out, a | b, in[2] & 0x3f, wire_size);
in += 3;
}

Expand All @@ -201,14 +204,15 @@ grpc_slice grpc_chttp2_base64_encode_and_huffman_compress(
case 0:
break;
case 1:
enc_add2(&out, in[0] >> 2, static_cast<uint8_t>((in[0] & 0x3) << 4));
enc_add2(&out, in[0] >> 2, static_cast<uint8_t>((in[0] & 0x3) << 4),
wire_size);
in += 1;
break;
case 2: {
const uint8_t low_to_high = static_cast<uint8_t>((in[0] & 0x3) << 4);
const uint8_t high_to_low = in[1] >> 4;
enc_add2(&out, in[0] >> 2, low_to_high | high_to_low);
enc_add1(&out, static_cast<uint8_t>((in[1] & 0xf) << 2));
enc_add2(&out, in[0] >> 2, low_to_high | high_to_low, wire_size);
enc_add1(&out, static_cast<uint8_t>((in[1] & 0xf) << 2), wire_size);
in += 2;
break;
}
Expand Down
6 changes: 5 additions & 1 deletion src/core/ext/transport/chttp2/transport/bin_encoder.h
View file
Expand Up @@ -21,6 +21,8 @@

#include <grpc/support/port_platform.h>

#include <stdint.h>

#include <grpc/slice.h>

// base64 encode a slice. Returns a new slice, does not take ownership of the
Expand All @@ -36,7 +38,9 @@ grpc_slice grpc_chttp2_huffman_compress(const grpc_slice& input);
// grpc_slice y = grpc_chttp2_huffman_compress(x);
// grpc_core::CSliceUnref( x);
// return y;
// *wire_size is the length of the base64 encoded string prior to huffman
// compression (as is needed for hpack table math)
grpc_slice grpc_chttp2_base64_encode_and_huffman_compress(
const grpc_slice& input);
const grpc_slice& input, uint32_t* wire_size);

#endif // GRPC_SRC_CORE_EXT_TRANSPORT_CHTTP2_TRANSPORT_BIN_ENCODER_H
73 changes: 47 additions & 26 deletions src/core/ext/transport/chttp2/transport/hpack_encoder.cc
View file
Expand Up @@ -131,21 +131,34 @@ struct WireValue {
: data(std::move(slice)),
huffman_prefix(huffman_prefix),
insert_null_before_wire_value(insert_null_before_wire_value),
length(data.length() + (insert_null_before_wire_value ? 1 : 0)) {}
length(data.length() + (insert_null_before_wire_value ? 1 : 0)),
hpack_length(length) {}
WireValue(uint8_t huffman_prefix, bool insert_null_before_wire_value,
Slice slice, size_t hpack_length)
: data(std::move(slice)),
huffman_prefix(huffman_prefix),
insert_null_before_wire_value(insert_null_before_wire_value),
length(data.length() + (insert_null_before_wire_value ? 1 : 0)),
hpack_length(hpack_length + (insert_null_before_wire_value ? 1 : 0)) {}
Slice data;
const uint8_t huffman_prefix;
const bool insert_null_before_wire_value;
const size_t length;
const size_t hpack_length;
};

// Construct a wire value from a slice.
// true_binary_enabled => use the true binary system
// is_bin_hdr => the header is -bin suffixed
WireValue GetWireValue(Slice value, bool true_binary_enabled, bool is_bin_hdr) {
if (is_bin_hdr) {
if (true_binary_enabled) {
return WireValue(0x00, true, std::move(value));
} else {
return WireValue(0x80, false,
Slice(grpc_chttp2_base64_encode_and_huffman_compress(
value.c_slice())));
uint32_t hpack_length;
Slice output(grpc_chttp2_base64_encode_and_huffman_compress(
value.c_slice(), &hpack_length));
return WireValue(0x80, false, std::move(output), hpack_length);
}
} else {
// TODO(ctiller): opportunistically compress non-binary headers
Expand Down Expand Up @@ -185,6 +198,8 @@ class BinaryStringValue {

Slice data() { return std::move(wire_value_.data); }

uint32_t hpack_length() { return wire_value_.hpack_length; }

private:
WireValue wire_value_;
VarintWriter<1> len_val_;
Expand Down Expand Up @@ -232,14 +247,21 @@ void Encoder::EmitIndexed(uint32_t elem_index) {
w.Write(0x80, output_.AddTiny(w.length()));
}

void Encoder::EmitLitHdrWithNonBinaryStringKeyIncIdx(Slice key_slice,
Slice value_slice) {
uint32_t Encoder::EmitLitHdrWithNonBinaryStringKeyIncIdx(Slice key_slice,
Slice value_slice) {
auto key_len = key_slice.length();
auto value_len = value_slice.length();
StringKey key(std::move(key_slice));
key.WritePrefix(0x40, output_.AddTiny(key.prefix_length()));
output_.Append(key.key());
NonBinaryStringValue emit(std::move(value_slice));
emit.WritePrefix(output_.AddTiny(emit.prefix_length()));
// Allocate an index in the hpack table for this newly emitted entry.
// (we do so here because we know the length of the key and value)
uint32_t index = compressor_->table_.AllocateIndex(
key_len + value_len + hpack_constants::kEntryOverhead);
output_.Append(emit.data());
return index;
}

void Encoder::EmitLitHdrWithBinaryStringKeyNotIdx(Slice key_slice,
Expand All @@ -252,14 +274,20 @@ void Encoder::EmitLitHdrWithBinaryStringKeyNotIdx(Slice key_slice,
output_.Append(emit.data());
}

void Encoder::EmitLitHdrWithBinaryStringKeyIncIdx(Slice key_slice,
Slice value_slice) {
uint32_t Encoder::EmitLitHdrWithBinaryStringKeyIncIdx(Slice key_slice,
Slice value_slice) {
auto key_len = key_slice.length();
StringKey key(std::move(key_slice));
key.WritePrefix(0x40, output_.AddTiny(key.prefix_length()));
output_.Append(key.key());
BinaryStringValue emit(std::move(value_slice), use_true_binary_metadata_);
emit.WritePrefix(output_.AddTiny(emit.prefix_length()));
// Allocate an index in the hpack table for this newly emitted entry.
// (we do so here because we know the length of the key and value)
uint32_t index = compressor_->table_.AllocateIndex(
key_len + emit.hpack_length() + hpack_constants::kEntryOverhead);
output_.Append(emit.data());
return index;
}

void Encoder::EmitLitHdrWithBinaryStringKeyNotIdx(uint32_t key_index,
Expand Down Expand Up @@ -308,8 +336,7 @@ void SliceIndex::EmitTo(absl::string_view key, const Slice& value,
encoder->EmitIndexed(table.DynamicIndex(it->index));
} else {
// Not current, emit a new literal and update the index.
it->index = table.AllocateIndex(transport_length);
encoder->EmitLitHdrWithNonBinaryStringKeyIncIdx(
it->index = encoder->EmitLitHdrWithNonBinaryStringKeyIncIdx(
Slice::FromStaticString(key), value.Ref());
}
// Bubble this entry up if we can - ensures that the most used values end
Expand All @@ -327,9 +354,8 @@ void SliceIndex::EmitTo(absl::string_view key, const Slice& value,
prev = it;
}
// No hit, emit a new literal and add it to the index.
uint32_t index = table.AllocateIndex(transport_length);
encoder->EmitLitHdrWithNonBinaryStringKeyIncIdx(Slice::FromStaticString(key),
value.Ref());
uint32_t index = encoder->EmitLitHdrWithNonBinaryStringKeyIncIdx(
Slice::FromStaticString(key), value.Ref());
values_.emplace_back(value.Ref(), index);
}

Expand Down Expand Up @@ -386,7 +412,7 @@ void Compressor<HttpStatusMetadata, HttpStatusCompressor>::EncodeWith(
if (GPR_LIKELY(index != 0)) {
encoder->EmitIndexed(index);
} else {
encoder->EmitLitHdrWithNonBinaryStringKeyIncIdx(
encoder->EmitLitHdrWithNonBinaryStringKeyNotIdx(
Slice::FromStaticString(":status"), Slice::FromInt64(status));
}
}
Expand Down Expand Up @@ -414,13 +440,12 @@ void Compressor<HttpMethodMetadata, HttpMethodCompressor>::EncodeWith(
}

void Encoder::EncodeAlwaysIndexed(uint32_t* index, absl::string_view key,
Slice value, size_t transport_length) {
Slice value, size_t) {
if (compressor_->table_.ConvertableToDynamicIndex(*index)) {
EmitIndexed(compressor_->table_.DynamicIndex(*index));
} else {
*index = compressor_->table_.AllocateIndex(transport_length);
EmitLitHdrWithNonBinaryStringKeyIncIdx(Slice::FromStaticString(key),
std::move(value));
*index = EmitLitHdrWithNonBinaryStringKeyIncIdx(
Slice::FromStaticString(key), std::move(value));
}
}

Expand All @@ -431,10 +456,8 @@ void Encoder::EncodeIndexedKeyWithBinaryValue(uint32_t* index,
EmitLitHdrWithBinaryStringKeyNotIdx(
compressor_->table_.DynamicIndex(*index), std::move(value));
} else {
*index = compressor_->table_.AllocateIndex(key.length() + value.length() +
hpack_constants::kEntryOverhead);
EmitLitHdrWithBinaryStringKeyIncIdx(Slice::FromStaticString(key),
std::move(value));
*index = EmitLitHdrWithBinaryStringKeyIncIdx(Slice::FromStaticString(key),
std::move(value));
}
}

Expand Down Expand Up @@ -474,11 +497,9 @@ void TimeoutCompressorImpl::EncodeWith(absl::string_view key,
previous_timeouts_.pop_back();
}
Slice encoded = timeout.Encode();
uint32_t index = table.AllocateIndex(key.length() + encoded.length() +
hpack_constants::kEntryOverhead);
uint32_t index = encoder->EmitLitHdrWithNonBinaryStringKeyIncIdx(
Slice::FromStaticString(key), std::move(encoded));
previous_timeouts_.push_back(PreviousTimeout{timeout, index});
encoder->EmitLitHdrWithNonBinaryStringKeyIncIdx(Slice::FromStaticString(key),
std::move(encoded));
}

Encoder::Encoder(HPackCompressor* compressor, bool use_true_binary_metadata,
Expand Down
16 changes: 8 additions & 8 deletions src/core/ext/transport/chttp2/transport/hpack_encoder.h
View file
Expand Up @@ -62,9 +62,12 @@ class Encoder {

void AdvertiseTableSizeChange();
void EmitIndexed(uint32_t index);
void EmitLitHdrWithNonBinaryStringKeyIncIdx(Slice key_slice,
Slice value_slice);
void EmitLitHdrWithBinaryStringKeyIncIdx(Slice key_slice, Slice value_slice);
GRPC_MUST_USE_RESULT
uint32_t EmitLitHdrWithNonBinaryStringKeyIncIdx(Slice key_slice,
Slice value_slice);
GRPC_MUST_USE_RESULT
uint32_t EmitLitHdrWithBinaryStringKeyIncIdx(Slice key_slice,
Slice value_slice);
void EmitLitHdrWithBinaryStringKeyNotIdx(Slice key_slice, Slice value_slice);
void EmitLitHdrWithBinaryStringKeyNotIdx(uint32_t key_index,
Slice value_slice);
Expand Down Expand Up @@ -234,12 +237,9 @@ class Compressor<MetadataTrait, SmallIntegralValuesCompressor<N>> {
}
auto key = Slice::FromStaticString(MetadataTrait::key());
auto encoded_value = MetadataTrait::Encode(value);
size_t transport_length =
key.length() + encoded_value.length() + hpack_constants::kEntryOverhead;
if (index != nullptr) {
*index = table.AllocateIndex(transport_length);
encoder->EmitLitHdrWithNonBinaryStringKeyIncIdx(std::move(key),
std::move(encoded_value));
*index = encoder->EmitLitHdrWithNonBinaryStringKeyIncIdx(
std::move(key), std::move(encoded_value));
} else {
encoder->EmitLitHdrWithNonBinaryStringKeyNotIdx(std::move(key),
std::move(encoded_value));
Expand Down
2 changes: 2 additions & 0 deletions src/core/ext/transport/chttp2/transport/hpack_encoder_table.cc
View file
Expand Up @@ -23,6 +23,8 @@
namespace grpc_core {

uint32_t HPackEncoderTable::AllocateIndex(size_t element_size) {
GPR_DEBUG_ASSERT(element_size >= 32);

uint32_t new_index = tail_remote_index_ + table_elems_ + 1;
GPR_DEBUG_ASSERT(element_size <= MaxEntrySize());

Expand Down
2 changes: 2 additions & 0 deletions src/core/ext/transport/chttp2/transport/hpack_encoder_table.h
View file
Expand Up @@ -50,6 +50,8 @@ class HPackEncoderTable {
uint32_t max_size() const { return max_table_size_; }
// Get the current table size
uint32_t test_only_table_size() const { return table_size_; }
// Get the number of entries in the table
uint32_t test_only_table_elems() const { return table_elems_; }

// Convert an element index into a dynamic index
uint32_t DynamicIndex(uint32_t index) const {
Expand Down

0 comments on commit 5d3e6e3

Please sign in to comment.