fix: Convert C-Style Casts to C++-Style Type Casts #36416

abd-770 · 2024-09-22T20:06:11Z

Issue:
#35900

Description:
This pull request replaces the instances of C-style casting in the codebase with C++ style type casting (static_cast, const_cast, and reinterpret_cast). This change is aimed at:

Vulnerability Fix: Fixes the casting vulnerability from the static scan.
Type Safety: C++-style casts are more explicit and specific than C-style casts
Readability: conveys explicitly what kind of cast is being performed
Adheres to the ES.49 C++ Core Guidelines

mergify · 2024-09-22T20:06:47Z

@abd-770 Thanks for your contribution. Please submit with DCO, see the contributing guide https://github.com/milvus-io/milvus/blob/master/CONTRIBUTING.md#developer-certificate-of-origin-dco.

mergify · 2024-09-22T20:06:48Z

@abd-770

Invalid PR Title Format Detected

Your PR submission does not adhere to our required standards. To ensure clarity and consistency, please meet the following criteria:

Title Format: The PR title must begin with one of these prefixes:

feat: for introducing a new feature.
fix: for bug fixes.
enhance: for improvements to existing functionality.
test: for add tests to existing functionality.
doc: for modifying documentation.
auto: for the pull request from bot.

Description Requirement: The PR must include a non-empty description, detailing the changes and their impact.

Required Title Structure:

[Type]: [Description of the PR]

Where Type is one of feat, fix, enhance, test or doc.

Example:

enhance: improve search performance significantly

Please review and update your PR to comply with these guidelines.

codecov · 2024-09-22T21:59:56Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 82.97%. Comparing base (a5e0a56) to head (3806754).
Report is 212 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff             @@
##           master   #36416      +/-   ##
==========================================
+ Coverage   81.05%   82.97%   +1.91%     
==========================================
  Files        1370     1081     -289     
  Lines      191486   165618   -25868     
==========================================
- Hits       155215   137421   -17794     
+ Misses      30788    22718    -8070     
+ Partials     5483     5479       -4

Components	Coverage Δ
Client	`73.67% <ø> (ø)`
Core	`∅ <ø> (∅)`
Go	`83.23% <ø> (-0.01%)`	⬇️

see 314 files with indirect coverage changes

alexanderguzhva · 2024-09-22T23:12:11Z

to @abd-770 and whoever reviews these changes.

These automated changes need to be reviewed VERY CAREFULLY and altered manually.
For example, I see

    std::vector<uint8_t> vals;
    auto execute_sub_batch = [](Index* index_ptr,
                                const std::vector<uint8_t>& vals) {
        TermIndexFunc<bool> func;

        return std::move(func(index_ptr, vals.size(), (bool*)vals.data()));
    };

    auto res = ProcessIndexChunks<bool>(execute_sub_batch, vals);

got changed into

    std::vector<uint8_t> vals;
    auto execute_sub_batch = [](Index* index_ptr,
                                const std::vector<uint8_t>& vals) {
        TermIndexFunc<bool> func;

        return std::move(func(index_ptr, vals.size(), 
                      const_cast<bool*>(static_cast<const bool*>(
                          static_cast<const void*>(vals.data())
                      ))));
    };

    auto res = ProcessIndexChunks<bool>(execute_sub_batch, vals);

while at a first glance the problem may be fixed by changing const std::vector<uint8_t>& vals to std::vector<uint8_t>& vals

mergify · 2024-09-25T04:09:02Z

@abd-770 go-sdk check failed, comment rerun go-sdk can trigger the job again.

mergify · 2024-09-25T04:10:14Z

@abd-770 E2e jenkins job failed, comment /run-cpu-e2e can trigger the job again.

abd-770 · 2024-09-25T06:08:04Z

rerun go-sdk

abd-770 · 2024-09-25T06:08:12Z

/run-cpu-e2e

mergify · 2024-09-25T06:10:35Z

@abd-770 E2e jenkins job failed, comment /run-cpu-e2e can trigger the job again.

mergify · 2024-09-25T06:10:35Z

@abd-770 go-sdk check failed, comment rerun go-sdk can trigger the job again.

abd-770 · 2024-09-27T06:26:03Z

@alexanderguzhva can you tag someone from the community to review this pr. Thanks

mergify · 2024-09-27T06:35:43Z

@abd-770 E2e jenkins job failed, comment /run-cpu-e2e can trigger the job again.

mergify · 2024-09-27T06:35:43Z

@abd-770 go-sdk check failed, comment rerun go-sdk can trigger the job again.

alexanderguzhva · 2024-09-27T21:56:06Z

@abd-770 would it be possible to run clang-format in order to fix the formatting? thanks

alexanderguzhva · 2024-10-01T14:54:05Z

internal/core/src/segcore/plan_c.cpp


        auto status = CStatus();
        status.error_code = milvus::Success;
        status.error_msg = "";
-        auto group = (CPlaceholderGroup)res.release();
+        auto group = static_cast<CPlaceholderGroup>(res.release());


same, is it really CPlaceholderGroup and not CPlaceholderGroup*?

Same as above.
CPlaceholderGroup is defined as void *.
typedef void* CPlaceholderGroup;

alexanderguzhva · 2024-10-01T14:54:14Z

internal/core/src/segcore/plan_c.cpp

@@ -147,7 +149,7 @@ CreateRetrievePlanByExpr(CCollection c_col,
        auto status = CStatus();
        status.error_code = milvus::Success;
        status.error_msg = "";
-        auto plan = (CRetrievePlan)res.release();
+        auto plan = static_cast<CRetrievePlan>(res.release());


Same as above. They are defined as void*

typedef void* CSearchPlan; typedef void* CPlaceholderGroup; typedef void* CRetrievePlan;

alexanderguzhva · 2024-10-01T14:55:14Z

@abd-770 I've added some comments, let's wait until my colleagues make a review. Thanks.

issue: milvus-io#35900 Signed-off-by: Abdullah Ahmed <abdullahdb6@gmail.com>

sre-ci-robot · 2024-10-02T17:00:49Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: abd-770
To complete the pull request process, please assign wxyucs after the PR has been reviewed.
You can assign the PR to them by writing /assign @wxyucs in a comment when ready.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

internal/core/OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

abd-770 · 2024-10-03T04:46:33Z

@alexanderguzhva I have made changes to the code based on your first review comment.
The rest, I have added respective reasons on the changes.

abd-770 · 2024-10-08T04:39:40Z

@alexanderguzhva @godchen0212 @bigsheeper any updates?

alexanderguzhva · 2024-10-08T19:36:37Z

@abd-770 I gave all of my comments. Waiting for my colleagues.

stale · 2024-11-09T01:28:03Z

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.
Rotten issues close after 30d of inactivity. Reopen the issue with /reopen.

abd-770 · 2024-11-15T05:37:32Z

Hey @alexanderguzhva @godchen0212 @bigsheeper ,
Can you guys review this pr? It's been over a month now, and marked stale.

mergify · 2024-12-02T19:05:38Z

@abd-770 go-sdk check failed, comment rerun go-sdk can trigger the job again.

stale · 2025-01-03T00:12:06Z

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.
Rotten issues close after 30d of inactivity. Reopen the issue with /reopen.

alexanderguzhva · 2025-01-03T14:46:00Z

/reopen

sre-ci-robot added the do-not-merge/work-in-progress Don't merge even CI passed. label Sep 22, 2024

sre-ci-robot requested review from bigsheeper and godchen0212 September 22, 2024 20:06

sre-ci-robot added the size/L Denotes a PR that changes 100-499 lines. label Sep 22, 2024

mergify bot added needs-dco DCO is missing in this pull request. do-not-merge/invalid-pr-format labels Sep 22, 2024

abd-770 changed the title ~~Convert C-Style Casts to C++-Style Type Casts. (DO NOT MERGE)~~ fix: Convert C-Style Casts to C++-Style Type Casts (DO NOT MERGE) Sep 24, 2024

mergify bot added kind/bug Issues or changes related a bug and removed do-not-merge/invalid-pr-format labels Sep 24, 2024

abd-770 force-pushed the forkedMilvus-fixCast branch from db8faa5 to 7a38102 Compare September 24, 2024 12:06

mergify bot added dco-passed DCO check passed. and removed needs-dco DCO is missing in this pull request. labels Sep 24, 2024

abd-770 marked this pull request as ready for review September 25, 2024 04:05

sre-ci-robot removed the do-not-merge/work-in-progress Don't merge even CI passed. label Sep 25, 2024

abd-770 changed the title ~~fix: Convert C-Style Casts to C++-Style Type Casts (DO NOT MERGE)~~ fix: Convert C-Style Casts to C++-Style Type Casts Sep 27, 2024

abd-770 force-pushed the forkedMilvus-fixCast branch from 7a38102 to 1d8b9d4 Compare September 30, 2024 08:46

alexanderguzhva reviewed Oct 1, 2024

View reviewed changes

abd-770 closed this Oct 2, 2024

abd-770 force-pushed the forkedMilvus-fixCast branch from 6e58ba0 to a05a37a Compare October 2, 2024 16:57

sre-ci-robot added size/XS Denotes a PR that changes 0-9 lines. and removed size/L Denotes a PR that changes 100-499 lines. labels Oct 2, 2024

fix: Convert C-Style Casts to C++-Style Type Casts

cea8e1f

issue: milvus-io#35900 Signed-off-by: Abdullah Ahmed <abdullahdb6@gmail.com>

abd-770 reopened this Oct 2, 2024

sre-ci-robot removed size/XS Denotes a PR that changes 0-9 lines. labels Oct 2, 2024

sre-ci-robot added size/L Denotes a PR that changes 100-499 lines. labels Oct 2, 2024

mergify bot added ci-passed and removed ci-passed labels Oct 2, 2024

abd-770 requested a review from alexanderguzhva October 4, 2024 04:20

stale bot added the stale indicates no udpates for 30 days label Nov 9, 2024

mergify bot removed the ci-passed label Nov 9, 2024

stale bot removed the stale indicates no udpates for 30 days label Nov 15, 2024

Merge branch 'master' into forkedMilvus-fixCast

3806754

stale bot added the stale indicates no udpates for 30 days label Jan 3, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: Convert C-Style Casts to C++-Style Type Casts #36416

fix: Convert C-Style Casts to C++-Style Type Casts #36416

abd-770 commented Sep 22, 2024

mergify bot commented Sep 22, 2024

mergify bot commented Sep 22, 2024

codecov bot commented Sep 22, 2024 •

edited

Loading

alexanderguzhva commented Sep 22, 2024

mergify bot commented Sep 25, 2024

mergify bot commented Sep 25, 2024

abd-770 commented Sep 25, 2024

abd-770 commented Sep 25, 2024

mergify bot commented Sep 25, 2024

mergify bot commented Sep 25, 2024

abd-770 commented Sep 27, 2024

mergify bot commented Sep 27, 2024

mergify bot commented Sep 27, 2024

alexanderguzhva commented Sep 27, 2024

alexanderguzhva Oct 1, 2024

abd-770 Oct 2, 2024

alexanderguzhva Oct 1, 2024

abd-770 Oct 2, 2024

alexanderguzhva commented Oct 1, 2024

sre-ci-robot commented Oct 2, 2024

abd-770 commented Oct 3, 2024

abd-770 commented Oct 8, 2024

alexanderguzhva commented Oct 8, 2024

stale bot commented Nov 9, 2024

abd-770 commented Nov 15, 2024

mergify bot commented Dec 2, 2024

stale bot commented Jan 3, 2025

alexanderguzhva commented Jan 3, 2025

fix: Convert C-Style Casts to C++-Style Type Casts #36416

Are you sure you want to change the base?

fix: Convert C-Style Casts to C++-Style Type Casts #36416

Conversation

abd-770 commented Sep 22, 2024

mergify bot commented Sep 22, 2024

mergify bot commented Sep 22, 2024

codecov bot commented Sep 22, 2024 • edited Loading

Codecov Report

alexanderguzhva commented Sep 22, 2024

mergify bot commented Sep 25, 2024

mergify bot commented Sep 25, 2024

abd-770 commented Sep 25, 2024

abd-770 commented Sep 25, 2024

mergify bot commented Sep 25, 2024

mergify bot commented Sep 25, 2024

abd-770 commented Sep 27, 2024

mergify bot commented Sep 27, 2024

mergify bot commented Sep 27, 2024

alexanderguzhva commented Sep 27, 2024

alexanderguzhva Oct 1, 2024

Choose a reason for hiding this comment

abd-770 Oct 2, 2024

Choose a reason for hiding this comment

alexanderguzhva Oct 1, 2024

Choose a reason for hiding this comment

abd-770 Oct 2, 2024

Choose a reason for hiding this comment

alexanderguzhva commented Oct 1, 2024

sre-ci-robot commented Oct 2, 2024

abd-770 commented Oct 3, 2024

abd-770 commented Oct 8, 2024

alexanderguzhva commented Oct 8, 2024

stale bot commented Nov 9, 2024

abd-770 commented Nov 15, 2024

mergify bot commented Dec 2, 2024

stale bot commented Jan 3, 2025

alexanderguzhva commented Jan 3, 2025

codecov bot commented Sep 22, 2024 •

edited

Loading