Bug #42888

ResourceManager chokes on non existing files

Added by Bastian Waidelich almost 9 years ago. Updated almost 9 years ago.

Status:
Needs Feedback
Priority:
Should have
Assignee:
-
Category:
Resource
Target version:
-
Start date:
2012-11-12
Due date:
% Done:

0%

Estimated time:
PHP Version:
Has patch:
No
Complexity:

Description

When importing a non existing file via ResourceManager::importResource() the ResourceManager kills the PHP process by allocating memory throws an exception
We should check the existence of the specified $uri.
My first draft looks like this:

    public function importResource($uri) {
        $pathInfo = pathinfo($uri);
        if (isset($pathInfo['extension']) && substr(strtolower($pathInfo['extension']), -3, 3) === 'php') {
            $this->systemLogger->log('Import of resources with a "php" extension is not allowed.', LOG_WARNING);
            return FALSE;
        }

        // NEW CODE:

        $responseHeaders = get_headers($uri);
        $responseCode = (integer)substr($responseHeaders[0], 9, 3);
        if ($responseCode === 404) {
            $this->systemLogger->log(sprintf('Could not copy resource from "%s", the file does not exist.', $uri), LOG_WARNING);
            return FALSE;
        } elseif ($responseCode < 200 || $responseCode > 300) {
            $this->systemLogger->log(sprintf('Could not copy resource from "%s", got response code %d.', $uri, $responseCode), LOG_WARNING);
            return FALSE;
        }

        // ...

But this breaks the unit tests with a warning

get_headers(): This function may only be used against URLs

for the vfs:// URIs.

What do you think about the general approach?
Actually it might be cleaner to switch to a proper request (maybe using Flows RequestEngineInterface) altogether here as copy() is pretty limited.

BTW: If we adjust this we could also think about changing the return value from object/FALSE to object/NULL as we do it in most places already

#1

Updated by Bastian Waidelich almost 9 years ago

The description was wrong before, I only ran out of memory cause the exception tried to render the affected domain object.
The actual issue is, that copy() issues a Warning resulting in following exception:

#1: Warning: copy(http://<nonexistinguri>): failed to open stream: HTTP request failed! HTTP/1.0 404 Not Found in /TYPO3_Flow_Resource_ResourceManager_Original.php line 196

prepending @ to the copy call solves the issue for me

#2

Updated by Adrian Föder almost 9 years ago

ah ok, good; because I wondered and hesitated to ask why a non existing file should result into a memory flood ;)

An @ is a bit hacky IMO, do you have another Idea? Wouldn't the pathinfo() invocation on line 2 trigger some error or something if the file doesn't exist?

#3

Updated by Adrian Föder almost 9 years ago

ah, aand.. in most cases the $uri would be a string <ins>pointing</ins> to a filesystem file <ins>location</ins>; how would get_headers($uri) react in such a case?

#4

Updated by Bastian Waidelich almost 9 years ago

Adrian Föder wrote:

An @ is a bit hacky IMO, do you have another Idea?

Right, but we already use it in other places where PHPs error handling is inconsistent (in Utility\Files for example).
I can't think of anything else..

Wouldn't the pathinfo() invocation on line 2 trigger some error or something if the file doesn't exist?

No, pathinfo doesn't care whether the URI exists or not

in most cases the $uri would be a string <ins>pointing</ins> to a filesystem file [...]

IMO it would always be a valid URI, thus should work. But the get_headers() solution was merely a basic work around I'd like to avoid anyways as it is quite slow, too.

Also available in: Atom PDF