Mirror
/
zulip
mirror of https://github.com/zulip/zulip.git
1
0
Fork 0
Code Issues Projects Releases Wiki Activity

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
This commit is contained in:
Alex Vandiver 2023-07-14 15:03:36 +00:00 committed by Tim Abbott
parent fcf096c52e
commit 003fa7adda
1 changed files with 4 additions and 2 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

6
zerver/actions/message_edit.py
View File

@ -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"))