1396301 - verified/kremlib.h:204:23: error: implicit declaration of function 'le64toh' [-Werror=implicit -function-declaration]

Status: RESOLVED FIXED

(NSS :: Libraries, defect, P3)

Description

[Jan Beich]

In file included from ecl/curve25519_64.c:6:
In file included from ecl/../verified/hacl_curve25519_64.h:17:
verified/kremlib.h:307:30: error: implicit declaration of function 'le64toh' is invalid in C99
      [-Werror,-Wimplicit-function-declaration]
    uint128_t l = (uint128_t)load64_le(b);
                             ^
verified/kremlib.h:204:23: note: expanded from macro 'load64_le'
#define load64_le(b) (le64toh(load64(b)))
                      ^
verified/kremlib.h:315:5: error: implicit declaration of function 'htole64' is invalid in C99
      [-Werror,-Wimplicit-function-declaration]
    store64_le(b, (uint64_t)n);
    ^
verified/kremlib.h:205:38: note: expanded from macro 'store64_le'
#define store64_le(b, i) (store64(b, htole64(i)))
                                     ^
verified/kremlib.h:315:5: note: did you mean 'store64'?
verified/kremlib.h:205:38: note: expanded from macro 'store64_le'
#define store64_le(b, i) (store64(b, htole64(i)))
                                     ^
verified/kremlib.h:189:1: note: 'store64' declared here
store64(uint8_t *b, uint64_t i)
^
verified/kremlib.h:322:30: error: implicit declaration of function 'be64toh' is invalid in C99
      [-Werror,-Wimplicit-function-declaration]
    uint128_t h = (uint128_t)load64_be(b);
                             ^
verified/kremlib.h:206:23: note: expanded from macro 'load64_be'
#define load64_be(b) (be64toh(load64(b)))
                      ^
verified/kremlib.h:322:30: note: did you mean 'le64toh'?
verified/kremlib.h:206:23: note: expanded from macro 'load64_be'
#define load64_be(b) (be64toh(load64(b)))
                      ^
verified/kremlib.h:307:30: note: 'le64toh' declared here
    uint128_t l = (uint128_t)load64_le(b);
                             ^
verified/kremlib.h:204:23: note: expanded from macro 'load64_le'
#define load64_le(b) (le64toh(load64(b)))
                      ^
verified/kremlib.h:330:5: error: implicit declaration of function 'htobe64' is invalid in C99
      [-Werror,-Wimplicit-function-declaration]
    store64_be(b, (uint64_t)(n >> 64));
    ^
verified/kremlib.h:207:38: note: expanded from macro 'store64_be'
#define store64_be(b, i) (store64(b, htobe64(i)))
                                     ^
verified/kremlib.h:330:5: note: did you mean 'htole64'?
verified/kremlib.h:207:38: note: expanded from macro 'store64_be'
#define store64_be(b, i) (store64(b, htobe64(i)))
                                     ^
verified/kremlib.h:315:5: note: 'htole64' declared here
    store64_le(b, (uint64_t)n);
    ^
verified/kremlib.h:205:38: note: expanded from macro 'store64_le'
#define store64_le(b, i) (store64(b, htole64(i)))
                                     ^
4 errors generated.

Comment 1

[Jan Beich]

BSDs provide byteswap functions via sys/endian.h. However, NSS is probably better off using compiler builtins on all platforms.

Comment 2

[Benjamin Beurdouche (do not use this account)]

Should we add those

Comment 3

[Eric Rescorla (:ekr)]

Jan,

What's probably easiest here is if you provide us a patch that works on BSD and also on the Tier 1 platforms we CI on, and then we can review that.

Comment 4

[Jonathan Protzenko [:protz]]

These are in the BSD headers, which are guarded via <features.h> on GNU/Linux. Last I dived into these man pages, the following two options make these functions visible on both Ubuntu 14.04 and 16.04 (and KreMLin, when used as a driver for the C compiler, enables them):

-D_BSD_SOURCE -D_DEFAULT_SOURCE

If this is to silence the warnings, then these two options might be helpful in Makefiles.

Comment 5

[Marco Perez [:mpk]]

Attachment: band-aid patch to fix ssl/tls on the BSDs for now

Unfortunately this bug breaks all https connections (at least on my FreeBSD 11-stable box). Attaching a band-aid patch in case some desperate soul's looking for a workaround. Oh, and I hope I got the OpenBSD part right...

Comment 6

[Marco Perez [:mpk]]

Attachment: band-aid patch to fix ssl/tls on the BSDs for now

Ooops!
v2 needed to wipe out a typo affecting DragonflyBSD.
Sorry for the bugspam.

Comment 7

[Jonathan Protzenko [:protz]]

