sigstore / cosign

Code signing and transparency for containers and binaries
Apache License 2.0
4.24k stars 507 forks source link

Fixing issue 3642 #3643

Closed Mukuls77 closed 3 months ago

Mukuls77 commented 3 months ago

This PR Closes #3642 The issue fixed is related to Memory leak happening in File: https://github.com/sigstore/cosign/blob/6206f5af392ccefcc6a6cbb167bbccc833b46ed4/pkg/oci/internal/signature/layer.go#L60 Function: func (s *sigLayer) Payload() ([]byte, error)

Summary

The cause of the problem as identified is missing Defer call in function func (s sigLayer) Payload() ([]byte, error) { // Compressed is a misnomer here, we just want the raw bytes from the registry. r, err := s.Layer.Compressed() if err != nil { return nil, err } payload, err := io.ReadAll(r) if err != nil { return nil, err } return payload, nil } so the code changes done is to introduce Defer clause before returning from function to close the file stream. func (s sigLayer) Payload() ([]byte, error) { // Compressed is a misnomer here, we just want the raw bytes from the registry. r, err := s.Layer.Compressed() if err != nil { return nil, err } payload, err := io.ReadAll(r) if err != nil { return nil, err } defer r.Close() return payload, nil }

Release Note

Bug fix for Issue #3642

Documentation

No change in Documentation

codecov[bot] commented 3 months ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 40.42%. Comparing base (2ef6022) to head (4dca07c). Report is 67 commits behind head on main.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #3643 +/- ## ========================================== + Coverage 40.10% 40.42% +0.32% ========================================== Files 155 155 Lines 10044 10088 +44 ========================================== + Hits 4028 4078 +50 + Misses 5530 5517 -13 - Partials 486 493 +7 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

Mukuls77 commented 3 months ago

i Moved defer r.Close() before payload, err := io.ReadAll(r) call this will close the stream in case we get issue in io.ReadAll also same code was present on file pkg/oci/signature/layer.go so added the same changes in that file also

Mukuls77 commented 3 months ago

@hectorj2f i think i did some modification just at the same time you merged the pull request. i just moved defer call before io.ReadAll() and also added the same changes in file pkg/oci/signature/layer.go so added the same changes in that file also

i hope these changes will also get merged and will be available in next release. pls let me know in case any thing more is needed from my side. Thanks

cpanato commented 3 months ago

@Mukuls77 can you open a new pr?

Mukuls77 commented 3 months ago

@cpanato ok i will open a new PR for the delta changes thanks

hectorj2f commented 3 months ago

@Mukuls77 sgtm