Crash during catalog update: invalid multiplication of string with int during tag cleanup

gurota commented 6 months ago

Description

Crash in catalog update due to invalid multiplication of string with int during tag cleanup.

Describe the bug

During catalog update (and elsewhere, where this code is called) tag cleanup fails due to invalid input that leads to an multiplication of types string and int:

function/method: private function _cleanup_vorbiscomment($tags): array

line 1278: $parsed['rating'][$rating_user] = floor($data[0] * 5 / 100);

file: src/Module/Util/VaInfo.php

To reproduce

Use a music library, containing an ogg vorbis file which in turn has the tag "rating" with a non-numeric value
Execute catlog update: bin/cli run:updateCatalog -ceag

Expected behavior

sanitize input, fall back to some numeric default value if invalid tag value is detected

Environment

Ampache version: 6.4.0
Web server + version: nginx 1.26.0
Server operating system: ArchLinux / Linux 6.8.8
Client operating system: n/a

Client type

cli
most likely webclient

Logs

sudo -u http bin/cli run:updateCatalog -ceag Reading Catalog: "Music"

Start cleaning orphaned media entries

Start adding new media TypeError Unsupported operand types: string * int (thrown in /usr/share/webapps/ampache/src/Module/Util/VaInfo.php:1278)

Stack Trace: 0) Ampache\Module\Util\VaInfo->_cleanup_vorbiscomment() at /usr/share/webapps/ampache/src/Module/Util/VaInfo.php:818 1) Ampache\Module\Util\VaInfo->_get_tags() at /usr/share/webapps/ampache/src/Module/Util/VaInfo.php:359 2) Ampache\Module\Util\VaInfo->gather_tags() at /usr/share/webapps/ampache/src/Module/Catalog/Catalog_local.php:913 3) Ampache\Module\Catalog\Catalog_local->_insert_local_song() at /usr/share/webapps/ampache/src/Module/Catalog/Catalog_local.php:447 4) Ampache\Module\Catalog\Catalog_local->add_file() at /usr/share/webapps/ampache/src/Module/Catalog/Catalog_local.php:308 5) Ampache\Module\Catalog\Catalog_local->add_files() at /usr/share/webapps/ampache/src/Module/Catalog/Catalog_local.php:365 6) Ampache\Module\Catalog\Catalog_local->add_file() at /usr/share/webapps/ampache/src/Module/Catalog/Catalog_local.php:308 7) Ampache\Module\Catalog\Catalog_local->add_files() at /usr/share/webapps/ampache/src/Module/Catalog/Catalog_local.php:365 8) Ampache\Module\Catalog\Catalog_local->add_file() at /usr/share/webapps/ampache/src/Module/Catalog/Catalog_local.php:308 9) Ampache\Module\Catalog\Catalog_local->add_files() at /usr/share/webapps/ampache/src/Module/Catalog/Catalog_local.php:365 10) Ampache\Module\Catalog\Catalog_local->add_file() at /usr/share/webapps/ampache/src/Module/Catalog/Catalog_local.php:308 11) Ampache\Module\Catalog\Catalog_local->add_files() at /usr/share/webapps/ampache/src/Module/Catalog/Catalog_local.php:365 12) Ampache\Module\Catalog\Catalog_local->add_file() at /usr/share/webapps/ampache/src/Module/Catalog/Catalog_local.php:308 13) Ampache\Module\Catalog\Catalog_local->add_files() at /usr/share/webapps/ampache/src/Module/Catalog/Catalog_local.php:531 14) Ampache\Module\Catalog\Catalog_local->add_to_catalog() at /usr/share/webapps/ampache/src/Module/Catalog/Update/UpdateCatalog.php:162 15) Ampache\Module\Catalog\Update\UpdateCatalog->update() at /usr/share/webapps/ampache/src/Module/Cli/UpdateCatalogCommand.php:84 16) Ampache\Module\Cli\UpdateCatalogCommand->execute() at /usr/share/webapps/ampache/vendor/adhocore/cli/src/Application.php:366 17) Ahc\Cli\Application->doAction() at /usr/share/webapps/ampache/vendor/adhocore/cli/src/Application.php:280 18) Ahc\Cli\Application->handle() at /usr/share/webapps/ampache/bin/cli:82

Possible Solution / Fix

I don't know how ratings (in tags) work so I just quick fixed it on my installation via:

// TODO are ratings always supposed to be numeric? if (is_numeric($data[0])) { $parsed['rating'][$rating_user] = floor($data[0] * 5 / 100); } else { // TODO what is a sane default/fallback value? $parsed['rating'][$rating_user] = 0; }

lachlan-00 commented 6 months ago

what is the value in those tags?

can you uncomment the debug in the Vainfo class and see what it's coming in as?

lachlan-00 commented 6 months ago

https://picard-docs.musicbrainz.org/downloads/MusicBrainz_Picard_Tag_Map.html

not sure how the id3 library parses it but picard seems to be RATING:user@email

fufroma commented 6 months ago

https://quodlibet.readthedocs.io/en/latest/development/formats.html

The rating tag has a subkey of an email address, and is formatted as e.g. rating:quodlibet@sacredchao.net. The email is used as a unique identifier to allow multiple users to share the same files (it need not actually be an email address, but having it as such ensures that it’s unique across users). It represents how much a user likes a song. Its value is a string representation of a floating point number between 0.0 and 1.0, inclusive. This format is chosen so the application may decide what precision it offers to the user, and how this information is presented. If no value is present, the rating should be assumed to be 0.5.

Example: rating:quodlibet@sacredchao.net=0.67

gurota commented 6 months ago

The value of 'data[0]' is 'Clean' in my case:

var_dump($data) => array(1) { [0]=> string(5) "Clean" }

gurota commented 6 months ago

I played around some more: After quickfixing this issue as well as #3965 I was able to perform the catalog update successfully - this was the initial update on a new ampache installation.

Doing an update catalog today, after reverting my quickfix, the bug was not triggered. I assume that ampache detects that the file in question is already in the catalog and skips it. Deleting the catalog and recreating it triggered the bug again.

Propably not relevant, but just in case ...

lachlan-00 commented 6 months ago

i'll just add checks to make sure that the data is numeric like you've done or ignore it definitely can't perform math on 'clean'

lachlan-00 commented 6 months ago

9b749e2bb0b9b0645761466567ec1caf417a4934 should ID valid numeric data like your check above

ampache / ampache