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

RESOLVED FIXED in 3.14

Status

RESOLVED FIXED
9 years ago
6 years ago

People

(Reporter: glandium, Assigned: glandium)

Tracking

3.12.5
3.14

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

9 years ago
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.
(Assignee)

Updated

7 years ago
Assignee: nobody → mh+mozilla
(Assignee)

Comment 1

7 years ago
Created attachment 627117 [details] [diff] [review]
Remove runtime check of NSPR version in NSS_VersionCheck
Attachment #627117 - Flags: review?(kaie)

Comment 2

7 years ago
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.
Attachment #627117 - Flags: review?(kaie) → review?
(Assignee)

Comment 3

7 years ago
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.
Attachment #627117 - Flags: review? → review?(kaie)

Comment 4

7 years ago
> 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.
(Assignee)

Comment 5

7 years ago
(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.
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.
(Assignee)

Comment 7

7 years ago
(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

7 years ago
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.
Status: NEW → RESOLVED
Last Resolved: 7 years ago
Resolution: --- → WONTFIX

Comment 9

7 years ago
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.
(Assignee)

Comment 10

7 years ago
(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.
Status: RESOLVED → REOPENED
Resolution: WONTFIX → ---

Comment 11

6 years ago
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?
Attachment #627117 - Flags: review?(kaie)

Comment 12

6 years ago
Created attachment 663573 [details] [diff] [review]
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
Attachment #627117 - Attachment is obsolete: true

Updated

6 years ago
Status: REOPENED → RESOLVED
Last Resolved: 7 years ago6 years ago
Resolution: --- → FIXED
Target Milestone: --- → 3.14
You need to log in before you can comment on or make changes to this bug.