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
base: master
from

Conversation

Projects
None yet
2 participants
@bagder
Member

bagder commented Oct 3, 2017

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

@mkauf

This comment has been minimized.

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

This comment has been minimized.

Member

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

This comment has been minimized.

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

This comment has been minimized.

Member

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

This comment has been minimized.

Member

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 Oct 5, 2017

@bagder

This comment has been minimized.

Member

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

This comment has been minimized.

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

This comment has been minimized.

Member

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

This comment has been minimized.

Member

bagder commented Oct 5, 2017

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

bagder added some commits Oct 3, 2017

setopt: avoid integer overflows when setting millsecond values
... 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.
setopt: range check most long options
... 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 Oct 14, 2017

@bagder

This comment has been minimized.

Member

bagder commented Oct 14, 2017

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

bagder added some commits Oct 14, 2017

@bagder bagder closed this in 172ce9c Oct 16, 2017

@bagder bagder deleted the bagder/setopt-ms-integer-overflows branch Oct 16, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment