Skip to content
This repository has been archived by the owner on Apr 26, 2024. It is now read-only.

Handle non-strings in the event_search table in synapse_port_db #12037

Merged
merged 13 commits into from
Feb 24, 2022
1 change: 1 addition & 0 deletions changelog.d/12037.bugfix
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Fix a long-standing bug where integers could get into the `event_search` table when using sqlite and prevent migration to PostgreSQL.
4 changes: 3 additions & 1 deletion scripts/synapse_port_db
Original file line number Diff line number Diff line change
Expand Up @@ -473,7 +473,9 @@ class Porter(object):
rows_dict = []
for row in rows:
d = dict(zip(headers, row))
if "\0" in d["value"]:
if not isinstance(d["value"], str) or "\0" in d["value"]:
# `value` must be a string and contain no null characters.
# Previous versions of Synapse allowed integers to slip into the column.
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure how actually.

#441 from 6 years ago addresses half of the code paths. Events with non-string name/topic/bodies can't reach the code paths fixed in this PR because we validate incoming events from clients. Perhaps it can happen when receiving events over federation?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Coming in over federation sounds like something that's worth trying?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I scraped together a test and the issue can indeed be triggered by receiving malformed events over federation.
Or it would if Synapse didn't error out in the null-cleaner introduced in 1.44.0 (#10820).

As suggested, I'm going to revert the change to scripts/synapse_port_db and create a background update to clean up the database.

logger.warning("dropping search row %s", d)
else:
rows_dict.append(d)
Expand Down
6 changes: 3 additions & 3 deletions synapse/storage/databases/main/events.py
Original file line number Diff line number Diff line change
Expand Up @@ -1954,19 +1954,19 @@ def _handle_redaction(self, txn, redacted_event_id):
)

def _store_room_topic_txn(self, txn, event):
clokep marked this conversation as resolved.
Show resolved Hide resolved
if hasattr(event, "content") and "topic" in event.content:
if hasattr(event, "content") and isinstance(event.content.get("topic"), str):
clokep marked this conversation as resolved.
Show resolved Hide resolved
self.store_event_search_txn(
txn, event, "content.topic", event.content["topic"]
)

def _store_room_name_txn(self, txn, event):
if hasattr(event, "content") and "name" in event.content:
if hasattr(event, "content") and isinstance(event.content.get("name"), str):
self.store_event_search_txn(
txn, event, "content.name", event.content["name"]
)

def _store_room_message_txn(self, txn, event):
if hasattr(event, "content") and "body" in event.content:
if hasattr(event, "content") and isinstance(event.content.get("body"), str):
self.store_event_search_txn(
txn, event, "content.body", event.content["body"]
)
Expand Down