Bug 553638

Created attachment 382470 [details]
File: backtrace

Thanks for the report!

Please report also the version of nss and attach the following file:

/etc/pki/nssdb/pkcs11.txt

Thanks in advance!

[lvillani@normandy bip (master)]$ rpm -q nss
nss-3.12.5-2.fc12.x86_64

Created attachment 382476 [details]
pkcs11

Created attachment 382477 [details]
diff

I have just discovered an .rpmsave (I never touched that file myself, though) file in that directory. Attached the diff between the two files.

Looks like that reverting to pkcs11.txt.rpmsave version fixes the issue.

(In reply to comment #6)
> Looks like that reverting to pkcs11.txt.rpmsave version fixes the issue.  

Great!

Otherwise the issue is 100% reproducible, right?

Could you please try the following with the failing configuration?

mv -v /home/lvillani/.pki /home/lvillani/.pki.hidden

(In reply to comment #7)
> 
> Otherwise the issue is 100% reproducible, right?
> 

Yes.

> Could you please try the following with the failing configuration?
> 
> mv -v /home/lvillani/.pki /home/lvillani/.pki.hidden  

[lvillani@normandy ~]$ mv -v /home/lvillani/.pki /home/lvillani/.pki.hidden
`/home/lvillani/.pki' -> `/home/lvillani/.pki.hidden'


[lvillani@normandy devel (master)]$ make new-sources FILES=bip-0.8.4.tar.gz 

Checking : bip-0.8.4.tar.gz on https://cvs.fedoraproject.org/repo/pkgs/upload.cgi...
ERROR: could not check remote file status
make: *** [new-sources] Error 255

Same issue (abrt increases the "crash count", I bet it triggers the same crash).

Thanks for your help! Now I see the nonsense just from that backtrace:

    if (sysdb && !strcmp(filename, sysdb))
            filename = NULL;
    if (userdb && !strcmp(filename, userdb))
            filename = NULL;

Created attachment 382482 [details]
humble fix

I guess the attached patch fixes at least the reported SIGSEGV.

However someone who know what the nsssysinit.c code is supposed to do should write some comments inside. Just to make it easier for other people fixing bugs like this. Thanks in advance!

(In reply to comment #10) See David's; latest comment's on Bug 547860 he has a slightly different way solving the SIGSEV though nsssysinit isn't working yet.

Well, I am new to the code.  Is it documented anywhere?  In particular is get_list() supposed to get non-NULL filename in all cases?  I in fact didn't realize the code causing SIGSEGV is the part of a bugfix :-)

I propose to make the patch for #547860 much more verbose.  Please include some comments into the patch to explain what the new code is supposed to do and how.  Thanks!

Well, there are some comments in the patch.  But what definitely needs to be more visible at first glance is the game you play with NULL vs. non-NULL filename ... which has actually caused this bug.  I propose to add at least assertion before the first strcmp().

Btw. if you really want to get the patch upstream at some point, it is not good idea to use strcmp from glibc at all ;-)

We have come to the point where we are patching each others patches. I'll apply David's changes to the other patches so I can check them in. We need an urgent fix the SIGSEV at the very least. I'll use PL_CompareStrings while I'm at it and see if I can add comments on what is essentially workaround or "a temporary hack" in David's own words :-)

Comment on attachment 382482 [details]
humble fix

Obsoleting the patch - it should be extended to get rid of the strcmp() calls as mentioned in comment #13.

Created attachment 382577 [details]
Patch for SIGSESV

Patch is based on what's described in Comment #17 From David Woodhouse in Bug 547860. Uses PL_CompareStrings and added some added comments which I hope accurately describe the intent of setting filename to NULL as per earlier patch.

Comment on attachment 382577 [details]
Patch for SIGSESV

>@@ -250,9 +251,13 @@ get_list(char *filename, char *stripped_
>     sysdb = getSystemDB();
>     userdb = getUserDB();
> 
>-    if (sysdb && !strcmp(filename, sysdb))
>+	/* Using a NULL filename as a Boolean flag to
>+	 * prevent registering both an application-defined
>+	 * db and the system db. Bug 546211.
>+	 */
>+    if (sysdb && PL_CompareStrings(filename, sysdb))

I suggest to insert PORT_Assert(filename) before the line above and add comment above the function saying the parameter filename must never be NULL.  If you skip this step, you'll most likely run to another SIGSEGV sooner or later, when the get_list() is used elsewhere.  Now you need to go through 3 non-trivial functions to figure out it really can't be NULL.

Additionally I spotted the comment for the get_list() function is not immediately above the function and considered it bug as well.

And maybe one minor nit - "Bug 546211" should be "rhbz #546211".  If you got the patch upstream it would clash with their bz numbers.

Created attachment 382588 [details]
Patch V2

Addresses Kamil's comments. For preliminary review as I still need have testing to do.

Created attachment 382590 [details]
nsssysinit.c after all patches have been applied

The after after all patches. To facilitate Bob's review somewhat as so many patches have been applied.

Comment on attachment 382588 [details]
Patch V2

>@@ -212,6 +213,13 @@ getFIPSMode(void)
> 
> #define NSS_DEFAULT_FLAGS "flags=readonly"
> 
>+static const char *nssDefaultFlags = "trustOrder=75 cipherOrder=100 \
>+slotParams={0x00000001=[slotFlags=RSA,RC4,RC2,DES,DH,SHA1,MD5,MD2,SSL,TLS,AES,RANDOM \
>+askpw=any timeout=30 ] }  ";
>+static const char *nssDefaultFIPSFlags = "trustOrder=75 cipherOrder=100 \
>+slotParams={0x00000003=[slotFlags=RSA,RC4,RC2,DES,DH,SHA1,MD5,MD2,SSL,TLS,AES,RANDOM \
>+askpw=any timeout=30 ] }  ";
>+

I know I am annoying today ... but the above looks pretty subtle for maintenance to me:

1) There is absolutely no info why the hard-wired parameters are there and info about their interaction with the same parameters from pcks11.txt.
2) There is not explained why e.g. 0x00000001 and 0x00000003.
3) There is copy-pasted code, which is generally always wrong.

