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

VNet: Use NSObject instead of C struct in XPC message #44868

Merged
merged 2 commits into from
Aug 1, 2024
Merged

Conversation

ravicious
Copy link
Member

Edoardo asked if we need to worry about the size of the C struct that's passed in the XPC message between the client and the daemon service (#44751 (comment)). This might be the case if the client uses a different version of tsh where the struct happens to have, say, new fields. It didn't occur to me to check this. But after a quick test it turned out that XPC is smart enough to reject messages where the size of the struct doesn't match (check the linked comment for more details).

However, this also means that instead of getting a clear error on the daemon side when incompatible tsh versions talk to each other, we'll see the error only when running Console.app. To make sure this doesn't happen in the future, I replaced the C struct with an NSObject, which by necessity must implement NSSecureCoding (docs for NSSecureCoding).

To that end, now that I know a little more about properties in Obj-C, I also cleaned up how they're defined and accessed.

A quick primer on properties in Obj-C

Each property is backed by an instance variable, automatically created by the compiler when you declare a property through @property … inside @interface. This is similar to any other object-oriented language where if you want to expose an instance variable you define some kind of a getter (and perhaps a setter). @property foo defines an ivar _foo.

By default, properties have the following attributes:

  • atomic: "Property atomicity is not synonymous with an object’s thread safety". As far as I understand it, a setter for example can perform multiple operations on the underlying ivar and this attribute makes sure that the ivar doesn't change for the duration of the setter.
  • readwrite: both accessors are generated for the property, unlike readonly which creates only the getter.
  • strong: uses a strong reference for the ivar. It has to do with reference counting, the linked docs includes an explanation, I also found a pretty entertaining one using balloons to demonstrate the concept.

As far as I understand, this has changed over time, e.g. before Arc strong wasn't the default attribute. The code I based the implementation of the launch daemon on is also pretty old, as well as half of the SO answers you can find about Obj-C. As such, I ended up including a bunch of cruft that can be removed. A couple of examples:

  • People often add nonatomic if they know they don't have to worry about thread safety because it makes accessing properties "faster". But we only ever send a single message, so we don't have to worry about it at all.
  • A good practice is to use copy for NSString properties in case NSMutableString is provided to the setter. But this doesn't matter if the property is readonly since there's no setter.
  • Properties can be accessed using the dot shorthand instead of having to use square brackets. [[foo bar] baz] can be just foo.bar.baz or [foo.bar baz] if baz is a method call.
  • Declaring ivars inside @implementation blocks was added circa 2012, so people used to add properties because there was no other choice. But a property is needed only if we need to make the ivar public. Here's an example of defining private ivars inside @implementation:

@implementation VNEDaemonService {
NSXPCListener *_listener;
dispatch_semaphore_t _gotVnetConfigSema;
}

@ravicious ravicious added no-changelog Indicates that a PR does not require a changelog entry backport/branch/v16 labels Jul 31, 2024
@ravicious ravicious requested review from codingllama and gzdunek July 31, 2024 11:31
@github-actions github-actions bot requested review from Joerger and probakowski July 31, 2024 11:31
Copy link
Contributor

@codingllama codingllama left a comment

Choose a reason for hiding this comment

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

LGTM.

Thanks for the detailed description, super helpful.

@codingllama codingllama requested a review from espadolini July 31, 2024 13:49
Copy link
Contributor

@espadolini espadolini 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 but I don't know enough about ObjC for my looking to mean much 🙈

@ravicious ravicious added this pull request to the merge queue Aug 1, 2024
Merged via the queue into master with commit e552c01 Aug 1, 2024
42 checks passed
@ravicious ravicious deleted the r7s/nsobject branch August 1, 2024 09:40
@public-teleport-github-review-bot

@ravicious See the table below for backport results.

Branch Result
branch/v16 Create PR

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport/branch/v16 no-changelog Indicates that a PR does not require a changelog entry size/sm
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants