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

          Attachments

            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