Page MenuHomePhabricator

UploadBase::checkWarnings could throw exception on null object access
Closed, DuplicatePublic


The function checkWarnings() in UploadBase.php calls:

$localFile = $this->getLocalFile();
$filename = $localFile->getName();

But getLocalFile() will return null if the title of the upload is invalid for some reason (like it contains a blacklisted extension). This can cause an exception to be thrown. There should be a null check in there.

Version: 1.20.x
Severity: normal
Whiteboard: gci2014



Event Timeline

bzimport raised the priority of this task from to Low.Nov 22 2014, 12:59 AM
bzimport set Reference to bz38222.
bzimport added a subscriber: Unknown Object (MLST).

Bryan.TongMinh wrote:

*** Bug 43832 has been marked as a duplicate of this bug. ***

Bryan.TongMinh wrote:

This should never happen because you should always call verifyUpload() before checkWarnings() (should be documented).

ApiUpload allows you to call checkWarnings() if you use async=true, which is I think the reason for the exceptions we see in production.

cc'd Andre and Quim. Seems like this would be an easy task for the code-in...

Thank you very much! Google Code-in task created. We hope to get code reviewers when a patch arrives.

Change 104012 had a related patch set uploaded by Mayankmadan:
verifyUpload should be called before checkWarnings

Change 105111 had a related patch set uploaded by Mayankmadan:
getApiWarnings() throws an exception if upload is invalid

Change 104012 abandoned by Mayankmadan:
verifyUpload should be called before checkWarnings

Added a new patch for this bug

Would anybody be willing to be a mentor for fixing the existing patch as part of Google Code-in 2014? If yes, could you add it to until
Sunday (we can still improve the description until December 1st)?
If time is spare I can also do that - I just need a statement who
would mentor it.

See for more information. Don't hesitate to ask if you have questions!

Krinkle set Security to None.
Krinkle removed a subscriber: Unknown Object (MLST).

I would like to work on this.
Doesn't checkWarnings() handle it if getLocalFile() return null ?
It has warnings for empty file, unwanted filetype and badfilename.

If getLocalFile returns null (instead of a LocalFile object), checkWarnings() won't even be able to proceed to the other checks.
Before those other checks are conducted, checkWarnings() will try to fetch $localFile->getName(), which will cause a fatal error (that method is not available in $localFile, if $localFile is null)
So no, it does not already handle this, but it probably should :)