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

setopt: avoid integer overflows when setting millsecond values #1938

Closed
wants to merge 4 commits into from

Conversation

bagder
Copy link
Member

@bagder bagder commented Oct 3, 2017

... that are multiplied by 1000 when stored. Also ignore negative timeout values.

@mkauf
Copy link
Contributor

mkauf commented Oct 3, 2017

Also ignore negative timeout values.

That may affect the backwards compatibility...

I think we should check for negative timeout values in the code:

  • replace all "if(data->set.timeout)" with "if(data->set.timeout > 0)"
  • replace all "if(data->set.connecttimeout)" with "if(data->set.connecttimeout > 0)"

@bagder
Copy link
Member Author

bagder commented Oct 3, 2017

(weird CI failures)

That may affect the backwards compatibility

You mean for application that on purpose set one of these values to a negative value? How would it otherwise affect existing apps?

Since these options are documented to be time-out values, in milliseconds, my view is that we have implied that negative values are not ok. I should probably amend this commit by clarifying that in the docs for them.

@mkauf
Copy link
Contributor

mkauf commented Oct 4, 2017

You mean for application that on purpose set one of these values to a negative value?

Yes. Maybe an application does not use a fixed timeout value, but calculates it in a weird way, so that it may get negative. This negative timeout should replace/disable the existing timeout.

@bagder
Copy link
Member Author

bagder commented Oct 4, 2017

But that's asking for bug or side-effect compatibility since setting a negative timeout has never been explicitly supported. I don't think we need to be very concerned about that, at least not as long as they are mostly theoretical edge cases.

@bagder
Copy link
Member Author

bagder commented Oct 4, 2017

One could argue that we help those wrong-doing applications by returning an error code for this case...

@bagder bagder force-pushed the bagder/setopt-ms-integer-overflows branch from 18e3d6d to 4635f2e Compare October 5, 2017 12:03
@bagder
Copy link
Member Author

bagder commented Oct 5, 2017

Refreshed, now with updated man pages and the commit message now mentions that the max timeout time these support is slightly over 596 hours on 32 bit long systems.

@mkauf
Copy link
Contributor

mkauf commented Oct 5, 2017

I think it's important that libcurl behaves in a consistent way regarding negative timeouts, so it should also reject them for CURLOPT_ACCEPTTIMEOUT_MS, CURLOPT_CONNECTTIMEOUT_MS, CURLOPT_EXPECT_100_TIMEOUT_MS, CURLOPT_TIMEOUT_MS

@bagder
Copy link
Member Author

bagder commented Oct 5, 2017

Agreed! While my initial focus was to address the undefined behavior, I quite agree that inconsistency here isn't very good and also accepting bad values is also not good. I'll amend this...

@bagder
Copy link
Member Author

bagder commented Oct 5, 2017

(I will probably try to land #1944 first, then rebase this work. To minimize the necessary conflict resolution work...)

... that are multiplied by 1000 when stored.

For 32 bit long systems, the max value accepted (2147483 seconds) is >
596 hours which is unlikely to ever be set by a legitimate application -
and previously it didn't work either, it just caused undefined behavior.

Also updated the man pages for these timeout options to mention the
return code.
... filter early instead of risking "funny values" having to be dealt
with elsewhere.
@bagder bagder force-pushed the bagder/setopt-ms-integer-overflows branch from 4635f2e to 14d9b4e Compare October 14, 2017 15:21
@bagder
Copy link
Member Author

bagder commented Oct 14, 2017

Or maybe this one first, then redo the #1944 ...

@bagder bagder closed this in 172ce9c Oct 16, 2017
@bagder bagder deleted the bagder/setopt-ms-integer-overflows branch October 16, 2017 08:11
@lock lock bot locked as resolved and limited conversation to collaborators Jan 18, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants