make implicit typecast explicit

[SethArchambault]

This came out of adding these compile options:
-Werror -Wall -Wextra -Wshadow -Wconversion -Wno-deprecated-declarations -Wno-unused-parameter

And finding this one error:

$ make CC="clang -Werror -Wall -Wextra -Wshadow -Wconversion -Wno-deprecated-declarations -Wno-unused-parameter"
clang -Werror -Wall -Wextra -Wshadow -Wconversion -Wno-deprecated-declarations -Wno-unused-parameter -pedantic -Wall -Wextra -O3 -march=native -I src -I src/optional -fPIC -c -o lib/monocypher.o src/monocypher.c
src/monocypher.c:881:40: error: implicit conversion loses integer precision: 'u64' (aka 'unsigned long long') to 'u32' (aka 'unsigned int') [-Werror,-Wshorten-64-to-32]
                                        u32  index     = lane * lane_size + (u32)ref;
                                             ~~~~~       ~~~~~~~~~~~~~~~~~^~~~~~~~~~
1 error generated.
make: *** [lib/monocypher.o] Error 1

Easy fix:

/src/monocypher.c
-u32  index     = lane * lane_size + (u32)ref;
+u32  index     = (u32)(lane * lane_size) + (u32)ref;

Though - I guess there's now the question - does this loss of precision matter under any circumstances?

[LoupVaillant]

Good catch, but I think we’ll need to change it a little.

In Argon2, all user provided sizes are u32 (from the specs). Monocypher’s API matches this. So the index, lane, lane_size, and ref should all be u32 … lemme check… OK, got it. Line 866, I’m defining lane as a u64:

	u64 lane         =
		pass == 0 && slice == 0
		? segment
		: (index_seed >> 32) % config.nb_lanes;

I believe this was a mistake:

• segment is a u32, so no overflow could happen there.
• config.nb_lanes is a u32, so no overflow there either. index_seed must definitely be a u64, but with the right shift it goes right back to 32 significant bits. Maybe we need an explicit cast there.

When we compute the index however the multiplication of lane and lane_size could overflow… except (i) they cannot, because nb_lanes * lane_size <= nb_blocks to begin with, and nb_blocks is a u32. And (ii) even if it did I think we’d get the same result after truncation. Oh, and the reference set stays in one lane, so the entire operation cannot possibly overflow.

---

To be tested, but I’m pretty sure the proper fix here is to:

• Line 866, lane should be a u32 (may require a cast or two)
• Line 880, ref should be a u32 (definitely requires a cast, possibly of the whole expression).
• Line 881, no cast should be needed at all. It’s all u32 at this point, with no overflow.

Could you try and change your PR to make that fix? You found the warning, you should take credit for the fix. If it passes the tests that will likely be good to merge.

[SethArchambault]

Cool - just changing the types works, and tests pass!

[LoupVaillant]

Perfect, thank you! I’ll need to pay special attention to line 880 (u32 ref = (window_start + z) % lane_size;), but if the test pass that’s probably correct. Merging right away.