The original version of this file is located at https://github.com/FStarLang/kremlin/blob/master/kremlib/kremlib.h ... would you mind submitting a pull request there? We added support for Solaris this week, so I'd be happy to take in proper support for the BSDs as well.

Thanks!

Comment 8

[Franziskus Kiefer [:franziskus]]

I created https://github.com/FStarLang/kremlin/pull/62

Comment 9

[Marco Perez [:mpk]]

Comment on attachment 8915757 [details] [diff] [review]
band-aid patch to fix ssl/tls on the BSDs for now

Sorry for accidentally mixing up the BSD patch with the Windows block.

Comment 10

[Jan Beich]

Upstream fix looks fine given it's similar to downstream one:
https://svnweb.freebsd.org/ports/head/security/nss/files/patch-bug1396301?revision=450203&view=markup

What we're waiting for to backport it into NSS?

Comment 11

[Benjamin Beurdouche (do not use this account)]

Does this look good ?
https://phabricator.services.mozilla.com/D141

Comment 12

[Landry Breuil (:gaston)]

Great, first splinter, then mozreview, then github, then phabricator :)

(In reply to Benjamin Beurdouche from comment #11)
> Does this look good ?
> https://phabricator.services.mozilla.com/D141

Not 100% for OpenBSD, as i already said in https://github.com/FStarLang/kremlin/pull/62#discussion_r143208459 (and quoting the header itself)

 * This file should be included as <endian.h> in userspace and as
 * <sys/endian.h> in the kernel.

Comment 13

[Benjamin Beurdouche (do not use this account)]

Fixed by using the fix from upstream Kremlin, can you check again please ?

Comment 14

[Landry Breuil (:gaston)]

Even if i havent built-tested any of this (nor experienced the build failure reported in this bug) this looks good to me for OpenBSD.

Comment 15

[Jan Beich]

(In reply to Benjamin Beurdouche from comment #11)
> Does this look good ?
> https://phabricator.services.mozilla.com/D141

Yes. Replacing downstream patch with this one doesn't break build on FreeBSD.
Example build log: http://sprunge.us/PcjE

Comment 16

[Franziskus Kiefer [:franziskus]]

This isn't a simple import (yet). We might land a hotfix and get the upstream changes later (depending on which one's done first).

Comment 17

[Jan Beich]

For Firefox this doesn't break build but https:// no longer works. Nightly (and even Beta now) cannot use --with-system-nss due to Gecko chasing NSS trunk and depending on unstable API (e.g., SSL_UseAltHandshakeType).

Comment 18

[Jan Beich]

needinfo myself to request tracking for Firefox ESR59 as Tor Browser (only tracks ESR) users may want to avoid --with-system-nss per

https://gitweb.torproject.org/tor-browser.git/commit/?id=76cee7a86e72
https://gitweb.torproject.org/tor-browser.git/commit/?id=e57fa6f6f8fd

Comment 19

[Benjamin Beurdouche (do not use this account)]

(In reply to Jan Beich from comment #17)
> For Firefox this doesn't break build but https:// no longer works. Nightly
> (and even Beta now) cannot use --with-system-nss due to Gecko chasing NSS
> trunk and depending on unstable API (e.g., SSL_UseAltHandshakeType).

I am not exactly sure to understand the discussion about API here nor how the breakage is done.
Can you please expand ? Is it just that additional compilation flags are passed to NSS ?

The current plan is to push the fix with the new iteration of HACL* by early
December for integration in NSS 3.35/Firefox 59.

Comment 20

[Franziskus Kiefer [:franziskus]]

The API discussion is unrelated to this bug.

Comment 21

[Benjamin Beurdouche (do not use this account)]

This should be fixed by https://bugzilla.mozilla.org/show_bug.cgi?id=1419173
Can you please confirm ? Thanks

Comment 22

[Jan Beich]

I confirm, Nightly 59 builds/works fine with bundled NSS.

Comment 23

[Jan Beich]

https://hg.mozilla.org/projects/nss/diff/6a27e4b4c92c/lib/freebl/verified/kremlib.h#l1.300

Comment 24

[Jan Beich]

Err, that change didn't land in Nightly 59 yet. https:// works but always says "connection is not secure", even for sites using EV.

Comment 25

[Jan Beich]

https://hg.mozilla.org/mozilla-central/diff/0c40802096cd/security/nss/lib/freebl/verified/kremlib.h#l1.300

Comment 26

[Phabricator Automation]

Attachment: Backporting KreMLin support for BSDs

Comment 27

[Phabricator Automation]

Comment on attachment 8940788 [details]
Backporting KreMLin support for BSDs

Martin Thomson [:mt:] has approved the revision.

https://phabricator.services.mozilla.com/D141