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

fix wrong result caused by prefix_eq (cherry-pick to 1.1-dev) #15195

Merged
merged 2 commits into from
Mar 27, 2024

Conversation

aunjgr
Copy link
Contributor

@aunjgr aunjgr commented Mar 27, 2024

What type of PR is this?

  • API-change
  • BUG
  • Improvement
  • Documentation
  • Feature
  • Test and CI
  • Code Refactoring

Which issue(s) this PR fixes:

issue https://github.com/matrixorigin/MO-Cloud/issues/2811

What this PR does / why we need it:

vectors allocated from mempool are not zero-initialized and MUST be overwritten in function implementation.

vectors allocated from mempool are not zero-initialized and MUST be overwritten in function implementation.

Approved by: @m-schen
@aunjgr aunjgr requested a review from m-schen as a code owner March 27, 2024 08:53
@matrix-meow matrix-meow added the size/XS Denotes a PR that changes [1, 9] lines label Mar 27, 2024
@mergify mergify bot requested a review from sukki37 March 27, 2024 08:54
@mergify mergify bot added the kind/bug Something isn't working label Mar 27, 2024
@matrix-meow
Copy link
Contributor

@aunjgr Thanks for your contributions!

Pull Request Review:

Title:

The title of the pull request is clear and concise, indicating that the fix is related to a wrong result caused by prefix_eq and it is being cherry-picked to the 1.1-dev branch.

Body:

The body of the pull request provides essential information about the type of PR (BUG), the specific issue it fixes (issue #2811), and the reason for the PR, which is related to zero-initialization of vectors allocated from mempool. The body is informative and helps in understanding the context of the changes.

Changes:

  1. In the func_prefix.go file, the changes include modifications in the PrefixEq and PrefixIn functions.
  2. In the PrefixEq function, the logic for setting the result value based on bytes.HasPrefix has been simplified by directly assigning the result of bytes.HasPrefix to res[i].
  3. In the PrefixIn function, the loop iteration has been changed from for i := range lcol to for i := 0; i < length; i++, which ensures a consistent iteration over the vectors.

Feedback and Suggestions:

  1. Zero-Initialization Issue:

    • The PR mentions that vectors allocated from mempool are not zero-initialized and must be overwritten in function implementation. It is crucial to ensure that memory allocated from the mempool is properly initialized to avoid potential security vulnerabilities or unexpected behavior due to uninitialized memory.
    • Suggestion: Consider adding explicit zero-initialization for vectors allocated from the mempool before using them to ensure data integrity and security.
  2. Code Optimization:

    • In the PrefixEq function, the change to directly assign the result of bytes.HasPrefix to res[i] improves code readability and simplifies the logic.
    • Suggestion: Ensure that the code remains clear and maintainable after optimizations, and consider adding comments to explain the logic if necessary.
  3. Consistent Loop Iteration:

    • Changing the loop iteration in the PrefixIn function to for i := 0; i < length; i++ provides a clear and consistent way to iterate over the vectors.
    • Suggestion: Verify that the loop iteration change does not introduce any off-by-one errors or impact the functionality of the function.
  4. Testing:

    • It is important to include relevant tests to validate the changes made in the functions PrefixEq and PrefixIn to ensure that the fixes work as intended and do not introduce regressions.
    • Suggestion: Add test cases that cover different scenarios to verify the correctness of the modified functions.
  5. Documentation:

    • Consider updating the documentation or code comments to reflect the changes made in the functions for better code understanding and maintainability.
    • Suggestion: Ensure that the documentation accurately reflects the behavior of the functions post-fix.

By addressing the zero-initialization issue, optimizing the code changes, adding tests, and updating documentation, the pull request can enhance the overall quality and security of the codebase.

@mergify mergify bot merged commit eae974a into matrixorigin:1.1-dev Mar 27, 2024
17 of 18 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/bug Something isn't working size/XS Denotes a PR that changes [1, 9] lines
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants