1908623 - Intermittent SUMMARY: ThreadSanitizer: data race /builds/worker/checkouts/gecko/security/nss/lib/softoken/pkcs11u.c:997:53 in sftk_PutObjectToList

Status: RESOLVED FIXED

(NSS :: Libraries, defect, P3)

Description

Filed by: nfay [at] mozilla.com
Parsed log: https://treeherder.mozilla.org/logviewer?job_id=466853033&repo=autoland
Full log: https://firefox-ci-tc.services.mozilla.com/api/queue/v1/task/SgdV1D-YRlO-6iNu6oi9ZA/runs/0/artifacts/public/logs/live_backing.log

---

TEST-START | toolkit/components/satchel/megalist/content/tests/browser/browser_viewmodel.js
GECKO(23271) | ==================
GECKO(23271) | WARNING: ThreadSanitizer: data race (pid=23271)
GECKO(23271) |   Read of size 4 at 0x7f1f880ff2d0 by thread T23 (mutexes: write M0):
GECKO(23271) |     #0 sftk_PutObjectToList /builds/worker/checkouts/gecko/security/nss/lib/softoken/pkcs11u.c:997:53 (libsoftokn3.so+0x54cd1)
GECKO(23271) |     #1 sftk_DestroyObject /builds/worker/checkouts/gecko/security/nss/lib/softoken/pkcs11u.c:1182:9 (libsoftokn3.so+0x54cd1)
[...]
GECKO(23271) |     #5 NSC_DecryptFinal /builds/worker/checkouts/gecko/security/nss/lib/softoken/pkcs11c.c:1893:5 (libsoftokn3.so+0x3fb86)
GECKO(23271) |     #6 pk11_Finalize /builds/worker/checkouts/gecko/security/nss/lib/pk11wrap/pk11cxt.c (libnss3.so+0x97361)
GECKO(23271) |     #7 PK11_Finalize /builds/worker/checkouts/gecko/security/nss/lib/pk11wrap/pk11cxt.c:1631:10 (libnss3.so+0x9a5cd)
GECKO(23271) |     #8 pk11Decrypt /builds/worker/checkouts/gecko/security/nss/lib/pk11wrap/pk11sdr.c:301:5 (libnss3.so+0xc6937)
GECKO(23271) |     #9 PK11SDR_Decrypt /builds/worker/checkouts/gecko/security/nss/lib/pk11wrap/pk11sdr.c:367:14 (libnss3.so+0xc66fb)
[...]
GECKO(23271) |     #12 BackgroundSdrDecryptStrings(nsTArray<nsTString<char>> const&, RefPtr<mozilla::dom::Promise>&) /builds/worker/checkouts/gecko/security/manager/ssl/SecretDecoderRing.cpp:75:22 (libxul.so+0x90a98f6) (BuildId: d5bb43fda91fe74196b280956799e9cf1ad3c42a)
GECKO(23271) |     #13 operator() /builds/worker/checkouts/gecko/security/manager/ssl/SecretDecoderRing.cpp:270:9 (libxul.so+0x90cbc4d) (BuildId: d5bb43fda91fe74196b280956799e9cf1ad3c42a)
GECKO(23271) |     #14 mozilla::detail::RunnableFunction<SecretDecoderRing::AsyncDecryptStrings(nsTArray<nsTString<char>> const&, JSContext*, mozilla::dom::Promise**)::$_0>::Run() /builds/worker/workspace/obj-build/dist/include/nsThreadUtils.h:548:5 (libxul.so+0x90cbc4d)
[...]
GECKO(23271) |   Previous write of size 4 at 0x7f1f880ff2d0 by thread T26 (mutexes: write M1, write M2):
GECKO(23271) |     #0 sftk_GetObjectFromList /builds/worker/checkouts/gecko/security/nss/lib/softoken/pkcs11u.c:962:24 (libsoftokn3.so+0x56ced)
[...]
GECKO(23271) |     #5 sftk_CryptInit /builds/worker/checkouts/gecko/security/nss/lib/softoken/pkcs11c.c:893:11 (libsoftokn3.so+0x3bf82)
[...]
GECKO(23271) |     #11 SecretDecoderRing::Encrypt(nsTSubstring<char> const&, nsTSubstring<char>&) /builds/worker/checkouts/gecko/security/manager/ssl/SecretDecoderRing.cpp:138:7 (libxul.so+0x90aa2eb)
GECKO(23271) |     #12 SecretDecoderRing::EncryptString(nsTSubstring<char> const&, nsTSubstring<char>&) /builds/worker/checkouts/gecko/security/manager/ssl/SecretDecoderRing.cpp:176:17 (libxul.so+0x90aa50f)
GECKO(23271) |     #13 BackgroundSdrEncryptStrings(nsTArray<nsTString<char>> const&, RefPtr<mozilla::dom::Promise>&) /builds/worker/checkouts/gecko/security/manager/ssl/SecretDecoderRing.cpp:44:22 (libxul.so+0x90a90e7)
GECKO(23271) |     #14 operator() /builds/worker/checkouts/gecko/security/manager/ssl/SecretDecoderRing.cpp:212:9 (libxul.so+0x90cb96d)
GECKO(23271) |     #15 mozilla::detail::RunnableFunction<SecretDecoderRing::AsyncEncryptStrings(nsTArray<nsTString<char>> const&, JSContext*, mozilla::dom::Promise**)::$_0>::Run() /builds/worker/workspace/obj-build/dist/include/nsThreadUtils.h:548:5 (libxul.so+0x90cb96d)
[...]

Comment 1

[Andrew McCreight [:mccr8]]

The test where we are hitting this race was just added in bug 1904477, which seems suspicious. Maybe the test is poking at crypto stuff in an unusual way?

Comment 2

[Andrew McCreight [:mccr8]]

Also this failure has happened 3 times so it doesn't seem like a fluke.

Comment 3

[BugBot [:suhaib / :marco/ :calixte]]

Set release status flags based on info from the regressing bug 1904477

:mtigley, since you are the author of the regressor, bug 1904477, could you take a look? Also, could you set the severity field?

For more information, please visit BugBot documentation.

Comment 4

[Andrew McCreight [:mccr8]]

Attachment: Full TSan report

Comment 5

[Andrew McCreight [:mccr8]]

I updated the first comment. It looks like two different StreamTrans threads are running a SecretDecoderRing::AsyncDecryptStrings runnable at the same time and using the global tokenObjectList, which seems to be involved in allocation.

Comment 6

[Micah [:mtigley] (she/her)]

It's not obvious to me where browser_viewmodel.js is causing this error. I wonder if it may have to do with us running this in a browser test.

Comment 7

[Micah [:mtigley] (she/her)]

I've created a patch for disabling this test: https://phabricator.services.mozilla.com/D216973.

The MegalistViewModel is responsible for fetching logins and serving them to the view. It's created in the parent process when the Passwords sidebar is open, but we also instantiate MegalistViewModel in the browser test itself for testing. So perhaps this might be what's causing the race?

Comment 8

[Andrew McCreight [:mccr8]]

You could disable this test only for TSan, with something like skip-if = ["tsan"] in a .toml file.

That does sound plausible, but it would be good to talk to an NSS person to understand what the issue is here, to make sure it is a testing-only issue, and to hopefully figure out how to prevent it entirely.

Comment 9

[John Schanck [:jschanck]]

Attachment: Bug 1908623 - move list size check after lock acquisition in sftk_PutObjectToList. r=rrelyea

Comment 10

[John Schanck [:jschanck]]

I don't see any security consequences here, so we can unhide this. The race is on a variable that is only used to control the size of a linked list. There's no real harm in letting the list grow beyond the maximum intended size.

Comment 11

[Andrew McCreight [:mccr8]]

Thanks for taking a look, John. You are right, nothing important seems to depend on this count, but it is still undefined behavior so I'll leave it at sec-low. Seems unlikely to be exploitable in any realistic scenario though.

Comment 12

[Andrew McCreight [:mccr8]]

I also cleared the "regression" indicator as this looks like a latent problem that was just brought to the fore by the new test.

Comment 13

[Intermittent Failures Robot]

6 failures in 3799 pushes (0.002 failures/push) were associated with this bug in the last 7 days.

Repository breakdown:

• autoland: 5
• mozilla-central: 1

Platform and build breakdown:

• linux1804-64-tsan-qr: 6
  • opt: 6

For more details, see:
https://treeherder.mozilla.org/intermittent-failures/bugdetails?bug=1908623&startday=2024-07-15&endday=2024-07-21&tree=all

Comment 14

[John Schanck [:jschanck]]

https://hg.mozilla.org/projects/nss/rev/0ec88bb52740d0d90271873068251c995ffdfadf

Note: There will continue to be intermittent failures in Firefox until NSS 1.103 is released and uplifted to M-C. That will happen in Bug 1906193.

Comment 15

[Iulian Moraru]

This is still happening: https://treeherder.mozilla.org/logviewer?job_id=468127947&repo=autoland&lineNumber=6917

Hi John! Can you please take another look at this?
Thank you!

Comment 16

[John Schanck [:jschanck]]

Please see Comment 14.

Comment 17

[Intermittent Failures Robot]

10 failures in 4124 pushes (0.002 failures/push) were associated with this bug in the last 7 days.

Repository breakdown:

• autoland: 8
• mozilla-central: 2

Platform and build breakdown:

• linux1804-64-tsan-qr: 10
  • opt: 10

For more details, see:
https://treeherder.mozilla.org/intermittent-failures/bugdetails?bug=1908623&startday=2024-07-22&endday=2024-07-28&tree=all