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

BUGFIX: Added cancel method to fix context leak #4767

Merged
merged 1 commit into from
Feb 15, 2024

Conversation

fazledyn-or
Copy link
Contributor

What this PR does / why we need it:
This PR fixes a context leak bug in your code.

Which issue(s) this PR fixes:
None

Does this PR introduce a user-facing change?: No

  • How are users affected by this change: N/A
  • Is this breaking change: No
  • How to migrate (if breaking change): N/A

Details

While triaging your project, our bug fixing tool generated the following message-

In file: server.go, method context.WithTimeout is called where the returned cancel function is ignored. It is suggested that the returned cancel function shouldn't be ignored.

In that line, a context is created using the WithTimeout method, where the returned cancelFunc handler is ignored. I have introduced the cancel handler and deferred it so that once the method completes execution, it can be safely cancelled.

Sponsorship and Support

This work is done by the security researchers from OpenRefactory and is supported by the Open Source Security Foundation (OpenSSF): Project Alpha-Omega. Alpha-Omega is a project partnering with open source software project maintainers to systematically find new, as-yet-undiscovered vulnerabilities in open source code - and get them fixed – to improve global software supply chain security.

The bug is found by running the Intelligent Code Repair (iCR) tool by OpenRefactory and then manually triaging the results.

Signed-off-by: fazledyn-or <ataf@openrefactory.com>
Copy link

codecov bot commented Feb 1, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (3c72f4a) 31.03% compared to head (7320b1b) 31.04%.
Report is 1 commits behind head on master.

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #4767   +/-   ##
=======================================
  Coverage   31.03%   31.04%           
=======================================
  Files         225      225           
  Lines       26257    26257           
=======================================
+ Hits         8150     8152    +2     
+ Misses      17457    17455    -2     
  Partials      650      650           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Member

@ffjlabo ffjlabo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you 🙏

Copy link
Member

@khanhtc1202 khanhtc1202 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice catch, thank you 👍

@khanhtc1202 khanhtc1202 merged commit 6b83e63 into pipe-cd:master Feb 15, 2024
13 checks passed
@github-actions github-actions bot mentioned this pull request Apr 8, 2024
t-kikuc pushed a commit that referenced this pull request Apr 8, 2024
Signed-off-by: fazledyn-or <ataf@openrefactory.com>
t-kikuc pushed a commit that referenced this pull request Apr 8, 2024
Signed-off-by: fazledyn-or <ataf@openrefactory.com>
t-kikuc added a commit that referenced this pull request Apr 8, 2024
* BUGFIX: Added cancel method to fix context leak (#4767)

Signed-off-by: fazledyn-or <ataf@openrefactory.com>

* Define piped pluggin api (#4815)

Signed-off-by: khanhtc1202 <khanhtc1202@gmail.com>

* Update BuldPlan API for piped pluggin (#4821)

Signed-off-by: khanhtc1202 <khanhtc1202@gmail.com>

* Relocate plugin proto (#4826)

Signed-off-by: khanhtc1202 <khanhtc1202@gmail.com>

* Update controller to use new planner logic (#4825)

* Update controller to use new planner logic

Signed-off-by: khanhtc1202 <khanhtc1202@gmail.com>

* Update proto path

Signed-off-by: khanhtc1202 <khanhtc1202@gmail.com>

* Fix typo

Signed-off-by: khanhtc1202 <khanhtc1202@gmail.com>

* Fix typo

Signed-off-by: khanhtc1202 <khanhtc1202@gmail.com>

* Update planner logic to call proto instead of self executing

Signed-off-by: khanhtc1202 <khanhtc1202@gmail.com>

---------

Signed-off-by: khanhtc1202 <khanhtc1202@gmail.com>

* Update plugin proto for ExecutorService and add piped pluginservice (#4834)

* Add plugin planner for k8s (#4819)

* [WIP] Add planner

Signed-off-by: Yoshiki Fujikane <ffjlabo@gmail.com>

* Not to use out.Version

Signed-off-by: Yoshiki Fujikane <ffjlabo@gmail.com>

* Use last_successful_commit_hash and last_successful_config_file_name

Signed-off-by: Yoshiki Fujikane <ffjlabo@gmail.com>

* Use in.WorkingDir

Signed-off-by: Yoshiki Fujikane <ffjlabo@gmail.com>

* Use in.PipedConfig

Signed-off-by: Yoshiki Fujikane <ffjlabo@gmail.com>

* Create git client

Signed-off-by: Yoshiki Fujikane <ffjlabo@gmail.com>

* Create secret encryptor

Signed-off-by: Yoshiki Fujikane <ffjlabo@gmail.com>

* Add startup server implementation

Signed-off-by: Yoshiki Fujikane <ffjlabo@gmail.com>

* Fix for relocation of proto api

Signed-off-by: Yoshiki Fujikane <ffjlabo@gmail.com>

* Add roughly implementation for planner plugin

Signed-off-by: Yoshiki Fujikane <ffjlabo@gmail.com>

* Rename pkg name

Signed-off-by: Yoshiki Fujikane <ffjlabo@gmail.com>

* Add licence

Signed-off-by: Yoshiki Fujikane <ffjlabo@gmail.com>

* Comment out for the testing code

Signed-off-by: Yoshiki Fujikane <ffjlabo@gmail.com>

---------

Signed-off-by: Yoshiki Fujikane <ffjlabo@gmail.com>

* Truncate `deploymentStatus` metrics after reporting stats (#4857)

* Truncate deploymentStatus metrics after reporting to avoid excess message size

Signed-off-by: t-kikuc <tkikuchi07f@gmail.com>

* Rename func to Flush() for clarity

Signed-off-by: t-kikuc <tkikuchi07f@gmail.com>

* Add comment of what's included in statsreporter's body

Signed-off-by: t-kikuc <tkikuchi07f@gmail.com>

* Fix indent in the comment

Signed-off-by: t-kikuc <tkikuchi07f@gmail.com>

* Copy change of metrics.go to pipedv1

Signed-off-by: t-kikuc <tkikuchi07f@gmail.com>

* Copy change of reporter.go to pipedv1

Signed-off-by: t-kikuc <tkikuchi07f@gmail.com>

---------

Signed-off-by: t-kikuc <tkikuchi07f@gmail.com>

---------

Signed-off-by: fazledyn-or <ataf@openrefactory.com>
Signed-off-by: khanhtc1202 <khanhtc1202@gmail.com>
Signed-off-by: Yoshiki Fujikane <ffjlabo@gmail.com>
Signed-off-by: t-kikuc <tkikuchi07f@gmail.com>
Co-authored-by: Ataf Fazledin Ahamed <ataf@openrefactory.com>
Co-authored-by: Khanh Tran <32532742+khanhtc1202@users.noreply.github.com>
Co-authored-by: Yoshiki Fujikane <40124947+ffjlabo@users.noreply.github.com>
@t-kikuc t-kikuc mentioned this pull request Apr 9, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants