I don't know if this will actually work, or how many of these "fixes" I
need to get msan to be happy on CI. For me locally, it all works fine.
On CI, for some reason it's not fine even though I run in the exact same
docker image as CI.
For coverity, which continues to think we're overrunning buffers when
at this point it's easy to prove we're not. Here would be the corrected
coverity finding:
6. Condition packet_length <= 105 /* 1 + 32 * 2 + 24 + 16 */, taking false branch.
7. Condition packet_length > 1024, taking false branch.
Now packet_length must be > 105 and <= 1024.
12. Condition len1 == packet_length - (89 /* 1 + 32 * 2 + 24 */) - 16, taking true branch.
len1 must be > 0 (105 - 89 - 16) and <= 919 (1024 - 89 - 16).
14. decr: Decrementing len1. The value of len1 is now between 0 and 919 (inclusive).
This is where coverity goes wrong: it thinks len1 could be up to 2147483629.
15. buffer access should be OK. Coverity thinks it's not.
Nothing very noteworthy, I just came across this and made it slightly
more readable.
I'm not making this function `bool` right now because it's used in NGC
and that will break.
As a side-effect, DHT now always accepts LAN discovery packets, even
when LAN discovery is disabled. When LAN discovery is disabled, those
packets are ignored.
These were found by the new cimple type check which is completely
unforgiving to implicit boolean conversions. After this PR, there should
be no more implicit int-to-bool or bool-to-int conversions.
These were found by the new stronger type check in cimple. The one
bugfix is in `crypto_sha512_cmp`, which used to think `crypto_verify_32`
returns bool while actually it's -1/0/1.
This uses mallocfail to further increase coverage using the existing
tests. Also:
* Moved the non-auto "tox_one_test" to gtest. This should be
split into smaller tests later.
* Changed `hole_punching` to `bool`.
Missed a few of those in check-c. check-cimple now catches these with a
stronger type check.
Other changes:
* `ptr + int` must always have the `ptr` first, so `int + ptr` is not
allowed anymore.
* `close` and `time` were shadowing libc functions. `file_data` was
shadowed in a function (and is not a good function name anyway), so
renamed to `send_file_data` which is more descriptive.
* Within a function, all local variables of the same name must have the
same type.
* The `strerror_r` change wasn't necessary, but I kept it because it
seems a bit clearer to me now. `#ifdef`s inside functions are a bit
confusing sometimes.
Previously we would try to send three random TCP relays that we're
connected to to each friend once every 5 minutes. The problem with
this method is that it could take an extraordinarily long time
to share every relay; some relays might be consistently skipped
while others might be sent repeatedly. Moreover, there's no
guarantee that the nodes you try to send are actually online.
This leads to a prety unreliable and flaky way of sharing.
Now we reduce the timer to two minutes, and cycle through the list
trying 3 nodes each share attempt. This guarantees that every online
node in our list gets shared with every friend after a fixed amount of
time (which depends on how many nodes are in the list)
One of these was creating a single 262144 byte stack frame. We now have
a way to check and limit the allocation size of a VLA. The `Cmp_Data`
ones were also fairly large. Now, no allocation is larger than 2KiB
(though rtp.c allocates close to that much).
This commit adds functionality for clients to interact with
the DHT, sending getnodes requests to their peers and receiving
nodes in getnodes responses.
These help static analysis and ubsan. We should eventually have all
functions annotated like this with a cimple check to make sure every
pointer has an explicit nullability annotation. The `nullable`
annotation does nothing in GCC, but will be used by cimple to validate
that every parameter has defined nullability.
It was kind of thread-safe, maybe, but there was a data race that makes
tsan unhappy. We now do interface detection once per Tox instance
instead of once per process.
* Function arguments must use `foo_cb *p` and can't just use `foo_cb p`
* You can no longer cast function pointers (if it's incompatible, you
must wrap the callback). I'm avoiding this with tokstyle exclusions.
