cleanup(libsinsp): remove a few more UB warnings from integer copies #1481
Conversation
Signed-off-by: Luca Guerra <luca@guerra.sh>
|
/milestone 0.14.0 |
|
LGTM label has been added. Git tree hash: 0882c03e71d68950af64175a56c7f1a77f020726
|
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: Andreagit97, FedeDP, LucaGuerra The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
| addr.m_fields.m_sport = sinfo->m_ipv4info.m_fields.m_sport; | ||
| addr.m_fields.m_dip = *(uint32_t*)db; | ||
| addr.m_fields.m_dip = sinfo->m_ipv4info.m_fields.m_dip; |
There was a problem hiding this comment.
Not sure why m_sport and m_dport don't need a memcpy (here and in line 529 or so)
There was a problem hiding this comment.
It looks like the source and destination fields are the same length. memcpying there would be pointless I think. Although later (on line 499) those fields are accessed with an array, do you think we need to memcpy() there?
There was a problem hiding this comment.
Oh wait. These are just copying from one struct to another. This is fine.
I can't really follow what's going on in the other chunk ATM (what is the source: an actual well behaved C struct or a packed struct at an arbitrary address in the event), so here's a general comment:
memcpy instead of an assignment is not about the size (it obviously must be the same for the source and destination) but about alignment: the destination is aligned properly for the type (simply because it is of that type) but if the source is a packed struct (or a random char* ptr) then technically treating it as a pointer to anything other than char is UB.
(AFAIK that also includes taking the address of a misaligned member in a packed struct)
What type of PR is this?
/kind cleanup
Any specific area of the project related to this PR?
/area libsinsp
Does this PR require a change in the driver versions?
No
What this PR does / why we need it:
One (of many) changes for this: #1470 . In this case, we're removing a few warnings that are not concerning
get_param(). We will be back with more refactorings there.Which issue(s) this PR fixes:
Fixes #
Special notes for your reviewer:
Does this PR introduce a user-facing change?: