fix #121 by initializing memory and avoid breaking strict aliasing rules #122

2024-11-26T04:07:32Z

MagicalTux commented

2024-11-26 04:07:32 +00:00

(Migrated from github.com)

This was quite a journey, but the conclusion is that the issue comes from the following (simplified) code:

sph_dec32le_aligned(const void *src)
{
	return *(const sph_u32 *)src;
}

This converts parts of a const unsigned char* into a sph_u32* which gcc will consider to be pointing to different parts of memory (see: gcc strict aliasing rules).

Considering these are different parts of memory, gcc was feeling OK to move around those reads and writes to the input buffer, resulting in garbage results. When compress_small was called twice in the same function, gcc would apparently assume the reads were from the same piece of memory and group these, or something like that. Because bmw32_close calls compress_small multiple times in sequence, this would trigger the issue.

This was fixed by using a fixed size memcpy, which the compiler should optimize nicely while understanding it is not OK to reorder those reads.

Additionally, bmw32_init/bmw64_init didn't properly initialize memory, which also caused some issues, this is also fixed here.

Fixes https://github.com/monacoinproject/monacoin/issues/121

This was quite a journey, but the conclusion is that the issue comes from the following (simplified) code: ```c sph_dec32le_aligned(const void *src) { return *(const sph_u32 *)src; } ``` This converts parts of a `const unsigned char*` into a `sph_u32*` which gcc will consider to be pointing to different parts of memory (see: gcc strict aliasing rules). Considering these are different parts of memory, gcc was feeling OK to move around those reads and writes to the input buffer, resulting in garbage results. When `compress_small` was called twice in the same function, gcc would apparently assume the reads were from the same piece of memory and group these, or something like that. Because `bmw32_close` calls `compress_small` multiple times in sequence, this would trigger the issue. This was fixed by using a fixed size memcpy, which the compiler should optimize nicely while understanding it is not OK to reorder those reads. Additionally, bmw32_init/bmw64_init didn't properly initialize memory, which also caused some issues, this is also fixed here. Fixes https://github.com/monacoinproject/monacoin/issues/121

👀 1

wakiyamap commented

2024-11-27 03:40:49 +00:00

(Migrated from github.com)

@MagicalTux
I'll try syncing without a checkpoint.
Thanks!

@cryptcoin-junkey
Can I have you try that too?

@MagicalTux I'll try syncing without a checkpoint. Thanks! @cryptcoin-junkey Can I have you try that too?

👀 1

cryptcoin-junkey commented

2024-11-28 07:43:23 +00:00

(Migrated from github.com)

@wakiyamap Thanks for the heads up.
@MagicalTux
Thank you for your contribution.
I read your patch.
In my first impression, I suppose it is reasonable.
And ... Could you give me a couple of days to understand the background of this issue ?
I'm going to read related issues like https://github.com/aidansteele/sphlib/issues/1 .

@wakiyamap Thanks for the heads up. @MagicalTux Thank you for your contribution. I read your patch. In my first impression, I suppose it is reasonable. And ... Could you give me a couple of days to understand the background of this issue ? I'm going to read related issues like https://github.com/aidansteele/sphlib/issues/1 .

MagicalTux commented

2024-11-28 09:47:20 +00:00

(Migrated from github.com)

@cryptcoin-junkey Could you give me a couple of days to understand the background of this issue ?

Sure no problem at all. I'll try to go a bit over what I found out, but each time I'm afraid it's a bit too complex.

https://github.com/MagicalTux/monadebug may be a good way to test this too, just go to the bug branch (the last commit contains the memcpy patch) and attempt to compile & run this with a recent compiler. As far as I saw this bug will affect builds made with vanilla gcc 12 and more recent with -O1 or more (some linux distributions ship gcc with a lot of patches that may alter various behaviors).

The official binary distribution of monacoin is not affected because it uses a older version of gcc: GCC: (Ubuntu 8.4.0-1ubuntu1~18.04) 8.4.0

Gcc rules say that different types of pointers (let's say a uint32_t* and a char*) cannot be pointing to the same part of memory (so called strict aliasing rules) which allows gcc to optimize functions like bmw's compress_small by moving reads together or doing one single read of the uint32_t* pointer even if a write happens on the char*, because gcc assumes those are different beasts.

Knowing exactly what goes wrong would require reading the asm gcc generates, but bmw is a rather complex algorithm and quite frankly I'm not sure there is anything useful we can learn from that we do not already know.

I tried to find various methods to alleviate this but the recommended method is a memcpy with fixed size, and let the compiler optimize things. It looks like it should not be doing anything, yet this bug disappears.

> @cryptcoin-junkey Could you give me a couple of days to understand the background of this issue ? Sure no problem at all. I'll try to go a bit over what I found out, but each time I'm afraid it's a bit too complex. https://github.com/MagicalTux/monadebug may be a good way to test this too, just go to the bug branch (the last commit contains the `memcpy` patch) and attempt to compile & run this with a recent compiler. As far as I saw this bug will affect builds made with vanilla gcc 12 and more recent with -O1 or more (some linux distributions ship gcc with a lot of patches that may alter various behaviors). The official binary distribution of monacoin is not affected because it uses a older version of gcc: `GCC: (Ubuntu 8.4.0-1ubuntu1~18.04) 8.4.0` Gcc rules say that different types of pointers (let's say a uint32_t* and a char*) cannot be pointing to the same part of memory (so called _strict aliasing rules_) which allows gcc to optimize functions like bmw's `compress_small` by moving reads together or doing one single read of the uint32_t* pointer even if a write happens on the char*, because gcc assumes those are different beasts. Knowing exactly what goes wrong would require reading the asm gcc generates, but bmw is a rather complex algorithm and quite frankly I'm not sure there is anything useful we can learn from that we do not already know. I tried to find various methods to alleviate this but the recommended method is a `memcpy` with fixed size, and let the compiler optimize things. It looks like it should not be doing anything, yet this bug disappears.

👀 1

cryptcoin-junkey (Migrated from github.com) reviewed

2024-11-29 02:48:24 +00:00

cryptcoin-junkey (Migrated from github.com) left a comment

A nit-pick. I bear "the section 6.2.6.1 in ISO 9899:2011" in mind.

src/crypto/Lyra2RE/bmw.c

					
				@ -609,6 +609,7 @@ static const sph_u32 final_s[16] = {

				static void

				bmw32_init(sph_bmw_small_context *sc, const sph_u32 *iv)

				{

					memset(sc, 0, sizeof(sph_bmw_small_context));

I think it can have some side effects using memset only (even if they are rare case).
How about this?

	memset(sc, 0, sizeof(sph_bmw_small_context));
	*sc = { 0 };

I think it can have some side effects using `memset` only (even if they are rare case). How about this? ```suggestion memset(sc, 0, sizeof(sph_bmw_small_context)); *sc = { 0 }; ```

src/crypto/Lyra2RE/bmw.c

Same above.

	memset(sc, 0, sizeof(sph_bmw_big_context));
	*sc = { 0 };

Same above. ```suggestion memset(sc, 0, sizeof(sph_bmw_big_context)); *sc = { 0 }; ```

cryptcoin-junkey commented

2024-11-29 03:02:24 +00:00

(Migrated from github.com)

Thank you for your detailed note. @MagicalTux

strict aliasing rules

I figured out the background of this issue.
I remember some older GCCs made warnings around strict aliasing rules.
And I'm not surprised if newer GCCs optimize code using an aliasing.

I raised some suggestions for this PR. You can reject them if you feel "too nit-picking".
Anyway I'll accept this patch.

Thank you for your detailed note. @MagicalTux > strict aliasing rules I figured out the background of this issue. I remember some older GCCs made warnings around strict aliasing rules. And I'm not surprised if newer GCCs optimize code using an aliasing. I raised some suggestions for this PR. You can reject them if you feel "too nit-picking". Anyway I'll accept this patch.

👍 1

MagicalTux (Migrated from github.com) reviewed

2024-11-29 03:06:41 +00:00

src/crypto/Lyra2RE/bmw.c

					
				@ -609,6 +609,7 @@ static const sph_u32 final_s[16] = {

				static void

				bmw32_init(sph_bmw_small_context *sc, const sph_u32 *iv)

				{

					memset(sc, 0, sizeof(sph_bmw_small_context));

I don't think you can do that.

bmw.c: In function ‘bmw32_init’:
bmw.c:612:15: error: expected expression before ‘{’ token
  612 |         *sc = { 0 };
      |               ^
make: *** [<builtin>: bmw.o] Error 1

This would work but look less nice:

*sc = (sph_bmw_small_context){ 0 };

I feel however the memset is more than appropriate considering the structure of sph_bmw_small_context and sph_bmw_big_context only contains integers and arrays of chars or integers, and those structures aren't going to change anytime soon.

I don't think you can do that. ``` bmw.c: In function ‘bmw32_init’: bmw.c:612:15: error: expected expression before ‘{’ token 612 | *sc = { 0 }; | ^ make: *** [<builtin>: bmw.o] Error 1 ``` This would work but look less nice: ```c *sc = (sph_bmw_small_context){ 0 }; ``` I feel however the memset is more than appropriate considering the structure of `sph_bmw_small_context` and `sph_bmw_big_context` only contains integers and arrays of chars or integers, and those structures aren't going to change anytime soon.

cryptcoin-junkey (Migrated from github.com) reviewed

2024-11-30 00:52:52 +00:00

src/crypto/Lyra2RE/bmw.c

					
				@ -609,6 +609,7 @@ static const sph_u32 final_s[16] = {

				static void

				bmw32_init(sph_bmw_small_context *sc, const sph_u32 *iv)

				{

					memset(sc, 0, sizeof(sph_bmw_small_context));