Cast to void

[nenad]

Prevent warnings from popping up in GCC >= 8.1

In file included from utp_api.cpp:26:
utp_internal.h: In constructor ‘UTPSocketKey::UTPSocketKey(const PackedSockAddr&, uint32)’:
utp_internal.h:79:32: warning: ‘void* memset(void*, int, size_t)’ clearing an object of non-trivial type ‘struct UTPSocketKey’; use assignment instead [-Wclass-memaccess]
   79 |   memset(this, 0, sizeof(*this));
      |                                ^
utp_internal.h:74:8: note: ‘struct UTPSocketKey’ declared here
   74 | struct UTPSocketKey {
      |        ^~~~~~~~~~~~

Resolved like in this issue: oneapi-src/oneTBB#54

[rmcdonald]

How about removing this line to resolve the problem? This structure contains two members, both of which are initialized in this constructor in statements immediately after this statement, so the memset call here seems unnecessary. I would hope that code isn't depending that any inter-member space in this structure be initialized to zeroes. Could even change the following assignments to be within an initialization list.

[nenad]

I am not experienced in C or C++, so I started reading around memset and usage in zeroing out structures. Seems like this is done intentionally, so that we don't leak information about the structure through the network. Since it's a bit controversial, I'd prefer if we don't remove it.

[mct]

It's been a number of years now, but if I recall correctly this line was added to try to work around a bug in the hash table implementation which was later fixed by Art. It may not be needed now, but you'll probably want to do a bunch of QA testing first to confirm that.

[rmcdonald]

If this structure is passed in its entirety via the network, I could see clearing it out to avoid disclosure. Either for this reason, or for the case that this was done to avoid a hash table bug, I'd like to see a comment that explains this inserted before the memset() call if the memset() line is retained, since this would be non-obvious valuable information that could easily be lost.

[nenad]

@rmcdonald / @mct Could you ping someone who worked on this to add the comment about the reason?

[arvidn]

my recollection is that the hash table implementation used in uTorrent (not sure if it's still used in libutp) hashes the storage bytes of the key, and compares them with memcmp(). In other words, the padding bits are used by the hash table (or at least "were", it may have changed). I would think that's at least the reason for this call to memset().

As an alternative fix for this, the hash table could have an assert like this:

static_assert(std::has_unique_object_representations_v<Key>, "Key type may not have padding bits")

Then it would be safe to remove the memset() here (afaict).

[nenad]

I believe these are some places in the code where we have hints (as far as I could understand) that there might be some reasoning behind the memset.

https://github.com/bittorrent/libutp/blob/master/utp_hash.h#L43-L75
https://github.com/bittorrent/libutp/blob/master/utp_hash.cpp#L113-L118

I don't know how to test this properly and would appreciate any help.

[nenad]

Apologies for breaking the silence 🙊. Is the PR good enough to be merged as it is without removing the line?

[nenad]

Hi again! I was going through the list of my PRs and I noticed this one hasn't been active for quite some time. Personally, I find warnings in my build logs not pretty, and this PR addresses that issue. Would it be able to get this merged, or should I close it and leave the warnings?

[Jorropo]

Hi, what is the status of this PR pls ?
I'm using libutp in golang and that (a small okay but still one) problem, I'm making a lib with it and gcc warning are not a good figure to end user even if there is 5 lib layer between libutp and the end user.

[arvidn]

I would approve a PR that removes the memset and adds the static_assert I suggested earlier. nut I don’t have privileges to merge

[anacrolix]

Any update on this? Would anyone be willing to merge this?

[arvidn]

I do not think this proposed patch is a good solution to the GCC warning. The warning is right, this patch just silences it instead of fixing the code.