542741 - NSS_VersionCheck checks the runtime version of NSPR is at least that of the one it was built against

Status: RESOLVED FIXED

(NSS :: Libraries, defect)

Description

[Mike Hommey [:glandium]]

NSS_VersionCheck contains the following snippet:
    if (PR_VersionCheck(PR_VERSION) == PR_FALSE) {
        return PR_FALSE;
    }

Which means the NSPR version that is checked is the one from build time. This seems to be a broken concept to me, especially considering how NSPR is supposed to be stable API and ABI. If NSS is not using anything from a newer NSPR at runtime (and symbols usage proves so), I don't see what grants this test at all.

Comment 1

[Mike Hommey [:glandium]]

Attachment: Remove runtime check of NSPR version in NSS_VersionCheck

Comment 2

[Kai Engert (:KaiE:)]

Comment on attachment 627117 [details] [diff] [review]
Remove runtime check of NSPR version in NSS_VersionCheck

There was probably a reason to add this check in the past.

We shouldn't remove it until you have time to prove that it's absolutely unnecessary.

Comment 3

[Mike Hommey [:glandium]]

Comment on attachment 627117 [details] [diff] [review]
Remove runtime check of NSPR version in NSS_VersionCheck

(In reply to Kai Engert (:kaie) from comment #2)
> Comment on attachment 627117 [details] [diff] [review]
> Remove runtime check of NSPR version in NSS_VersionCheck
> 
> There was probably a reason to add this check in the past.
> 
> We shouldn't remove it until you have time to prove that it's absolutely
> unnecessary.

I can't prove it's unnecessary, but I can prove it breaks things, when, in a linux distro, you use a nss built e.g. against nspr 4.9.1, with nspr 4.9, which is just perfectly fine.

Comment 4

[Kai Engert (:KaiE:)]

> I can't prove it's unnecessary, but I can prove it breaks things, when, in a
> linux distro, you use a nss built e.g. against nspr 4.9.1, with nspr 4.9,
> which is just perfectly fine.

But it has the potential to create confusion, if 4.9.1 fixes a bug, and the application version is expected to have that bug fixed, but it doesn't.

Comment 5

[Mike Hommey [:glandium]]

(In reply to Kai Engert (:kaie) from comment #4)
> But it has the potential to create confusion, if 4.9.1 fixes a bug, and the
> application version is expected to have that bug fixed, but it doesn't.

If nss was built against 4.9, and 4.9.1 effectively fixes a bug in nss (which, at this point of nss and nspr lifespan, is almost a stretch), this test is not going to be effective either. If the application using NSS_VersionCheck really wants to know whether the nss/nspr combination is okay, it needs to use both NSS_VersionCheck and NSPR_VersionCheck *itself*, not relying on nss maybe having been built against the right version of nspr.

Comment 6

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

AFAICT, you must always ship a specific version of NSPR with a particular version of NSS, and that you must always build NSS against that version of NSS. Mixing and matcing NSPR and NSS versions is not supported. So, I think this check is working as intended.

Comment 7

[Mike Hommey [:glandium]]

(In reply to Brian Smith (:bsmith) from comment #6)
> AFAICT, you must always ship a specific version of NSPR with a particular
> version of NSS, and that you must always build NSS against that version of
> NSS. Mixing and matcing NSPR and NSS versions is not supported. So, I think
> this check is working as intended.

Then there should be a build-time check that the nspr version being linked to is the right version. There is no such check. So nothing prevents a newer nss to be built against an old nspr, and the resulting nss's NSS_VersionCheck to be happy with any version of nspr above that old version.

Comment 8

[Kai Engert (:KaiE:)]

We discussed this today in the NSS conf call.

Basically we're convinced NSS is doing the right thing.
If you build against a new version, we expect that you use at least that version at runtime.

If you want your application be able to run against an older version of NSPR, then you must build against that older version of NSPR.

When you do packaging, you should ensure that your package version requirements always require at least the version of NSPR that you built against.

I think this is WONTFIX. Maybe Wan-Teh and Bob can jump in with additional arguments if the discussion continues.

Comment 9

[Wan-Teh Chang]

Comment on attachment 627117 [details] [diff] [review]
Remove runtime check of NSPR version in NSS_VersionCheck

This is a thought-provoking question.

It seems that if NSS really requires a particular version
of NSPR, then this PR_VersionCheck call should be moved from
NSS_VersionCheck to NSS_Initialize:

>-    /* Check dependent libraries */
>-    if (PR_VersionCheck(PR_VERSION) == PR_FALSE) {
>-        return PR_FALSE;
>-    }

In other words, NSS should not assert the minimum required
NSPR version only when an application calls NSS_VersionCheck.

But moving the PR_VersionCheck call to NSS_Initialize will
make the original problem even worse.

So I am fine with either simply removing this PR_VersionCheck call,
or replacing the PR_VERSION argument with a hardcoded version
that we need to update manually whenever NSS requires a newer
version of NSPR.

Comment 10

[Mike Hommey [:glandium]]

(In reply to Wan-Teh Chang from comment #9)
> So I am fine with either simply removing this PR_VersionCheck call,
> or replacing the PR_VERSION argument with a hardcoded version
> that we need to update manually whenever NSS requires a newer
> version of NSPR.

Obviously, I am fine with the former ;)
But for the sake of argument, I'll add that a hardcoded version would actually make more sense than PR_VERSION, and while I'd prefer the former, I'd be kind of fine with the latter. If you do the latter, an additional build-time check might be helpful.

Comment 11

[Kai Engert (:KaiE:)]

Comment on attachment 627117 [details] [diff] [review]
Remove runtime check of NSPR version in NSS_VersionCheck

I believe this patch is now obsolete, because it's waiting for the discussion to continue and (eventually) for an updated patch?

Comment 12

[Wan-Teh Chang]

Attachment: Remove runtime check of NSPR version in NSS_VersionCheck, v2

Also remove the comments that say the version checking functions
of the dependent libraries are invoked.

Patch checked in on the NSS trunk (NSS 3.14).

Checking in lib/ssl/ssl.h;
/cvsroot/mozilla/security/nss/lib/ssl/ssl.h,v  <--  ssl.h
new revision: 1.59; previous revision: 1.58
done
Checking in lib/nss/nss.h;
/cvsroot/mozilla/security/nss/lib/nss/nss.h,v  <--  nss.h
new revision: 1.97; previous revision: 1.96
done
Checking in lib/nss/nssinit.c;
/cvsroot/mozilla/security/nss/lib/nss/nssinit.c,v  <--  nssinit.c
new revision: 1.118; previous revision: 1.117
done
Checking in lib/smime/smime.h;
/cvsroot/mozilla/security/nss/lib/smime/smime.h,v  <--  smime.h
new revision: 1.13; previous revision: 1.12
done