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 analyzing mp3 with lyrics3 tag through externally opened file pointer #242

Closed
paulijar opened this issue Apr 9, 2020 · 1 comment

Comments

@paulijar
Copy link
Contributor

paulijar commented Apr 9, 2020

This report describes my findings when investigating the issue owncloud/music#123. That issue is so old that there might have been more than one root cause for similar looking error messages over the years, but now I at least found one which is still relevant for the most recent versions of getID3.

The error happens when we use the file pointer API introduced in #172, and the file being analyzed has lyrics3 tag set. In that case, getID3 spawns another instance of itself, and then tries to reopen the file using just the filename and without the file pointer:

$getid3_temp->openfile($this->getid3->filename);

The thing is, our environment (ownCloud or Nextcloud) is such, that we don't have a fully qualified path of the user files available. We just have some internal path and the file pointer. Giving this internal path to fopen naturally fails.

Now, what made this error a bit sneaky, is that there is no error handling of any kind in getid3_lyrics3::Analyze after the call to openfile. The code just keeps going after failing to open the file, and calls $getid3_apetag->Analyze();. There, it executes this line:

if (!getid3_lib::intValueSupported($info['filesize'])) {

...which tries to access the element 'filesize' of $info, but there is no such element as the file failed to open. Now, depending on the PHP version, this may emit a notice like

[PHP] Error: Undefined index: filesize at /var/www/nextcloud/apps/music/3rdparty/getID3/getid3/module.tag.apetag.php#41

This is the error which users have seen. On older PHP versions, such invalid array access apparently just silently returns NULL.

To fix this, I think that getid3_lyrics3 should call openfile like this:

$getid3_temp->openfile($this->getid3->filename, $info['filesize'], $this->getid3->fp);

However, this isn't the only instance of similar construct. With text search, I found 7 other modules having the same exact source line

$getid3_temp->openfile($this->getid3->filename);

Probably all of these should be modified so that they would reuse the existing file pointer.

As a related note, I think there is a resource leak on each of those occasions when openfile is called on the secondary getID3 instance: no-one seems to be closing those new file pointers opened with fopen (in case the environment is such where the fopen will succeed). Also this would be taken care of, if the secondary instance would share the same file pointer used by the primary instance.

JamesHeinrich added a commit that referenced this issue Apr 9, 2020
@JamesHeinrich
Copy link
Owner

Thanks, I agree with your analysis and fix, applied in 276fd3e

paulijar added a commit to paulijar/getID3 that referenced this issue Mar 13, 2021
The fixed problem is similar to the one reported earlier in
JamesHeinrich#242. The problem was
fixed for MP3 and several other file types in 276fd3e but the WAV
files were missed at that time. Since then, also support for DSDIFF
files has been added with the same problem.

To recap, the problem case is when the file being analyzed is passed
to getID3 as a file pointer without giving a fully qualified file path.
When the file type specific module then spawns a secondary instance of
getID3, it has to pass it also the filesize given by the client
application. If null is passed as filesize to getID3::openfile, then it
tries to obtain the filesize from the system using the filename. But
this will fail if the filename is not fully qualified.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants