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

Fix substraction with overflow in range request #23036

Merged
merged 1 commit into from Mar 15, 2019

Conversation

Projects
None yet
5 participants
@gterzian
Copy link
Collaborator

gterzian commented Mar 14, 2019


  • ./mach build -d does not report any errors
  • ./mach test-tidy does not report any errors
  • These changes fix #23030 (GitHub issue number if applicable)
  • There are tests for these changes OR
  • These changes do not require tests because ___

This change is Reviewable

@highfive

This comment has been minimized.

Copy link

highfive commented Mar 14, 2019

Heads up! This PR modifies the following files:

@highfive

This comment has been minimized.

Copy link

highfive commented Mar 14, 2019

warning Warning warning

  • These commits modify net code, but no tests are modified. Please consider adding a test!
@gterzian

This comment has been minimized.

Copy link
Collaborator Author

gterzian commented Mar 14, 2019

@jdm r?

Sorry I had missed your self-assignment to the issue...

@gterzian

This comment has been minimized.

Copy link
Collaborator Author

gterzian commented Mar 14, 2019

@bors-servo try=wpt

bors-servo added a commit that referenced this pull request Mar 14, 2019

Auto merge of #23036 - gterzian:fix_range_request, r=<try>
Checked arithmetics in cache range request

<!-- Please describe your changes on the following line: -->

---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `___` with appropriate data: -->
- [ ] `./mach build -d` does not report any errors
- [ ] `./mach test-tidy` does not report any errors
- [ ] These changes fix #23030 (GitHub issue number if applicable)

<!-- Either: -->
- [ ] There are tests for these changes OR
- [ ] These changes do not require tests because ___

<!-- Also, please make sure that "Allow edits from maintainers" checkbox is checked, so that we can help you if you get stuck somewhere along the way.-->

<!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/23036)
<!-- Reviewable:end -->
@bors-servo

This comment has been minimized.

Copy link
Contributor

bors-servo commented Mar 14, 2019

⌛️ Trying commit 7220a01 with merge 827af8f...

@bors-servo

This comment has been minimized.

Copy link
Contributor

bors-servo commented Mar 14, 2019

💔 Test failed - linux-rel-wpt

@gterzian gterzian force-pushed the gterzian:fix_range_request branch from 7220a01 to b907729 Mar 14, 2019

@gterzian gterzian force-pushed the gterzian:fix_range_request branch from b907729 to 79ac789 Mar 14, 2019

@gterzian gterzian changed the title Checked arithmetics in cache range request Fix substraction with overflow in range request Mar 14, 2019

@gterzian gterzian force-pushed the gterzian:fix_range_request branch 3 times, most recently from c0f2cec to 9ae43e8 Mar 14, 2019

@gterzian

This comment has been minimized.

Copy link
Collaborator Author

gterzian commented Mar 14, 2019

Ok I think I've addressed all potential overflowing operations, maybe a bit over the top for some, but better safe than sorry!

@gterzian gterzian force-pushed the gterzian:fix_range_request branch from 9ae43e8 to ab346db Mar 14, 2019

@jdm
Copy link
Member

jdm left a comment

These changes achieve the goal of removing over/underflow hazards, but lots of checked operations make the code harder to follow in my opinion. I think we can simplify some of the changes by establishing preconditions when extracting the ContentRange values like this:

match (range.bytes_range(), range.bytes_len()) {
    (Some(bytes_range), Some(total)) if total > 0 => (bytes_range.0, bytes_range.1, total),
    _ => continue,
}
_ => continue,
}
};
if res_beginning_minus_one < beginning && res_end_plus_one > end {

This comment has been minimized.

Copy link
@jdm

jdm Mar 14, 2019

Member

Instead of this block, can't we do if res_beginning <= beginning && res_end >= end?

let resource_body = &*partial_resource.body.lock().unwrap();
let requested = match resource_body {
&ResponseBody::Done(ref body) => {
let b = beginning as usize - res_beginning as usize;
let e = end as usize - res_beginning as usize + 1;
let (b, e) = {

This comment has been minimized.

Copy link
@jdm

jdm Mar 14, 2019

Member

Since we have already established that res_beginning <= beginning we do not need a checked_sub. Similarly, if we establish that end >= res_beginning then we don't need to check that operation either.

@@ -474,11 +498,22 @@ fn handle_range_request(
} else {
continue;
};
if res_beginning < beginning && res_end == total - 1 {
let total_minus_one = {

This comment has been minimized.

Copy link
@jdm

jdm Mar 14, 2019

Member

If we check that total != 0 earlier then we don't need a checked subtraction here.

let resource_body = &*partial_resource.body.lock().unwrap();
let requested = match resource_body {
&ResponseBody::Done(ref body) => {
let from_byte = beginning as usize - res_beginning as usize;
let from_byte = {
match beginning.checked_sub(res_beginning) {

This comment has been minimized.

Copy link
@jdm

jdm Mar 14, 2019

Member

Since we have established that res_beginning < beginning there is no need to check this operation.

@@ -519,7 +554,23 @@ fn handle_range_request(
} else {
continue;
};
if (total - res_beginning) > (offset - 1) && (total - res_end) < offset + 1 {
let (total_minus_res_beginning, total_minus_res_end) = {

This comment has been minimized.

Copy link
@jdm

jdm Mar 14, 2019

Member

If we check that total >= res_beginning and total >= res_end then we don't need these checked operations.

@jdm jdm assigned jdm and unassigned nox Mar 14, 2019

@gterzian gterzian force-pushed the gterzian:fix_range_request branch 4 times, most recently from 0d8753b to 21470e0 Mar 15, 2019

@gterzian gterzian force-pushed the gterzian:fix_range_request branch from 21470e0 to 28c0a0c Mar 15, 2019

@gterzian gterzian force-pushed the gterzian:fix_range_request branch from 28c0a0c to 5836bd2 Mar 15, 2019

@gterzian

This comment has been minimized.

Copy link
Collaborator Author

gterzian commented Mar 15, 2019

@jdm Ok, much simpler indeed!

@jdm

This comment has been minimized.

Copy link
Member

jdm commented Mar 15, 2019

@bors-servo

This comment has been minimized.

Copy link
Contributor

bors-servo commented Mar 15, 2019

📌 Commit 5836bd2 has been approved by jdm

@bors-servo

This comment has been minimized.

Copy link
Contributor

bors-servo commented Mar 15, 2019

⌛️ Testing commit 5836bd2 with merge 858f919...

bors-servo added a commit that referenced this pull request Mar 15, 2019

Auto merge of #23036 - gterzian:fix_range_request, r=jdm
Fix substraction with overflow in range request

<!-- Please describe your changes on the following line: -->

---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `___` with appropriate data: -->
- [ ] `./mach build -d` does not report any errors
- [ ] `./mach test-tidy` does not report any errors
- [ ] These changes fix #23030 (GitHub issue number if applicable)

<!-- Either: -->
- [ ] There are tests for these changes OR
- [ ] These changes do not require tests because ___

<!-- Also, please make sure that "Allow edits from maintainers" checkbox is checked, so that we can help you if you get stuck somewhere along the way.-->

<!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/23036)
<!-- Reviewable:end -->
@bors-servo

This comment has been minimized.

Copy link
Contributor

bors-servo commented Mar 15, 2019

@bors-servo bors-servo merged commit 5836bd2 into servo:master Mar 15, 2019

3 checks passed

Taskcluster (pull_request) TaskGroup: success
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
homu Test successful
Details

@gterzian gterzian deleted the gterzian:fix_range_request branch Mar 16, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.