Skip to content

ext/standard: Improve handling of timeout values - #16246

Open
Girgias wants to merge 1 commit into
php:masterfrom
Girgias:streamfuncs-timeout
Open

Girgias wants to merge 1 commit into
php:masterfrom
Girgias:streamfuncs-timeout

Conversation

@Girgias

@Girgias Girgias commented Oct 5, 2024

Copy link
Copy Markdown
Member

No description provided.

@Girgias

Girgias commented Oct 5, 2024

Copy link
Copy Markdown
Member Author

@cmb69 quick question, I see that suseconds_t is defined in win32/time.h for Windows, do I just need to include time.h at the beginning of the file for the definition to be found?

I'm guessing if I do <time.h> it will load the system header instead of the one from win32.

@cmb69

cmb69 commented Oct 5, 2024

Copy link
Copy Markdown
Member

@Girgias, you can use

#include "win32/time.h"

@Girgias
Girgias force-pushed the streamfuncs-timeout branch from 84861ea to e595ede Compare October 5, 2024 21:10
@Girgias
Girgias force-pushed the streamfuncs-timeout branch from e595ede to 822dc1a Compare October 6, 2024 23:21

@TimWolla TimWolla left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall this looks reasonable.

Comment on lines +830 to +834
} else if (usec >= 1000000) {
php_error_docref(NULL, E_WARNING, "must be less than 1000000");
if (UNEXPECTED(EG(exception))) {
RETURN_THROWS();
}

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can't this just be a zend_argument_value_error? We've shipped improved parameter validation all the time without an RFC and here the <0 case already throws.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know if people rely on passing a large int or not for the microseconds

zend_argument_value_error(4, "must be greater than or equal to 0");
RETURN_THROWS();
} else if (usec < 0) {
zend_argument_value_error(5, "must be greater than or equal to 0");

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could update the message to must be between 0 and 999999 to be consistent with stream_set_timeout.

Comment thread ext/standard/streamsfuncs.c Outdated

/* prepare the timeout value for use */
struct timeval *tv_pointer;
if (timeout < 0.0 || timeout >= (double) PHP_TIMEOUT_ULL_MAX / 1000000.0) {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we no longer need the overflow check?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We probably do, actually.

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants