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

make sure mtime is always an int #28066

Merged
merged 9 commits into from
Jun 12, 2017
Merged

make sure mtime is always an int #28066

merged 9 commits into from
Jun 12, 2017

Conversation

individual-it
Copy link
Member

Description

HTTP_X_OC_MTIME is cast to float, then checked if its outside the int range if yes it set to the max values and if not the original value is cast to int
That should make sure afterwards we have an int value that is in PHP range, in any PHP version
This is an alternative approach to #27615 , don't throw any errors but simply make sure the value is legal.

This fix partly fixes #23228. On a system that uses 32bit integer it would protect the database to receive a value that is too big, but on a 64bit system the value would still be given to the database.

Related Issue

#27960
#23228

Motivation and Context

currently mtime can only be stored as int, but the server accepts also other types.

How Has This Been Tested?

run new tests against pgsql, mysql & sqlite on PHP 7.1

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have read the CONTRIBUTING document.
  • I have added tests to cover my changes.
  • All new and existing tests passed.

@butonic
Copy link
Member

butonic commented Jun 2, 2017

the tests all fail with:

OCA\DAV\Tests\unit\Connector\Sabre\FileTest::testPutSingleFileLegalMtime with data set "string starting with number following by char" ('2345asdf', 2345)
PHPUnit_Framework_Exception: PHP Fatal error:  Uncaught Error: Call to a member function getMailHeaderColor() on null in /var/lib/jenkins/workspace/owncloud-core_core_PR-28066-B7BY7XW3TLFFWQC2SWMA4Z6JVKZQNISNGYFNFF4TAEYZY3ESC5IA/core/templates/mail.php:5
Stack trace:
#0 -(1928): require_once()
#1 {main}
  thrown in /var/lib/jenkins/workspace/owncloud-core_core_PR-28066-B7BY7XW3TLFFWQC2SWMA4Z6JVKZQNISNGYFNFF4TAEYZY3ESC5IA/core/templates/mail.php on line 5

Copy link
Contributor

@phil-davis phil-davis left a comment

Choose a reason for hiding this comment

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

This looks good. The only problem is what happened to the tests with the weird mail error thing.
So approving here, with the proviso that of course the tests have to be made to pass somehow.

@individual-it individual-it force-pushed the tests-m-time-fix branch 2 times, most recently from 6ed251d to bcef283 Compare June 2, 2017 12:14
@individual-it
Copy link
Member Author

added changes as requested
looks like @preserveGlobalState disabled fixed the tests

@individual-it
Copy link
Member Author

some tests on Primary Objectstore: swift are failing when negative mtime. Maybe swift does not support negative mtime

@phil-davis
Copy link
Contributor

I wonder what the requirement is to be able to store files from before 1970?
Is there a way to know the "capabilities" of a back-end storage and e.g. if it cannot accept negative mtime then set any negative mtime to zero.

@individual-it
Copy link
Member Author

@individual-it
Copy link
Member Author

all test passing now.
please re-review

@phil-davis
Copy link
Contributor

Note: Travis litmus-v1 job fails because of:
wget: unable to resolve host address `www.webdav.org'

anything that triggers real work in that job is failing.
See #28103 for a fix.

I guess other PRs that have changes that trigger the litmus-v1 job will also have this problem.

@phil-davis
Copy link
Contributor

Apologies to @individual-it - I was thinking about how the negative mtime support checks could be refactor a bit and to make it easy in future to add any other conditionals for when negative mtime [is|is not] supported. I accidentally pushed straight to this branch, when I intended to check/test the changes myself. Anyway, the tests have run here and hopefully somebody will think my edits are reasonable.
This is once again ready for review.

@individual-it
Copy link
Member Author

@phil-davis I like your changes 👍

individual-it and others added 3 commits June 8, 2017 16:42
Signed-off-by: Phil Davis <phil@jankaritech.com>
Signed-off-by: Phil Davis <phil@jankaritech.com>
Copy link
Member

@butonic butonic left a comment

Choose a reason for hiding this comment

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

Well, I'd like to use the OCP api for getting file information, but since the DAV app still isn't using it I guess we can omit it for now...

@butonic
Copy link
Member

butonic commented Jun 8, 2017

Looking at the console output of Jenkins the integration tests seem to pass, but why doeas the overview at https://jenkins.owncloud.org/job/owncloud-core/job/core/job/PR-28066/ say it was "paused for 0ms"?

@butonic
Copy link
Member

butonic commented Jun 8, 2017

@phil-davis
Copy link
Contributor

I did try to stop the job for "Refactor negative mtime support checking" commit yesterday when I realized that I had pushed to @individual-it 's branch, and that I still had a syntax error. I never worked out how I could stop the job, but I had clicked the pause button. I didn't touch anything in Jenkins for that last run where @individual-it had rebased and trigger the build/tests again. Hmmm...

@individual-it
Copy link
Member Author

it had a strange error after rebase https://jenkins.owncloud.org/job/owncloud-core/job/core/job/PR-28066/17/ and I could not work out why that was happening that's why I tried to restart it. And now it came up green. If you look at the detailed log it did run the integration tests https://jenkins.owncloud.org/job/owncloud-core/job/core/job/PR-28066/18/consoleText
Only it does not display it in the overview. So I think @butonic is correct in saying that its only a display error and we can merge this PR

@butonic butonic merged commit 018d45c into master Jun 12, 2017
@butonic butonic deleted the tests-m-time-fix branch June 12, 2017 08:15
@PVince81 PVince81 added this to the 10.1 milestone Jun 21, 2017
@PVince81
Copy link
Contributor

please backport to stable10 where the bug first appeared

@phil-davis
Copy link
Contributor

Backport to stable10 #28186

@lock
Copy link

lock bot commented Aug 2, 2019

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@lock lock bot locked as resolved and limited conversation to collaborators Aug 2, 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.

Upload files with mtime in the far future crashes request and file scan
5 participants