Moodle
  1. Moodle
  2. MDL-26277

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

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Major 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
    • Rank:
      15883

      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.

        Activity

        Hide
        Eloy Lafuente (stronk7) added a comment -

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

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

        Pushed better version of this patch to github.

        Show
        Charles Fulton added a comment - Pushed better version of this patch to github.
        Hide
        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
        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
        Dan Poltawski added a comment -

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

        Show
        Dan Poltawski added a comment - Oh also, you mention two instances, but I only saw one in the patch
        Hide
        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
        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
        Dan Poltawski added a comment -

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

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

        Thanks Charles, changes looks and have been integrated now

        Show
        Sam Hemelryk added a comment - Thanks Charles, changes looks and have been integrated now
        Hide
        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
        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
        Jason Fowler added a comment -

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

        Show
        Jason Fowler added a comment - I'm assuming the error I am seeing is something unrelated to this issue, and passing it
        Hide
        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
        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: