Skip to content
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

Take the current header truncated bit into account #1384

Merged
merged 3 commits into from Mar 5, 2021

Conversation

ilaidlaw
Copy link

We deserialize a message, examine the contents, and re-serialize it. We noticed that the new serialized message was missing the truncate bit. This patch fixes it in our case, but not sure if this is the correct approach in general. Welcome for feedback and suggestions on a different fix!

@codecov
Copy link

codecov bot commented Feb 23, 2021

Codecov Report

Merging #1384 (a1f92ac) into main (9eebdad) will increase coverage by 0.01%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main    #1384      +/-   ##
==========================================
+ Coverage   85.21%   85.22%   +0.01%     
==========================================
  Files         153      153              
  Lines       15017    15023       +6     
==========================================
+ Hits        12796    12803       +7     
+ Misses       2221     2220       -1     

@bluejekyll
Copy link
Member

Thanks for the PR.

If I remember this code correctly, the reason it was here is during traction by the server. Have you tested this locally to see if it is working as you expect?

Are you constructing a truncated request by hand?

@ilaidlaw
Copy link
Author

Yes we've added a test in our project which takes a response with the truncate bit set, then de-serializes into a Message, re-serializes and verifies the truncate bit is still set.

@djc
Copy link
Collaborator

djc commented Mar 1, 2021

It would be good to make sure we have test coverage for that in the trust-dns test suite, as well.

@ilaidlaw
Copy link
Author

ilaidlaw commented Mar 3, 2021

I tweaked the tests to use the truncate flag, let me know if that is sufficient.

@bluejekyll
Copy link
Member

Thanks for the PR, merging. The cleanliness failure will be fixed OOB.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants