gnrc_sock_ip: fix memcpy()/memset() sizeof-type #6083
Conversation
miri64
added
Type: bug
| @@ -41,17 +41,17 @@ int sock_ip_create(sock_ip_t *sock, const sock_ip_ep_t *local, | |||
| if (gnrc_af_not_supported(local->family)) { | |||
| return -EAFNOSUPPORT; | |||
| } | |||
| memcpy(&sock->local, local, sizeof(sock_ip_t)); | |||
| memcpy(&sock->local, local, sizeof(sock_ip_ep_t)); | |||
There was a problem hiding this comment.
In case like this I prefer to use the variable instead of the type.
memcpy(&sock->local, local, sizeof(sock->local));
And why is the memset before this not changed? I see
memset(&sock->local, 0, sizeof(sock_ip_t));
There was a problem hiding this comment.
Overlooked that. I made some bad experiences with the variable solution "oops that was a pointer" ;-).
There was a problem hiding this comment.
Take a look at all the memset's and memcpy's. We could use some consistency.
Yes, of course you must know what the variable is. But hey, we're programmers. We know. :-)
There was a problem hiding this comment.
Well than I would like to be consistent with gnrc_sock_udp where I also used size notation ;-).
There was a problem hiding this comment.
Can we not discuss minor style questions in a hotfix for the release, please?
| @@ -36,7 +36,7 @@ int sock_ip_create(sock_ip_t *sock, const sock_ip_ep_t *local, | |||
| (local->netif != remote->netif)) { | |||
| return -EINVAL; | |||
| } | |||
| memset(&sock->local, 0, sizeof(sock_ip_t)); | |||
| memset(&sock->local, 0, sizeof(sock_ip_ep_t)); | |||
There was a problem hiding this comment.
Please consider my suggestion about using a variable instead of a type. This bug wouldn't exist if we did it that way.
There was a problem hiding this comment.
And, hmm, that was a quick fix. It compiled, so it must be correct ? ;-)
There was a problem hiding this comment.
Well than I would like to be consistent with gnrc_sock_udp where I also used size notation ;-).
There was a problem hiding this comment.
And, hmm, that was a quick fix. It compiled, so it must be correct ? ;-)
Nope, tested with tests/gnrc_sock_ip on IoT-LAB, but since this and the following variable were initialized with 0 it fell through.
There was a problem hiding this comment.
I have mixed feelings whether to use the type or the instance as a parameter for sizeof here. I understand @keestux's rationale, but I also think that using the type makes the developer aware of how much memory is actually initialized/copied.
However, I totally agree with @miri64 that this is out of scope for this PR.
OlegHahm
added
the
CI: needs squashing
The type in the `sizeof()` is just plain wrong. This fixes it.
miri64
force-pushed
the
gnrc_sock_ip/fix/fix-memcpy-type
branch
from
252fee8
to
653e362
Compare
miri64
added
CI: ready for build
|
Backport provided in #6098 |
miri64
removed
the
Process: needs backport
The type in the
sizeof()is just plain wrong. This fixes it.