Uploaded image for project: 'Moodle'
  1. Moodle
  2. MDL-26277

Unvalidated file pointer can cause endless loop when passed to feof()

    Details

    • Type: Bug
    • Status: Closed
    • Priority: Major
    • Resolution: Fixed
    • Affects Version/s: 2.0.1, 2.3
    • Fix Version/s: 2.1.7, 2.2.4
    • Component/s: Gradebook
    • Labels:
    • Environment:
      N/A
    • Testing Instructions:
      Hide

      1. Create a course or enter an existing course.
      2. Create some grades.
      3. Export the grades as a CSV.
      4. Import the grades as a CSV.

      Show
      1. Create a course or enter an existing course. 2. Create some grades. 3. Export the grades as a CSV. 4. Import the grades as a CSV.
    • Difficulty:
      Easy
    • Affected Branches:
      MOODLE_20_STABLE, MOODLE_23_STABLE
    • Fixed Branches:
      MOODLE_21_STABLE, MOODLE_22_STABLE
    • Pull from Repository:
    • Pull Master Branch:
      MDL-26277-master

      Description

      As described in http://us2.php.net/manual/en/function.feof.php, passing a bad file pointer to feof can cause an endless loop. File pointers should always be tested before being passed to feof(). The linked patch fixes the two instances I found in the code of pointers not being tested. I'm not aware of anyone actually encountering this bug in the wild so it's more a question of best practices.

        Gliffy Diagrams

          Activity

          Hide
          stronk7 Eloy Lafuente (stronk7) added a comment -

          Sending to STABLE backlog and raising priority a bit... thanks for the report!

          Show
          stronk7 Eloy Lafuente (stronk7) added a comment - Sending to STABLE backlog and raising priority a bit... thanks for the report!
          Hide
          cfulton Charles Fulton added a comment -

          Pushed better version of this patch to github.

          Show
          cfulton Charles Fulton added a comment - Pushed better version of this patch to github.
          Hide
          poltawski Dan Poltawski added a comment -

          Hi Charles,

          print_error causes the execution to end, so rather than wrapping in a big if statement, could we just test the file pointer and print_error if its failing?

          I also like that because it makes the diff smaller for ease, but really i'm nitpicking..

          Show
          poltawski Dan Poltawski added a comment - Hi Charles, print_error causes the execution to end, so rather than wrapping in a big if statement, could we just test the file pointer and print_error if its failing? I also like that because it makes the diff smaller for ease, but really i'm nitpicking..
          Hide
          poltawski Dan Poltawski added a comment -

          Oh also, you mention two instances, but I only saw one in the patch

          Show
          poltawski Dan Poltawski added a comment - Oh also, you mention two instances, but I only saw one in the patch
          Hide
          cfulton Charles Fulton added a comment -

          Dan: rebased and updated; thanks for the suggestion. The other instance (originally) was in lib/alfresco, but that should probably be dealt with separately, if at all.

          Show
          cfulton Charles Fulton added a comment - Dan: rebased and updated; thanks for the suggestion. The other instance (originally) was in lib/alfresco, but that should probably be dealt with separately, if at all.
          Hide
          poltawski Dan Poltawski added a comment -

          Looks good, sending for integration. Apologies for the delay.

          Show
          poltawski Dan Poltawski added a comment - Looks good, sending for integration. Apologies for the delay.
          Hide
          samhemelryk Sam Hemelryk added a comment -

          Thanks Charles, changes looks and have been integrated now

          Show
          samhemelryk Sam Hemelryk added a comment - Thanks Charles, changes looks and have been integrated now
          Hide
          phalacee Jason Fowler added a comment -

          I don't know if it's something I am doing wrong, but I keep getting

          user mapping error, could not find user with id "Bobby"

          Errors when I try to import the file back in ...

          Show
          phalacee Jason Fowler added a comment - I don't know if it's something I am doing wrong, but I keep getting user mapping error, could not find user with id "Bobby" Errors when I try to import the file back in ...
          Hide
          phalacee Jason Fowler added a comment -

          I'm assuming the error I am seeing is something unrelated to this issue, and passing it

          Show
          phalacee Jason Fowler added a comment - I'm assuming the error I am seeing is something unrelated to this issue, and passing it
          Hide
          stronk7 Eloy Lafuente (stronk7) added a comment -

          We could celebrate it today... but better if we perform a bigger party after releasing Moodle 2.3.

          Print this message and come to Perth that day, it's valid for one beer, wine, coke or... water, as you wish.

          Many thanks for your collaboration!

          Ciao

          Show
          stronk7 Eloy Lafuente (stronk7) added a comment - We could celebrate it today... but better if we perform a bigger party after releasing Moodle 2.3. Print this message and come to Perth that day, it's valid for one beer, wine, coke or... water, as you wish. Many thanks for your collaboration! Ciao

            People

            • Votes:
              16 Vote for this issue
              Watchers:
              3 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:
                Fix Release Date:
                9/Jul/12