Open Bug 1648978 Opened 3 months ago Updated 3 months ago

Fix a race in OpenPGP key acceptance storage (sqlite)

Categories

(MailNews Core :: Security: OpenPGP, defect)

defect

Tracking

(Not tracked)

People

(Reporter: KaiE, Assigned: KaiE)

Details

(Keywords: leave-open)

Attachments

(1 file)

The OpenPGP code uses sqlite to store the key acceptance decisions made by the user.

When working with the security info dialog in composer (composeKeyStatus, oneRecipientStatus, keyDetailsDlg), I experience a race.

For example, oneRecipientStatus calls keyDetailsDlg, and waits for its result. (Using console.debug statements, I can confirm that the code in oneRecipientStatus that opens keyDetailsDlg waits until after keyDetailsDlg closes, before it proceeds its processing).

If a change is made then keyDetailsDlg will update the database.
After keyDetailsDlg is done and the execution returns to oneRecipientStatus, then oneRecipientStatus will query the database to obtain the new values and update the display.

That doesn't work as expected.
The value that oneRecipientStatus reads the the database is still the old value - not the one that keyDetailsDlg just wrote to the database.

We get the old value, despite using await statements, in an attempt to ensure that we wait until the update is done, prior to attempting to read from the database.

I don't understand why we get this race.

As a workaround, I suggest we introduce a cache for the last recently updated value.

In my experiments, if I began with updating the database, and only afterwards update the cache variables, then the fix didn't work. The code that read the values from the cache didn't find the new values. I conclude that the await functionality doesn't work correctly with the JavaScript sqlite database code.

In order to make the fix work, it was necessary to set the JS cache variables first, and only afterwards start writing to the database.

(In reply to Kai Engert (:KaiE:) from comment #0)

That doesn't work as expected.
The value that oneRecipientStatus reads the the database is still the old value - not the one that keyDetailsDlg just wrote to the database.

I believe that you ignore that database transactions are used. Transactions follow the ACID principle, and think your expectation would violate the Isolation aspect.

  • transaction A starts and reads or writes something
  • transaction B starts, makes a change and commits it
  • transaction A reads a value that transaction B modified

In this scenario, transaction A is older and is not supposed to see changes made by transaction B.

Because of the await statements I'm using, I believe that transaction A is committed, prior to starting transaction B and reading. Therefore transaction B should see the new value.

Hmm, maybe you're right, because I don't explicitly start a transaction for reading!

Thinking about it again, it's not clear how starting a new transaction would help, because we're creating a new connection for the new read operation. That should imply B is starting a new transaction.

I'll commit the change, but will leave this bug open. Help with further investigation and a better fix would be welcome.

Keywords: leave-open
Pushed by kaie@kuix.de:
https://hg.mozilla.org/comm-central/rev/9b3a60e665cc
Workaround for a race in OpenPGP key acceptance storage. r=PatrickBrunschwig DONTBUILD

Comment on attachment 9159923 [details]
Bug 1648978 - Workaround for a race in OpenPGP key acceptance storage. r=PatrickBrunschwig

Fix for OpenPGP user interaction

Attachment #9159923 - Flags: approval-comm-beta?

Comment on attachment 9159923 [details]
Bug 1648978 - Workaround for a race in OpenPGP key acceptance storage. r=PatrickBrunschwig

Approved for beta

Attachment #9159923 - Flags: approval-comm-beta? → approval-comm-beta+
You need to log in before you can comment on or make changes to this bug.