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

Warn if compartment call returns void #34

Open
rmn30 opened this issue Jul 3, 2024 · 11 comments · Fixed by #47
Open

Warn if compartment call returns void #34

rmn30 opened this issue Jul 3, 2024 · 11 comments · Fixed by #47
Labels
good first issue Good for newcomers

Comments

@rmn30
Copy link

rmn30 commented Jul 3, 2024

It would be good if clang would warn if the return type of a compartment call is void. Compartment calls can fail unexpectedly and return -1 (e.g. due to trusted stack exhaustion). Callers should be aware of and check for this, therefore it is never appropriate for a compartment call to return void. For the same reason we should also consider automatically adding the nodiscard attribute to compartment calls.

@v01dXYZ
Copy link

v01dXYZ commented Nov 12, 2024

Hi,
I'm working on this. How to deal with C++ constructors with a cheri_compartment attr ?

@v01dXYZ
Copy link

v01dXYZ commented Nov 13, 2024

I think it is worth to take the c++ cases into account (non static method, constructor/destructor, lambda ?, etc.). But it would be better to write another ticket for that.

@davidchisnall
Copy link

I'm not sure what it would even mean to expose a constructor across a compartment boundary. A factory method, maybe, but what would the security model be for a constructor? Non-static methods can't be compartment exports, because they need to in the vtable and the ABI does not allow that. It would be possible to make a lambda's call operator a cross-compartment call, but there's no way of exposing that in the source language.

@v01dXYZ
Copy link

v01dXYZ commented Nov 15, 2024

I'll open a ticket about generating an error when non static methods are used as compartment entry points. (Or you can if you have other edge cases in mind).

There are more things to mention as I progress:

  • in Attr.td (the tablegen file defining attributes), cheri_compartment has Subjects = [Function]. Contrary to FunctionLike, it does not include function pointers. It seems strange to me but there is maybe a reason.
  • cheri_compartment is a Calling Convention attribute. Clang considers it as a Function type attribute. Thus, when the function declaration is annotated with such an attribute, clang will also try to add this attribute to the function type. But there is a problem, later when trying to handle the attributes, it will iterate first on the different parts of the type and then on the declaration itself (at each iteration, it essentially takes the attribute list and call the same processing function on it). Which means the warning will be shown twice. I could disable that but it will also disable showing multiple warnings when there are redundant cheri_compartment attributes. Which path should I take ?

@davidchisnall
Copy link

To the first, yes this makes no sense for function pointers. Function pointers must use the Cheri callback convention. They are not (and cannot be) explicitly tied to a compartment.

I think showing the warning twice to start with is fine for a first cut.

@v01dXYZ
Copy link

v01dXYZ commented Nov 18, 2024

I wrote a first version for cheri_compartment. I reported the duplicate attribute problem caused by GetTypeForDeclarator which is the root cause of the previous problem (llvm/llvm-project#116543).

There are still to support cheri_ccall and cheri_ccallback. But this is a little bit more complicated than expected as the attribute is added to the AST in a different part where it's more difficult to also add an additional WarnUnusedResult attr. Emitting a diagnosis about void return type could easily be done though.

@resistor resistor reopened this Dec 16, 2024
@resistor resistor reopened this Jan 2, 2025
@resistor
Copy link
Collaborator

resistor commented Jan 2, 2025

Keeping this open until the warning is enabled by default.

@davidchisnall
Copy link

We also need a mechanism for silencing it before it can be on by default. There are some cases (callbacks, compartment entry points) where a void return is fine.

@resistor
Copy link
Collaborator

resistor commented Jan 2, 2025

We also need a mechanism for silencing it before it can be on by default. There are some cases (callbacks, compartment entry points) where a void return is fine.

Would #pragma clang diagnostic ignored "-Wcheri-compartment-return-void" suffice?

@resistor
Copy link
Collaborator

@resistor
Copy link
Collaborator

#103

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue Good for newcomers
Projects
None yet
4 participants