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

\OC_Helper::tmpFile can break Imagemagick #12282

Closed
oparoz opened this issue Nov 19, 2014 · 12 comments
Closed

\OC_Helper::tmpFile can break Imagemagick #12282

oparoz opened this issue Nov 19, 2014 · 12 comments
Assignees
Labels
Milestone

Comments

@oparoz
Copy link
Contributor

oparoz commented Nov 19, 2014

Steps to reproduce

OC generates temp filenames without any extension (i.e. .mp3). It concatenates everything instead.
This test case proves that this will break Imagemagick.

  1. Install Imagemagick in your test env
  2. Upload a OTF font
  3. Rename the font 23926b6bec09d849d770a606d8403b96otf
  4. Use identify to let Imagemagick determine what's in the file

Tested with OTF fonts, but could happen with more file types down the line.

Expected behaviour

Imagemagick should be able to identify the file as an OTF font

Actual behaviour

Instead

# identify /tmp/23926b6bec09d849d770a606d8403b96otf
identify: no decode delegate for this image format `' @ error/constitute.c/ReadImage/501.

If you rename the file so that extension is kept after the dot.

# identify /tmp/23926b6bec09d849d770a606d8403b96.otf
/tmp/23926b6bec09d849d770a606d8403b96.otf OTF 800x480 800x480+0+0 16-bit sRGB 96.6KB 0.039u 0:00.039

Server configuration

Operating system: FreeBSD

Web server: Apache

PHP version: 5.5 with Imagemagick 6.8 or 6.9

ownCloud version: 7.0.3

Updated from an older ownCloud or fresh install: Updated

The content of config/config.php:

  'enabledPreviewProviders' =>
    array(
      'OC\Preview\Image',
      'OC\Preview\MP3',
      'OC\Preview\TXT',
      'OC\Preview\MarkDown',
      'OC\Preview\Movies',
      'OC\Preview\MSOffice2003',
      'OC\Preview\MSOffice2007',
      'OC\Preview\MSOfficeDoc',
      'OC\Preview\OpenDocument',
      'OC\Preview\PDF',
      'OC\Preview\StarOffice',
      'OC\Preview\SVG',
      'OC\Preview\TIFF',
      'OC\Preview\Illustrator',
      'OC\Preview\Postscript',
      'OC\Preview\Photoshop',
      'OC\Preview\SFNT',
    ),

Are you using encryption: no

@oparoz
Copy link
Contributor Author

oparoz commented Nov 19, 2014

In master, the function has moved to TempManager, but it has the same problem.

If this is considered to be only an issue with Imagemagick, I could take care of the problem locally by adding the dot to $tmpfile, if extensions are normalised (3 chars), but this problem could affect other parts of the app.

@oparoz
Copy link
Contributor Author

oparoz commented Nov 19, 2014

Updated the test case to make it easy to test

@karlitschek
Copy link
Contributor

@georgehrke What is your opinion?

@oparoz
Copy link
Contributor Author

oparoz commented Dec 5, 2014

I think this will only affect ImageMagick as it tries to use the file extension in order to magically convert files it can't identify after a first parse.
If that's the case, then the responsibility could be put on the shoulders of the classes which extend Provider and if most use Imagick, then an extra method could be added to introduce a dot.

@georgehrke
Copy link
Contributor

iirc you can hand over a suffix for \OC_Helper::tmpFile(), I'll check and prepare a pull request

@georgehrke georgehrke self-assigned this Jan 16, 2015
@georgehrke
Copy link
Contributor

@oparoz Can you still reproduce it on latest master? This should have been fixed with your commit, as we are now using toTmpFile() which uses \OC_Helper::tmpFile with extensions.

@oparoz
Copy link
Contributor Author

oparoz commented Jan 19, 2015

I need to rebase my patch to support font previews to be able to test. It's going to take a few days...

@DeepDiver1975 DeepDiver1975 added this to the need more information milestone Jan 20, 2015
@MorrisJobke MorrisJobke removed this from the need more information milestone Jan 23, 2015
@oparoz
Copy link
Contributor Author

oparoz commented Jan 24, 2015

Still a problem.
The tmp file is named oc_tmp_d20d57d7da3bcb6236f3cd4faacacd32otf
We need a dot in front the extension: oc_tmp_d20d57d7da3bcb6236f3cd4faacacd32.otf

@oparoz
Copy link
Contributor Author

oparoz commented Jan 24, 2015

The behaviour can be tested from within OC using this PR #13648
You won't get a preview for OTF fonts unless you fix \OC_Helper::tmpFile()

@oparoz
Copy link
Contributor Author

oparoz commented Jan 24, 2015

This is also killing any Raw picture conversion as these formats can embed other formats as well for quick previewing

@oparoz
Copy link
Contributor Author

oparoz commented Jan 25, 2015

Fix is in #13654

@MorrisJobke MorrisJobke added this to the 8.1-next milestone Feb 7, 2015
@oparoz oparoz removed the needs info label Mar 20, 2015
@MorrisJobke
Copy link
Contributor

Fixed via #13654

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

No branches or pull requests

5 participants