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

Clean up enumeration on macOS #218

Merged
merged 8 commits into from
Oct 14, 2024

Conversation

sirhcel
Copy link
Contributor

@sirhcel sirhcel commented Sep 26, 2024

I spotted missing checks for the returned status for several Core Foundation and IOKit function calls. While looking into this and adding checks, I came across the core-foundation crate which comes in handy for getting less verbose with basic operation with Core Foundation types.

The commit history shows the initial adding of checks and the later migration to core-foundation.

@sirhcel
Copy link
Contributor Author

sirhcel commented Sep 26, 2024

As you provided some oft the last fixes for macOS @huntc, it would be great, if you could have a look at this PR as well.

@sirhcel
Copy link
Contributor Author

sirhcel commented Sep 26, 2024

I'm sorry for bothering you @jessebraham! I failed to click in the right place when asking @eldruin for a review.

Copy link
Contributor

@eldruin eldruin 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! The new code does look better.

Cargo.toml Show resolved Hide resolved
@huntc
Copy link
Contributor

huntc commented Sep 26, 2024

Hi. I’ve not looked into the code in any detail, and have no strong opinion on the changes. What I recommend though is confirmation here that the new code has been profiled for leaks. I spent a lot of effort on profiling previously given the nature of the problems being fixed back then. Thanks.

CHANGELOG.md Outdated Show resolved Hide resolved
@sirhcel sirhcel changed the title Clean up enumeration an macOS Clean up enumeration on macOS Sep 26, 2024
@sirhcel
Copy link
Contributor Author

sirhcel commented Sep 26, 2024

Hi. I’ve not looked into the code in any detail, and have no strong opinion on the changes. What I recommend though is confirmation here that the new code has been profiled for leaks. I spent a lot of effort on profiling previously given the nature of the problems being fixed back then. Thanks.

This is a valid point. How did your setup for profiling this looks like? You are mentioning using Instuments in #98. Can you give some more details on how you profiled the device enumeration on macOS?

I have to admit that I have little experience with XCode and its graphical tool chest. Have you used the CLI tool leaks yet?

@sirhcel
Copy link
Contributor Author

sirhcel commented Sep 26, 2024

A first shot with leaks shows no issues so far:

$ cargo build --features usbportinfo-interface --example list_ports
$ leaks --atExit -- ./target/debug/examples/list_ports
list_ports(99996) MallocStackLogging: could not tag MSL-related memory as no_footprint, so those pages will be included in process footprint - (null)
list_ports(99996) MallocStackLogging: recording malloc and VM allocation stacks using lite mode
Found 12 ports:
[...]
    /dev/tty.usbserial-112130
    Type: USB
    VID:10c4 PID:ea60
    Serial Number: [...]
    Manufacturer: Silicon Labs
        Product: CP2102N USB to UART Bridge Controller
        Interface: 00
Process 99996 is not debuggable. Due to security restrictions, leaks can only show or save contents of readonly memory of restricted processes.

Process:         list_ports [99996]
Path:            /Users/USER/*/list_ports
Load Address:    0x1029f0000
Identifier:      list_ports
Version:         0
Code Type:       ARM64
Platform:        macOS
Parent Process:  leaks [99995]

Date/Time:       2024-09-26 16:26:27.931 +0200
Launch Time:     2024-09-26 16:26:27.679 +0200
OS Version:      macOS 13.[...]
Report Version:  7
Analysis Tool:   /usr/bin/leaks

Physical footprint:         3665K
Physical footprint (peak):  3665K
Idle exit:                  untracked
----

leaks Report Version: 4.0, multi-line stacks
Process 99996: 717 nodes malloced for 49 KB
Process 99996: 0 leaks for 0 total leaked bytes.

The message Process 99996 is not debuggable. [...] is not building up trust but I've seen it several places describing the use of leaks and cross-checking the result with std::mem::forget-ing as of sirhcel@88077e5 reports the leaked strings.

Copy link
Contributor

@eldruin eldruin left a comment

Choose a reason for hiding this comment

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

Looks good to me, thank you!

@sirhcel
Copy link
Contributor Author

sirhcel commented Oct 13, 2024

Friendly ping regarding #218 (comment) @huntc

@huntc
Copy link
Contributor

huntc commented Oct 14, 2024

Friendly ping regarding #218 (comment) @huntc

Apologies for not replying.

I don't recall the exact steps that I took now. I vaguely recall launching xcode for my project from the command line and then instrumenting it. Sorry for not being able to provide more info.

The code in question still builds fine for aarch64-apple-ios. So there
is no obvious reason for not pulling in more convenience.
This reduces verbosity for this function as well.
This transitive dependency is used by some of our examples. The advisory
is informational and not about an actual issue.

Our MSRV currently prevents us from upgrading to a newer version of
clap. When bumping MSRV, we shall look into newer versions of clap to
resolve this issue.
They are used with IOKit and there are no reexports of them from
core_foundation or io_kit_sys.
It's the transitive dependency atty again which we are pulling in
through clap. Same situation as with RUSTSEC-2024-0370, it's just no
maintained as of now.
@sirhcel
Copy link
Contributor Author

sirhcel commented Oct 14, 2024

I don't recall the exact steps that I took now. I vaguely recall launching xcode for my project from the command line and then instrumenting it. Sorry for not being able to provide more info.

Thanks for your feedback! In this case I will take the test made in #218 (comment) as an indicator that I've not introduced new leaks with this refactoring.

@sirhcel sirhcel merged commit 0d13a7c into serialport:main Oct 14, 2024
30 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants