Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

BUG: fix aliasing UB in the BPF simulator #425

Closed
wants to merge 1 commit into from
Closed

Conversation

q66
Copy link

@q66 q66 commented Dec 24, 2023

This restores test suite on Clang 17.

@pcmoore pcmoore changed the title scmp_bpf_sim: fix aliasing UB BUG: fix aliasing UB in the BPF simulator Jan 6, 2024
@pcmoore pcmoore added this to the v2.6.0 milestone Jan 6, 2024
@pcmoore
Copy link
Member

pcmoore commented Jan 7, 2024

Hi @q66, thank you for the patch! It looks good to me, but we do ask that contributors use their real name in their DCO sign-off. Would you mind updating your sign-off with your name?

https://github.com/seccomp/libseccomp/blob/main/CONTRIBUTING.md#sign-your-work

@jvoisin
Copy link

jvoisin commented Jan 7, 2024

Hi @q66, thank you for the patch! It looks good to me, but we do ask that contributors use their real name in their DCO sign-off. Would you mind updating your sign-off with your name?

Then it would be nice to update the CONTRIBUTING.md file to reflect this, because I haven't found where it's mentioned anywhere. Interestingly, the "Developer's Certificate of Origin" part is based on the Linux one, that doesn't require "real names".

@q66
Copy link
Author

q66 commented Jan 7, 2024

The Linux kernel project recently dropped the real name DCO requirement as far as I know. While I don't mind disclosing my real name, in practice what a "real name" is often tends to be blurry, and in context of developer contributions it makes no sense; if it's about me being reachable, I've been around with the same handle for easily 20 years now; meanwhile, requiring a legal name is potentially discriminatory or maybe even dangerous for certain individuals, so such requirement ranges from merely pointless to possibly harmful.

Therefore, I'd really appreciate if you dropped this requirement; not for me, but for anybody who may actually be affected by this.

@pcmoore
Copy link
Member

pcmoore commented Jan 7, 2024

That's something we can consider, but that is a much larger issue that the minor fix in this PR. If you are not comfortable using your real name in the sign-off, or if you are opposed to it as a matter of principle, I completely understand, but at this point in time I can't merge your patch as-is.

@pcmoore
Copy link
Member

pcmoore commented Jan 7, 2024

With the huge caveat that IANAL, our contributor doc intentionally uses the term "real name" and not "legal name"; the ambiguity of a "real name" is intentional. I don't want anyone to have to deadname (or similar) themselves.

@pcmoore
Copy link
Member

pcmoore commented Jan 7, 2024

Hi @q66, thank you for the patch! It looks good to me, but we do ask that contributors use their real name in their DCO sign-off. Would you mind updating your sign-off with your name?

Then it would be nice to update the CONTRIBUTING.md file to reflect this, because I haven't found where it's mentioned anywhere.

I think you may have missed it, please re-read the doc and you should find it at the end of the section (which is why I linked it there).

@jvoisin
Copy link

jvoisin commented Jan 7, 2024

I think you may have missed it, please re-read the doc and you should find it at the end of the section (which is why I linked it there).

oh, you're right, my bad: "... then you just add a line to the bottom of your patch description, with your real name"

@rusty-snake
Copy link
Contributor

While I don't mind disclosing my real name, in practice what a "real name" is often tends to be blurry, and in context of developer contributions it makes no sense;

Futhermore, everybody has to trust you that "John Smith" is your real name. So names that look like a non-real name (e.g. X Æ A-12) will be scrutinized while everything that looks like a real name will get accepted.

@pcmoore pcmoore removed this from the v2.6.0 milestone Jan 13, 2024
@drakenclimber
Copy link
Member

The technical aspect of this change looks fine to me. I also agree with @pcmoore's comments above about names and its legal aspects. If you have issues with this or questions, @q66, feel free to ask here or send @pcmoore or me a DM. Thanks.

@q66
Copy link
Author

q66 commented Jun 22, 2024

i'm going to close this; due to reasons, i will no longer be using my legal name online, and i don't currently feel comfortable using my real name in random foss contributions either

if somebody else wants to pick up the one-liner, go right ahead

@q66 q66 closed this Jun 22, 2024
@thesamesam
Copy link
Contributor

thesamesam commented Jun 22, 2024

I'll take it later. The change is obviously correct and it's the canonical fix for this type of problem, so there's no issue in putting it in my name.

@q66
Copy link
Author

q66 commented Jun 22, 2024

sure, thanks :)

thesamesam added a commit to thesamesam/libseccomp that referenced this pull request Jun 22, 2024
See seccomp#425.

Punning sys_data_b between uint32_t* and struct* seccomp_data isn't legal,
use memcpy to fix the testsuite with Clang 17.

Modern compilers recognise this idiom and optimise it out anyway.

Signed-off-by: Sam James <[email protected]>
pcmoore pushed a commit that referenced this pull request Sep 5, 2024
See #425.

Punning sys_data_b between uint32_t* and struct* seccomp_data isn't legal,
use memcpy to fix the testsuite with Clang 17.

Modern compilers recognise this idiom and optimise it out anyway.

Signed-off-by: Sam James <[email protected]>
Acked-by: Tom Hromatka <[email protected]>
Signed-off-by: Paul Moore <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants