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

the $skip and $top query options allow arithmetic overflows #578

Closed
m7milan opened this issue Dec 4, 2015 · 4 comments

Comments

Projects
None yet
3 participants
@m7milan
Copy link

commented Dec 4, 2015

example: a get request to a collection with the following query option will cause an overflow:
$top=55555555555

Looking at TopQueryOption.cs and SkipQueryOption.cs
I see that the issue is due to how the Value property is calculated. A try parse should be used, or maybe a checked block.

@xuzhg

This comment has been minimized.

Copy link
Member

commented Feb 3, 2016

@m7milan

There is an assert code as:

Contract.Assert(topValue.Value <= Int32.MaxValue);

Does the $top=55555555555 make sense?

I know the issue happens at _value = (int?)topValue;, but what result should be return after try parse?

@xuzhg xuzhg self-assigned this Feb 3, 2016

@xuzhg xuzhg added the FollowUp label Feb 3, 2016

@m7milan

This comment has been minimized.

Copy link
Author

commented Feb 3, 2016

@xuzhg
Greetings, I am looking at this SkipQueryOption.cs

Specifically the get of the Value property:
`

                try
                {
                    _value = Convert.ToInt32(RawValue, CultureInfo.InvariantCulture);
                }

`
take a look at the RawValue and Value in this screenshot

From what I can see, that should be in a checked block, or a try parse.

I refer you to section 7.6.12 of the C# specification, a portion of which I reproduce here for your convenience:

For non-constant expressions (expressions that are evaluated at run-time) that are not enclosed by any checked or unchecked operators or statements, the default overflow checking context is unchecked unless external factors (such as compiler switches and execution environment configuration) call for checked evaluation.

Does the $top=55555555555 make sense?

I know the issue happens at _value = (int?)topValue;, but what result should be return after try parse?

I would consider that a $top higher than int.maxvalue should cause an error of some sort. currently it is converting to a negative number as you can see in the screenshot above.

@xuzhg

This comment has been minimized.

Copy link
Member

commented Apr 11, 2016

  1. Throw as early as fast
  2. use MaxValue.
@VikingsFan

This comment has been minimized.

Copy link
Contributor

commented Sep 13, 2016

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.