Skip to content
This repository has been archived by the owner on Jan 30, 2023. It is now read-only.

libprio does not compile with MSVC #17

Closed
rhelmer opened this issue Aug 24, 2018 · 6 comments
Closed

libprio does not compile with MSVC #17

rhelmer opened this issue Aug 24, 2018 · 6 comments
Labels
bug Something isn't working help wanted Extra attention is needed

Comments

@rhelmer
Copy link
Contributor

rhelmer commented Aug 24, 2018

Most Firefox builders use Clang/LLVM nowadays but one of our builders still uses MSVC so this came up:

https://treeherder.mozilla.org/logviewer.html#?job_id=195697225&repo=mozilla-central&lineNumber=6090

10:05:12 INFO - z:/build/build/src/third_party/prio/prio/encrypt.c(80): error C2057: expected constant expression
10:05:12 INFO - z:/build/build/src/third_party/prio/prio/encrypt.c(80): error C2466: cannot allocate an array of constant size 0
10:05:12 INFO - z:/build/build/src/third_party/prio/prio/encrypt.c(80): error C2133: 'key_bytes': unknown size
10:05:12 INFO - z:/build/build/src/third_party/prio/prio/encrypt.c(84): error C2057: expected constant expression
10:05:12 INFO - z:/build/build/src/third_party/prio/prio/encrypt.c(84): error C2466: cannot allocate an array of constant size 0
10:05:12 INFO - z:/build/build/src/third_party/prio/prio/encrypt.c(84): error C2133: 'spki_data': unknown size
10:05:12 INFO - z:/build/build/src/third_party/prio/prio/encrypt.c(281): error C2057: expected constant expression
10:05:12 INFO - z:/build/build/src/third_party/prio/prio/encrypt.c(281): error C2466: cannot allocate an array of constant size 0
10:05:12 INFO - z:/build/build/src/third_party/prio/prio/encrypt.c(281): error C2133: 'aadBuf': unknown size
10:05:12 INFO - z:/build/build/src/third_party/prio/prio/encrypt.c(319): error C2057: expected constant expression
10:05:12 INFO - z:/build/build/src/third_party/prio/prio/encrypt.c(319): error C2466: cannot allocate an array of constant size 0
10:05:12 INFO - z:/build/build/src/third_party/prio/prio/encrypt.c(319): error C2133: 'aad_buf': unknown size

I believe this is because MSVC doesn't support VLAs (variable length arrays), it wants the array size to be specified at compile time:

https://stackoverflow.com/questions/5246900/enabling-vlasvariable-length-arrays-in-ms-visual-c

@rhelmer
Copy link
Contributor Author

rhelmer commented Aug 24, 2018

dmajor (our "human-compiler relations" expert :P) suggests:

"given the limited users of PublicKey_import, I'd just set it to the max value currently used (CURVE25519_KEY_LEN+1?) and crash if you need anything bigger"

I think this makes sense, especially given that we call PrioEncoder::IsValidHexPublicKey() https://hg.mozilla.org/mozilla-central/file/190b827aaa2b/dom/prio/PrioEncoder.cpp#l164 before we call PublicKey_import so it's unlikely to actually crash, just throw / reject the promise from JS, if someone messes up the public key in the prefs.

@rhelmer
Copy link
Contributor Author

rhelmer commented Aug 24, 2018

Oh, I see this is also a problem in PrivateKey_decrypt and set_gcm_params.

The size for spki_data is: const int spki_len = sizeof (curve25519_spki_zeros)
For aadBuf / aad_buf it is: #define AAD_LEN (strlen (PRIO_TAG) + CURVE25519_KEY_LEN + GCM_IV_LEN_BYTES)

@henrycg, do you think we could just assign constants for these? I assume strlen and sizeof are being used here for convenience, it looks like these are known and don't change right (or if any of them do we could determine some kind of upper bound?)

rhelmer added a commit to rhelmer/libprio-1 that referenced this issue Aug 24, 2018
@rhelmer
Copy link
Contributor Author

rhelmer commented Aug 24, 2018

Hm. So, I do think it's valuable for libprio to compile w/ MSVC so this is worth fixing, but since this is a Tier-2 platform for Firefox, and confidence is high that we will switch to Clang soon, I might just not build prio on Firefox when MSVC is in use so we don't have to rush on this.

@henrycg
Copy link
Collaborator

henrycg commented Aug 24, 2018

If we get libprio compiling with C90, will that be enough to ensure that it compiles with MSVC? I don't have access to a machine running MSVC, but I can patch libprio to work with C90.

@rhelmer
Copy link
Contributor Author

rhelmer commented Aug 26, 2018

@henrycg I believe C90 support is the only problem, but won't know for sure until we get past this error and see if there are any other problems :)

I am working on disabling Prio for Firefox when built with MSVC in this bug:
https://bugzilla.mozilla.org/show_bug.cgi?id=1485946

But, if it's easy enough to fix, that would be an even smaller change :)

moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this issue Aug 28, 2018
libprio does not currently build with MSVC (since it only supports
C90 as a compiler), this is being worked on upstream at mozilla/libprio#17

