Closed
Bug 1428538
Opened 7 years ago
Closed 7 years ago
Use UTF-8 file paths for NSS database
Categories
(Core :: Security: PSM, defect, P1)
Tracking
()
RESOLVED
FIXED
mozilla59
People
(Reporter: emk, Assigned: emk)
References
Details
(Whiteboard: [psm-assigned])
Attachments
(2 files)
(deleted),
patch
|
keeler
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
Details | Diff | Splinter Review |
Filed as requested in bug 1427273.
Assignee | ||
Comment 1•7 years ago
|
||
Transferred from bug 1427273.
We can land this patch without waiting for NSS fix. This patch will not add more breakage without NSS fix because currently non-ASCII paths do not work anyway.
Comment on attachment 8940417 [details] [diff] [review]
Use UTF-8 file paths for NSS database
Review of attachment 8940417 [details] [diff] [review]:
-----------------------------------------------------------------
::: security/manager/ssl/nsNSSComponent.cpp
@@ +1756,5 @@
> return NS_OK;
> }
>
> #if defined(XP_WIN)
> + // SQLite always takes UTF-8 file paths regardless of the current system
... but again, NSS doesn't. We should at least document this, if not explore it further (for example, will the PKCS#11 module database still work after this? Please ensure the answer is "yes" before landing this.)
Also, this is a larger discussion we should have with the NSS team. If I'm understanding correctly, this would all work properly if NSS only accepted UTF-8 paths but then converted them to unicode when doing file operations on Windows. Since it doesn't, though, things are likely to continue to not work as expected.
Attachment #8940417 -
Flags: review?(dkeeler) → review+
Priority: -- → P1
Whiteboard: [psm-assigned]
Comment 3•7 years ago
|
||
I assume this should be landed at the same time as uplifting a fixed NSS snapshot. We can help to do that at the same time.
Pushed by kaie@kuix.de:
https://hg.mozilla.org/integration/mozilla-inbound/rev/617446a093ba
Use UTF-8 file paths for NSS database. r=keeler
Assignee | ||
Comment 5•7 years ago
|
||
(In reply to David Keeler [:keeler] (use needinfo) from comment #2)
> ... but again, NSS doesn't. We should at least document this, if not explore
> it further (for example, will the PKCS#11 module database still work after
> this? Please ensure the answer is "yes" before landing this.)
> Also, this is a larger discussion we should have with the NSS team. If I'm
> understanding correctly, this would all work properly if NSS only accepted
> UTF-8 paths but then converted them to unicode when doing file operations on
> Windows. Since it doesn't, though, things are likely to continue to not work
> as expected.
I changed NSS so that it consistently uses UTF-8 for "sql:" prefixed configdir. All other file paths will still use ANSI encoding.
Backout by apavel@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/6705010893cd
Backed out 4 changesets (bug 1428538, bug 1420060) for mochitest mass failures UPGRADE_NSS_RELEASE on a CLOSED TREE
Comment 7•7 years ago
|
||
Backed out for mochitest mass failures
Push with failures: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=7604bcd50b63cc8f3e867d8844d7bab871dc0af3
Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=155649398&repo=mozilla-inbound&lineNumber=3301
Backout: https://hg.mozilla.org/integration/mozilla-inbound/rev/6705010893cd6d0a72f6adbcb81a92f052a041c2
Assignee | ||
Comment 8•7 years ago
|
||
> Push with failures: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=7604bcd50b63cc8f3e867d8844d7bab871dc0af3
Isn't it cased by bug 1382251?
https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=6159c8eb544245f0a3ed8766608202ee72530101
This changeset (just before this backout) did not have so much failures (oranges looks known intermittent.)
Flags: needinfo?(apavel)
Comment 9•7 years ago
|
||
Hi, i was just about to update the other bug from that pus as both bugs were backed out.
The problematic failures where here: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=7604bcd50b63cc8f3e867d8844d7bab871dc0af3
Reason why this was backed out alongside bug 1420060.
Bug 1382251 was also backed out for other mochitest failures, specifically for mochitest assertion failures at /builds/worker/workspace/build/src/obj-firefox/dist/include/mozilla/RefPtr.h:318
Aryx: if you have anything to add here, please do.
Flags: needinfo?(apavel) → needinfo?(aryx.bugmail)
Comment 10•7 years ago
|
||
The mochitest-headless failures at https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=68e0118f11a2e8a71aaff24b57384b15fa087977&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry&filter-resultStatus=usercancel&filter-resultStatus=runnable start with those NSS patches and end after the backout. Both bugs got backed out because this one also touches UTF-8 and the follow-ups used the other bug in the commit message.
If the changes for this bug don't cause any of the seen issues and don't depend on bug 1420060, please reland. Thank you.
Flags: needinfo?(aryx.bugmail)
Assignee | ||
Comment 11•7 years ago
|
||
(In reply to Sebastian Hengst [:aryx][:archaeopteryx] (needinfo on intermittent or backout) from comment #10)
> The mochitest-headless failures at
> https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=68e0118f11a2e8a71aaff24b57384b15fa087977&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry&filter-resultStatus=usercancel&filter-resultStatus=runnable
> start with those NSS
> patches and end after the backout. Both bugs got backed out because this one
> also touches UTF-8 and the follow-ups used the other bug in the commit
> message.
Actually mochitest-headless failures start at bug 1382251 patches at:
https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=cc3ec37e0dbe422223a699ff76b94cbc11b28b45
(bug 1428538 and bug 1420060 was already landed here.)
and end after the backout:
https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=6159c8eb544245f0a3ed8766608202ee72530101
(bug 1428538 and bug 1420060 was not backed out yet here.)
Two bustage fixes were about "build" failures (not about test failures). I think we can just reland bug 1428538 and bug 1420060.
Assignee | ||
Comment 12•7 years ago
|
||
Comment 13•7 years ago
|
||
Pushed by kaie@kuix.de:
https://hg.mozilla.org/integration/mozilla-inbound/rev/95e2f1556c17
Use UTF-8 file paths for NSS database. r=keeler
Updated•7 years ago
|
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla59
Comment 14•7 years ago
|
||
bugherder |
Assignee | ||
Comment 15•7 years ago
|
||
Assignee | ||
Updated•7 years ago
|
Attachment #8942409 -
Flags: review?(dkeeler)
Comment on attachment 8942409 [details] [diff] [review]
Reression test to make sure non-ASCII file paths work
Review of attachment 8942409 [details] [diff] [review]:
-----------------------------------------------------------------
I'd prefer this be a separate, more specialized test rather than changing the original cert overrides test to run with this particular setup (feel free to `hg cp test_cert_overrides_read_only.js` and remove the unnecessary extra leftovers).
Also, in the future I'd prefer that tests either be landed at the same time as fixes or as part of a separate bug to avoid complicating our understanding of what landed when (I realize that since someone else pushed this before you marked it "checkin-needed" in this case).
Attachment #8942409 -
Flags: review?(dkeeler)
Assignee | ||
Comment 17•7 years ago
|
||
(In reply to David Keeler [:keeler] (use needinfo) from comment #16)
> I'd prefer this be a separate, more specialized test rather than changing
> the original cert overrides test to run with this particular setup (feel
> free to `hg cp test_cert_overrides_read_only.js` and remove the unnecessary
> extra leftovers).
> Also, in the future I'd prefer that tests either be landed at the same time
> as fixes or as part of a separate bug to avoid complicating our
> understanding of what landed when (I realize that since someone else pushed
> this before you marked it "checkin-needed" in this case).
Filed bug 1430973. I'll attach a new patch there.
Updated•7 years ago
|
You need to log in
before you can comment on or make changes to this bug.
Description
•