Moodle
  1. Moodle
  2. MDL-10177

Mediaplugin filter returns blank $text when bad UTF-8 is found

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 1.7.2
    • Fix Version/s: 1.7.3, 1.8.3, 1.9
    • Component/s: Filters, Resource, Unicode
    • Labels:
      None
    • Environment:
      PHP5-only issue.
    • Affected Branches:
      MOODLE_17_STABLE
    • Fixed Branches:
      MOODLE_17_STABLE, MOODLE_18_STABLE, MOODLE_19_STABLE
    • Rank:
      29389

      Description

      I've been hours trying to find out why, under certain circumstances, a link like "/file.php/263/onefile.htm" would return a blank page while "/file.php/263/otherfile.htm" does not. Tracing took me through file.php->filelib.php (send_file function) -> filter_text function and finally to mediaplugin_filter function, where I found that preg_replace() was the one returning a blank $text.

      The problem arose when upgraded the server to PHP5, so I suspect preg_replace() is returning an empty $text when the original $text is not well-formed UTF-8. This is very probable, since the files in moodledata not necessarily are in UTF-8 (this particular installation was a 1.4, then 1.5, and now 1.7.2).

        Activity

        Hide
        Pablo Etcheverry added a comment -

        I've made an ugly hack to solve this problem. I just check the result of preg_replace and if it is null, I simply ignore it and replace it with the original text.

        filter/mediaplugin/filter.php:

        -$text = preg_replace($search, $replace, $text);

        +$newtext = preg_replace($search, $replace, $text);
        +$text = $newtext ? $newtext : $text;

        Show
        Pablo Etcheverry added a comment - I've made an ugly hack to solve this problem. I just check the result of preg_replace and if it is null, I simply ignore it and replace it with the original text. filter/mediaplugin/filter.php: -$text = preg_replace($search, $replace, $text); +$newtext = preg_replace($search, $replace, $text); +$text = $newtext ? $newtext : $text;
        Hide
        Martin Dougiamas added a comment -

        Nick, can you please check this in to 1.7, 1.8 and head?

        Show
        Martin Dougiamas added a comment - Nick, can you please check this in to 1.7, 1.8 and head?
        Hide
        Joseph Rézeau added a comment - - edited

        Nicolas,
        You shouldn't have applied Pablo's lovely/ugly hack... Now, the mutlimedia filter is broken: it won't filter HTML uploaded files or Composed Web pages or other resources etc.for MP3 links to display the MP3 player. GRRRR!

        The problem appears when more than one filter is enabled (which is the usual case). Then, the $text passed as parameter is lost because of the $newtext variable you introduced...

        Joseph

        Show
        Joseph Rézeau added a comment - - edited Nicolas, You shouldn't have applied Pablo's lovely/ugly hack... Now, the mutlimedia filter is broken: it won't filter HTML uploaded files or Composed Web pages or other resources etc.for MP3 links to display the MP3 player. GRRRR! The problem appears when more than one filter is enabled (which is the usual case). Then, the $text passed as parameter is lost because of the $newtext variable you introduced... Joseph
        Hide
        Petr Škoda added a comment -

        Oh - I did not notice this bug, seems to be similar to MDL-10155 - I guess there should be an is_null() check instead, see:
        http://moodle.cvs.sourceforge.net/moodle/moodle/filter/multilang/filter.php?r1=1.19&r2=1.20

        Show
        Petr Škoda added a comment - Oh - I did not notice this bug, seems to be similar to MDL-10155 - I guess there should be an is_null() check instead, see: http://moodle.cvs.sourceforge.net/moodle/moodle/filter/multilang/filter.php?r1=1.19&r2=1.20
        Hide
        Nicolas Connault added a comment -

        Sorry Joseph, this is fixed now. I must not have been thinking straight

        Show
        Nicolas Connault added a comment - Sorry Joseph, this is fixed now. I must not have been thinking straight
        Hide
        Nicolas Connault added a comment -

        I haven't tested this, but I haven't heard any complaints since I fixed it and committed it over a week ago, so I'm now closing the bug.

        Show
        Nicolas Connault added a comment - I haven't tested this, but I haven't heard any complaints since I fixed it and committed it over a week ago, so I'm now closing the bug.

          People

          • Votes:
            0 Vote for this issue
            Watchers:
            4 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved: