Should check return value of external utilities

Think you found a bug in getID3()? Post here with details.
Post Reply
gokusandwich
User
Posts: 16
Joined: Wed Jan 26, 2005 12:53 am

Should check return value of external utilities

Post by gokusandwich » Mon Apr 24, 2006 3:45 pm

getid3 version: 1.7.6
my email: cw264701 at ohiou dot edu

Although getid3 does check the stderr/stdout output of the external utilities which it invokes, it does not check the actual return values of the utilities. For most cases this probably won't matter. However, I've found that it may occasionally lead to some confusion, for instance, in the case of writing vorbis comments.

Consider what happens if I attempt the following:

Code: Select all

        $vbCommentWriter = new getid3_write_vorbiscomment;
        $vbCommentWriter->filename = $pathToOgg;
        $vbCommentWriter->tag_data = array(
          'title' => array( 0 => "A Title" ),
          'comment' => array( 0 => "this is a \n comment with \n multiple lines." ));
        $vbCommentWriter->WriteVorbisComment();

The WriteVorbisComment() function will place some error messages in the getid3_write_vorbiscomment's errors member and return false, indicating failure, when the writing actually did not fail, but only writing of the 2nd and 3rd lines of the "comment" value failed. The vorbiscomment utility returns a value of zero, indicating success, and outputs a few messages to stderr, like:

Code: Select all

  bad comment: " comment with "
  bad comment: " multiple lines."

What would be expected, is, for WriteVorbisComment() to return true, indicating success, but place the above messages (from vorbiscomment), in the getid3_write_vorbiscomment's warnings member.

So, I attempted to write a small patch, for this particular case, which simply used PHP's exec() function rather than the backtick operator, because exec() allows for retrieving the return value of the command/utility executed. I ended up largely re-organizing the WriteVorbisComment() function, to get the fix to work without having a nasty, hard-to-understand function:

Code: Select all

	function WriteVorbisComment() {

		if (ini_get('safe_mode')) {

			$this->errors[] = 'PHP running in Safe Mode, so exec() function may not be available - cannot call vorbiscomment, tags not written';
			return false;
		}

		// Create file with new comments
		$tempcommentsfilename = tempnam('*', 'getID3');
		if ($fpcomments = @fopen($tempcommentsfilename, 'wb')) {

			foreach ($this->tag_data as $key => $value) {
				foreach ($value as $commentdata) {
					fwrite($fpcomments, $this->CleanVorbisCommentName($key).'='.$commentdata."\n");
				}
			}
			fclose($fpcomments);

		} else {

			$this->errors[] = 'failed to open temporary tags file "'.$tempcommentsfilename.'", tags not written';
			return false;

		}

		$oldignoreuserabort = ignore_user_abort(true);
		if (GETID3_OS_ISWINDOWS) {

			if (file_exists(GETID3_HELPERAPPSDIR.'vorbiscomment.exe')) {

				//$commandline = '"'.GETID3_HELPERAPPSDIR.'vorbiscomment.exe" -w --raw -c "'.$tempcommentsfilename.'" "'.str_replace('/', '\\', $this->filename).'"';
				//  vorbiscomment works fine if you copy-paste the above commandline into a command prompt,
				//  but refuses to work with `backtick` if there are "doublequotes" present around BOTH
				//  the metaflac pathname and the target filename. For whatever reason...??
				//  The solution is simply ensure that the metaflac pathname has no spaces,
				//  and therefore does not need to be quoted

				// On top of that, if error messages are not always captured properly under Windows
				// To at least see if there was a problem, compare file modification timestamps before and after writing
				clearstatcache();
				$timestampbeforewriting = filemtime($this->filename);

				$vorbiscommentExecutable = GETID3_HELPERAPPSDIR.'vorbiscomment.exe';

			} else {

				$this->errors[] = 'vorbiscomment.exe not found in '.GETID3_HELPERAPPSDIR;
				return false;
			}

		} else {

			$vorbiscommentExecutable = 'vorbiscomment';

		}

		$commandline = 'vorbiscomment -w --raw -c "'.$tempcommentsfilename.'" "'.$this->filename.'" 2>&1';
		$vbcommentOutputArray = $vbcommentRetValue = null;
		exec($commandline, $vbcommentOutputArray, $vbcommentRetValue);
		$vbcommentOutput = implode("\n", $vbcommentOutputArray);

		if (GETID3_OS_ISWINDOWS && empty($vbcommentOutput)) {
			clearstatcache();
			if ($timestampbeforewriting == filemtime($this->filename)) {
				$this->errors[] = 'vorbiscomment gave no error output, but file modification timestamp has not changed ' .
				                  '- it looks like the tags were not written';
				return false;
			}
		}

		// Remove temporary comments file
		unlink($tempcommentsfilename);
		ignore_user_abort($oldignoreuserabort);

		// If the vorbiscomment utility returned an error code...
		if ($vbcommentRetValue) {

			$errorMsg = 'system call to vorbiscomment failed with error code '.$vbcommentRetValue;
			if (!empty($vbcommentOutput)) {
				$errorMsg .= ' and message: '."\n".$vbcommentOutput;
			}
			else {
				$errorMsg .= ' but gave no error message';
			}

			$this->errors[] = $errorMsg;
			return false;
		}
		else if (count($vbcommentOutput) > 0) {
			$this->warnings[] = 'vorbiscomment gave a return value indicating success, but gave the following output:'."\n".$vbcommentOutput;
		}

		return true;
	}

The patch posted here (http://www.getid3.org/phpBB2/viewtopic.php?t=512) may need to be taken into consideration, as it sort-of conflicts with this change. They could be merged, though.

I realize that this particular case (of writing multi-line vorbiscomment's) would not arise if I used the interface provided by getid3_writetags, but, there are likely other cases where this implementation might save some confusion...

Perhaps, when calling other external utilities, the same considerations should be made, but I have not looked into these cases.

Post Reply