Moodle
  1. Moodle
  2. MDL-42839

MUC: Memcache server configuration too sensitive to whitespace

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: 2.4.4
    • Fix Version/s: 2.4.8, 2.5.4, 2.6
    • Component/s: Caching
    • Labels:
    • Testing Instructions:
      Hide

      NOTE: Test requires access to dataroot directory.

      1. Go to cache configuration page (System admin/Plugins/Caching/Configuration).
      2. Under 'Installed cache stores', click 'Add instance' in the memcache row
      3. For store name, type 'Test'. For servers, type: ' test.example.org ' (with spaces at start and end), then hit return to add a line feed after the single line. Also add a line like ' test.example.org:12345 ' (including extra space)
      4. Save changes.
      5. In the dataroot directory, find the file muc/config.php. Search for 'Test' and look for the servers array.

      EXPECTED: The servers array should have a single entry 0 => 0 => 'test.example.org'.

      BEFORE FIX: The array has two entries, one of which is test.example.org but surrounded by spaces and with a carriage return after it, and the other is an empty string.

      Repeat with Memcached

      Show
      NOTE: Test requires access to dataroot directory. 1. Go to cache configuration page (System admin/Plugins/Caching/Configuration). 2. Under 'Installed cache stores', click 'Add instance' in the memcache row 3. For store name, type 'Test'. For servers, type: ' test.example.org ' (with spaces at start and end), then hit return to add a line feed after the single line. Also add a line like ' test.example.org:12345 ' (including extra space) 4. Save changes. 5. In the dataroot directory, find the file muc/config.php. Search for 'Test' and look for the servers array. EXPECTED: The servers array should have a single entry 0 => 0 => 'test.example.org'. BEFORE FIX: The array has two entries, one of which is test.example.org but surrounded by spaces and with a carriage return after it, and the other is an empty string. Repeat with Memcached
    • Affected Branches:
      MOODLE_24_STABLE
    • Fixed Branches:
      MOODLE_24_STABLE, MOODLE_25_STABLE, MOODLE_26_STABLE
    • Pull Master Branch:
      MDL-42839-master

      Description

      The memcache configuration forms are highly sensitive to whitespace. In particular, if you include a blank line at the end of the form, it will result in unexpected behaviour. This is easy to do when copy-pasting lists of servers; we've done this multiple times in live configuration.

      The server lists should have a basic level of tolerance for whitespace.

        Gliffy Diagrams

          Activity

          Hide
          Sam Marshall added a comment - - edited

          NOTES:

          1. I decided to call trim() twice rather than duplicate the default argument and add colon, to avoid code duplication since this code is not performance-sensitive.

          2. Like all the existing code, this change is duplicated between memcache and memcached.

          3. This is a minor bug but I guess it is a bug, so I'm making branches for the old releases. These are direct cherry-picks without change.

          4. I don't think this bugfix is critical for 2.6 release; although it is a disturbing (hard to diagnose) problem, it's been like this forever. 2.6.1 should be fine.

          Show
          Sam Marshall added a comment - - edited NOTES: 1. I decided to call trim() twice rather than duplicate the default argument and add colon, to avoid code duplication since this code is not performance-sensitive. 2. Like all the existing code, this change is duplicated between memcache and memcached. 3. This is a minor bug but I guess it is a bug, so I'm making branches for the old releases. These are direct cherry-picks without change. 4. I don't think this bugfix is critical for 2.6 release; although it is a disturbing (hard to diagnose) problem, it's been like this forever. 2.6.1 should be fine.
          Hide
          Tim Hunt added a comment -

          +1 looks good to me. Submitting for integration.

          Show
          Tim Hunt added a comment - +1 looks good to me. Submitting for integration.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Integrated, thanks!

          Show
          Eloy Lafuente (stronk7) added a comment - Integrated, thanks!
          Hide
          Frédéric Massart added a comment -

          Passing. I took the liberty of amending the testing instructions to test the hosts with a port number, and to mention that the test should happen with Memcached as well.

          Show
          Frédéric Massart added a comment - Passing. I took the liberty of amending the testing instructions to test the hosts with a port number, and to mention that the test should happen with Memcached as well.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          It's Friday, I'm tired so I won't be very imaginative today.

          No matter of that, yes, you did it! Thanks for your collaboration!

          Closing this as fixed!

          Show
          Eloy Lafuente (stronk7) added a comment - It's Friday, I'm tired so I won't be very imaginative today. No matter of that, yes, you did it! Thanks for your collaboration! Closing this as fixed!

            People

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

              Dates

              • Created:
                Updated:
                Resolved: