hyperboria / bugs

Peer-to-peer IPv6 networking, secure and near-zero-conf.
154 stars 17 forks source link

Node 6 Math.random() entropy changes causing build errors #126

Closed prurigro closed 8 years ago

prurigro commented 8 years ago

Improvements in the node 6.0.0+ Math.random() functionality have changed the values it generates, causing larger hex values to be produced as seen in the following screenshot:

node-random-comparison

This causes (at least) this function to produce values too large for the unsigned long its used to populate in memory/Allocator.c, which results in a build failure with the following output:

/home/prurigro/cjdns/node_build/builder.js:485
            if (err) { throw err; }
                       ^

Error: gcc -c -x cpp-output -o build_linux/memory_Allocator_c.o -std=c99 -Wall -Wextra -Werror -Wno-pointer-sign -pedantic -D linux=1 -D CJD_PACKAGE_VERSION="cjdns-v17.3-dirty" -Wno-unused-parameter -fomit-frame-pointer -D Log_DEBUG -g -D NumberCompress_TYPE=v3x5x8 -D Identity_CHECK=1 -D Allocator_USE_CANARIES=1 -D PARANOIA=1 -DHAS_ETH_INTERFACE=1 -fPIE -fno-stack-protector -fstack-protector-all -Wstack-protector -O3 build_linux/memory_Allocator_c.o.i

memory/Allocator.c: In function ‘Allocator_new’:
memory/Allocator.c:790:56: error: integer constant is too large for its type [-Werror]
             .canary = (unsigned long long) Constant_rand64(),
                                                        ^
memory/Allocator.c:791:60: error: integer constant is too large for its type [-Werror]
             .nextCanary = (unsigned long long) Constant_rand64(),
                                                            ^
cc1: all warnings being treated as errors

    at error (/home/prurigro/cjdns/node_build/builder.js:53:15)
    at /home/prurigro/cjdns/node_build/builder.js:122:22
    at /home/prurigro/cjdns/node_build/builder.js:92:13
    at ChildProcess.<anonymous> (/home/prurigro/cjdns/tools/lib/Semaphore.js:7:30)
    at emitTwo (events.js:106:13)
    at ChildProcess.emit (events.js:191:7)
    at maybeClose (internal/child_process.js:850:16)
    at Socket.<anonymous> (internal/child_process.js:323:11)
    at emitOne (events.js:96:13)
    at Socket.emit (events.js:188:7)

To make sure this wasn't a difference in the hex conversion functionality toString(16), I plugged a value generated by the Math.random() function in v6.0.0 into a variable in v5.11.0 and the toString(16) operation resulted in the same value as v6.0.0 generated.

I asked in #Node.js on freenode and a mod said this change in behaviour is an improvement and not a bug, and won't be returning to how it was done in < 6.0.0. Unfortunately, this means that there are now two sets of possible behavioural styles to consider, and I'm not sure whether it would be best to find a meet-in-the-middle solution that would handle both or simply find a new source for random values (what are opinions on /dev/random these days?).

Kubuxu commented 8 years ago

I will fix it. It should be quite trivial.

prurigro commented 8 years ago

Thanks for taking a look!

Kubuxu commented 8 years ago

@prurigro can you try: https://github.com/Kubuxu/cjdns/tree/fix/js-random

prurigro commented 8 years ago

Build completed successfully, type ./cjdroute to begin setup. -- works like a charm, nicely done! Any idea if that fix will maintain the integrity of the values generated by 5.x and below? Edit: Actually, I misread what you did, your solution seems legit

prurigro commented 8 years ago

This has been merged into https://github.com/cjdelisle/cjdns/tree/crashey and will eventually trickle down into this repo so I see no reason to keep this issue open any longer. Thanks again @Kubuxu!