8000 src/update_handler: fix memory leak on G_IO_ERROR_NO_SPACE in clear_slot() by Jo3Zhou · Pull Request #1706 · rauc/rauc · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

src/update_handler: fix memory leak on G_IO_ERROR_NO_SPACE in clear_slot() #1706

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
May 12, 2025
Merged

src/update_handler: fix memory leak on G_IO_ERROR_NO_SPACE in clear_slot() #1706

merged 1 commit into from
May 12, 2025

Conversation

Jo3Zhou
Copy link
Contributor
@Jo3Zhou Jo3Zhou commented May 2, 2025 •
edited
Loading

Summary

This PR improves the robustness of clear_slot() by addressing a potential memory leak when G_IO_ERROR_NO_SPACE occurs

Changes

  • Added explicit g_clear_error() when the error is expected and recoverable.

Question

Using ioctl(fd, BLKDISCARD) to discard blocks rather than zero-write may be more efficient for flash-based block devices?

@jluebbe jluebbe added the fixed label May 7, 2025
@jluebbe jluebbe added this to the Release v1.15 milestone May 7, 2025
@jluebbe
Copy link
Member
jluebbe commented May 7, 2025

How did you find this leak? :)

@codecov Codecov
Copy link
codecov bot commented May 7, 2025 •
edited
Loading

Codecov Report

Attention: Patch coverage is 66.66667% with 2 lines in your changes missing coverage. Please review.

Project coverage is 84.48%. Comparing base (6ee3731) to head (86fd2aa).
Report is 2 commits behind head on master.

Files with missing lines Patch % Lines
src/update_handler.c 66.66% 2 Missing ⚠️

❌ Your patch status has failed because the patch coverage (66.66%) is below the target coverage (75.00%). You can increase the patch coverage or adjust the target coverage.

Additional details and impacted files 
@@           Coverage Diff           @@
##           master    #1706   +/-   ##
=======================================
  Coverage   84.48%   84.48%           
=======================================
  Files          76       76           
  Lines       22286    22288    +2     
=======================================
+ Hits        18828    18830    +2     
  Misses       3458     3458
Flag Coverage Δ
service=false 80.96% <66.66%> (+<0.01%) ⬆️
service=true 84.43% <66.66%> (+<0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

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

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@a3f
Copy link
Contributor
a3f commented May 7, 2025

Using ioctl(fd, BLKDISCARD) to discard blocks rather than zero-write may be more efficient for flash-based block devices?

At least for eMMCs, this doesn't guarantee reading back zeroes per reading of the code and checking the eMMC v5.1 spec. We want it to translate to a TRIM command, not a DISCARD, so we would need to do BLKZEROOUT instead.

See for more information: barebox/barebox@91a11c7

@jluebbe
Copy link
Member
jluebbe commented May 7, 2025

See also #1120 for some discussion regarding discard and zeroing.

@Jo3Zhou Jo3Zhou closed this May 7, 2025
@Jo3Zhou Jo3Zhou reopened this May 7, 2025
@Jo3Zhou
Copy link
Contributor Author
Jo3Zhou commented May 7, 2025 •
edited
Loading

How did you find this leak? :)

I found it while analysing error-handling paths in the block device clearing logic. Specifically, I noticed that when g_error_matches() returns FALSE, the GError (ierror) isn't cleared or propagated in all cases — which could lead to a memory leak if left unhandled.

Glad to help clean it up! :)

@Jo3Zhou
Copy link
Contributor Author
Jo3Zhou commented May 7, 2025

Using ioctl(fd, BLKDISCARD) to discard blocks rather than zero-write may be more efficient for flash-based block devices?

At least for eMMCs, this doesn't guarantee reading back zeroes per reading of the code and checking the eMMC v5.1 spec. We want it to translate to a TRIM command, not a DISCARD, so we would need to do BLKZEROOUT instead.

See for more information: barebox/barebox@91a11c7

Thanks for explaining that, I take a look of it

@Jo3Zhou
Copy link
Contributor Author
Jo3Zhou commented May 7, 2025

See also #1120 for some discussion regarding discard and zeroing.

Thanks for reference, I will have a look

@Jo3Zhou @jluebbe
src/update_handler: fix memory leak on G_IO_ERROR_NO_SPACE in clear_s…
86fd2aa 
…lot()

Signed-off-by: Jialu Zhou <jialu.zhou@viperinnovations.com>
Signed-off-by: Jan Luebbe <jlu@pengutronix.de>
@jluebbe jluebbe self-requested a review May 12, 2025 14:55
@jluebbe jluebbe changed the title fix(GError memleak): avoid memory leak on G_IO_ERROR_NO_SPACE in clear_slot src/update_handler: fix memory leak on G_IO_ERROR_NO_SPACE in clear_slot() May 12, 2025
@jluebbe
Copy link
Member
jluebbe commented May 12, 2025

Force-pushed to align the commit title with our conventions.

@jluebbe jluebbe merged commit db62919 into rauc:master May 12, 2025
18 of 19 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jluebbe jluebbe jluebbe approved these changes

Assignees
No one assigned
Labels
fixed
Projects
None yet
Milestone
Release v1.15
Development

Successfully merging this pull request may close these issues.
3 participants
@Jo3Zhou @jluebbe @a3f
0