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.

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

validate resource's integrity before using it #14883

Merged
merged 1 commit into from
Mar 16, 2015

Conversation

georgehrke
Copy link
Contributor

fixes #14036

please review @oparoz @MorrisJobke

@georgehrke georgehrke force-pushed the imagepng_expects_resource_issue branch from 7176a92 to 39d9023 Compare March 13, 2015 16:25
@MorrisJobke MorrisJobke modified the milestones: 8.0.4-next-maintenance, 8.1-current Mar 13, 2015
@DeepDiver1975
Copy link
Member

@georgehrke looks like you missed the memo: b533aad#diff-6a3371457528722a734f3c51d9238c13

🙊

please add a unit test - THX

@georgehrke
Copy link
Contributor Author

Missed that 🙈
Will add the test tomorrow

@DeepDiver1975
Copy link
Member

Will add the test tomorrow

thx

@georgehrke georgehrke force-pushed the imagepng_expects_resource_issue branch from 39d9023 to 1a5de00 Compare March 14, 2015 20:34
@ghost
Copy link

ghost commented Mar 14, 2015

Refer to this link for build results (access rights to CI server needed):
https://ci.owncloud.org//job/pull-request-analyser-ng-simple/10457/
Test PASSed.

@MorrisJobke
Copy link
Contributor

👍

*/
function data() {
if (!is_resource($this->resource)) {
$this->logger->debug('OC_Image->data. Not a resource', array('app' => 'core'));
Copy link
Member

Choose a reason for hiding this comment

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

debug messages like this (and others in this file) are basically of 0 use. One has no idea on which file this happened.

I'd vote for dropping - having no resource loaded is a valid case.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll remove it

@DeepDiver1975
Copy link
Member

THX @georgehrke for the unit test - and here: have come 🍰 and 🍻 - Happy birthday!

@georgehrke
Copy link
Contributor Author

and here: have come 🍰 and 🍻 - Happy birthday!

Thx 😃

@georgehrke georgehrke force-pushed the imagepng_expects_resource_issue branch from 1a5de00 to 4556d11 Compare March 16, 2015 08:57
*/
function data() {
if (!is_resource($this->resource)) {
Copy link
Member

Choose a reason for hiding this comment

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

Having a closer look - calling valid() here might be a better idea - just in case we ever have to add further validations.

https://github.com/owncloud/core/blob/master/lib/private/image.php#L80

@georgehrke georgehrke force-pushed the imagepng_expects_resource_issue branch from 4556d11 to 09b0537 Compare March 16, 2015 09:57
@ghost
Copy link

ghost commented Mar 16, 2015

Refer to this link for build results (access rights to CI server needed):
https://ci.owncloud.org//job/pull-request-analyser-ng-simple/10469/
Test PASSed.

@scrutinizer-notifier
Copy link

The inspection completed: 1 updated code elements

@DeepDiver1975
Copy link
Member

👍

DeepDiver1975 added a commit that referenced this pull request Mar 16, 2015
validate resource's integrity before using it
@DeepDiver1975 DeepDiver1975 merged commit 1075914 into master Mar 16, 2015
@DeepDiver1975 DeepDiver1975 deleted the imagepng_expects_resource_issue branch March 16, 2015 11:22
@georgehrke
Copy link
Contributor Author

@DeepDiver1975 Do we want to back port this?

@DeepDiver1975
Copy link
Member

@DeepDiver1975 Do we want to back port this?

should not hurt - please open a pr against stable8 - THX

@georgehrke
Copy link
Contributor Author

backport pr: #14917

@lock lock bot locked as resolved and limited conversation to collaborators Aug 13, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

imagepng() expects parameter 1 to be resource
4 participants