Page MenuHomePhabricator

Upload failed with fatal error: Call to load() on a non-object in UploadBase.php
Open, Needs TriagePublic

Description

Error

Request ID: W9tAFQpAEDMAAAUovYYAAABI
Request URL: /w/api.php
Request Method: POST

message
[{exception_id}] {exception_url} BadMethodCallException from line 645 of /srv/mediawiki/php-1.33.0-wmf.2/includes/upload/UploadBase.php: Call to a member function load() on a non-object (null)
trace
#0 /srv/mediawiki/php-1.33.0-wmf.2/includes/api/ApiUpload.php(661): UploadBase->checkWarnings()
#1 /srv/mediawiki/php-1.33.0-wmf.2/includes/api/ApiUpload.php(126): ApiUpload->getApiWarnings()
#2 /srv/mediawiki/php-1.33.0-wmf.2/includes/api/ApiUpload.php(104): ApiUpload->getContextResult()
#3 /srv/mediawiki/php-1.33.0-wmf.2/includes/api/ApiMain.php(1570): ApiUpload->execute()
#4 /srv/mediawiki/php-1.33.0-wmf.2/includes/api/ApiMain.php(531): ApiMain->executeAction()
#5 /srv/mediawiki/php-1.33.0-wmf.2/includes/api/ApiMain.php(502): ApiMain->executeActionWithErrorHandling()
#6 /srv/mediawiki/php-1.33.0-wmf.2/api.php(87): ApiMain->execute()
#7 /srv/mediawiki/w/api.php(3): include(string)
#8 {main}

Impact

Unknown, but based on the trace and request details, I would assume it is causing user uploads to be aborted and fail without descriptive error message, and without a way for the user to recover (fatal exception).

Notes

Appears to have started today/yesterday with the deployment of 1.33-wmf.2.

Low frequency (seen 7 times since then), but not seen for several weeks before that and might be indicative of a deeper problem that is more common but not captured via our detection. To be confirmed.

Details

Request ID
XbHxRApAIDgAABL9sPAAAAAI
Related Gerrit Patches:
mediawiki/core : masterValidate name for async uploads
mediawiki/extensions/UploadWizard : masterFix title max length to match backend, and actually validate it
mediawiki/core : masterAlways validate uploads over api

Event Timeline

Krinkle created this task.Nov 1 2018, 7:54 PM
Restricted Application added a project: Multimedia. · View Herald TranscriptNov 1 2018, 7:54 PM
Restricted Application added a subscriber: Aklapper. · View Herald Transcript
Tgr added a subscriber: Tgr.Nov 1 2018, 9:17 PM

At a glance this happens when files are uploaded with an invalid target name.

(Still seen on 1.33-wmf.21.)

Change 498984 had a related patch set uploaded (by Umherirrender; owner: Umherirrender):
[mediawiki/core@master] Handle null from UploadBase::getLocalFile

https://gerrit.wikimedia.org/r/498984

		// Check if the uploaded file is sane
		if ( $this->mParams['chunk'] ) {
			$maxSize = UploadBase::getMaxUploadSize();
			if ( $this->mParams['filesize'] > $maxSize ) {
				$this->dieWithError( 'file-too-large' );
			}
			if ( !$this->mUpload->getTitle() ) {
				$this->dieWithError( 'illegal-filename' );
			}
		} elseif ( $this->mParams['async'] && $this->mParams['filekey'] ) {
			// defer verification to background process
		} else {
			wfDebug( __METHOD__ . " about to verify\n" );
			$this->verifyUpload();
		}

if 'chunk' is set, UploadBase::getTitle is checked for null and api dies
elseif 'async' and 'filekey' is set, nothing is validated -> here a very long file name can cause this issue
else calling ApiUpload::verifyUpload -> UploadBase::verifyUpload -> UploadBase::validateName -> UploadBase::getTitle to check for null

So when doing a chunked upload (with a short valid filename) and than for the publish step (without the chunk param) the filekey and async is given with a long filename (>240) this error can occur.

First request:
action = upload
comment = test
token = csrf
filename = shortname.jpg
stash = 
chunk = your file
offset = 0
filesize = max

Result:
result=Success
filekey=xxxxxxxxxxxx.tmp

Second request:
action = upload
comment = test
token = csrf
filekey=xxxxxxxxxxxx.tmp
filename = verylongname012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789.jpg
async =

Without the async it gives the expected error: code=filename-toolong, info=Filenames may not be longer than 240 bytes.

I have not checked if my combine of the params makes sense, but there is a validation gap when defered to the background process
I see no security problem here (bypassing any validation for blacklist or such), because getTitle is called when looking for warnings and than the fatal has happen. The warning check path of the code is always running and not depending on params

Hm.. yeah, I don't know either. The path from the client interaction to this particular line of code where $localFile = $this->getLocalFile(); $localFile->load(…); happens is quite long.

Without the necessary context, I'm inclined to think that if something shouldn't be possible, it'd be better to fail explicitly (and we'll find out if it's really possible). Instead of skipping the validation and assuming it's fine.

Anyway, I don't know how to verify this or what the use cases are.

@MarkTraceur Could you/your team take a look here to see what we can do to fix and/or avoid this error?

I'm not entirely certain I follow the example code above (I looked for it in the patched file but it must be elsewhere), but I'm inclined to agree that it's better to fail on "impossible" behavior, then dig more to figure out why it's not impossible.

The async API parameter seems to only fire (in UploadWizard, at least) when uploading very large files (>10MB, higher than most images uploaded to Commons), and obviously this error will only occur when someone then further tries to input a too-long filename. That may be a problem for GLAM cases (where uploads are large, and filenames tend to have a lot of information in them), but as the error count shows, the impact here is relatively low.

Without a deep dive through the code path, I'm not sure how best to fix this. The bug is in our backlog and will be tracked and triaged in time, but unless we see a much larger spike, I doubt we'll get to it very soon. Especially given our day-to-day is currently focused on attempting to finally release an extraordinarily delayed feature.

If this becomes more urgent, I'll bring it up to the team.

Thanks. I'm landing Umherirrender patch and will circle back to see if this rare issue is still reported in Logstash after that. Also, be on the look out for any new upload-related errors or bugs.

Change 498984 merged by jenkins-bot:
[mediawiki/core@master] Always validate uploads over api

https://gerrit.wikimedia.org/r/498984

Given the timing, this is not going to be the sole cause of T190988, but possibly related? That's API-based chunked uploads being mis-reconstructed, and has a related category of failure in production: https://commons.wikimedia.org/wiki/Category:Incomplete_files_(5_MB_interruption)

Change 510905 had a related patch set uploaded (by Matthias Mullie; owner: Matthias Mullie):
[mediawiki/extensions/UploadWizard@master] Fix title max length to match backend, and actually validate it

https://gerrit.wikimedia.org/r/510905

Change 510893 had a related patch set uploaded (by Matthias Mullie; owner: Matthias Mullie):
[mediawiki/core@master] Validate name for async uploads

https://gerrit.wikimedia.org/r/510893

@Umherirrender Can you take a look at https://gerrit.wikimedia.org/r/c/mediawiki/core/+/510893 to see whether you think this would also adequately solve this task?

Change 510905 merged by jenkins-bot:
[mediawiki/extensions/UploadWizard@master] Fix title max length to match backend, and actually validate it

https://gerrit.wikimedia.org/r/510905

(Re-assigning per CR from Umherirrender on Gerrit.)

Not seen in WMF Logstash for 30 days.

mmodell changed the subtype of this task from "Task" to "Production Error".Aug 28 2019, 11:08 PM

Seen again.

Request: commons.wikimedia.org/w/api.php

Fatal error:
Call to a member function load() on null
exception.trace
#0 /includes/api/ApiUpload.php(664): UploadBase->checkWarnings()
#1 /includes/api/ApiUpload.php(127): ApiUpload->getApiWarnings()
#2 /includes/api/ApiUpload.php(104): ApiUpload->getContextResult()
#3 /includes/api/ApiMain.php(1602): ApiUpload->execute()
#4 /includes/api/ApiMain.php(538): ApiMain->executeAction()
#5 /includes/api/ApiMain.php(509): ApiMain->executeActionWithErrorHandling()
#6 /api.php(83): ApiMain->execute()
#7 /srv/mediawiki/w/api.php(3): require(string)
#8 {main}
Krinkle set Request ID to XbHxRApAIDgAABL9sPAAAAAI.Thu, Oct 24, 6:49 PM
Krinkle set Phatality ID to 86c313431a15a502bb495c54e6eab7d7914d3ea1c9bf5987dc837e572fb32554.
Krinkle added subscribers: cneubauer, TerraCodes, Harjotsingh and 5 others.

Change 104012 had a related patch set uploaded by Mayankmadan:
verifyUpload should be called before checkWarnings
https://gerrit.wikimedia.org/r/104012

Change 105111 had a related patch set uploaded by Mayankmadan:
getApiWarnings() throws an exception if upload is invalid
https://gerrit.wikimedia.org/r/105111

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 :)