Bug 804802 - SECITEM_CompareItem() return invalid result
Summary: SECITEM_CompareItem() return invalid result
Keywords:
Status: CLOSED CURRENTRELEASE
Alias: None
Product: Fedora
Classification: Fedora
Component: nss
Version: 16
Hardware: Unspecified
OS: Unspecified
high
high
Target Milestone: ---
Assignee: Elio Maldonado Batiz
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
Depends On:
Blocks:
TreeView+ depends on / blocked
 
Reported: 2012-03-19 19:49 UTC by John Dennis
Modified: 2013-01-16 21:44 UTC (History)
3 users (show)

Fixed In Version: nss-3.13.4-1.fc16
Clone Of:
Environment:
Last Closed: 2013-01-16 21:44:43 UTC
Type: ---
Embargoed:


Attachments (Terms of Use)
fix SECITEM_CompareItem() (597 bytes, patch)
2012-03-19 19:52 UTC, John Dennis
no flags Details | Diff


Links
System ID Private Priority Status Summary Last Updated
Mozilla Foundation 737395 0 None None None Never

Description John Dennis 2012-03-19 19:49:01 UTC
SECITEM_CompareItem() 

Look at nss/lib/util/secitem.c:153

and you'll see SECITEM_CompareItem is defined to return a SECComparison enumerated value

SECComparison
SECITEM_CompareItem(const SECItem *a, const SECItem *b)

This is the definition of SECComparision:

typedef enum _SECComparison {
    SECLessThan = -1,
    SECEqual = 0,
    SECGreaterThan = 1
} SECComparison;

Thus only SECLessThan, SECEqual or SECGreaterThan should ever be returned (-1,0,1 respectively)

But these lines of code in SECITEM_CompareItem() will produce an invalid value to be returned.

    rv = (SECComparison) PORT_Memcmp(a->data, b->data, m);
    if (rv) {
	return rv;
    }

That is because memcmp is defined according to POSIX:

------

The sign of a non-zero return value shall be determined by the sign of the difference between the values of the first pair of bytes (both interpreted as type unsigned char) that differ in the objects being compared.

RETURN VALUE

The memcmp() function shall return an integer greater than, equal to, or less than 0, if the object pointed to by s1 is greater than, equal to, or less than the object pointed to by s2, respectively.

------

That means when using memcmp you MUST examine at the SIGN of the return value, not the return value. Some memcmp implementations return the signed difference between the first non-equal octets instead of -1 or 1.

Thus the above code needs to modified thusly:

    int rv;

    ...

    rv = PORT_Memcmp(a->data, b->data, m);
    if (rv) {
	return rv < 0 ? SECLessThan : SECGreaterThan;
    }

The problem manifests itself because there are several places in the code where the result of SECITEM_CompareItem() is directly compared to the enumerated constants SECLessThan or SECGreaterThan. It's also logically incorrect because it's not returning one of the enumerated values.

A patch is attached.

Comment 1 John Dennis 2012-03-19 19:52:19 UTC
Created attachment 571224 [details]
fix SECITEM_CompareItem()

Comment 2 John Dennis 2012-03-19 19:59:00 UTC
BTW, this is causing the python-nss unit tests to fail. We need to release a new version of python-nss but it's unit tests need to pass before we can release it. Who knows what else will fail in NSS or applications or libraries which use NSS due to this.

As far as I can tell the manifestation of the problem is new. I'm guessing it's showing up because the implementation of memcmp is different. I first saw it when running NSS under a new version of valgrind, which in fact supplies it's own version of memcmp.

Comment 3 Kai Engert (:kaie) (inactive account) 2012-03-20 13:45:45 UTC
John, thanks for your report the analysis and the patch.

This bug should be handled upstream.
I've filed https://bugzilla.mozilla.org/show_bug.cgi?id=737395

Comment 4 Kai Engert (:kaie) (inactive account) 2012-03-25 09:55:50 UTC
Your patch has been accepted and added to upstream. The fix will be contained in NSS 3.13.4, and will be available in Fedora when we upgrade to that version.

Thanks again

Comment 5 Fedora End Of Life 2013-01-16 21:22:12 UTC
This message is a reminder that Fedora 16 is nearing its end of life.
Approximately 4 (four) weeks from now Fedora will stop maintaining
and issuing updates for Fedora 16. It is Fedora's policy to close all
bug reports from releases that are no longer maintained. At that time
this bug will be closed as WONTFIX if it remains open with a Fedora 
'version' of '16'.

Package Maintainer: If you wish for this bug to remain open because you
plan to fix it in a currently maintained version, simply change the 'version' 
to a later Fedora version prior to Fedora 16's end of life.

Bug Reporter: Thank you for reporting this issue and we are sorry that 
we may not be able to fix it before Fedora 16 is end of life. If you 
would still like to see this bug fixed and are able to reproduce it 
against a later version of Fedora, you are encouraged to click on 
"Clone This Bug" and open it against that version of Fedora.

Although we aim to fix as many bugs as possible during every release's 
lifetime, sometimes those efforts are overtaken by events. Often a 
more recent Fedora release includes newer upstream software that fixes 
bugs or makes them obsolete.

The process we are following is described here: 
http://fedoraproject.org/wiki/BugZappers/HouseKeeping

Comment 6 Elio Maldonado Batiz 2013-01-16 21:44:43 UTC
Fixed with the rebase to nss-3.13.4 and we are now at 3.14.1.


Note You need to log in before you can comment on or make changes to this bug.