-
Notifications
You must be signed in to change notification settings - Fork 7.9k
ext/pdo: Refactor validation of fetch mode in PDO statement #17699
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
Conversation
249ece9
to
6440b24
Compare
|
||
if (mode < 0 || mode >= PDO_FETCH__MAX) { | ||
/* Mode must be a positive */ | ||
if (mode_and_flags < 0 || mode_and_flags >= PDO_FIRST_INVALID_FLAG) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think this change is right. What if mode is equal to 15
? This number doesn't correspond to a real mode but it will not trigger the mode_and_flags >= PDO_FIRST_INVALID_FLAG
condition.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right, I'll add a test for this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay, just this test then and then it's good to go.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually this is properly guarded by the switch (mode) {
below, but the test just ensure this continues to be the case.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, now we know for sure!
This affects the value of the fetch mode flags
Co-authored-by: Niels Dossche <7771979+nielsdos@users.noreply.github.com>
492357e
to
0cf1e6b
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The change of value should probably get an upgrading entry. I hope no one is (implicitly) relying on those
No description provided.