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: memory leak and string corruption in ARM Mac temperature sensors #1767

Conversation

chuangbo
Copy link
Contributor

This PR addresses several issues in the temperature sensor implementation for ARM Mac:

  1. Memory leak caused by unreleased system resources
  2. Increasing port counts in Activity Monitor
  3. String corruption where the first character becomes "\x00"

Changes

  1. Resource Management:

    • Added proper resource cleanup using defer statements
    • Refactored code to share the HID system client instead of creating new ones
    • Fixed memory leaks by ensuring all CoreFoundation objects are properly released
  2. String Handling:

    • Fixed buffer allocation for string conversion
    • Properly handle null terminators in C string to Go string conversion
    • Added correct string length calculations
  3. Code Structure:

    • Reduced resource allocation by sharing system client between functions
    • Enhanced code readability with better comments

Testing

Tested on Apple Silicon Mac with the following improvements:

  • Memory usage remains stable over time
  • Port count in Activity Monitor stays constant
  • No more string corruption issues

How to Reproduce

You can reproduce these issues using the following code:

package main

import (
	"context"
	"time"

	"log/slog"

	"github.com/shirou/gopsutil/v4/sensors"
)

func main() {
	slog.SetLogLoggerLevel(slog.LevelDebug)
	ticker := time.NewTicker(10 * time.Millisecond)

	for range ticker.C {
		temps, err := sensors.TemperaturesWithContext(context.Background())
		if err != nil {
			slog.Debug("Sensor error", "err", err)
		}
		slog.Debug("Temperature", "sensors", temps)
	}
}

go run demo.go | grep "x00"

Memory leaks screen recordings: https://github.com/user-attachments/assets/0168feba-5637-4f3f-8a45-a56a3b5cbfdb

String corruption screenshot:
String corruption

@uubulb would you mind taking a look? Many thanks!

This commit addresses several issues in the temperature sensor implementation for ARM Mac:
1. Memory leak caused by unreleased system resources
2. Increasing port counts in Activity Monitor
3. String corruption where the first character becomes "\x00"

Changes:

1. Resource Management:
   - Added proper resource cleanup using `defer` statements
   - Refactored code to share the HID system client instead of creating new ones
   - Fixed memory leaks by ensuring all CoreFoundation objects are properly released

2. String Handling:
   - Fixed buffer allocation for string conversion
   - Properly handle null terminators in C string to Go string conversion
   - Added correct string length calculations

3. Code Structure:
   - Reduced resource allocation by sharing system client between functions
   - Enhanced code readability with better comments
@uubulb
Copy link
Contributor

uubulb commented Dec 30, 2024

Thanks for the patch!

The original code was roughly ported from the original C code and lacks idiomatic style or additional checks for memory management, Maybe it's time for an overall refactoring

Copy link
Owner

@shirou shirou 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 very much. The reproduction code and video were very clear. Unfortunately, I don't own a mac, so I can't verify the behavior myself, but it seems that @uubulb has also confirmed it, and the code itself looks fine. Thank you for your contribution!

@shirou shirou merged commit bcd0c0a into shirou:master Dec 30, 2024
1 check passed
@chuangbo chuangbo deleted the fix/memory-leak-and-string-corruption-in-arm-mac-temperature-sensors branch December 30, 2024 12:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants