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

Chore: AEAD_Mode can deal with generic containers #3317

Merged
merged 3 commits into from
Mar 22, 2023
Merged

Chore: AEAD_Mode can deal with generic containers #3317

merged 3 commits into from
Mar 22, 2023

Conversation

reneme
Copy link
Collaborator

@reneme reneme commented Feb 22, 2023

This introduces AEAD_Mode::set_associated_data(std::span<>) alongside the ptr-length-overloads. It switches the virtual method to the std::span<> overload and adapts the AEAD implementations accordingly. Also set_associated_data_vec() is marked as deprecated as it is just an alias now.

Caveat

This might be somewhat a show-stopper for the overload-based introduction of std::span<> we had in mind (also affecting #3297, #3294, #3195, #3201). In short: overloads of virtual methods in the base class are not necessarily dispatched to from an object of a derived type. A godbolt says more than a hundred words.

Same code for reference:

class AEAD_Mode {
public:
    virtual void set_associated_data(std::span<const uint8_t> ad) = 0;
    void set_associated_data(const uint8_t ad[], size_t ad_len)
        { set_associated_data(std::span(ad, ad_len)); }
};

class GCM : public AEAD_Mode {
public:
    // Otherwise the compiler does not statically dispatch to the
    // ptr-length overload in the base class
    using AEAD_Mode::set_associated_data;

    void set_associated_data(std::span<const uint8_t> ad) override final
        { std::cout << "Yeah!" << std::endl; }

};

int main()
{
    GCM gcm;

    std::vector<uint8_t> ad{'a', 'b', 'c'};

    // this works
    gcm.set_associated_data(ad);
    
    // this does not (without the `using` declaration in GCM)
    gcm.set_associated_data(ad.data(), ad.size());

    // - - - - - - - - - - - - - - - - - - - - - - - - - - - - - -

    std::unique_ptr<AEAD_Mode> gcm_ptr = std::make_unique<GCM>();
    
    // this works regardless of the `using` declaration
    gcm_ptr->set_associated_data(ad);
    gcm_ptr->set_associated_data(ad.data(), ad.size());

    return 0;
}

Frankly, I'm quite surprised that the method dispatch doesn't find the (publicly available) overload in the base class. 😢 One can work around it by explicitly using Base::method in the derived class, but that's easy to miss in large refactorings.
On the other hand, one could simply use other method names for the std::span overloads. But that complicates the public API even more and forces us to come up with new names for established concepts.

What a mess! Or am I actually missing something?

@codecov-commenter
Copy link

codecov-commenter commented Feb 22, 2023

Codecov Report

Patch coverage: 97.87% and no project coverage change.

Comparison is base (ff51148) 88.14% compared to head (b1b0ad1) 88.15%.

❗ Current head b1b0ad1 differs from pull request most recent head 2c983c5. Consider uploading reports for the commit 2c983c5 to get more accurate results

📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #3317   +/-   ##
=======================================
  Coverage   88.14%   88.15%           
=======================================
  Files         617      617           
  Lines       70356    70353    -3     
  Branches     6987     6984    -3     
=======================================
+ Hits        62016    62020    +4     
+ Misses       5399     5394    -5     
+ Partials     2941     2939    -2     
Impacted Files Coverage Δ
src/lib/modes/aead/aead.cpp 92.72% <ø> (+6.28%) ⬆️
src/lib/tls/tls12/tls_cbc/tls_cbc.cpp 83.41% <80.00%> (ø)
src/cli/cipher.cpp 89.65% <100.00%> (ø)
src/cli/pk_crypt.cpp 71.81% <100.00%> (ø)
src/cli/timing_tests.cpp 88.61% <100.00%> (ø)
src/lib/modes/aead/ccm/ccm.cpp 90.37% <100.00%> (ø)
...b/modes/aead/chacha20poly1305/chacha20poly1305.cpp 94.31% <100.00%> (ø)
src/lib/modes/aead/eax/eax.cpp 97.72% <100.00%> (ø)
src/lib/modes/aead/gcm/gcm.cpp 92.77% <100.00%> (ø)
src/lib/modes/aead/ocb/ocb.cpp 98.38% <100.00%> (ø)
... and 7 more

... and 11 files with indirect coverage changes

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@reneme
Copy link
Collaborator Author

reneme commented Feb 22, 2023

Based on a suggestion by @lieser I added an indirection to work around above-mentioned caveat. The protected pure-virtual method set_ad_n() is now derived by all AEAD implementations. The idx parameter is ignored by most (except SIV), while the public API wrapper ensures that only SIV can successfully receive anything but 0 for the idx.

Come to think of it, that's exactly what we ended up implementing in #3195.

reneme added 3 commits March 21, 2023 07:36
This changes the public interface of set_associated_data_n() to only
take a std::span. The method was introduced just recently, so we
hopefully won't bother too many users with that API change. As a result,
set_associated_data_n() can be used as the virtual entry point for the
AEAD implementations.
@reneme
Copy link
Collaborator Author

reneme commented Mar 21, 2023

Rebased to master after one of yesterday's PRs conflicted with it.

@randombit randombit added this to the Botan 3.0.0 milestone Mar 21, 2023
@reneme reneme merged commit 02ae99a into master Mar 22, 2023
@randombit randombit deleted the span/aead branch March 22, 2023 16:40
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