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

Fixed the memory burn #1185

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Conversation

wendig0x
Copy link
Contributor

Fixed the memory clearing function a little bit

Fixed the memory clearing function a little bit
@idrassi
Copy link
Member

idrassi commented Aug 26, 2023

Thank you for your contribution to VeraCrypt!

I've reviewed your proposed changes to the burn macro, and I have some concerns:

  • Performance Overhead: The original burn macro is designed to be as efficient and fast as possible. By introducing the conditional checks and a for loop, there's added overhead that can affect the performance, especially when clearing larger memory blocks.
  • Redundant Checks: The checks if (&burnm[i] >= mem && &burnm[i] < mem + size) are redundant. Given that burnm is initialized as (volatile char *)(mem), it is guaranteed that the range of memory addresses from burnm to burnm + size are within the valid range of mem to mem + size. We don't need to perform this check for every byte; it complicates the macro and might mislead someone into thinking there's a case where this isn't true.
  • Type Consistency: In the original macro, the size counter burnc is an int. While changing this to size_t might be motivated by wanting to make sure non-negative sizes are passed, this is an internal macro, and we would typically ensure elsewhere that the size passed in is valid.
  • Avoiding Compiler Optimizations: One of the main purposes of the burn macro is to prevent compiler optimizations from removing the memory clearing operation. The original while-loop structure, due to its simplicity, does a good job at this. By introducing the more complex logic and conditional checks, we could inadvertently allow compilers to find optimization opportunities that might defeat the purpose of the macro. I'd be interested in knowing if you've reviewed the generated assembly code to ensure that the new logic isn't being optimized out.
  • Checking for NULL: The if (burnm) check seems unnecessary. If mem is NULL, we would typically want to handle this error elsewhere in the codebase, not within the burn macro. Silently returning in the event of a NULL pointer can mask potential issues.

Could you provide more information about the motivation behind these changes? Did you notice a specific issue with the original implementation? And as previously mentioned, have you validated at the assembly level that the generated assembly code does not optimize out this logic?

@wendig0x
Copy link
Contributor Author

wendig0x commented Aug 26, 2023

The idea of the change was to make the memory cleanup macro more autonomous and universal. Additional checks were implemented for this purpose. As for the speed, I assumed that this macro is not used for large volumes of data, so these changes would not significantly affect the speed. As for compiler optimizations, it's interesting, I will conduct several experiments with different compilation flags and provide a listing.
Regarding the null check, if you pass an empty pointer, there will be an access violation. However, in case of a check, such a situation is somehow handled.

@idrassi
Copy link
Member

idrassi commented Aug 27, 2023

Thank you for clarifying your intentions behind the changes to the burn macro.

Regarding the notion of making the memory cleanup macro "more autonomous and universal", could you provide more insight into what you mean by that? Are there specific use cases or scenarios you have in mind where the current macro doesn't work as intended? Understanding your perspective will help in evaluating the need for such changes.

While it's true that the burn macro might not be used for large volumes of data, performance is still a critical concern for us. Even small overheads can add up when the macro is called frequently, and I want to ensure that any changes we make do not inadvertently introduce performance bottlenecks.

As for the NULL check, I understand the rationale behind avoiding access violations. Still, I believe that a more appropriate place to handle such scenarios would be elsewhere in the codebase. The burn macro should ideally be focused on its primary function - clearing memory. Adding NULL checks within the macro could potentially mask other issues that might arise due to passing NULL pointers inadvertently.

Lastly, I appreciate your willingness to experiment with different compilation flags to assess the impact on compiler optimizations. Please do share your findings, as they will be crucial in determining the viability of your proposed changes.

In conclusion, while I appreciate your initiative and the thought process behind your proposal, I'm still trying to understand the core issue that your changes aim to address. If you could shed more light on the specific problem you're trying to solve, it would help guide my discussion further.

Looking forward to your response!

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.

2 participants