Page MenuHomePhabricator

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



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

[{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)
#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}


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


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.

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

		// 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" );

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


Second request:
action = upload
comment = test
token = csrf
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

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:

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

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

@Umherirrender Can you take a look at 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

(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".Wed, Aug 28, 11:08 PM