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

Error when uploading a file to media store #609

Closed
AndreasBilke opened this issue May 8, 2019 · 16 comments
Closed

Error when uploading a file to media store #609

AndreasBilke opened this issue May 8, 2019 · 16 comments
Labels
backend backport needed Fix that has to be backported to older release branches. bugs
Milestone

Comments

@AndreasBilke
Copy link

s9y version: 2.3-beta
PHP version: 7.3.4 (debian buster package)
Postgres version: 11.2 (debian buster package)

When I upload a file to the media store, I get the following error site displayed:

== ERROR-REPORT (BETA/ALPHA-BUILDS) ==

Warning: pg_query(): Query failed: ERROR: value too long for type character varying(5) in <...>web/include/db/postgres.inc.php on line 222.
For more details set $serendipity['production'] = 'debug' in serendipity_config_local.inc.php to receive a stack-trace.



Fatal error:  Uncaught ErrorException: Warning: pg_query(): Query failed: ERROR:  value too long for type character varying(5) in <...>web/include/db/postgres.inc.php:222
Stack trace:
#0 [internal function]: errorToExceptionHandler(2, 'pg_query(): Que...', '/data/www/<...>...', 222, Array)
#1 <...>web/include/db/postgres.inc.php(222): pg_query(Resource id #1, 'INSERT INTO ser...')
#2 <...>web/include/functions_images.inc.php(633): serendipity_db_query('INSERT INTO ser...')
#3 <...>web/include/functions_images.inc.php(1631): serendipity_insertImageInDatabase('header1.pngt134...', '') 
#4 <...>web/include/functions_images.inc.php(3697): serendipity_displayImageList(1, 2, true, NULL, false, NULL, Array)
#5 <...>web/include/admin/images.inc.php(424): showMediaLibrary(true)
#6 <...>web/serendipity_admin.php(109): include('/data/w in <...>web/include/db/postgres.inc.php on line 222 

 == ERROR-REPORT (BETA/ALPHA-BUILDS) ==  

Fatal Error: Uncaught ErrorException: Warning: pg_query(): Query failed: ERROR:  value too long for type character varying(5) in <...>web/include/db/postgres.inc.php:222
Stack trace:
#0 [internal function]: errorToExceptionHandler(2, 'pg_query(): Que...', '/data/www/<...>...', 222, Array)
#1 <...>web/include/db/postgres.inc.php(222): pg_query(Resource id #1, 'INSERT INTO ser...')
#2 <...>web/include/functions_images.inc.php(633): serendipity_db_query('INSERT INTO ser...')
#3 <...>web/include/functions_images.inc.php(1631): serendipity_insertImageInDatabase('header1.pngt134...', '') 
#4 <...>web/include/functions_images.inc.php(3697): serendipity_displayImageList(1, 2, true, NULL, false, NULL, Array)
#5 <...>web/include/admin/images.inc.php(424): showMediaLibrary(true)
#6 <...>web/serendipity_admin.php(109): include('/data/w in <...>web/include/db/postgres.inc.php on line 222.
For more details set $serendipity['production'] = 'debug' in serendipity_config_local.inc.php to receive a stack-trace.



Fatal error:  Uncaught ErrorException: Fatal Error: Uncaught ErrorException: Warning: pg_query(): Query failed: ERROR:  value too long for type character varying(5) in <...>web/include/db/po
Stack trace:
#0 [internal function]: errorToExceptionHandler(2, 'pg_query(): Que...', '/data/www/<...>...', 222, Array)
#1 <...>web/include/db/postgres.inc.php(222): pg_query(Resource id #1, 'INSERT INTO ser...')
#2 <...>web/include/functions_images.inc.php(633): serendipity_db_query('INSERT INTO ser...')
#3 <...>web/include/functions_images.inc.php(1631): serendipity_insertImageInDatabase('header1.pngt134...', '') 
#4 <...>web/include/functions_images.inc.php(3697): serendipity_displayImageList(1, 2, true, NULL, false, NULL, Array)
#5 <...>web/include/admin/images.inc.php(424): showMediaLibrary(true)
#6 <...>web/serend in <...>web/include/db/postgres.inc.php on line 222

