botan icon indicating copy to clipboard operation
botan copied to clipboard

Update deprecated mem_ops.h function calls to use std::span-based versions and resolve some TODOs

Open KaganCanSit opened this issue 9 months ago • 9 comments

Hello,

This draft PR includes updates to modernize the codebase by replacing deprecated mem_ops.h function calls with their std::span-based counterparts in line with C++20 standards. Additionally, I also discussed some TODO comments that I thought would be quick fixes.

My reference points the C++ Core Guidelines and CppReference. Since I am not actively developing in C++20 at the moment, I am marking this PR as a draft to review and feedback.

Changes made:

  • Replaced xor_buf calls with std::span-based equivalents.
  • Handled several smaller TODOs identified in the codebase.

Confirmed that builds and test suites pass with:

ninja clean && ./configure.py --without-documentation --cc=clang --compiler-cache=ccache --build-targets=static,cli,tests --build-tool=ninja && ninja && ./botan-test

Check code format with:

python3 src/scripts/dev_tools/run_clang_format.py --clang-format=clang-format-17 --src-dir=src

Outstanding considerations: I intentionally did not modify the following function, as it touches multiple areas and could introduce bugs without extensive review:

// TODO: deprecate and replace, use .subspan()
inline void xor_buf(std::span<uint8_t> out, std::span<const uint8_t> in, size_t n) {
   BOTAN_ARG_CHECK(out.size() >= n, "output span is too small");
   BOTAN_ARG_CHECK(in.size() >= n, "input span is too small");
   xor_buf(out.first(n), in.first(n));
}

Files affected by this function;

src/lib/modes/aead/eax/eax.cpp
src/lib/mac/cmac/cmac.cpp
src/lib/pubkey/dlies/dlies.cpp

Additional notes: There is a change in TLS, so additional checking may be required.

Thanks in advance for your time and guidance.

KaganCanSit avatar Mar 22 '25 03:03 KaganCanSit

Coverage Status

coverage: 90.624% (+0.002%) from 90.622% when pulling b1d6285abfba8ba58363ba3477ef59b3eb761bbe on KaganCanSit:feature-handling-some-todo-messages into e27d332089247f04e89854a2e60a3af2ec4e430f on randombit:master.

coveralls avatar Mar 22 '25 10:03 coveralls

Thank you for your input and guidance. I will incorporate the suggested changes, verify the behavior through testing, and submit a review request accordingly.

KaganCanSit avatar Mar 27 '25 12:03 KaganCanSit

Hi @reneme,

I've been reviewing this PR during my free time. While rebasing my recent PRs to the current master branch, I realized I could also address the function I had previously left out of scope. I'm not sure, but I don't have as many problems as before. Maybe some indirect changes made it easier to do. I mentioned this in the PR description and we discussed it here.

I worked on this after hours today and have now removed all TODO messages while implementing the deprecation notifications. The changes pass ./botan-test successfully in my local environment. Since you've already reviewed this PR and it will require re-review, I wanted to consult with you before proceeding. Here's the commit containing my changes.

I'd like to request cherry-picking these changes to this branch, which would allow us to remove all TODO messages and publish the deprecation notifications together. This PR is closer to completion than my other requests, which motivates me to finalize it.

Since I don't actively use C++20 here, I may have mistakes. Therefore, a re-check will be needed.

What are your thoughts?

Best regards.

KaganCanSit avatar Jun 25 '25 18:06 KaganCanSit

Hello again Mr. @reneme,

I included the commit that I made improvements for the function I mentioned in the comment above, which I did not change, in the branch here. I also rebase to the master branch. I expect the tests to be successful, I will follow it.

I have not squashed the commits so that you can easily analyze the last parts I added here. Please let me know if there are any changes you want and I will try to fix them as soon as I get a chance. I think this branch is close to being merged.

After review, I will squash the commits and make them ready for merging.

Best regards.

KaganCanSit avatar Jul 10 '25 19:07 KaganCanSit

I have addressed all feedback from the review:

  • Changes to src/lib/modes/xts/xts.cpp have been rolled back as suggested to avoid conflicts with upcoming modernizations (#4880, #4151)
  • Retained deprecation warnings in headers to prevent new ptr-length style additions.
  • The existing branch has been re-baselined to the parent branch.
  • Implemented other requested fixes and improvements. (Open typing is a bit my style, sorry.)

roughtime std::span modernization proposal BufferSlicer is noted for future work - I may address this in a separate PR once it is merged. Comment

I will follow the results of the CI tests and then we can merge if you see fit. I see you're quite busy with clang-tidy today, I'll rebase it from time to time if needed, just FYI.

KaganCanSit avatar Jul 13 '25 13:07 KaganCanSit

Looks good to me. Thanks for your efforts.

Thank you for your time and for sharing your expertise. Your guidance has been very helpful throughout this process.

I’ll need to spend some additional time getting more familiar with the span construct. Since I'm currently use the C++11 standard, I've not using it in job.

@randombit, this development is ready to be merged at your discretion.

Best regards.

KaganCanSit avatar Jul 14 '25 13:07 KaganCanSit

Sorry for the wait here. Changes are generally fine but the issue is mem_ops.h is currently a public header but I don't think the library should be in the business of providing general utility functions like this, which is why the header is deprecated and will be removed in Botan4. Any new functionality along these lines should go into the (internal) mem_utils.h header. Ideally we can update all of the callers within the library and then guard the declarations here, which are no longer used internally but which we must continue to provide for now for compatability, with !defined(BOTAN_IS_BEING_BUILT) as is done with various other functions, so it's not possible to add new code to the library which uses them.

I recall we previously discussed removing the header file here. Actually, adding a new function for ‘mem_ops.h’ in this branch is not currently on the agenda. I was only focused on the TODO message; my goal was to deprecate the use of existing function calls and update them with std::span-based overloads.

Within the project, the use of functions marked with the declaration BOTAN_DEPRECATED(‘This function is deprecated, use the range-based version instead’) has now ended. These can be completely removed. However, since there may still be users who use them, I thought it would be prudent to declare them BOTAN_DEPRECATED. But additionally, to ensure they are not reused within the project, they can be wrapped with the !defined(BOTAN_IS_BEING_BUILT) macro, as you suggested.

I thought moving them to the (internal) mem_utils.h header would be a separate task. I'm not sure, maybe I misunderstood.

Some time has passed, so I need to review it again, but if you guide me step by step, I can work on it as soon as possible and apply the update you want.

KaganCanSit avatar Sep 02 '25 19:09 KaganCanSit

Given that this PR doesn't add any functionality to mem_ops.h, I would suggest the following steps :

  1. Let's change the deprecation messages in mem_ops.h from "This function is deprecated, use range-based version instead" to something along the lines of "This function is deprecated without a replacement". That wording doesn't suggest that we're planning to provide support for this function post-Botan4.
  2. Merge this PR (because the comment history is becoming unwieldy)
  3. In a follow-up PR: Move the declarations and internal usages of the relevant functions over to the internal mem_utils.h, guard mem_ops.h with !defined(BOTAN_IS_BEING_BUILT) for backward compatibility, and eventually remove it in Botan4

reneme avatar Sep 29 '25 11:09 reneme

Given that this PR doesn't add any functionality to mem_ops.h, I would suggest the following steps :

  1. Let's change the deprecation messages in mem_ops.h from "This function is deprecated, use range-based version instead" to something along the lines of "This function is deprecated without a replacement". That wording doesn't suggest that we're planning to provide support for this function post-Botan4.
  2. Merge this PR (because the comment history is becoming unwieldy)
  3. In a follow-up PR: Move the declarations and internal usages of the relevant functions over to the internal mem_utils.h, guard mem_ops.h with !defined(BOTAN_IS_BEING_BUILT) for backward compatibility, and eventually remove it in Botan4

Thank you for informing me and guiding me @reneme. I would like to take on this task in this thread and in the pull request that will be opened later. Is @randombit suitable for you?


Updates: To avoid further clutter in the comments, I will update this comment. I am implementing the solution proposed by @reneme here. If deemed appropriate and these changes are merged, I am ready to take part in developing the remaining changes. For this, I will wait for this branch to be merged. (I don't want to repeatedly rebase a new branch based on this branch with every change.)

  • 09.11.25: Rebased to main branch.

KaganCanSit avatar Sep 29 '25 16:09 KaganCanSit