From 003fa7adda8ee31e925be2001637cd7333d29a6e Mon Sep 17 00:00:00 2001 From: Alex Vandiver Date: Fri, 14 Jul 2023 15:03:36 +0000 Subject: [PATCH] message_edit: Lock the Message row in check_update_message. Fundamentally, we should take a write lock on the message, check its validity for a change, and then make and commit that change. Previously, `check_update_message` did not operate in a transaction, but `do_update_message` did -- which led to the ordering: - `check_update_message` reads Message, not in a transaction - `check_update_message` verifies properties of the Message - `do_update_message` starts a transaction - `do_update_message` takes a read lock on UserMessage - `do_update_message` writes on UserMessage - `do_update_message` writes Message - `do_update_message` commits This leads to race conditions, where the `check_update_message` may have verified based on stale data, and `do_update_message` may improperly overwrite it; as well as deadlocks, where other (properly-written) codepaths take a write lock on Message _before_ updating UserMessage, and thus deadlock with `do_update_message`. Change `check_update_message` to open a transaction, and take the write lock when first accessing the Message row. We update the comment above `do_update_message` to clarify this expectation. The new ordering is thus: - `check_update_message` starts a transaction - `check_update_message` takes a write lock on Message - `check_update_message` verifies properties of the Message - `do_update_message` writes on UserMessage - `do_update_message` writes Message - `check_update_message` commits --- zerver/actions/message_edit.py | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/zerver/actions/message_edit.py b/zerver/actions/message_edit.py index 6ee9d80356..cf51eb207c 100644 --- a/zerver/actions/message_edit.py +++ b/zerver/actions/message_edit.py @@ -347,7 +347,8 @@ def get_visibility_policy_after_merge( return UserTopic.VisibilityPolicy.INHERIT -# We use transaction.atomic to support select_for_update in the attachment codepath. +# This must be called already in a transaction, with a write lock on +# the target_message. @transaction.atomic(savepoint=False) def do_update_message( user_profile: UserProfile, @@ -1163,6 +1164,7 @@ def check_time_limit_for_change_all_propagate_mode( ) +@transaction.atomic(durable=True) def check_update_message( user_profile: UserProfile, message_id: int, @@ -1178,7 +1180,7 @@ def check_update_message( and raises a JsonableError if otherwise. It returns the number changed. """ - message, ignored_user_message = access_message(user_profile, message_id) + message, ignored_user_message = access_message(user_profile, message_id, lock_message=True) if content is not None and not user_profile.realm.allow_message_editing: raise JsonableError(_("Your organization has turned off message editing"))