Skip to content

Commit

Permalink
fix(server): Panic when last envelope item is empty (#894)
Browse files Browse the repository at this point in the history
Trailing newlines can be omitted in envelopes. When the last envelope
item was empty, the parsing code could yield two invalid results:

- If an explicit length of `0` was specified, then the envelope was
  rejected with `UnexpectedEof`.
- If implicit length was used, the parser would panic.

This can both be solved by aligning the parse offset with the end of the
buffer.
  • Loading branch information
jan-auer authored Dec 23, 2020
1 parent f5ec7a7 commit ef7699b
Show file tree
Hide file tree
Showing 2 changed files with 42 additions and 1 deletion.
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@

- Make all fields but event-id optional to fix regressions in user feedback ingestion. ([#886](https://github.com/getsentry/relay/pull/886))
- Remove `kafka-ssl` feature because it breaks development workflow on macOS. ([#889](https://github.com/getsentry/relay/pull/889))
- Accept envelopes where their last item is empty and trailing newlines are omitted. This also fixes a panic in some cases. ([#894](https://github.com/getsentry/relay/pull/894))

**Internal**:

Expand Down
42 changes: 41 additions & 1 deletion relay-server/src/envelope.rs
Original file line number Diff line number Diff line change
Expand Up @@ -892,7 +892,9 @@ impl Envelope {
let headers_end = stream.byte_offset();
Self::require_termination(slice, headers_end)?;

let payload_start = headers_end + 1;
// The last header does not require a trailing newline, so `payload_start` may point
// past the end of the buffer.
let payload_start = std::cmp::min(headers_end + 1, bytes.len());
let payload_end = match headers.length {
Some(len) => {
let payload_end = payload_start + len as usize;
Expand Down Expand Up @@ -1097,6 +1099,26 @@ mod tests {
assert_eq!(items[1].len(), 0);
}

#[test]
fn test_deserialize_envelope_empty_item_eof() {
// With terminating newline after item payload
let bytes = Bytes::from(
"\
{\"event_id\":\"9ec79c33ec9942ab8353589fcb2e04dc\",\"dsn\":\"https://e12d836b15bb49d7bbf99e64295d995b:@sentry.io/42\"}\n\
{\"type\":\"attachment\",\"length\":0}\n\
\n\
{\"type\":\"attachment\",\"length\":0}\
",
);

let envelope = Envelope::parse_bytes(bytes).unwrap();
assert_eq!(envelope.len(), 2);

let items: Vec<_> = envelope.items().collect();
assert_eq!(items[0].len(), 0);
assert_eq!(items[1].len(), 0);
}

#[test]
fn test_deserialize_envelope_implicit_length() {
// With terminating newline after item payload
Expand Down Expand Up @@ -1133,6 +1155,24 @@ mod tests {
assert_eq!(items[0].len(), 10);
}

#[test]
fn test_deserialize_envelope_implicit_length_empty_eof() {
// Empty item with implicit length ending the envelope
// Panic regression test.
let bytes = Bytes::from(
"\
{\"event_id\":\"9ec79c33ec9942ab8353589fcb2e04dc\",\"dsn\":\"https://e12d836b15bb49d7bbf99e64295d995b:@sentry.io/42\"}\n\
{\"type\":\"attachment\"}\
",
);

let envelope = Envelope::parse_bytes(bytes).unwrap();
assert_eq!(envelope.len(), 1);

let items: Vec<_> = envelope.items().collect();
assert_eq!(items[0].len(), 0);
}

#[test]
fn test_deserialize_envelope_multiple_items() {
// With terminating newline
Expand Down

0 comments on commit ef7699b

Please sign in to comment.