As we are almost certainly not going to ship Firefox build with MSVC anymore,
let's disable this to get it working on this Tier-2 platform.

Differential Revision: https://phabricator.services.mozilla.com/D4292

--HG--
extra : moz-landing-system : lando
moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this issue Aug 28, 2018
libprio does not currently build with MSVC (since it only supports
C90 as a compiler), this is being worked on upstream at mozilla/libprio#17

As we are almost certainly not going to ship Firefox build with MSVC anymore,
let's disable this to get it working on this Tier-2 platform.

Differential Revision: https://phabricator.services.mozilla.com/D4292

--HG--
extra : moz-landing-system : lando
jankeromnes pushed a commit to jankeromnes/gecko that referenced this issue Aug 29, 2018
libprio does not currently build with MSVC (since it only supports
C90 as a compiler), this is being worked on upstream at mozilla/libprio#17

As we are almost certainly not going to ship Firefox build with MSVC anymore,
let's disable this to get it working on this Tier-2 platform.

Differential Revision: https://phabricator.services.mozilla.com/D4292
jankeromnes pushed a commit to jankeromnes/gecko that referenced this issue Aug 29, 2018
libprio does not currently build with MSVC (since it only supports
C90 as a compiler), this is being worked on upstream at mozilla/libprio#17

As we are almost certainly not going to ship Firefox build with MSVC anymore,
let's disable this to get it working on this Tier-2 platform.

Differential Revision: https://phabricator.services.mozilla.com/D4292
@rhelmer rhelmer added bug Something isn't working help wanted Extra attention is needed labels Oct 12, 2018
gecko-dev-updater pushed a commit to marco-c/gecko-dev-wordified-and-comments-removed that referenced this issue Oct 3, 2019
libprio does not currently build with MSVC (since it only supports
C90 as a compiler), this is being worked on upstream at mozilla/libprio#17

As we are almost certainly not going to ship Firefox build with MSVC anymore,
let's disable this to get it working on this Tier-2 platform.

Differential Revision: https://phabricator.services.mozilla.com/D4292

UltraBlame original commit: df602a252b663b56faed75655ce13c04884e0ad3
gecko-dev-updater pushed a commit to marco-c/gecko-dev-wordified-and-comments-removed that referenced this issue Oct 3, 2019
libprio does not currently build with MSVC (since it only supports
C90 as a compiler), this is being worked on upstream at mozilla/libprio#17

As we are almost certainly not going to ship Firefox build with MSVC anymore,
let's disable this to get it working on this Tier-2 platform.

Differential Revision: https://phabricator.services.mozilla.com/D4292

UltraBlame original commit: 0400aff8c51861ba348964dce82dfa5742a901b5
gecko-dev-updater pushed a commit to marco-c/gecko-dev-comments-removed that referenced this issue Oct 3, 2019
libprio does not currently build with MSVC (since it only supports
C90 as a compiler), this is being worked on upstream at mozilla/libprio#17

As we are almost certainly not going to ship Firefox build with MSVC anymore,
let's disable this to get it working on this Tier-2 platform.

Differential Revision: https://phabricator.services.mozilla.com/D4292

UltraBlame original commit: df602a252b663b56faed75655ce13c04884e0ad3
gecko-dev-updater pushed a commit to marco-c/gecko-dev-comments-removed that referenced this issue Oct 3, 2019
libprio does not currently build with MSVC (since it only supports
C90 as a compiler), this is being worked on upstream at mozilla/libprio#17

As we are almost certainly not going to ship Firefox build with MSVC anymore,
let's disable this to get it working on this Tier-2 platform.

Differential Revision: https://phabricator.services.mozilla.com/D4292

UltraBlame original commit: 0400aff8c51861ba348964dce82dfa5742a901b5
gecko-dev-updater pushed a commit to marco-c/gecko-dev-wordified that referenced this issue Oct 3, 2019
libprio does not currently build with MSVC (since it only supports
C90 as a compiler), this is being worked on upstream at mozilla/libprio#17

As we are almost certainly not going to ship Firefox build with MSVC anymore,
let's disable this to get it working on this Tier-2 platform.

Differential Revision: https://phabricator.services.mozilla.com/D4292

UltraBlame original commit: df602a252b663b56faed75655ce13c04884e0ad3
gecko-dev-updater pushed a commit to marco-c/gecko-dev-wordified that referenced this issue Oct 3, 2019
libprio does not currently build with MSVC (since it only supports
C90 as a compiler), this is being worked on upstream at mozilla/libprio#17

As we are almost certainly not going to ship Firefox build with MSVC anymore,
let's disable this to get it working on this Tier-2 platform.

Differential Revision: https://phabricator.services.mozilla.com/D4292

UltraBlame original commit: 0400aff8c51861ba348964dce82dfa5742a901b5
@henrycg
Copy link
Collaborator

henrycg commented Feb 18, 2021

My sense is that this is no longer an issue. Let me know if MSVC support is still important.

@henrycg henrycg closed this as completed Feb 18, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bug Something isn't working help wanted Extra attention is needed
Projects
None yet
Development

No branches or pull requests

2 participants