Moodle

race condition in calendar

Details

  • Type: Bug Bug
  • Status: Closed Closed
  • Priority: Major Major
  • Resolution: Fixed
  • Affects Version/s: 1.6
  • Fix Version/s: 1.6.6, 1.7.4, 1.8.4, 1.9
  • Component/s: Calendar
  • Labels:
    None
  • Environment:
    All
  • Affected Branches:
    MOODLE_16_STABLE
  • Fixed Branches:
    MOODLE_16_STABLE, MOODLE_17_STABLE, MOODLE_18_STABLE, MOODLE_19_STABLE

Description

I've just realised there's quite a serious race condition for how repeat events are handled in the calendar. I thought the repeatid column just pointed to the id of the original event.

But, no:

if ($form->repeat) {
$fetch = get_record_sql('SELECT 1, MAX(repeatid) AS repeatid FROM '.$CFG->prefix.'event');
$form->repeatid = empty($fetch) ? 1 : $fetch->repeatid + 1;
}

cvs:/calendar/edit.php

Activity

Ascending order - Click to sort in descending order
Hide
Permalink
Martin Dougiamas added a comment -

From (penny at catalyst.net.nz) Thursday, 29 June 2006, 07:12 AM:

adding martinl to cc list

From Luke Hudson (lingo at paradise.net.nz) Friday, 28 July 2006, 05:36 AM:

The attached patch solves this problem using the following method:

When a discussion is moved to a new forum, a skeleton discussion is left behind with the same title, but with (Moved) appended to it. The discussion will contain only one post, with the text:

This discussion has been moved to here, in the forum (new location)

The user can click either the 'here' or the name of the new forum to go the the relevant places.

From Luke Hudson (lingo at paradise.net.nz) Friday, 28 July 2006, 05:37 AM:

Oh, bother. I've put this in the wrong place

Please excuse me, this attachemnt and message refer to MDL-4874

Show
Martin Dougiamas added a comment - From (penny at catalyst.net.nz) Thursday, 29 June 2006, 07:12 AM: adding martinl to cc list From Luke Hudson (lingo at paradise.net.nz) Friday, 28 July 2006, 05:36 AM: The attached patch solves this problem using the following method: When a discussion is moved to a new forum, a skeleton discussion is left behind with the same title, but with (Moved) appended to it. The discussion will contain only one post, with the text: This discussion has been moved to here, in the forum (new location) The user can click either the 'here' or the name of the new forum to go the the relevant places. From Luke Hudson (lingo at paradise.net.nz) Friday, 28 July 2006, 05:37 AM: Oh, bother. I've put this in the wrong place Please excuse me, this attachemnt and message refer to MDL-4874
Hide
Permalink
Martín Langhoff added a comment -

Luke has a patch that I like, and agree with. The bit that I am not 100% familiar with is the change to $backup_version in backup/version.php – I am not familiar with how that is managed.

http://git.catalyst.net.nz/gitweb?p=moodle-r2.git;a=commitdiff;h=5e256e8a1f283883c18efdef669b26bdaa8eba45;hp=8f12f5e1b93b27c1479548ce6b0106408445944a

The patch has my signoff except for that detail. If Eloy or someone else knowledgeable in backup versioning nods...

Show
Martín Langhoff added a comment - Luke has a patch that I like, and agree with. The bit that I am not 100% familiar with is the change to $backup_version in backup/version.php – I am not familiar with how that is managed. http://git.catalyst.net.nz/gitweb?p=moodle-r2.git;a=commitdiff;h=5e256e8a1f283883c18efdef669b26bdaa8eba45;hp=8f12f5e1b93b27c1479548ce6b0106408445944a The patch has my signoff except for that detail. If Eloy or someone else knowledgeable in backup versioning nods...
Hide
Permalink
Martín Langhoff added a comment -

Added Eloy (who I'd like to review the patch) and Luke (who's coded it in the first place!)

Show
Martín Langhoff added a comment - Added Eloy (who I'd like to review the patch) and Luke (who's coded it in the first place!)
Hide
Permalink
Eloy Lafuente (stronk7) added a comment -

Well, patch looks really outdated now (old upgrade scripts and so on). Some questions to understand better what's wrong:

1) When a calendar event is saved with n repeats, n records are created in the event table all them sharing the repeatid value (pointing to the first event of the group). That should be the correct behaviour, yep?

2) The race condition happens when inserting new event "groups" yep?

If both answers are yes, then I think that:

1) The "calendar/event.php" patch looks perfect, because it enforces 1) perfectly.
2) The "backup/restorelib.php" patch looks glorious too, because enforces 1) perfectly.
3) It's not needed to increment the backup version at all.
4) About upgrade scripts... I really would leave them unmodified. It's a "rare" race condition and iterating over all repeated events can be heavy in some servers. In fact, I think that current queries in the patch are useless, because if the race condition has arrived, then two events will share the "repeatid" and we cannot differentiate them at all by "repeatid" (an that seems to be the base for those upgrade queries).

Any comment?

Show
Eloy Lafuente (stronk7) added a comment - Well, patch looks really outdated now (old upgrade scripts and so on). Some questions to understand better what's wrong: 1) When a calendar event is saved with n repeats, n records are created in the event table all them sharing the repeatid value (pointing to the first event of the group). That should be the correct behaviour, yep? 2) The race condition happens when inserting new event "groups" yep? If both answers are yes, then I think that: 1) The "calendar/event.php" patch looks perfect, because it enforces 1) perfectly. 2) The "backup/restorelib.php" patch looks glorious too, because enforces 1) perfectly. 3) It's not needed to increment the backup version at all. 4) About upgrade scripts... I really would leave them unmodified. It's a "rare" race condition and iterating over all repeated events can be heavy in some servers. In fact, I think that current queries in the patch are useless, because if the race condition has arrived, then two events will share the "repeatid" and we cannot differentiate them at all by "repeatid" (an that seems to be the base for those upgrade queries). Any comment?
Hide
Permalink
Martín Langhoff added a comment -

Good point (4) that if the race condition has hit, the data is probably already messed up, and not fixable. +1 for applying the core of the patch.

Show
Martín Langhoff added a comment - Good point (4) that if the race condition has hit, the data is probably already messed up, and not fixable. +1 for applying the core of the patch.
Hide
Permalink
Eloy Lafuente (stronk7) added a comment -

Done, from 16_STABLE to HEAD. Fixed.

Thanks all! Ciao

Show
Eloy Lafuente (stronk7) added a comment - Done, from 16_STABLE to HEAD. Fixed. Thanks all! Ciao

People

Vote (0)
Watch (5)

Dates

  • Created:
    Updated:
    Resolved:
Atlassian JIRA (v4.4.3#663-r165197) | Report a problem
Powered by a free Atlassian JIRA open source license for Moodle. Try JIRA - bug tracking software for your team.