Red Hat Bugzilla – Bug 804802
SECITEM_CompareItem() return invalid result
Last modified: 2013-01-16 16:44:43 EST
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.
Created attachment 571224 [details] fix SECITEM_CompareItem()
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.
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
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
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
Fixed with the rebase to nss-3.13.4 and we are now at 3.14.1.