1512923 - SHA_HTONL function error on arm 32be platform

Status: RESOLVED FIXED

(NSS :: Libraries, defect)

Description

[zhengrq.fnst]

Attachment: 0001-Subject-PATCH-Fix-SHA_HTONL-bug-for-arm-32be.patch

User Agent: Mozilla/5.0 (Windows NT 6.1; Win64; x64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/68.0.3440.106 Safari/537.36

Steps to reproduce:

Rpm use nss as digest crypto library and which will cause an error on
arm 32be platform as follows:
#rpm -qv test-manual-1.2.3-20181012.noarch.rpm



Actual results:

#rpm -qv test-manual-1.2.3-20181012.noarch.rpm
error: test-manual-1.2.3-20181012.noarch.rpm: Header SHA1 digest: BAD
(Expected
f1deb7dc4a10742d88ccd1e967dbc62ae45095a5
!=4ad9d7dad6d70d6086eefec62612ad5d77f2fe81) => this value is wrong
error: test-manual-1.2.3-20181012.noarch.rpm: not an rpm package (or
package manifest)

The error is caused by SHA_HTONL in nss, for there is no need to reverse
the host value for arm 32be as it is originally big endian, so fix it.


Expected results:

No error msg for rpm output.

Comment 1

[zhengrq.fnst]

I have submitted my patch to fix it.
See attachment 0001 [details] [diff] [review]-Subject-PATCH-Fix-SHA_HTONL-bug-for-arm-32be.patch

Comment 2

[J.C. Jones [:jcj] (he/him)]

I guess the question is whether IS_LITTLE_ENDIAN is portable enough for all supported ARM platforms.

Pushed to try: https://treeherder.mozilla.org/#/jobs?repo=nss-try&revision=f42990c833b0d5c8d4ba08e20d13f96b2708de33

Comment 3

[J.C. Jones [:jcj] (he/him)]

Try run looks good. Franziskus - anything else you can think of that I should be wary of before reviewing/landing?

Comment 4

[Franziskus Kiefer [:franziskus]]

(In reply to J.C. Jones [:jcj] (he/him) from comment #2)
> I guess the question is whether IS_LITTLE_ENDIAN is portable enough for all
> supported ARM platforms.
> 
> Pushed to try:
> https://treeherder.mozilla.org/#/jobs?repo=nss-
> try&revision=f42990c833b0d5c8d4ba08e20d13f96b2708de33

That was my question as well. I'm not sure. I don't see `IS_LITTLE_ENDIAN` being set anywhere. The `swap4b` is only defined for a couple ARM architectures so I guess it doesn't hurt adding the `ifdef` here but I'd expect this to break on some other ARM machines.
So there are 3 options
1) don't do anything
2) land the patch as is and wait for reports that something is broken
3) set `IS_LITTLE_ENDIAN` on the ARM machines that are listed in the `ifdef` here

Comment 5

[J.C. Jones [:jcj] (he/him)]

zhengrq:

Do you think you could update the patch to define a portable IS_LITTLE_ENDIAN if it's not already defined?

Comment 6

[zhengrq.fnst]

Hi All:

I Found "IS_LITTLE_ENDIAN" is come from the source code of nspr
https://developer.mozilla.org/en-US/docs/Mozilla/Projects/NSPR

cat nspr/pr/include/md/_linux.cfg

 602 #elif defined(__arm__)
 603
 604 #ifdef __ARMEB__
 605 #undef  IS_LITTLE_ENDIAN
 606 #define IS_BIG_ENDIAN 1
 607 #elif defined(__ARMEL__)
 608 #define IS_LITTLE_ENDIAN 1
 609 #undef  IS_BIG_ENDIAN
 610 #else
 611 #error "Unknown ARM endianness."
 612 #endif

Maybe for ARM architecture, thers is only two effective MACRO "IS_LITTLE_ENDIAN" and "IS_BIG_ENDIAN".

Comment 7

[J.C. Jones [:jcj] (he/him)]

OK, thanks. Pushed as https://hg.mozilla.org/projects/nss/rev/66cae6a8f4b640bec5e8510dcfc247e0546c973c.