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 1 commit
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 && (sizeof(filename) != sizeof(":memory:") || memcmp(filename, ":memory:", sizeof(":memory:")) != 0)) {
Copy link
Member Author

Choose a reason for hiding this comment

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

)))) was too hard to see, so I added != 0. The meaning remains the same.

Copy link
Member

Choose a reason for hiding this comment

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

This is wrong, sizeof(filename) is not the length, it is the size of the pointer.
I also think the != 0 addition is wrong for memcmp?

Copy link
Member

Choose a reason for hiding this comment

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

why bothering with memcmp anyway ? why not just any str*cmp ?

Copy link
Member Author

Choose a reason for hiding this comment

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

@nielsdos

This is wrong, sizeof(filename) is not the length, it is the size of the pointer.

Right. I entered sizeof with the intention of strlen...

I also think the != 0 addition is wrong for memcmp?

Since it becomes true when 1 and -1, it seems to be the same because it becomes true when != 0, but am I wrong?

@devnexen

why bothering with memcmp anyway ? why not just any str*cmp ?

I thought I had to convert it to zend_string, but there were some that could be used as char, so I fixed it.

Copy link
Member

Choose a reason for hiding this comment

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

Right, I misread the original condition. So !=0 is fine.

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!