search

signalapp / Signal-Desktop

A private messenger for Windows, macOS, and Linux.
https://signal.org/download
GNU Affero General Public License v3.0
14.68k stars 2.68k forks source link

TS Code Error in Migration to Schema Version 14 #7066

Open Prime-Rose opened 3 weeks ago

Prime-Rose commented 3 weeks ago

Using a supported version?

Overall summary

It seems in version 7.29 of Signal Desktop a migration of the 'messages' table from schema version 13 to schema version 14 was introduced.

This migration to schema version 14, however, fails if the json field of the message contains the entry: "digest":{}

The problems seems to be that {} is not considered to be "falsy" in TypeScript, so the ?? operator does not replace it by '' as intended. ( {} is neither null, undefined or alike.)

The problem lies in ts/types/Message2.ts (line 649 in v7.30.0): const logId = `Message2.toVersion14(digest=${redactGenericText(attachment.digest ?? '')})`;

Here the direct link to the source code: https://github.com/signalapp/Signal-Desktop/blob/203fa95e08b8088d7d19437a5b87bf74f567f579/ts/types/Message2.ts#L649

This causes "split" to be executed on {} in byredactGenericText, and to fail with the following error: {"level":50,"time":"REDACTED","msg":"Message._withSchemaVersion: error updating message [REDACTED]a21: TypeError: e.slice is not a function\n at redactGenericText ([REDACTED]/resources/app.asar/preload.bundle.js:62:34781)\n at [REDACTED]/resources/app.asar/preload.bundle.js:110:115174\n at upgradeWithContext ([REDACTED]/resources/app.asar/preload.bundle.js:110:111592)\n at Array.map (<anonymous>)\n at [REDACTED]/resources/app.asar/preload.bundle.js:110:111663\n at [REDACTED]/resources/app.asar/preload.bundle.js:110:111773\n at [REDACTED]/resources/app.asar/preload.bundle.js:110:111326\n at upgradeSchema ([REDACTED]/resources/app.asar/preload.bundle.js:110:116672)\n at process.processTicksAndRejections (node:internal/process/task_queues:95:5)\n at async m.concurrency ([REDACTED]/resources/app.asar/preload.bundle.js:110:446099)"}

I have seen such "digest":{} entries being present in various installation in numerous messages received roughly between end of 2017 and early 2019.

While only a fraction of messages with attachments contains such an "digest":{} entry, it would be important to make the migration more robust and consider such entries in the migration. Otherwise these messages get stuck on schema version 13 with potential problems in the future.

Hope this helps to fix the bug - thank you for your efforts!

Steps to reproduce

Start Signal and wait for the errors to appear in the logfile.

Later when checking the db.sqlite database, it becomes clear that several messages (with attachments, containing "digest":{}) got stuck at schema version 13. All messages not containing "digest":{} in this place have been migrated to schema version 14.

Expected result

No errors should occur and all messages should be successfully migrated to version 14.

Actual result

Lots of errors (usually two per message containing "digest":{}) appear in the logfile.

Here again a sample: {"level":50,"time":"REDACTED","msg":"Message._withSchemaVersion: error updating message [REDACTED]a21: TypeError: e.slice is not a function\n at redactGenericText ([REDACTED]/resources/app.asar/preload.bundle.js:62:34781)\n at [REDACTED]/resources/app.asar/preload.bundle.js:110:115174\n at upgradeWithContext ([REDACTED]/resources/app.asar/preload.bundle.js:110:111592)\n at Array.map (<anonymous>)\n at [REDACTED]/resources/app.asar/preload.bundle.js:110:111663\n at [REDACTED]/resources/app.asar/preload.bundle.js:110:111773\n at [REDACTED]/resources/app.asar/preload.bundle.js:110:111326\n at upgradeSchema ([REDACTED]/resources/app.asar/preload.bundle.js:110:116672)\n at process.processTicksAndRejections (node:internal/process/task_queues:95:5)\n at async m.concurrency ([REDACTED]/resources/app.asar/preload.bundle.js:110:446099)"}

Screenshots

No response

Signal version

Problem exists since version 7.29 and is also present in version 7.30

Operating system

Linux

Version of Signal on your phone

No response

Link to debug log

No response

indutny-signal commented 3 weeks ago

Thanks for this report! We are aware that some legacy messages have been saved with invalid encoding, but I believe that after a few attempts the migrator should ignore them. Is this what you see? Or do you see it looping without stop?

eaon commented 2 weeks ago

For me the migrator seems to be looping and led to #7054.

Prime-Rose commented 2 weeks ago

Thank you very much for looking into this matter and all your amazing work on Signal.

I agree that #6984 is caused by legacy messages with invalid encoding (of sent_at timestamps), but this problem here is of a different nature.

Here the problem lies in the program logic: It seems to be a speciality of TS that {} is the empty object, but not undefined or null. Now there are numerous older messages with attachment, which state the digest to be the empty object {}.

When passed to the ?? operator, the operator does not replace {} by the fallback value as intended, but passes it on to redactGenericText, which in turn runs the slice function on {} causing the observed error. (I found this article that explains the ?? operator very nicely: https://mariusschulz.com/blog/nullish-coalescing-the-operator-in-typescript)

I would suggest to handle "digest":{} as if digest was undefined/not present (and remove it completely from the json field during the migration) to avoid this problem in newer versions. Then it would not be passed to redactGenericText and not cause the errors to occur. Further, in my eyes it does not make sense to store an empty digest anyway.

Regarding your question on the loop behaviour: Whereas with #6984 the migration could get stuck and loop permanently (with breaks of 10 seconds only), now it seems to have at least some longer pauses between the runs. Still, it can produce a significant amount of errors (up to several hundred) per message/attachment containing "digest":{} in the json field, which could also lead to bug #7054. This behaviour seems also to depend on whether the update is done from version 7.28 or an much older one - say 7.15. Interestingly updating from older version yields less errors than from 7.28 to 7.29/7.30. Unfortunately, I have only a few samples from friends and family.

If needed I can try to get more details, but for the time being I think it would the best way forward to fix the special case with "digest":{} rather than aborting the migration. It seems very likely that no errors would occur at all (and the migration succeed) if {} was ignored and not passed to redactGenericText.

I am wondering if skipping messages in migrations could cause severe problems in the future as they would get stuck at a certain schema version (13 here).

Hope this helps. I am more than happy to assist you further where I can.

Would an SQL INSERT statement of a message causing the errors be of any help with debugging this issue?

Prime-Rose commented 2 weeks ago

Sorry, it seems I accidentally closed and reopened this issue!