Skip to content

Commit

Permalink
usm: http2: fix index calculation (#20931)
Browse files Browse the repository at this point in the history
* usm: http2: Change type from u8 to u64, to allow indexes bigger than 255

* usm: http2: Add missing parenthesis

That bug cause wrong order or execution and lead into truncation

* usm: http2: Added test
  • Loading branch information
guyarb authored Nov 18, 2023
1 parent 35dc55b commit adf3b79
Show file tree
Hide file tree
Showing 2 changed files with 64 additions and 11 deletions.
22 changes: 11 additions & 11 deletions pkg/network/ebpf/c/protocols/http2/decoding.h
Original file line number Diff line number Diff line change
Expand Up @@ -43,18 +43,18 @@ static __always_inline http2_stream_t *http2_fetch_stream(const http2_stream_key
}

// Similar to read_var_int, but with a small optimization of getting the current character as input argument.
static __always_inline bool read_var_int_with_given_current_char(struct __sk_buff *skb, skb_info_t *skb_info, __u8 current_char_as_number, __u8 max_number_for_bits, __u8 *out) {
static __always_inline bool read_var_int_with_given_current_char(struct __sk_buff *skb, skb_info_t *skb_info, __u64 current_char_as_number, __u64 max_number_for_bits, __u64 *out) {
current_char_as_number &= max_number_for_bits;

if (current_char_as_number < max_number_for_bits) {
*out = current_char_as_number;
return true;
}

__u8 next_char = 0;
if (bpf_skb_load_bytes(skb, skb_info->data_off, &next_char, sizeof(next_char)) >=0 && (next_char & 128) == 0) {
__u64 next_char = 0;
if (bpf_skb_load_bytes(skb, skb_info->data_off, &next_char, 1) >=0 && (next_char & 128) == 0) {
skb_info->data_off++;
*out = current_char_as_number + next_char & 127;
*out = current_char_as_number + (next_char & 127);
return true;
}

Expand All @@ -68,9 +68,9 @@ static __always_inline bool read_var_int_with_given_current_char(struct __sk_buf
// n must always be between 1 and 8.
//
// The returned remain buffer is either a smaller suffix of p, or err != nil.
static __always_inline bool read_var_int(struct __sk_buff *skb, skb_info_t *skb_info, __u8 max_number_for_bits, __u8 *out) {
__u8 current_char_as_number = 0;
if (bpf_skb_load_bytes(skb, skb_info->data_off, &current_char_as_number, sizeof(current_char_as_number)) < 0) {
static __always_inline bool read_var_int(struct __sk_buff *skb, skb_info_t *skb_info, __u64 max_number_for_bits, __u64 *out) {
__u64 current_char_as_number = 0;
if (bpf_skb_load_bytes(skb, skb_info->data_off, &current_char_as_number, 1) < 0) {
return false;
}
skb_info->data_off++;
Expand Down Expand Up @@ -119,8 +119,8 @@ READ_INTO_BUFFER(path, HTTP2_MAX_PATH_LEN, BLK_SIZE)

// parse_field_literal handling the case when the key is part of the static table and the value is a dynamic string
// which will be stored in the dynamic table.
static __always_inline bool parse_field_literal(struct __sk_buff *skb, skb_info_t *skb_info, http2_header_t *headers_to_process, __u8 index, __u64 global_dynamic_counter, __u8 *interesting_headers_counter) {
__u8 str_len = 0;
static __always_inline bool parse_field_literal(struct __sk_buff *skb, skb_info_t *skb_info, http2_header_t *headers_to_process, __u64 index, __u64 global_dynamic_counter, __u8 *interesting_headers_counter) {
__u64 str_len = 0;
if (!read_var_int(skb, skb_info, MAX_6_BITS, &str_len)) {
return false;
}
Expand Down Expand Up @@ -162,8 +162,8 @@ static __always_inline __u8 filter_relevant_headers(struct __sk_buff *skb, skb_i
const __u32 end = frame_end < skb_info->data_end + 1 ? frame_end : skb_info->data_end + 1;
bool is_literal = false;
bool is_indexed = false;
__u8 max_bits = 0;
__u8 index = 0;
__u64 max_bits = 0;
__u64 index = 0;

__u64 *global_dynamic_counter = get_dynamic_counter(tup);
if (global_dynamic_counter == NULL) {
Expand Down
53 changes: 53 additions & 0 deletions pkg/network/usm/monitor_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -539,6 +539,59 @@ func TestHTTP2(t *testing.T) {
})
}

func (s *USMHTTP2Suite) TestHTTP2ManyDifferentPaths() {
t := s.T()
cfg := networkconfig.New()
cfg.EnableHTTP2Monitoring = true

startH2CServer(t)

monitor, err := NewMonitor(cfg, nil, nil, nil)
require.NoError(t, err)
require.NoError(t, monitor.Start())
defer monitor.Stop()

// Should be bigger than the length of the http2_dynamic_table which is 1024
numberOfRequests := 1500
clients := getClientsArray(t, 1)
for i := 0; i < numberOfRequests; i++ {
for j := 0; j < 2; j++ {
req, err := clients[0].Post(fmt.Sprintf("%s/test-%d", http2SrvAddr, i+1), "application/json", bytes.NewReader([]byte("test")))
require.NoError(t, err, "could not make request")
req.Body.Close()
}
}

matches := PrintableInt(0)

seenRequests := map[string]int{}
assert.Eventuallyf(t, func() bool {
stats := monitor.GetProtocolStats()
http2Stats, ok := stats[protocols.HTTP2]
if !ok {
return false
}
http2StatsTyped := http2Stats.(map[http.Key]*http.RequestStats)
for key, stat := range http2StatsTyped {
if (key.DstPort == http2SrvPort || key.SrcPort == http2SrvPort) && key.Method == http.MethodPost && strings.HasPrefix(key.Path.Content.Get(), "/test") {
if _, ok := seenRequests[key.Path.Content.Get()]; !ok {
seenRequests[key.Path.Content.Get()] = 0
}
seenRequests[key.Path.Content.Get()] += stat.Data[200].Count
matches.Add(stat.Data[200].Count)
}
}

return matches.Load() == 2*numberOfRequests
}, time.Second*10, time.Millisecond*100, "%v != %v", &matches, 2*numberOfRequests)

for i := 0; i < numberOfRequests; i++ {
if v, ok := seenRequests[fmt.Sprintf("/test-%d", i+1)]; !ok || v != 2 {
t.Logf("path: /test-%d should have 2 occurrences but instead has %d", i+1, v)
}
}
}

func (s *USMHTTP2Suite) TestSimpleHTTP2() {
t := s.T()
cfg := networkconfig.New()
Expand Down

0 comments on commit adf3b79

Please sign in to comment.