From 1101dc368c535b3292ff6b222ba9534c6552e9fa Mon Sep 17 00:00:00 2001 From: Nick Mills-Barrett Date: Mon, 16 May 2022 09:24:26 -0400 Subject: [PATCH 1/4] Remove any NULL characters from displaynames to avoid breaking database txn --- synapse/handlers/user_directory.py | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/synapse/handlers/user_directory.py b/synapse/handlers/user_directory.py index 74f7fdfe6ce5..40840ce40b2d 100644 --- a/synapse/handlers/user_directory.py +++ b/synapse/handlers/user_directory.py @@ -464,6 +464,10 @@ async def _handle_possible_remote_profile_change( prev_name = prev_event.content.get("displayname") new_name = event.content.get("displayname") + + # Replace any NULL characters in the name as these cannot be stored in the database + new_name = new_name.replace("\x00", "\uFFFD") + # If the new name is an unexpected form, do not update the directory. if not isinstance(new_name, str): new_name = prev_name From 29aff267345cc771f3093e75160a82ff896e0602 Mon Sep 17 00:00:00 2001 From: Nick Mills-Barrett Date: Tue, 17 May 2022 14:55:39 -0400 Subject: [PATCH 2/4] Make user directory null string handling same as member events Extract the shared function into storage utils and reuse in both locations for consistent results. --- synapse/storage/databases/main/events.py | 4 +--- synapse/storage/databases/main/user_directory.py | 7 +++---- synapse/storage/util/__init__.py | 6 ++++++ 3 files changed, 10 insertions(+), 7 deletions(-) diff --git a/synapse/storage/databases/main/events.py b/synapse/storage/databases/main/events.py index 42d484dc98d9..c2ea2d199397 100644 --- a/synapse/storage/databases/main/events.py +++ b/synapse/storage/databases/main/events.py @@ -47,6 +47,7 @@ from synapse.storage.databases.main.events_worker import EventCacheEntry from synapse.storage.databases.main.search import SearchEntry from synapse.storage.engines.postgres import PostgresEngine +from synapse.storage.util import non_null_str_or_none from synapse.storage.util.id_generators import AbstractStreamIdGenerator from synapse.storage.util.sequence import SequenceGenerator from synapse.types import JsonDict, StateMap, get_domain_from_id @@ -1728,9 +1729,6 @@ def _store_room_members_txn( not affect the current local state. """ - def non_null_str_or_none(val: Any) -> Optional[str]: - return val if isinstance(val, str) and "\u0000" not in val else None - self.db_pool.simple_insert_many_txn( txn, table="room_memberships", diff --git a/synapse/storage/databases/main/user_directory.py b/synapse/storage/databases/main/user_directory.py index df772d472102..59b86e6cd457 100644 --- a/synapse/storage/databases/main/user_directory.py +++ b/synapse/storage/databases/main/user_directory.py @@ -42,6 +42,7 @@ from synapse.storage.databases.main.state import StateFilter from synapse.storage.databases.main.state_deltas import StateDeltasStore from synapse.storage.engines import PostgresEngine, Sqlite3Engine +from synapse.storage.util import non_null_str_or_none from synapse.types import ( JsonDict, UserProfile, @@ -470,10 +471,8 @@ async def update_profile_in_user_dir( Update or add a user's profile in the user directory. """ # If the display name or avatar URL are unexpected types, overwrite them. - if not isinstance(display_name, str): - display_name = None - if not isinstance(avatar_url, str): - avatar_url = None + display_name = non_null_str_or_none(display_name) + avatar_url = non_null_str_or_none(avatar_url) def _update_profile_in_user_dir_txn(txn: LoggingTransaction) -> None: self.db_pool.simple_upsert_txn( diff --git a/synapse/storage/util/__init__.py b/synapse/storage/util/__init__.py index 5e83dba2ed6f..04d5b42911b6 100644 --- a/synapse/storage/util/__init__.py +++ b/synapse/storage/util/__init__.py @@ -11,3 +11,9 @@ # WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. # See the License for the specific language governing permissions and # limitations under the License. + +from typing import Any, Optional + + +def non_null_str_or_none(val: Any) -> Optional[str]: + return val if isinstance(val, str) and "\u0000" not in val else None From 621d8f11440049cdd8ac9834d09d8106bfeee559 Mon Sep 17 00:00:00 2001 From: Nick Mills-Barrett Date: Tue, 17 May 2022 14:57:59 -0400 Subject: [PATCH 3/4] Add changelog file --- changelog.d/12743.bugfix | 2 ++ 1 file changed, 2 insertions(+) create mode 100644 changelog.d/12743.bugfix diff --git a/changelog.d/12743.bugfix b/changelog.d/12743.bugfix new file mode 100644 index 000000000000..1227e583ca16 --- /dev/null +++ b/changelog.d/12743.bugfix @@ -0,0 +1,2 @@ +Fix a long-standing bug wherein remote display names or avatar URLs containing null bytes caused updating the user directory to fail resulting in high CPU on the main process. Contributed +by Nick @ Beeper. From cf63121afe46120216ded71459623ed6901575f4 Mon Sep 17 00:00:00 2001 From: Nick Mills-Barrett Date: Tue, 17 May 2022 15:02:02 -0400 Subject: [PATCH 4/4] Remove old replace fix --- synapse/handlers/user_directory.py | 3 --- 1 file changed, 3 deletions(-) diff --git a/synapse/handlers/user_directory.py b/synapse/handlers/user_directory.py index 40840ce40b2d..ea03ac959722 100644 --- a/synapse/handlers/user_directory.py +++ b/synapse/handlers/user_directory.py @@ -465,9 +465,6 @@ async def _handle_possible_remote_profile_change( prev_name = prev_event.content.get("displayname") new_name = event.content.get("displayname") - # Replace any NULL characters in the name as these cannot be stored in the database - new_name = new_name.replace("\x00", "\uFFFD") - # If the new name is an unexpected form, do not update the directory. if not isinstance(new_name, str): new_name = prev_name