This is not the case for every picture, but for many (I didn't found a pattern).

Putting $serendipity['production'] = 'debug' in the config does not help, since then no error is shown at all.

@AndreasBilke
Copy link
Author

I forgot to mention that, despite of the error, the media file is uploaded nevertheless.

@onli onli added the bugs label May 8, 2019
@onli onli added this to the Minor milestone May 8, 2019
@onli
Copy link
Member

onli commented May 8, 2019

Hi. Thanks for testing the beta!

So what seems to happen here based on the stack trace (I hate that s9y/PHP does truncate that!) is that a string that is too long for a field in the image table gets inserted. The stacktrace references

$query = sprintf(
"INSERT INTO {$serendipity['dbPrefix']}images (
name,
extension,
mime,
size,
dimensions_width,
dimensions_height,
thumbnail_name,
date,
authorid,
path,
realname
) VALUES (
'%s',
'%s',
'%s',
%s,
%s,
%s,
'%s',
%s,
%s,
'%s',
'%s'
)",
and a field with type character varying(5). There is one of that in the definition of the images table: extension.

Are you trying to upload images that have an extension longer than 5 characters?

In any case this is a bug on our side if we are not catching that, but if the file extension is shorter there might be an additional bug in serendipity_parseFileName where the extension is read, or maybe

list($filebase, $extension) = serendipity_parseFileName($filename);
is not working anymore with PHP 7.3.4.

@AndreasBilke
Copy link
Author

AndreasBilke commented May 9, 2019 via email

@ophian
Copy link
Member

ophian commented May 9, 2019

@AndreasBilke Interesting. I never found anyone using postgresql with Serendipity. Thanks for reporting.
You might want to test the Serendipity Styx Edition at https://github.com/ophian/styx and read about it at https://ophian.github.io, which is just about to release a new 2.8.0 version that has quite some tweaks regarding sql error reporting and the MediaLibrary, beside tons of other fixes and enhancements. I would be very interested to hear about any issue popping up over my Styx issues page, before I release that version soon. I wont mess around here so you both can handle this "bug" quietly.

Regards, Ian

@AndreasBilke
Copy link
Author

AndreasBilke commented May 9, 2019 via email

@onli
Copy link
Member

onli commented May 9, 2019

It also crashes for "test.jpg".

Okay, then this is likely indeed related to PHP 7.3. My development installation is sadly stuck on PHP 7.2 so far. @th-h or @yellowled, you ran tests under PHP 7.3, no? Does uploading an image work for you under PHP 7.3 and ends up with the proper extension in the database?

@ophian
Copy link
Member

ophian commented May 9, 2019

@AndreasBilke

I fear that I cannot simply switch from s9y to your Styx edition (with current db)?

If that is a question please handle at Styx https://github.com/ophian/styx/issues. Thanks.

@th-h
Copy link
Member

th-h commented Aug 10, 2019

Okay, then this is likely indeed related to PHP 7.3. My development installation is sadly stuck on PHP 7.2 so far. @th-h or @yellowled, you ran tests under PHP 7.3, no? Does uploading an image work for you under PHP 7.3 and ends up with the proper extension in the database?

Both our test blogs (current and stable - identical for now) run under PHP 7.3 as of now, if you want to debug that yourself. Uploading of pictures does work there (with mySQL as backend).

But now for something completey different (see #618):

What doesn't seem to work is selecting one or more images and deleting them via the mass delete button - s9y 2.3.0 (german language) reports "Verzeichnis und Dateien wurden erfolgreich nach verschoben" (yes, with obviously missing target) and does not delete anything ...

@yellowled
Copy link
Member

Does uploading an image work for you under PHP 7.3 and ends up with the proper extension in the database?

Yes, it works. Tested with GIF, JPG and PNG in s9y 2.3 running on PHP 7.3.5. It seems faster than ever, but that may be my new-ish bandwidth (haven't actually used the blog in a while).

@th-h th-h modified the milestones: Minor, 2.3.1 Aug 15, 2019
@mattsches
Copy link
Member

I can reproduce the error. I added a file called test.jpg, and then tried to add the same file again.

For some reason, in my uploads/ directory a file named test.jpg1234 turns up.

Right after each upload, a redirect to the media library happens, where the function serendipity_displayImageList() is called. You can see this in the original error message:

#3 <...>web/include/functions_images.inc.php(1631): serendipity_insertImageInDatabase('header1.pngt134...', '')
#4 <...>web/include/functions_images.inc.php(3697): serendipity_displayImageList(1, 2, true, NULL, false, NULL, Array)
#5 <...>web/include/admin/images.inc.php(424): showMediaLibrary(true)

In this rather unreadable function, files in the uploads/ directory are collected, thumbnails are filtered out, and if any other "unmatched" image is found, it is added to the database (L1631) - that's when the error occurs. test.jpg1234 is exploded into test and the suffix jpg1234, and the latter is too long for the column in the db.

foreach ($aUnmatchedOnDisk AS $sFile) {
if (preg_match('@\.' . $serendipity['thumbSuffix'] . '\.@', $sFile)) {
if ($debug) echo "<p>Skipping thumbnailed file $sFile</p>";
continue;
} else {
if ($debug) echo "<p>Checking $sFile</p>";
}
// MTG: 21/01/06: put files which have just 'turned up' into the database
$aImageData = serendipity_getImageData($sFile);
if (serendipity_isImage($aImageData, false, '(image)|(video)|(audio)/')) {
$nPos = strrpos($sFile, "/");
if (is_bool($nPos) && !$nPos) {
$sFileName = $sFile;
$sDirectory = "";
} else {
++$nPos;
$sFileName = substr($sFile, $nPos);
$sDirectory = substr($sFile, 0, $nPos);
}
if ($debug) echo "<p>Inserting image $sFileName from $sDirectory <pre>" . print_r($aImageData, true) . "</pre> into database</p>";
# TODO: Check if the thumbnail generation goes fine with Marty's code
serendipity_makeThumbnail($sFileName, $sDirectory);
serendipity_insertImageInDatabase($sFileName, $sDirectory);
++$nCount;
}
}

Now I wonder where the file comes from in the first place, why it has this strange suffix, and why it's added to the db when we're just trying to list images?!?

@th-h th-h modified the milestones: 2.3.1, 2.3.2 Aug 21, 2019
@th-h th-h added the backend label Aug 21, 2019
@th-h
Copy link
Member

th-h commented Aug 21, 2019

I can reproduce the error. I added a file called test.jpg, and then tried to add the same file again.

For some reason, in my uploads/ directory a file named test.jpg1234 turns up.

If I upload a file test.jpg with the media manager, and then upload again, I get an error message about duplicate filenames, and the file is renamed to test1.jpg (Screenshot). And that's what happens on disk, I checked.

It seems possible that the name-changing code runs amok and adds '1234' to the extension, but can't reproduce here (2.4-alpha1, i.e. tip of master branch, on PHP 7.3.5.

@mattsches
Copy link
Member

Now I wonder where the file comes from in the first place, why it has this strange suffix, and why it's added to the db when we're just trying to list images?!?

At least figured out this part: I manually entered test.jpg1234 in the Save the file as input field while testing uploading an image. 😊

There don't seem to be any checks on allowed extensions when uploading (which is okay, I guess). The file is uploaded correctly.

But then when trying to add the image to the DB, an error occurs. I guess the MIME type check correctly identifies the file as an image, regardless of the file extension.

The same thing happens when opening the media library: The routine I quoted above looks for files/images that are missing from the DB and tries to add them -> same error.

Thumbnail creation fails for such oddly named images, too.

In the original issue, there seems to be a file called header1.pngt134, and this is causing the error.

So, to sum it up: It's not a problem with PostgreSQL (IMHO), but with how S9y handles odd file extension for images and MIME type detection.

@th-h
Copy link
Member

th-h commented Aug 21, 2019

There don't seem to be any checks on allowed extensions when uploading (which is okay, I guess). The file is uploaded correctly.

We should just enforce length constraints for the extension, corresponding to the database fields. That would fix this bug and keep the flexibility.

@mattsches
Copy link
Member

We should just enforce length constraints for the extension, corresponding to the database fields. That would fix this bug and keep the flexibility.

And keep the otherwise awkward behavior 🤷‍♂️ But yes, let's implement the length constraint and see what happens. Could be we end up with broken images in the media library, though.

@th-h
Copy link
Member

th-h commented Aug 21, 2019

And keep the otherwise awkward behavior

We could use a larger database field for the extension, but I don't think that makes much sense here; it's good practice to use standardized extension for media.

We could try to fix wrong extensions, but that's error prone, and I don't think it's needed - errors like that are rare and easily fixed by the uploader.

We could use a whitelist of extensions, but that would limit flexibility. "Media" can be sound files, too (think of podcasting), videos, PDF files ...

Lastly, I think it's correct to scan the directory/directories for unknwon files on disk that are not known in the database, as that allows to upload files by FTP (or similar means, like SCP).

Could be we end up with broken images in the media library, though.

If the extension is wrong, it can be easily fixed in the ML by renaming, I think. Or which scenario do you imagine?

Yes, I see. - We'll have to decide: if we check media against a whitelist, we may make it impossible to upload media of other types at all. If we don't, someone may upload files with wrong extensions, but he'll always be able to delete the wrong file and re-upload it.

@AndreasBilke
Copy link
Author

I can confirm that the installation where the error occurs has multiple files with weird file extensions.

Examples:

  • [...].serendipityThumb.jpgresize4202C420
  • [...].jpgw300
  • header1.pngt1348323194

Since I host this installation for someone else I can't reconstruct how and when they appeared.

@th-h th-h added the backport needed Fix that has to be backported to older release branches. label Sep 5, 2019
@th-h th-h modified the milestones: 2.3.2, 2.3.3 Oct 16, 2019
@th-h th-h modified the milestones: 2.3.3, 2.3.4 Mar 23, 2020
@th-h th-h closed this as completed in 4ee1066 Mar 25, 2020
th-h added a commit that referenced this issue Mar 28, 2020
That's the max length of the extensin
database field.

Fixes #609.

Signed-off-by: Thomas Hochstein <thh@inter.net>
th-h added a commit to th-h/Serendipity that referenced this issue Apr 20, 2020
That's the max length of the extension
database field.

Fixes s9y#609.

Signed-off-by: Thomas Hochstein <thh@inter.net>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backend backport needed Fix that has to be backported to older release branches. bugs
Projects
None yet
Development

No branches or pull requests

6 participants