I guess 1+2 might be solved by adding an URL to the documentation of the parameters and documentation of the nsssysinit. As for the copy-pasted code, what about a macro like this?

#define NSS_FLAGS "trustOrder=75 cipherOrder=100"
#define NSS_SLOT_FLAGS "[slotFlags=RSA,RC4,RC2,DES,DH,SHA1,MD5,MD2,SSL,TLS,AES,RANDOM" \
    " askpw=any timeout=30 ]"

static const char *nssDefaultFlags = NSS_FLAGS " slotParams={0x00000001=" NSS_SLOT_FLAGS " }  ";
static const char *nssDefaultFIPSFlags = NSS_FLAGS " slotParams={0x00000003=" NSS_SLOT_FLAGS " }  ";

>@@ -250,9 +251,14 @@ get_list(char *filename, char *stripped_
>     sysdb = getSystemDB();
>     userdb = getUserDB();
> 
>-    if (sysdb && !strcmp(filename, sysdb))
>+	/* Using a NULL filename as a Boolean flag to
>+	 * prevent registering both an application-defined
>+	 * db and the system db. rhbz #546211.
>+	 */
>+	PORT_Assert(filename);

broken indentation here ^^^

Nothing of the comments above should prevent the patch from being applied, but I'd be happy if they were addressed later.  It will help you to decrease the frequency of filling bug reports against nsssysinit.

Created attachment 382735 [details]
Patch V3

In response to Kamil's review comments this one
1) Points to the Wiki for info on configuration parameters
2) Explains magic constants 0x00000001 and 0x00000003.
3) Avoids copy and pasting duplicated common portions of the literals

Created attachment 382736 [details]
Effective cummulative patch

Provides a wider context for ease of review.

Created attachment 382739 [details]
Test results before and after

Thank you Kamil for suggesting this simple test.

Scratch build at http://koji.fedoraproject.org/koji/taskinfo?taskID=1911830

Created attachment 382772 [details]
fix broken indentation

Thanks for considering the review comments, it looks much better now. I am attaching an incremental patch making the indentation more consistent (no change in behavior).

It looks to me like you have wrongly set your editor for nsssysinit.c - the code uses tabs 8 characters width and shift width of 4 characters, or am I wrong?

*** Bug 554135 has been marked as a duplicate of this bug. ***

(In reply to comment #27)  In the slot comments I should have mentioned http://www.mozilla.org/projects/security/pki/nss/ref/ssl/gtstd.html#1011970
The writing and Fig 2.1 should clarify things quite a bit.

Thanks for the indentation fixes. Yes, that's caused by me switching among eclipse, gedit and vim at different times.

By the way, another test that passes was that as root I added a cert to the system database and not show up in the wrong place, something I will mention in bug 547860.

*** Bug 554291 has been marked as a duplicate of this bug. ***

I can confirm that the scratch build of NSS from comment #26 fixes the issue I was seeing in bug 554291

(In reply to comment #31)
> I can confirm that the scratch build of NSS from comment #26 fixes the issue I
> was seeing in bug 554291    

Same here.

> It looks to me like you have wrongly set your editor for nsssysinit.c - the
> code uses tabs 8 characters width and shift width of 4 characters, or am I
> wrong?  

That's the historical upstream coding standard for all of NSS.

bob

> That's the historical upstream coding standard for all of NSS.

If "historical" means still valid, we should respect it to avoid later re-indentation and make it easier to follow downstream patches. At least my attachment 382772 [details] respects it.

Elio, as for the vim, you want to add following to your ~/.vimrc:

set noexpandtab
set shiftwidth=4
set tabstop=8
set tw=80

... not sure with eclipse and gedit since I don't use them myself.

Comment on attachment 382772 [details]
fix broken indentation

I would use this to clean up my patch a bit rather than a patch on its own.

(In reply to comment #35)
Sure thing ... that's how it has been intended actually :-)

Comment on attachment 382735 [details]
Patch V3

r+ rrelyea

nss-3.12.5-7.fc12 has been submitted as an update for Fedora 12.
http://admin.fedoraproject.org/updates/nss-3.12.5-7.fc12

nss-3.12.5-7.fc12 has been pushed to the Fedora 12 testing repository.  If problems still persist, please make note of it in this bug report.
 If you want to test the update, you can install it with 
 su -c 'yum --enablerepo=updates-testing update nss'.  You can provide feedback for this update here: http://admin.fedoraproject.org/updates/F12/FEDORA-2010-0685

Looks like it's fixed now.

nss-3.12.5-8.fc12 has been submitted as an update for Fedora 12.
http://admin.fedoraproject.org/updates/nss-3.12.5-8.fc12

Created attachment 386711 [details]
Patch V4 fixes regression and cleans up code

Attaching this revised patch here were it belongs. It was reviewed in Bug 545779 and checked in.

nss-3.12.5-8.fc12 has been pushed to the Fedora 12 testing repository.  If problems still persist, please make note of it in this bug report.
 If you want to test the update, you can install it with 
 su -c 'yum --enablerepo=updates-testing update nss'.  You can provide feedback for this update here: http://admin.fedoraproject.org/updates/F12/FEDORA-2010-1127

nss-3.12.5-8.fc12 has been pushed to the Fedora 12 stable repository.  If problems still persist, please make note of it in this bug report.