search

genesismining / sgminer-gm

A multi-algo GPU miner
GNU General Public License v3.0
340 stars 146 forks source link

Implicit Type casting in Cryptonight algorithm #107

Open user78713 opened 6 years ago

user78713 commented 6 years ago

FYI, following code is "unsafe" due to implicit castings:

CNKeccak(CNCtx.State, Input, Length); ==> Input is uint8_t* but method expects uint64_t*

Also

void cryptonight(uint8_t *Output, uint8_t *Input, uint32_t Length, int Variant)
{
...
...
for (int i = 0; i < 0x80000; ++i)
{
    uint64_t c[2];
    memcpy(c, CNCtx.Scratchpad + ((a[0] & 0x1FFFF0) >> 3), 16);
   CNAESRnd(c, a); ==>implicit cast from uint64 to uint32

Implicit casting is especially dangerous when dealing with pointers. Might lead to crashes in release mode.

NaN-git commented 6 years ago

Well, sgminer relies on undefined behaviour in several places, i.e. uint8_t tmp[4]; *(uint32_t*) tmp = ...; is undefined due to the different alignment of the data types, but it works on x86. Furthermore little endian is assumed in several places... Therefore the implicit casts are not the biggest issue. I'm cleaning up the code at the moment a little bit, but the CN code is really bad. Furthermore the Monero v7 commit is still broken and needs to be fixed first.