Closed Bug 90956 Opened 23 years ago Closed 22 years ago

Dot (.) instead of localized characters in pipnss module

Categories

(Core Graveyard :: Security: UI, defect, P4)

1.0 Branch
defect

Tracking

(Not tracked)

VERIFIED FIXED
psm2.2

People

(Reporter: m.wawoczny, Assigned: KaiE)

References

Details

(Keywords: intl, l12y, Whiteboard: [adt2 RTM] [eta 6/24])

Attachments

(8 files, 8 obsolete files)

(deleted), image/gif
Details
(deleted), image/gif
Details
(deleted), image/gif
Details
(deleted), image/gif
Details
(deleted), image/gif
Details
(deleted), image/gif
Details
(deleted), patch
KaiE
: review+
Details | Diff | Splinter Review
(deleted), image/gif
Details
I can see only dot (.) in some dialogs from pipnss module. See atached screenshoot. Chars are coded in escape-unicode, so this is not problem with coding (see second screenshoot where localized characters are viewed properly - this screenshoot is from composer). This happen with .properties files. String for this dialog is in pipnss.properties - InternalToken/PrivateTokenDescription property.
I cannnot find "pipnss.properties" in lxr.mozilla.org. Could you specify the full path of the file?
Could you explain what is wrong in the first screen shot? Is the problem in the window title, buttons, or the alert text? The alert text actually shows some non ASCII characters.
Keywords: l12y
In first screenshoot we got "Programowe Urz.dzenie Zabezpiecz." instead of "Programowe Urz±dzenie Zabezpieczaj±ce". Char "±" (escape-unicode code is \u0105) is viewed as ".". Second "Zabezpieczaj±ce" was trimed to "Zabezpiecz." "Programowe Urz±dzenie Zabezpieczaj±ce" = english "Software Security Device" = escape-unicode "Programowe Urz\u0105dzenie Zabezpieczaj\u0105ce" String is escape-unicode, when it is UTF-8 char also aren't displayed corectly.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Thank you for the info. Change component to security and reassign to mstoltz@netscape.com, cc to l10n people.
Assignee: nhotta → mstoltz
Component: Internationalization → Security: General
Keywords: intl
Changed QA contact to ylong@netscape.com.
QA Contact: andreasb → ylong
NSS/PSM issues should be assigned to the PSM product, not the Security:General component.
Assignee: mstoltz → ssaux
Component: Security: General → Client Library
Product: Browser → PSM
QA Contact: ylong → junruh
Version: other → 2.0
P4 t->2.1
Priority: -- → P4
Target Milestone: --- → 2.1
Moving all P3 and P4 bugs targetted to 2.1 to future.
Target Milestone: 2.1 → Future
Mass assigning QA to ckritzer.
QA Contact: junruh → ckritzer
Keywords: intlnsBranch
Marking nsbranch- as it was decided in the August bug triage that we wouldn't have enough time in eMojo to fix this. Let's revisit for MachV.
Keywords: nsbranch-
removed keyword nsbranch since bug now has nsbranch-, per pdt mtg.
Keywords: nsbranch
OS > all
OS: Windows 2000 → All
QA Contact: ckritzer → junruh
Hardware: PC → All
Version: 2.0 → 2.1
Blocks: 107067
Keywords: nsbranch-
ssaux - can you please provide status on this one. It would be nice to have it fixed for machv. thanks
The string used for "Software Security Device" must be UTF8 encode as per the PKCS 11 specs, and the encoded string of byte is limited to 32 bytes. If the string is less then 32 bytes, the string must be padded with blanks. The string that fails to display correctly in that language would take more than 32 bytes to store in UTF8 and therefore is not valid. The string needs to use abbreviations and the translators need to understand that whatever string they come up with must fit in a 32 byte quantity. I'm invesigating whether a string of the proper length with "±" will display correctly.
Seems that other strings connected with Security Devices are screwed up. See another screenshoot. > The string used for "Software Security Device" must be UTF8 encode Why in UTF8 and not unicode escape - this strings are in .properties file which should be unicode escape? > I'm invesigating whether a string of the proper length with "±" will display > correctly. I've checked this with UTF8 and escape unicode and this is not working :( Build ID 2001111708
Attached image Device Manager Window (deleted) —
The UTF8 requirement is as per the PKCS11 specs. We need help from the internationalization group to get a working dev env with the proper language packs. CVS only stores en-US, and that's just not enough. I need the ability to regenerate localized jar for the affected language to reproduce the bug in a dev env.
UTF8 is the standard chosen by the PKCS #11 working group as a way to internationalize PKCS #11, which originally specified the strings as ASCII strings. These strings are passed in fixed arrays of 32 bytes, padded with spaces (optionally terminated with NULLs). We can specify the strings anyway we want, but the must be converted to UTF8 and truncted to 32 bytes somewhere in the code, then returned back as UTF8 before display. In Communicator these restrictions were even more strictly enforced (strings passed down to for rejected if the were not exactly 32 bytes long). This meant that formatting problems with those strings were found very early in the translation process, and the requirements quickly communicated back. As a result of one of these, it was requested that we allow strings that do not meet the exact formating requirements, and reformat the string internally. I believe we need to provide clear documentation on these properties so translaters know that must fit their strings in 32 bytes, either by abreviating some of the words in a meaningful way in that language, or by providing and alternate, shorter phrase to describe the software security token (other than direct translation). There are about 8 or so strings that have these requirements (some limitted to 32 bytes, others limitted to 64 bytes.) bob
> We need help from the internationalization group to get a working dev env with > the proper language packs. CVS only stores en-US, and that's just not enough. I > need the ability to regenerate localized jar for the affected language to > reproduce the bug in a dev env. Any chance we can use the contributed langpacks in http://www.mozilla.org/projects/l10n/mlp_status.html#contrib for testing purpose?
Comments from rchen: For the debugging purpose, you don't need the correct translation which we may not even have. You can just copy the any strings from any files in the same encoding which you want to verify. Does bug 90956 happen to Ja strings? Will our JA files help to verify the bug? Anyhow, I attach JA pipnss.properties in the email. The so mentioned property file will be attached.
How do I install this in the dev env? Do I make a copy of the entire en-US dir? I'm asking because one of the things I tried was to changed the en-US/pipnss.properties file by adding the character that fails to show up, but as a result, Mozilla ends up using a completely different set of strings for the security device dialog. It appears that while loading the file, something goes wrong and some default file which I was unable to locate is loaded instead. I don't want a released package, I want something I can compile in.
You can just replace the English strings with the Unicode strings for debugging. You don't need to change the environment. I copy some of them here: SignedBy=\u7f72\u540d\u8005\uff1a%S CertPassPrompt=%S \u306e\u30de\u30b9\u30bf\u30fc \u30d1\u30b9\u30ef\u30fc\u30c9\u3092\u5165\u529b\u3057\u3066\u304f\u3060\u3055\u 3044\u3002 RootCertModuleName=Builtin Roots Module ManufacturerID=Mozilla.org LibraryDescription=PSM Internal Crypto Services TokenDescription=Generic Crypto Services PrivateTokenDescription=Software Security Device SlotDescription=PSM Internal Cryptographic Services PrivateSlotDescription=PSM \u79d8\u5bc6\u30ad\u30fc FipsSlotDescription=PSM Internal FIPS-140-1 Cryptographic Services FipsPrivateSlotDescription=PSM FIPS-140-1 User Private Key Services InternalToken=Software Security Device VerifySSLClient=SSL \u30af\u30e9\u30a4\u30a2\u30f3\u30c8\u8a3c\u660e\u66f8 VerifySSLServer=SSL \u30b5\u30fc\u30d0\u8a3c\u660e\u66f8 VerifySSLStepUp=SSL \u30b5\u30fc\u30d0\uff0f\u30b9\u30c6\u30c3\u30d7\u30a2\u30c3\u30d7 VerifySSLCA=SSL \u8a8d\u8a3c\u5c40 VerifyEmailSigner=\u96fb\u5b50\u30e1\u30fc\u30eb\u7f72\u540d\u8005\u8a3c\u660e\u 66f8
And you need to switch to Japanese font to display them correctly.
Ok so I updated one string in my locale/en-US/pipnss.properties to read: PrivateTokenDescription=Software Securit\u0105 Device Which should display the character. The character is indeed loaded, but the window shows a little square instead of the expected character. I must not have the fonts for the character. Can somebody help? This won't be fixed unless somebody with the correct expertise helps us.
This is what I see when I look at comment # 6 in this bug. Is it what should appear in the device manager window? When I try to copy and paste this character from the bug window to the pipnss.properties and recompile pipnss.properties, the files doesn't load and NSS default strings are used. When I use \u0105, I get the little square. I'll attach a picture of that.
Stephane, I think you are doing fine. \u105 is a Latin Extended. I think you need to have a right font to display it. Let me dig out more info for you.
So You see right character, char in http://bugzilla.mozilla.org/attachment.cgi?id=58452&action=view is ISO-8859-2 encoded "±" char. Mayby this 32 byte strings should be in *.dtd file not *.properties? Default encoding for *.dtd files is UTF8 (like in PKCS 11 specs) but for *.properties it is escape-unicode. Mayby thats why this strings are wrong displayed?
Well, there is only one unicode. It doesn't matter it come from UTF8 or escape sequence. Internally browser converts them to unicode.
it seems to matter for this bug. I can add escape sequences, but not raw characters: If I add the following to my pipnss.properties file: PrivateTokenDescription=So±tware Securit\u0105 Device then something goes wrong, and the values I get in the device manager are not the values that come from the file, but from the hard-coded default values in the NSS libraries, and that's true of all the strings from the device manager.
Another example, CRL validation, in "CRL settings" window I've got "." instead of some chars, but when I'm getting message box which says that new certificate is older (hmmm... this is only example) all chars are viewed properly. Stephane Saux: I can make language packs which will be working for your build, please send me en-*.xpi and US.xpi. Or there is another method: Unpack all lang files and change in installed-chrome.txt and chrome.rdf files jar:resource:/chrome/en-US.jar!/ jar:resource:/chrome/en-win.jar!/ jar:resource:/chrome/en-linux.jar!/ jar:resource:/chrome/en-mac.jar!/ jar:resource:/chrome/US.jar!/ to resource:/chrome/ All strings will be taken from unpacked lang pack (after unpacking check directory structure - it should be the same as in *.xpi).
Attached image CRL settings window and message box (deleted) —
No longer blocks: 107067
Keywords: nsbeta1
nsbeta1+
Keywords: nsbeta1nsbeta1+
I think I have a fix for this bug. The CRL issue is a separate one and a new bug should be filed for this.
Attached patch Convert to/from UTF8 when appropriate (obsolete) (deleted) — Splinter Review
When creating the software security device, take the ucs2 strings and convert to UTF8. Convert from utf8 to unicode for the token name when displaying in the certManager.
kai please review. Bug depends on 125728.
Depends on: 125728
Keywords: patch, review
Comment on attachment 70214 [details] [diff] [review] Convert to/from UTF8 when appropriate The patch looks reasonable to me. As Bob said PKCS11 uses UTF8 internally, this should work. r=kaie
Attachment #70214 - Flags: review+
Should we set the milestone to mozilla 1.0? since we have the fix at hand?
Setting milestone to 2.2 Unfortunately, the patch needs some additions to completely nail this. I'll post a more comprehensive patch soon.
Target Milestone: Future → 2.2
Comment on attachment 70214 [details] [diff] [review] Convert to/from UTF8 when appropriate Needs work. need so change a few more places.
Attachment #70214 - Flags: needs-work+
This bug also depends on 127345
Depends on: 127345
Attached patch updated patch. (obsolete) (deleted) — Splinter Review
This handles all the places where pk11 strings need to be displayed: certManager. device manager. set password. get password. change password.
Attachment #70214 - Attachment is obsolete: true
Attached patch updated patch to eliminate an assertion. (obsolete) (deleted) — Splinter Review
Attachment #70997 - Attachment is obsolete: true
Kai please review this patch.
*** Bug 113497 has been marked as a duplicate of this bug. ***
This patch fixes the issue I brought up in bug 113497. Thanks. However, I would also like it you could add localization comments to the properties file telling translators which values can only be 32/64 bytes long. Otherwise, it is trial and error.
Stephane, the idea of adding comments to the locale files sounds like a good idea to me, do you want to add them?
Attached patch Add localization comments to pipnss.properties. (obsolete) (deleted) — Splinter Review
Attachment #71011 - Attachment is obsolete: true
Kai, can you review.
Comment on attachment 76336 [details] [diff] [review] Add localization comments to pipnss.properties. >+ tokenName="_blank"; I don't understand why you do this in the exception case. Is "_blank" the name of a special token? If not, should we just set it to ""? >- char *modNameCString = ToNewCString(modName); >+ NS_ConvertUCS2toUTF8 modNameUTF8(modName); > int modType; >- SECMOD_DeleteModule(modNameCString, &modType); >- SECMOD_AddNewModule(modNameCString, fullModuleName, 0, 0); >+ SECMOD_DeleteModule((char *)modNameUTF8.get(), &modType); >+ SECMOD_AddNewModule((char *)modNameUTF8.get(), fullModuleName, 0, 0); I understand that NSS functions expect non-const pointers, and that you wanted to avoid making a copy. I would like to suggest, that instead of the hard cast, use: NS_CONST_CAST(new-type, variable) That way, you can search the sources for that kind of cast, when someone is looking for potential problems. >- mTokenLabel.AssignWithConversion((char *)tok_info.label, >- sizeof(tok_info.label)); >+ nsCString tmpLabel((char *)tok_info.label, sizeof(tok_info.label)); >+ mTokenLabel = ToNewUnicode(NS_ConvertUTF8toUCS2(tmpLabel)); Your change is fine, but I'm not sure whether that extra ToNewUnicode call is necessary. Either this assignment transfers ownership of the value, then it is necessary. But if the assignment operator of the target string class will create a copy, then this call produces a leak. We should find out. In addition, to avoid another temporary copy of the string, you should not use nsCString. Please use nsDependentCString instead, which will create a read-only wrapper object. (both multiple times in your patch) > NS_IMETHODIMP > nsPKCS11Module::GetLibName(PRUnichar **aName) > { >+ if ( mModule->dllName ) { > *aName = ToNewUnicode(NS_ConvertUTF8toUCS2(mModule->dllName)); >+ } else { >+ *aName = NULL; >+ } In this case, the call to ToNewUnicode is correct.
Is this going to make it into Mozilla 1.0?
I need an additional fix.
Attached patch Updated patch to address Kai's comments (obsolete) (deleted) — Splinter Review
I also added code to extract the largest possible uft8 string from strings given to us by external libraries. Without this code NS_ConvertUTF8toUCS2 could fail.
Attachment #76336 - Attachment is obsolete: true
I'm posting a new patch that pulls the new functions out of nsNSSComponent.cpp into new files. We need this fix for RTM.
Whiteboard: [adt2 RTM]
Attached patch updated patch (obsolete) (deleted) — Splinter Review
Attachment #81964 - Attachment is obsolete: true
Attachment #83633 - Flags: needs-work+
I'm taking the bug. Let me summarize what the current patch does: - moves token names to a parameter block, rather than abusing the window name of a dialog - add comments to inform translators of string size limits - correct handling of token names, no longer handling them as C strings, but as UTF-8 (Note, I yesterday already corrected this one on a larger scope in the context of bug 144823) - when converting from Unicode strings, preparing for giving them to NSS, convert to UTF-8, rather than converting to plain ASCII strings. I'm fine with the above, however the latest patch does more. Stephane, you are trying to handle garbgage strings coming from the driver. I think this should not be handled at the application level, but the NSS level should avoid producing garbage strings. In any case, that other problem is outside the scope of this bug, and I think we should handle it separately. I have already filed bug 145228 and have proposed a different solution. If you agree, please let's discuss it there. I suggest that PSM should expect PKCS#11 strings returned from NSS be valid UTF-8 strings. I'll attach a new patch. Something else I want to change in the patch is using NS_CONST_CAST instead of C-style casts.
Assignee: ssaux → kaie
Attached patch Suggested patch v6 (obsolete) (deleted) — Splinter Review
Attachment #83633 - Attachment is obsolete: true
Btw. Is this patch (http://bugzilla.mozilla.org/showattachment.cgi?attach_id=84049) correct? When I try to change password I'm getting FipsSlotDescription description instead of PrivateTokenDescription (or InternalToken) in alert window. Attaching screenshot. In other windows everything seems fine, dots are no longer displayed :) Second is there any possibility to change 32bit length to 64bit? In some cases I can't translate it (using only 32bit) without changing meaning of string :(
Seems that with new profile everything is fine ;)
Keywords: intl
*** Bug 130988 has been marked as a duplicate of this bug. ***
Attached patch Suggested Patch v7 (obsolete) (deleted) — Splinter Review
After some more discussion with Stephane: - the previous alone makes siuation a bit worse with some pkcs#11 drivers, because we now do UTF-8 conversion, we would display empty strings instead of strings with some garbage in the middle. Therefore Stephane suggested that this should be blocked by bug 145228 - however, I don't want to wait on the NSS bug to be fixed before 90956 lands - while the patch in bug 145228 makes sense to add, I meanwhile learned more about the actual problem. I believe the strings returned from pkcs#11 are always either zero terminated or blank padded, i.e. at least one of both. (I believe if a driver does neither of both, that is a really really buggy driver, and while working around those drivers might make sense (as suggested with bug 145228), I think that should not block this bug 90956). - however, I do agree that we should care for the likelyhood of a zero terminated buffer received from the driver at the application level. Therefore I'm attaching a new patch, that still uses a substring of the whole buffer, if there is no zero termination, but if there is a zero termination, it uses the shorter substring, and therefore the UTF-8 conversion will correctly work up to that position.
Attachment #84049 - Attachment is obsolete: true
Comment on attachment 85541 [details] [diff] [review] Suggested Patch v7 r=ssaux This patch is very important for localization.
Attachment #85541 - Flags: review+
Status: NEW → ASSIGNED
Attached patch Patch v8 (deleted) — Splinter Review
I asked Alec to sr on AIM, and he did not like the wrapper class I introduced. He prefers avoiding it and suggested a way to get rid of it. Basically, the only reason for nsBuffer2DependentCSubstring's existence was to find the valid length of the buffer. It could be either the maximum, if the buffer is fully filled with non-null bytes, or a shorter length, if there is a null found in the string. Alec recommended to use PL_strnlen instead, which does exactly that. I'm attaching a new patch, that has no longer the wrapper class, and has the expressions rewritten to use PL_strnlen.
Attachment #85541 - Attachment is obsolete: true
Comment on attachment 88525 [details] [diff] [review] Patch v8 sr=alecf
Attachment #88525 - Flags: superreview+
Attachment #88525 - Flags: review+
Comment on attachment 88525 [details] [diff] [review] Patch v8 carrying over r=ssaux
Whiteboard: [adt2 RTM] → [adt2 RTM] [eta 6/24]
Checked in to trunk.
Status: ASSIGNED → RESOLVED
Closed: 22 years ago
Resolution: --- → FIXED
junruh - can you pls verify this fix on the trunk, the resolve as verified? thanks!
Blocks: 143047
Verified on 6/21 trunk.
Status: RESOLVED → VERIFIED
Andreas B. You mentioned you could test with the Jap. language pack. When could this be done? Thx. stephane.
Working fine on Polish build, attaching screenshoot :) Thnx for fix :)
Attached image Trunk 20020621 (deleted) —
Stephane, we would need a Japanese language pack which is based on the latest trunk build. rchen, yxia, is this something you could help us with?
verifying for l10n based on Comment #75 From Marek Wawoczny, and the attachment. looks good! adt1.0.1+ (on ADT's behalf) approval for checkin to the 1.0 branch, pending drivers approval. pls check this in asap, then add the "fixed1.0.1" keyword.
Keywords: adt1.0.1adt1.0.1+
Andreas, apparently, japanese is only available for branch builds. The test with polish that Marek performed (see comment #74 and #75) shows that using a conversion from UTF8 is the right thing to do. The Polish test already shows that multibyte utf8 encoded strings do work properly. After that, the japanese test could only reveal problems in Moz utf8 conversion routines, which have been extensively tested outside of PSM. I would certainly welcome as early a test of japanese on the branch as is possible.
Attachment #88525 - Flags: approval+
please checkin to the 1.0.1 branch. once there, remove the "mozilla1.0.1+" keyword and add the "fixed1.0.1" keyword.
Checked in to branch.
Verified that the code was checked into the branch.
verified on the branch with 2002-06-24-1.0.0 build +japanse langpack. All PSM dlgs look correct to me. Cc'ing iqa
*** Bug 155565 has been marked as a duplicate of this bug. ***
Product: PSM → Core
Please use the standard notation for localization warnings: LOCALIZATION NOTE (<entityname>): <explanation> This will help a lot with automated tools, thanks!
Version: psm2.1 → 1.0 Branch
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: