Access violation in release builds of agreement_tests under MSVC 2017

[Ralith]

Built from master. Does not occur in unoptimized builds, only those at optimization level 1 or 2. Discovered via a similar crash in routine use of rustls, but reproduces consistently in the unit test. Backtrace from Visual Studio:

[Inline Frame] agreement_tests-e01b6fbaeaaa9310.exe!fe_limbs_zero(unsigned int *) Line 70	C
[Inline Frame] agreement_tests-e01b6fbaeaaa9310.exe!fe_0(fe *) Line 174	C
[Inline Frame] agreement_tests-e01b6fbaeaaa9310.exe!fe_1(fe *) Line 183	C
agreement_tests-e01b6fbaeaaa9310.exe!GFp_x25519_scalar_mult_generic_masked(unsigned char * out, const unsigned char * scalar_masked, const unsigned char * point) Line 1804	C
[Inline Frame] agreement_tests-e01b6fbaeaaa9310.exe!ring::ec::curve25519::x25519::x25519_ecdh::scalar_mult(unsigned char[32] * out, ring::ec::curve25519::scalar::MaskedScalar * scalar, unsigned char[32] * point, ring::cpu::Features) Line 128	Unknown
agreement_tests-e01b6fbaeaaa9310.exe!ring::ec::curve25519::x25519::x25519_ecdh(mut slice<u8>* out, ring::ec::keys::Seed * my_private_key, untrusted::Input peer_public_key) Line 139	Unknown
[External Code]	
agreement_tests-e01b6fbaeaaa9310.exe!test::run_test::run_test_inner::{{closure}}() Line 450	Unknown
agreement_tests-e01b6fbaeaaa9310.exe!std::sys_common::backtrace::__rust_begin_short_backtrace<closure-0,()>() Line 131	Unknown
agreement_tests-e01b6fbaeaaa9310.exe!core::ops::function::FnOnce::call_once<closure-0,()>() Line 232	Unknown
agreement_tests-e01b6fbaeaaa9310.exe!std::sys::windows::thread::{{impl}}::new::thread_start() Line 56	Unknown
[External Code]	

Exception details:

Exception thrown at 0x00007FF7C1AF344B in agreement_tests-e01b6fbaeaaa9310.exe: 0xC0000005: Access violation accessing location 0x0000000000000000. occurred

[ctz]

I can't repro this with 2019 -- what precise version are you using of 2017? From the release notes there are codegen bugs fixed in later versions.

Ultimately the code being compiled is extremely simple:

fe x2;
for (size_t i = 0; i < FE_NUM_LIMBS; ++i) {
  x2.v[i] = 0;  //<- crashes here
}
x2.v[0] = 1;

[Ralith]

This was in Visual Studio 15.2 (26430.6). Is that the right info? I'm not much of a VS guy.

I can't see how the code could possibly be going wrong either.

[briansmith]

Built from master.

Can you reproduce it with a release build of ring from crates.io? Which Rust toolchain are you using (which channel, which version, which build date)?

Does not occur in unoptimized builds, only those at optimization level 1 or 2.

I have noticed that LTO seems to cause problems on Windows in release builds. For example, currently the AppVeyor builds fail in -nightly and now -beta channels of the rust toolchain. All the tests pass up until the doctests are built, and then the doctests fail to link.

Could you try reproducing with lto = true removed from the Cargo.toml?

[briansmith]

I have noticed that LTO seems to cause problems on Windows in release builds. For example, currently the AppVeyor builds fail in -nightly and now -beta channels of the rust toolchain. All the tests pass up until the doctests are built, and then the doctests fail to link.

I forgot to add: removing "lto = true" from the Cargo.toml fixes this doctest linking issue.

[Ralith]

I couldn't find any git tags, so I guessed that 1e97137 was a suitable git commit to try. The test crashes with lto = true, lto = false, and with the lto lines removed entirely.

[Ralith]

Updating to the most recent version of the 2017 build tools appears to have resolved the error, so maybe this should be closed as a fixed miscompile.

[briansmith]

I'm doing some work that should help determine if this is possibly ring's fault. I've also filed issue #1136 to break the build on some ancient versions of Visual Studio. If you have needs for compatibility with ancient versions of Visual Studio, please comment in that issue.

[briansmith]

@Geobert wrote in #1136 (comment), wrote:

Microsoft Visual Studio Professional 2015
Version 14.0.25431.01 Update 3
[....]
(We workaround #1031 by disabling optim in Windows build atm)

[briansmith]

Updating to the most recent version of the 2017 build tools appears to have resolved the error, so maybe this should be closed as a fixed miscompile.

I am indeed going to close this now, based on your suggestion, and based on the fact that nobody seems to be experiencing this.