Skip to content

Fix GH-13984: Buffer size is now checked before memcmp #13991

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.

Sign up for GitHub

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

Closed
wants to merge 5 commits into from
Closed

Fix GH-13984: Buffer size is now checked before memcmp #13991

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
e93c35b
Buffer size is now checked before memcmp
SakiTakamachi Apr 17, 2024
28a51dc
address comments
SakiTakamachi Apr 17, 2024
15e34dd
fixed test
SakiTakamachi Apr 17, 2024
d248a60
remove `-1`
SakiTakamachi Apr 17, 2024
5b64ffe
use strcmp
SakiTakamachi Apr 17, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion ext/pdo_sqlite/sqlite_driver.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -751,7 +751,7 @@ static char *make_filename_safe(const char *filename)
}
return estrdup(filename);
}
if (*filename && memcmp(filename, ":memory:", sizeof(":memory:"))) {
if (*filename && zend_binary_strcmp(filename, strlen(filename)-1, ":memory:", strlen(":memory:")-1)) {
Copy link
Member

Choose a reason for hiding this comment

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

I know very little pdo sqlite, but I do not get why -1 for the 2 strlen ? Legit question :)

Copy link
Member

Choose a reason for hiding this comment

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

Subtracting 1 from strlen is not necessary, the subtraction of 1 from sizeof is because sizeof also counts the NULL terminator but strlen does not.

Copy link
Member

Choose a reason for hiding this comment

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

I was thinking about that same kind of confusion, just wanted to confirm.

Copy link
Member Author

Choose a reason for hiding this comment

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

It seems like I'm a little too sleepy....Sorry for making me check again and again. (AM 1:43 JST)

Copy link
Member

Choose a reason for hiding this comment

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

it s ok Saki :) relax, as nielsdos mentioned, an "attacker" can t do anything really malicious here. no rush :)

char *fullpath = expand_filepath(filename, NULL);

if (!fullpath) {
Expand Down
16 changes: 16 additions & 0 deletions ext/pdo_sqlite/tests/gh13991.phpt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
--TEST--
Fix GH-13984: Buffer size is now checked before memcmp
--EXTENSIONS--
pdo_sqlite
--FILE--
<?php
$dbfile = 'z.db';
Copy link
Member Author

@SakiTakamachi SakiTakamachi Apr 17, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I did this because specifying the absolute path would make the string longer and not be overloaded.

Copy link
Member

Choose a reason for hiding this comment

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

Hm right... This can clash with local files though, so perhaps it's better to use 13991 as filename?

$db = new PDO('sqlite:' . $dbfile, null, null, [PDO::ATTR_PERSISTENT => true]);
echo 'done!';
?>
--CLEAN--
<?php
@unlink(getcwd() . '/z.db');
Copy link
Member Author

Choose a reason for hiding this comment

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

Is there a better way?

Copy link
Member

Choose a reason for hiding this comment

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

We should be careful somehow that we don't destroy files or users running the tests.

Copy link
Member Author

Choose a reason for hiding this comment

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

Changed to skip if a file with the same name already exists.

?>
--EXPECT--
done!