Skip to content
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

Jump to bottom

Ensure that the hash method does not return null #46218

Merged
merged 2 commits into from
Sep 16, 2024
Merged

Ensure that the hash method does not return null #46218

merged 2 commits into from
Sep 16, 2024

Conversation

artonge
Copy link
Contributor

@artonge artonge commented Jul 1, 2024 •
edited
Loading

@artonge artonge self-assigned this Jul 1, 2024
@artonge artonge added bug 3. to review Waiting for reviews feature: files php Pull requests that update Php code labels Jul 1, 2024
@artonge artonge added this to the Nextcloud 30 milestone Jul 1, 2024
@artonge
Copy link
Contributor Author

artonge commented Jul 1, 2024

/backport to stable29

@artonge
Copy link
Contributor Author

artonge commented Jul 1, 2024

/backport to stable28

@artonge
Copy link
Contributor Author

artonge commented Jul 1, 2024

/backport to stable27

@artonge artonge force-pushed the artonge/fix/hash_return_type branch from 4896b63 to 4269166 Compare July 1, 2024 12:30
github-advanced-security[bot]
github-advanced-security bot found potential problems Jul 1, 2024
View reviewed changes
lib/private/Files/Storage/Local.php Fixed Show fixed Hide fixed
lib/private/Files/Storage/Local.php Fixed
public function hash($type, $path, $raw = false) {
return hash_file($type, $this->getSourcePath($path), $raw);
public function hash($type, $path, $raw = false): string|false {
return hash_file($type, $this->getSourcePath($path), $raw) ?? false;

Check failure

Code scanning / Psalm

TypeDoesNotContainNull Error

Cannot resolve types for $11272 - false|string does not contain null
Copy link
Contributor

Choose a reason for hiding this comment

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

https://www.php.net/manual/en/function.hash-file.php says that the method never returns null, is that wrong?

Copy link
Contributor

Choose a reason for hiding this comment

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

lib/private/Files/Storage/Wrapper/Availability.php may return null from hash().
lib/private/Security/Bruteforce/Backend/MemoryCacheBackend.php also but I am not sure it’s the same hash.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

lib/private/Files/Storage/Wrapper/Availability.php may return null from hash().

Where do you see that?

server/lib/private/Files/Storage/Wrapper/Availability.php

Lines 331 to 339 in 8dcf933

public function hash($type, $path, $raw = false) {
$this->checkAvailability();
try {
return parent::hash($type, $path, $raw);
} catch (StorageNotAvailableException $e) {
$this->setUnavailable($e);
}
}

I am also surprised that hash_file returns null even though the doc says otherwise.

lib/private/Security/Bruteforce/Backend/MemoryCacheBackend.php also but I am not sure it’s the same hash.

Indeed, I don't think this is the same hash.

Copy link
Contributor

Choose a reason for hiding this comment

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

In the catch branch there is no return

Copy link
Contributor

Choose a reason for hiding this comment

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

Do you have evidence that hash_file returns null apart from this stacktrace? If this is true it should be reported to the PHP project.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In the catch branch there is no return

Right, missed that 🙈. I pushed a fixed to add a proper return value.
I will also investigate where does the null value come from.

@artonge artonge force-pushed the artonge/fix/hash_return_type branch from 4269166 to ae8c728 Compare July 1, 2024 14:52
@solracsf
Copy link
Member

solracsf commented Jul 1, 2024

/backport to stable27

Should this be backported to (eol) 27?

@artonge artonge force-pushed the artonge/fix/hash_return_type branch from ae8c728 to 3a61867 Compare July 2, 2024 09:39
come-nc
come-nc reviewed Jul 2, 2024
View reviewed changes
lib/private/Files/Storage/Local.php Outdated
Comment on lines 406 to 408
if ($hash === null) {
return false;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

I read the C code of the PHP function and I really see no possibility for hash_file to return null.
https://github.com/php/php-src/blob/master/ext/hash/hash.c#L460
points to
https://github.com/php/php-src/blob/master/ext/hash/hash.c#L358
It returns false, string or throws.

Copy link
Contributor

Choose a reason for hiding this comment

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

I would suggest to remove this null check, only keep the fix in lib/private/Files/Storage/Wrapper/Availability.php and the strong return type here, and merge that.

@solracsf solracsf mentioned this pull request Jul 29, 2024
Closed
4 tasks
This was referenced Jul 30, 2024
Merged
Merged
This was referenced Aug 5, 2024
Merged
Merged
@skjnldsv skjnldsv mentioned this pull request Aug 13, 2024
Merged
@skjnldsv skjnldsv modified the milestones: Nextcloud 30, Nextcloud 31 Aug 14, 2024
artonge and others added 2 commits September 16, 2024 15:07
@artonge @skjnldsv
fix(files): Ensure that the hash method does not return null
6674f79 
To match https://github.com/nextcloud/server/blob/beececf66068f57c416225efcde9b44ce5c2e835/lib/private/Files/View.php#L1050

- Fix #44110

Signed-off-by: Louis Chemineau <louis@chmn.me>
@skjnldsv @susnux
chore: improve hash_file php usage in Local Storage
9acaf07 
Co-authored-by: Ferdinand Thiessen <opensource@fthiessen.de>
Signed-off-by: John Molakvoæ <skjnldsv@users.noreply.github.com>
@skjnldsv skjnldsv merged commit dde0b48 into master Sep 16, 2024
172 of 175 checks passed
@skjnldsv skjnldsv deleted the artonge/fix/hash_return_type branch September 16, 2024 15:15
This was referenced Sep 16, 2024
Open
Merged
Merged
@solracsf
Copy link
Member

/backport to stable30

@backportbot backportbot bot mentioned this pull request Sep 16, 2024
Merged
@icewind1991 icewind1991 mentioned this pull request Sep 25, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@come-nc come-nc come-nc left review comments

@icewind1991 icewind1991 icewind1991 approved these changes

@susnux susnux susnux approved these changes

@solracsf solracsf solracsf approved these changes

@skjnldsv skjnldsv skjnldsv approved these changes

Assignees

@artonge artonge

Labels
3. to review Waiting for reviews bug feature: files php Pull requests that update Php code
Projects
None yet
Milestone
Nextcloud 31
Development

Successfully merging this pull request may close these issues.

[Bug]: \OC\Files\View::hash(): Return value must be of type string|bool, null returned
6 participants
@artonge @solracsf @icewind1991 @susnux @skjnldsv @come-nc