Skip to content

Correctly set author_key_digest and report_id fields in js_verify_snp_attestation #7004

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

Conversation

achamayou
Copy link
Member

@achamayou achamayou commented May 13, 2025

Fields were incorrectly set due to a presumed copy-paste mistake.

@achamayou achamayou requested a review from a team as a code owner May 13, 2025 12:47
@maxtropets
Copy link
Collaborator

Wonder why never caught with smth like use-after-move warning

@achamayou
Copy link
Member Author

@maxtropets that's a good question, because as far I understand it, we enable bugprone-* checks in clang-tidy:

clang-analyzer-*,
, which should cover https://clang.llvm.org/extra/clang-tidy/checks/bugprone/use-after-move.html

@eddyashton
Copy link
Member

@maxtropets that's a good question, because as far I understand it, we enable bugprone-* checks in clang-tidy:

clang-analyzer-*,

, which should cover https://clang.llvm.org/extra/clang-tidy/checks/bugprone/use-after-move.html

This is very weird, and worth a little more investigation? Possible explanations:

  • clang-tidy isn't running on this code
  • This check doesn't consider another "move" to be an additional "use" (ie move(x); y=x+1 is obviously wrong, but move(x); move(x) might be safe?)
  • This check can't handle the fact we're making this call inside a macro (which could interfere with comma expansion, I guess, but drastically reduces the coverage we're actually getting from clang-tidy)

@achamayou
Copy link
Member Author

@eddyashton it doesn't seem to be option 2, a trivial move after move is definitely noisily caught:

    std::string foo = "Hello, World!";
    std::string bar = std::move(foo);
    std::string baz = std::move(foo);
$ clang-tidy --checks=-bugprone-unused-local-non-trivial-variable test.cpp 
...
/home/amchamay/CCF/test.cpp:8:23: error: Moved-from object 'foo' of type 'std::basic_string' is moved [clang-analyzer-cplusplus.Move,-warnings-as-errors]
    8 |     std::string baz = std::move(foo);
      |                       ^~~~~~~~~~~~~~
/home/amchamay/CCF/test.cpp:7:23: note: Object 'foo' of type 'std::basic_string' is left in a valid but unspecified state after move
    7 |     std::string bar = std::move(foo);
      |                       ^~~~~~~~~~~~~~
/home/amchamay/CCF/test.cpp:8:23: note: Moved-from object 'foo' of type 'std::basic_string' is moved
    8 |     std::string baz = std::move(foo);
      |                       ^~~~~~~~~~~~~~
/home/amchamay/CCF/test.cpp:8:33: error: 'foo' used after it was moved [bugprone-use-after-move,-warnings-as-errors]
    8 |     std::string baz = std::move(foo);
      |                                 ^
/home/amchamay/CCF/test.cpp:7:23: note: move occurred here
    7 |     std::string bar = std::move(foo);
      |                       ^

@achamayou
Copy link
Member Author

achamayou commented May 13, 2025

Well duh, it's option 1: https://github.com/microsoft/CCF/blob/main/CMakeLists.txt#L426 (#6905, #6907)

@eddyashton eddyashton enabled auto-merge May 19, 2025 08:38
@eddyashton eddyashton added this pull request to the merge queue May 19, 2025
Merged via the queue into microsoft:main with commit ef433ce May 19, 2025
18 of 19 checks passed
@eddyashton eddyashton deleted the fix_field_duplication_in_js_verify_snp_attestation branch May 19, 2025 09:45
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 this pull request may close these issues.

3 participants