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

reserved identifier violation #201

Closed
elfring opened this issue Feb 13, 2014 · 15 comments
Closed

reserved identifier violation #201

elfring opened this issue Feb 13, 2014 · 15 comments

Comments

@elfring
Copy link

elfring commented Feb 13, 2014

I would like to point out that identifiers like "__COCKPIT_H__" and "_Daemon" do not fit to the expected naming convention of the C language standard.
Would you like to adjust your selection for unique names?

@magcius
Copy link
Contributor

magcius commented Feb 13, 2014

Is there a practical fallout that can happen as a result of this? We've been writing code like this for years in other projects, to the point where it's common practice.

@elfring
Copy link
Author

elfring commented Feb 13, 2014

How do you think about to avoid that this software depends on undefined behaviour?

@stefwalter
Copy link
Contributor

Hmmm, it's not undefined behavior in any of the compilers we build on, like gcc. There are probably hundreds of thousands of such defines and usages all over the open source.

For example the linux kernel uses the form __HEADER_H__ for it's header guards. You can see this all over glibc, gtk+, glib, and thousands of other projects.

It seems to be one part of the C where the undefined behavior is in fact defined.

If you wish to have a broader conversation about this, you probably want to start a discussion on a broader mailing list. Or if you already have had such a discussion and a consensus was reached, could you point it out?

@elfring
Copy link
Author

elfring commented Feb 13, 2014

How do you think about to stop bad habits eventually?

@magcius
Copy link
Contributor

magcius commented Feb 13, 2014

I don't know of any compiler that will make it truly undefined behavior. If it builds the Linux kernel fine, it will build our project fine. That includes gcc, clang, suncc, Intel's C compiler, and others.

If you successfully manage to convince the glibc and the Linux kernel maintainers to remove their invalid identifiers, then we'll talk about fixing GTK+, glib, and Cockpit.

@elfring
Copy link
Author

elfring commented Feb 13, 2014

Other software developers applied corrections for the affected implementation detail already. I am curious if you would also like to care for standard-compliance one day.

@magcius
Copy link
Contributor

magcius commented Feb 13, 2014

Are you sure? I went into the Linux source code, picked a random header file, and look what I found:

https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/include/video/gbe.h

Almost all the header guards I can find in any header in the Linux kernel source starts with an underscore.

So, as I said, if you successfully convince the Linux kernel and glibc to adopt standards-compliant header guards, we'll follow suit.

@srh
Copy link

srh commented Feb 13, 2014

Ignoring the standards weeneeism for a minute, dangerous things can happen when name conflicts happen at the same time, like an include guard conflict in combination with a struct name conflict where the structs share a common field name. A name like __TYPES_H__ is kind of asking for trouble. Almost all potential problems could be mitigated if you had a project-unique prefix in the include guards.

stefwalter added a commit to stefwalter/cockpit that referenced this issue Feb 13, 2014
Certain header guards used defines like __TYPE_H__ which are likely
to be shared by other projects or the standard library. Add a
COCKPIT_ prefix to them.

Closes: cockpit-project#201
@stefwalter
Copy link
Contributor

@srh, good point. #204 should fix that.

@elfring
Copy link
Author

elfring commented Feb 14, 2014

How do you think about to make your include guards not only standard-compliant but also really unique by appending a kind of UUID?

@maddie-boby
Copy link

You seriously want to add a UUID to an include guard? Are you kidding? You've got to be trolling with this. It's seriously the most pedantic thing I've ever seen in my life, especially since there's literally no actual real world consequences for using this sort of undefined behavior.

@ketsuban
Copy link

I would charitably suggest they mean "universally unique ID" in a nontechnical sense ("a sigil to prevent name collisions") rather than the specific technical meaning. "Universally unique ID" is an annoyingly generic term to have reserved for one specific kind of thing, it's like ImageMagick taking "convert" all for itself.

@magcius
Copy link
Contributor

magcius commented Feb 16, 2014

No, he is actually suggesting something like this.

@ketsuban
Copy link

Wow, okay, yeah, that's completely insane. I was sympathetic to the original argument, but that's absurd given that their own citation says the likelihood of any harm coming from the violation is low.

stefwalter added a commit that referenced this issue Feb 17, 2014
Certain header guards used defines like __TYPE_H__ which are likely
to be shared by other projects or the standard library. Add a
COCKPIT_ prefix to them.

Closes: #201
Signed-off-by: Marius Vollmer <mvollmer@redhat.com>
@stefwalter
Copy link
Contributor

Pushed fix for non-prefixed include guards pointed out by @srh.

If anyone wants to reopen this, do so with a patch ready for review.

elfring added a commit to elfring/cockpit that referenced this issue Feb 18, 2014
… unique.

Some underscores were replaced to make affected identifiers standard-compliant.

Include guards prevent the multiple inclusion of header files only correctly
if the chosen identifiers are different from others with such a functionality.
The probability for name clashes was reduced by the addition of a kind
of universally unique identifier as a suffix.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
elfring added a commit to elfring/cockpit that referenced this issue Feb 18, 2014
…ompliant.

Some data structure identifiers did not fit to the expected naming convention
of the C language standard.
https://www.securecoding.cert.org/confluence/display/seccode/DCL37-C.+Do+not+declare+or+define+a+reserved+identifier

This implementation detail was fixed by the deletion of leading underscores.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
elfring added a commit to elfring/cockpit that referenced this issue Feb 18, 2014
Some include guards did not fit to the expected naming convention of the
C language standard.
https://www.securecoding.cert.org/confluence/display/seccode/DCL37-C.+Do+not+declare+or+define+a+reserved+identifier#DCL37-C.Donotdeclareordefineareservedidentifier-NoncompliantCodeExample%28HeaderGuard%29

This implementation detail was fixed by the deletion of underscores.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
elfring added a commit to elfring/cockpit that referenced this issue Feb 18, 2014
…ompliant.

Some data structure identifiers did not fit to the expected naming convention
of the C language standard.
https://www.securecoding.cert.org/confluence/display/seccode/DCL37-C.+Do+not+declare+or+define+a+reserved+identifier

This implementation detail was fixed by the movement of underscores from the
beginning in affected identifiers to the end.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
marusak added a commit to marusak/cockpit that referenced this issue Jan 4, 2021
None of our VMs need yum anymore. And there is not yum on Fedora 31.

Closes cockpit-project#201
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants