Skip to content

Commit 148af48

Browse files
iMattProiMattPro
committed
Fix possible race and lock issues
1 parent d9838de commit 148af48

5 files changed

Lines changed: 159 additions & 30 deletions

File tree

‎controller/admin_controller.php‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -364,7 +364,7 @@ public function save_ruleset_intro($language)
364364
{
365365
$this->ruleset_operator->set_intro_text($language, $intro_text);
366366
}
367-
catch (\InvalidArgumentException $e)
367+
catch (\InvalidArgumentException|\RuntimeException $e)
368368
{
369369
trigger_error($this->lang->lang($e->getMessage()) . adm_back_link($this->u_action), E_USER_WARNING);
370370
}
@@ -466,7 +466,7 @@ public function set_ruleset_published($language, $published, $return_to = '')
466466
{
467467
$this->ruleset_operator->set_published($language, $published);
468468
}
469-
catch (\InvalidArgumentException $e)
469+
catch (\InvalidArgumentException|\RuntimeException $e)
470470
{
471471
trigger_error($this->lang->lang($e->getMessage()) . adm_back_link($return_url), E_USER_WARNING);
472472
}

‎operators/ruleset.php‎

Lines changed: 87 additions & 28 deletions
Original file line numberDiff line numberDiff line change
@@ -204,14 +204,22 @@ public function draft_if_empty($language)
204204
throw new \InvalidArgumentException('ACP_BOARDRULES_INVALID_LANGUAGE');
205205
}
206206

207-
if ($this->get_rule_count($language) > 0)
207+
$acquired_lock = $this->acquire_write_lock();
208+
try
208209
{
209-
return false;
210-
}
210+
if ($this->get_rule_count($language) > 0)
211+
{
212+
return false;
213+
}
211214

212-
$this->save_published_state($language, false);
215+
$this->save_published_state($language, false);
213216

214-
return true;
217+
return true;
218+
}
219+
finally
220+
{
221+
$this->release_write_lock($acquired_lock);
222+
}
215223
}
216224

217225
/**
@@ -252,12 +260,20 @@ public function set_published($language, $published)
252260
throw new \InvalidArgumentException('ACP_BOARDRULES_INVALID_LANGUAGE');
253261
}
254262

255-
if ($this->get_rule_count($language) === 0)
263+
$acquired_lock = $this->acquire_write_lock();
264+
try
256265
{
257-
throw new \InvalidArgumentException('ACP_BOARDRULES_STATUS_CHANGE_EMPTY');
258-
}
266+
if ($this->get_rule_count($language) === 0)
267+
{
268+
throw new \InvalidArgumentException('ACP_BOARDRULES_STATUS_CHANGE_EMPTY');
269+
}
259270

260-
$this->save_published_state($language, (bool) $published);
271+
$this->save_published_state($language, (bool) $published);
272+
}
273+
finally
274+
{
275+
$this->release_write_lock($acquired_lock);
276+
}
261277
}
262278

263279
/**
@@ -396,30 +412,73 @@ protected function save_published_state($language, $published)
396412
*/
397413
protected function save_ruleset_value($language, $column, $value)
398414
{
399-
$sql = 'SELECT language_iso
400-
FROM ' . $this->rulesets_table . "
401-
WHERE language_iso = '" . $this->db->sql_escape($language) . "'";
402-
$result = $this->db->sql_query_limit($sql, 1);
403-
$exists = (bool) $this->db->sql_fetchfield('language_iso');
404-
$this->db->sql_freeresult($result);
405-
406-
if ($exists)
415+
$acquired_lock = $this->acquire_write_lock();
416+
try
407417
{
408-
$sql = 'UPDATE ' . $this->rulesets_table . '
409-
SET ' . $this->db->sql_build_array('UPDATE', array($column => $value)) . "
418+
$sql = 'SELECT language_iso
419+
FROM ' . $this->rulesets_table . "
410420
WHERE language_iso = '" . $this->db->sql_escape($language) . "'";
421+
$result = $this->db->sql_query_limit($sql, 1);
422+
$exists = (bool) $this->db->sql_fetchfield('language_iso');
423+
$this->db->sql_freeresult($result);
424+
425+
if ($exists)
426+
{
427+
$sql = 'UPDATE ' . $this->rulesets_table . '
428+
SET ' . $this->db->sql_build_array('UPDATE', array($column => $value)) . "
429+
WHERE language_iso = '" . $this->db->sql_escape($language) . "'";
430+
}
431+
else
432+
{
433+
$sql_data = array(
434+
'language_iso' => $language,
435+
'rules_published' => 1,
436+
'rules_intro_text' => '',
437+
);
438+
$sql_data[$column] = $value;
439+
$sql = 'INSERT INTO ' . $this->rulesets_table . ' ' . $this->db->sql_build_array('INSERT', $sql_data);
440+
}
441+
442+
$this->db->sql_query($sql);
443+
}
444+
finally
445+
{
446+
$this->release_write_lock($acquired_lock);
447+
}
448+
}
449+
450+
/**
451+
* Acquire the shared write lock unless this request already owns it.
452+
*
453+
* @return bool True when this method acquired the lock
454+
* @throws \RuntimeException If another request owns the lock
455+
*/
456+
protected function acquire_write_lock()
457+
{
458+
if ($this->lock->owns_lock())
459+
{
460+
return false;
411461
}
412-
else
462+
463+
if (!$this->lock->acquire())
413464
{
414-
$sql_data = array(
415-
'language_iso' => $language,
416-
'rules_published' => 1,
417-
'rules_intro_text' => '',
418-
);
419-
$sql_data[$column] = $value;
420-
$sql = 'INSERT INTO ' . $this->rulesets_table . ' ' . $this->db->sql_build_array('INSERT', $sql_data);
465+
throw new \RuntimeException('RULES_NESTEDSET_LOCK_FAILED_ACQUIRE');
421466
}
422467

423-
$this->db->sql_query($sql);
468+
return true;
469+
}
470+
471+
/**
472+
* Release the shared write lock only when acquired by the current method.
473+
*
474+
* @param bool $acquired_lock
475+
* @return void
476+
*/
477+
protected function release_write_lock($acquired_lock)
478+
{
479+
if ($acquired_lock)
480+
{
481+
$this->lock->release();
482+
}
424483
}
425484
}

‎operators/ruleset_interface.php‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -47,6 +47,7 @@ public function is_published($language);
4747
* @param string $language
4848
* @return bool True when draft state was saved
4949
* @throws \InvalidArgumentException If the language is not installed
50+
* @throws \RuntimeException If the shared write lock cannot be acquired
5051
*/
5152
public function draft_if_empty($language);
5253

@@ -67,6 +68,7 @@ public function get_intro_text($language);
6768
* @param string $intro_text
6869
* @return void
6970
* @throws \InvalidArgumentException If the language is not installed
71+
* @throws \RuntimeException If the shared write lock cannot be acquired
7072
*/
7173
public function set_intro_text($language, $intro_text);
7274

@@ -77,6 +79,7 @@ public function set_intro_text($language, $intro_text);
7779
* @param bool $published
7880
* @return void
7981
* @throws \InvalidArgumentException If the language is not installed or its ruleset is empty
82+
* @throws \RuntimeException If the shared write lock cannot be acquired
8083
*/
8184
public function set_published($language, $published);
8285
}

‎tests/controller/admin_controller_test.php‎

Lines changed: 28 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -372,6 +372,20 @@ public function test_save_ruleset_intro_rejects_unknown_language(): void
372372
$this->controller->save_ruleset_intro('xx');
373373
}
374374

375+
public function test_save_ruleset_intro_reports_lock_failure(): void
376+
{
377+
$ruleset_operator = $this->getMockBuilder(\phpbb\boardrules\operators\ruleset::class)
378+
->disableOriginalConstructor()
379+
->setMethods(array('set_intro_text'))
380+
->getMock();
381+
$ruleset_operator->method('set_intro_text')
382+
->willThrowException(new \RuntimeException('RULES_NESTEDSET_LOCK_FAILED_ACQUIRE'));
383+
$this->replace_controller_service('ruleset_operator', $ruleset_operator);
384+
$this->setExpectedTriggerError(E_USER_WARNING, 'RULES_NESTEDSET_LOCK_FAILED_ACQUIRE');
385+
386+
$this->controller->save_ruleset_intro('fr');
387+
}
388+
375389
public function test_display_rules_rejects_unknown_language(): void
376390
{
377391
$this->setExpectedTriggerError(E_USER_WARNING, 'ACP_BOARDRULES_INVALID_LANGUAGE|back:adm.php?i=boardrules');
@@ -486,6 +500,20 @@ public function test_empty_ruleset_cannot_be_published(): void
486500
$this->controller->set_ruleset_published('fr', true);
487501
}
488502

503+
public function test_ruleset_status_reports_lock_failure(): void
504+
{
505+
$ruleset_operator = $this->getMockBuilder(\phpbb\boardrules\operators\ruleset::class)
506+
->disableOriginalConstructor()
507+
->setMethods(array('set_published'))
508+
->getMock();
509+
$ruleset_operator->method('set_published')
510+
->willThrowException(new \RuntimeException('RULES_NESTEDSET_LOCK_FAILED_ACQUIRE'));
511+
$this->replace_controller_service('ruleset_operator', $ruleset_operator);
512+
$this->setExpectedTriggerError(E_USER_WARNING, 'RULES_NESTEDSET_LOCK_FAILED_ACQUIRE');
513+
514+
$this->controller->set_ruleset_published('en', true);
515+
}
516+
489517
public function test_add_rule_initial_form_uses_real_entity(): void
490518
{
491519
$this->controller->add_rule('fr');

‎tests/operators/ruleset_operator_test.php‎

Lines changed: 39 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -102,6 +102,45 @@ public function test_empty_ruleset_is_forced_to_draft_before_first_rule(): void
102102
self::assertTrue($this->operator->is_published('en'));
103103
}
104104

105+
public function test_draft_write_reuses_and_preserves_caller_owned_lock(): void
106+
{
107+
self::assertTrue($this->lock->acquire());
108+
109+
try
110+
{
111+
self::assertTrue($this->operator->draft_if_empty('fr'));
112+
self::assertTrue($this->lock->owns_lock());
113+
}
114+
finally
115+
{
116+
$this->lock->release();
117+
}
118+
119+
self::assertFalse($this->operator->is_published('fr'));
120+
}
121+
122+
public function test_intro_write_rejects_competing_ruleset_creation(): void
123+
{
124+
$competing_lock = new \phpbb\lock\db('boardrules.table_lock.boardrules_table', $this->config, $this->db);
125+
self::assertTrue($competing_lock->acquire());
126+
127+
try
128+
{
129+
$this->operator->set_intro_text('fr', 'Bienvenue.');
130+
self::fail('Ruleset creation should not run while the shared lock is held.');
131+
}
132+
catch (\RuntimeException $e)
133+
{
134+
self::assertSame('RULES_NESTEDSET_LOCK_FAILED_ACQUIRE', $e->getMessage());
135+
}
136+
finally
137+
{
138+
$competing_lock->release();
139+
}
140+
141+
self::assertSame('', $this->operator->get_intro_text('fr'));
142+
}
143+
105144
public function test_unknown_ruleset_cannot_be_prepared_for_first_rule(): void
106145
{
107146
$this->expectException(\InvalidArgumentException::class);

0 commit comments

Comments
 (0)