Item14563: Race Condition in AUTOINC topics

I've just noticed AUTOINC-named topics suffer from a race condition. I'm not sure whether this is by design or a bug, so I'm reporting it.

I have a wiki installation that uses forms on AUTOINC-named pages to store user reports. A while ago one of these reports went missing. Today I checked the event log and found two save events, in the same second, for the same page by two different users. So the first user's report got "overwritten" by the second user filling his.

The wiki correctly created a new revision for the second save event, but this shouldn't have happened, because the save script also set write permissions. Here's what I expected to happen:

1. User A saves Topic ReportAUTOINC.... Wiki determines next free number is 47.
2. User B saves Topic ReportAUTOINC.... Wiki determines next free number is 48.

This isn't what happened. Both topics got number 47. OK, so the next thing I would have expected to happen is this:

1. User A saves Topic ReportAUTOINC.... Wiki determines next free number is 47.
2. User B saves Topic ReportAUTOINC.... Wiki determines next free number is 47 (because, I assume, User A's file isn't written yet)
3. Wiki stores User A's Topic Report47 to disk with write permissions that only include him and a supervisor group.
4. Wiki tries to store User B's Topic Report47 and gets an error message because he doesn't have write permissions.

What actually happened:

1. User A saves Topic ReportAUTOINC.... Wiki determines next free number is 47.
2. User B saves Topic ReportAUTOINC.... Wiki determines next free number is 47 (because, I assume, User A's file isn't written yet)
3. Wiki stores User A's Topic Report47 to disk with write permissions that only include him and a supervisor group.
4. Wiki stores User B's data as revision 2 in Report47, even though he doesn't have write permissions for Report47.

Can someone tell me why this fails? Personally, I think that step 2 should result in either a failure or a correct numbering. It didn't, but I could live with that. However, step 4 should definitely fail, yet it didn't.

-- PascalSchuppli - 08 Dec 2017

It does seem as though you've found a race condition. ACL checking is done early in the save process, as part of the UI implementation of the Save script., so the ACL checks for the non-existing topic pass. ACL checks are expensive, so they are done early and not repeated.

When Store finally is handed the topic to save, it doesn't re-check the ACLs, it just does the "right thing" when it detects an existing topic and saves a new revision. The store never does ACL checking.

I have no idea on how best to handle this. Pushing ACL checking down into the store is probably not the right solution ACLs are at a higher level. Maybe some sort of topic create lock taken earlier in the process, or maybe a "new topic" flag that tells the store to throw an error if the intent to create a new topic finds a collision.

I can see how this can happen, so I'm marking this confirmed and urgent.

-- GeorgeClark - 08 Dec 2017

More AUTOINC related bugs: Item10219

-- MichaelDaum - 11 Dec 2017

Marking this for 2.2. This will really require an internal reorganization to solve this correctly. AUTOINC should be done in Meta during actual save operation rather than in the UI. It needs to be an atomic operation rather than a simple glob of the existing topic names. This is too big for a patch release.

-- GeorgeClark - 11 Dec 2017

I've got a fix for this and Item10219 started in Item14563 branch but I want to make to minor Store API changes which I need some advice on.

1. Extend the Store::atomicLock() and Store::atomicUnlock() functions to allow the topic name to be overridden. Meta::saveAs() would then:
   • atomicLock( $topicObject, $cuid, $altName ) This allows META to lock TopicAUTOINCnnnn as the name, blocking the range.
   • Resolve the autolink name
   • Lock atomicLock( $topicObject, $cuid ) the actual topic to be created.
   • Save the topic, and then unlock both names.
2. Scrapped that idea. Too complicated. Implemented as:
   • Meta locks the initial topic name, which might be the AUTOINC template, or the real name.
   • Then run the AUTOINC code. If the name changes, lock the topic again.
   • Save the topic
   • Unlock the topic
   • If the topic name was changed, then restore the original name, unlock it, then go back to the actual name.
3. Extend Store::saveTopic() with a new option: forceinsert. Store would then die if it detects an existing topic when a new topic was expected. This way meta can prevent a topic name collision where the store ends up overwriting a topic.

I suppose we don't really need both the forceinsert option and the double locking, but this way is a bit safer.

Crawford? Any concerns?

-- GeorgeClark - 18 Feb 2018

I think I can set this to Needs Merge. It's working, unit tests pass. Just needs review.

-- GeorgeClark - 20 Feb 2018

You are right, AUTOINC should be atomic at the store level. I did have a plan for this, which involved extending the store API, but as you found it gets rather complex. Your solution sounds fine.

-- Main.CrawfordCurrie - 20 Feb 2018 - 09:27

Commenting is disabled while not logged in