-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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.
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
correctly update ContentLength for uncompressed response body #498
Conversation
Signed-off-by: ruitianzhong <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you add a unit test?
* fix the wrong Content-Length in 952282712204824.bin, which leads to , and this test should fail but actually not because of there is not detection in unit test for it. * add unit test for gzipped http body and add detection for it. Signed-off-by: ruitianzhong <[email protected]>
cat 952253698002000.bin | gunzip
With all steps above, it can detect all related bugs on version that is only added unit test related code with the bug not fixed. === RUN TestEventProcessor_Serve
processor_test.go:81: 2024/03/01 20:00:21 [http response] DumpResponse error: unexpected EOF
processor_test.go:81: 2024/03/01 20:00:21 [http response] DumpResponse error: http: ContentLength=1145 with Body length 2443
processor_test.go:89: some errors occurred
--- FAIL: TestEventProcessor_Serve (3.00s) It can be verified through my demo branch https://github.com/ruitianzhong/ecapture/tree/demo
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
PTAL
Further Explanation On previous commit(before this PR): When running: wget -d --header 'Accept-Encoding: gzip' https://1.1.1.1 It works well and no However, when running: wget -d --header 'Accept-Encoding: gzip' https://www.baidu.com
After further investigation, I found that it's because of the response structure. for baidu, its response header looks like
for 1.1.1.1, its response header looks like:
Obviously, Content-Length is different. According to the HTTP/1.1 standard, with // ContentLength records the length of the associated content. The
// value -1 indicates that the length is unknown. Unless Request.Method
// is "HEAD", values >= 0 indicate that the given number of bytes may
// be read from Body.
ContentLength [int64](https://pkg.go.dev/builtin#int64) So However, when the Content-Length is set like first example, if we just uncompressed the response body without adjusting the Content-Length, error would be returned from Just to give more context info here. Thanks! |
LGTM, thanks. In fact, when a larger string is called with SSL_Write, it will be split into multiple fragments and SSL_Write will be called multiple times. Moreover, there are concurrent scenarios in the upper-level business logic. Therefore, a better solution is to use the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
merged
I agree with you. There exists a semantic gap between eCapture and the application which means that eCapture might not fully understand the application-specific behavior (like parsing different interleaving protocol or concurrent scenario). When that really happen, some errors occur which is expected though. |
fix the bug that is found when reproducing in issue #491 .
When Content-Encoding is
gzip
, the uncompressed response body length is different from the compressed one. So the Content-Length need to be updated to stay consistent with the uncompressed body length.More detail in #491 (comment)