Introduces `BlessError` to provide detailed error explanation #14

jakaplan · 2022-06-16T11:25:26Z

Instead of just passing through Apple's (lackluster) CFError value, BlessedError actually runs through each requirement and provides a detailed explanation for each one that fails to be met. This is done in part by adding dependencies on the EmbeddedPropertyList and Required packages.

Instead of just passing through Apple's (lackluster) CFError value, `BlessedError` actually runs through each requirement and provides a detailed explanation for each one that fails to be met.

jakaplan · 2022-06-16T11:27:36Z

@amomchilov I believe in the past this is the type of improved error handling you wanted to see added to Blessed. Would you be interested in taking a look at this and letting me know your thoughts?

amomchilov

This is pretty much exactly what I had in mind! <3

amomchilov · 2022-06-17T00:49:21Z

Sources/Blessed/BlessError.swift

+            asssessHelperToolLaunchdPropertyList(label: label), // 5 & 6
+            asssessHelperToolInfoPropertyListAuthorizedClients(label: label, type: .bundled), // 7 & 8 - bundled
+            asssessHelperToolInfoPropertyListAuthorizedClients(label: label, type: .installed), // 7 & 8 - installed
+            asssessHelperToolInfoPropertyListBundleVersion(label: label), // 9
+            asssessAppInfoPropertyList(label: label) // 10


Suggested change

asssessHelperToolLaunchdPropertyList(label: label), // 5 & 6

asssessHelperToolInfoPropertyListAuthorizedClients(label: label, type: .bundled), // 7 & 8 - bundled

asssessHelperToolInfoPropertyListAuthorizedClients(label: label, type: .installed), // 7 & 8 - installed

asssessHelperToolInfoPropertyListBundleVersion(label: label), // 9

asssessAppInfoPropertyList(label: label) // 10

assessHelperToolLaunchdPropertyList(label: label), // 5 & 6

assessHelperToolInfoPropertyListAuthorizedClients(label: label, type: .bundled), // 7 & 8 - bundled

assessHelperToolInfoPropertyListAuthorizedClients(label: label, type: .installed), // 7 & 8 - installed

assessHelperToolInfoPropertyListBundleVersion(label: label), // 9

assessAppInfoPropertyList(label: label) // 10

Oh whoops(s)!

amomchilov · 2022-06-17T01:06:48Z

Sources/Blessed/BlessError.swift

+}
+
+// 2 & 3
+fileprivate func assessHelperToolIsExecutable(label: String) -> Assessment {


kind of a nit, but if you grouped these functions into something like a HelperToolValidator, then the method names can be shorted like isExecutable.

That also lets you remove all these label: String parameters, in favor of a single ivar

Ah good idea, will do

amomchilov · 2022-06-17T01:09:35Z

Sources/Blessed/BlessError.swift

+    if result == errSecSuccess {
+        return .satisfied
+    } else if result == errSecCSUnsigned {
+        return .notSatisfied(explanation: "This application does not have a valid signature")
+    } else {
+        return .notDetermined(explanation: "Signature checking failed with error \(result)")
+    }


Perfect use-case for switch:

Suggested change

if result == errSecSuccess {

return .satisfied

} else if result == errSecCSUnsigned {

return .notSatisfied(explanation: "This application does not have a valid signature")

} else {

return .notDetermined(explanation: "Signature checking failed with error \(result)")

}

switch result {

case errSecSuccess: return .satisfied

case errSecCSUnsigned: return .notSatisfied(explanation: "This application does not have a valid signature")

default: return .notDetermined(explanation: "Signature checking failed with error \(result)")

}

Likewise below

Ah yeah that's a bit clearer

amomchilov · 2022-06-17T01:22:36Z

Sources/Blessed/BlessError.swift

+    let validMagicValues: Set<UInt32> = [MH_MAGIC, MH_CIGAM, MH_MAGIC_64, MH_CIGAM_64, FAT_MAGIC, FAT_CIGAM]
+    guard validMagicValues.contains(firstFourBytes) else {
+        return .notSatisfied(explanation: "The bundle helper tool is not a Mach-O executable")


Is there a built in API for checking this, without needing to specify the valid magic headers explicitly?

There doesn't appear to be and that makes sense because normally there'd be different code paths needed depending on what architecture the Mach-O executable is for. There are a bunch of functions for reading parts of a Mach-O executable (which the EmbeddedPropertyList package makes use of).

amomchilov · 2022-06-17T01:35:21Z

Sources/Blessed/BlessError.swift

+    // The helper tool's embedded launchd property list **must** have an entry with `Label` as the key and the
+    // value **must** be the filename of the helper tool.


Redundant comment, it's pretty explicitly spelt out below in code already

While this is true, this is intentionally a word-for-word copy of the requirements for bless(...) function documentation. Each checked portion of the requirement has this throughout BlessError.

Ahhh, didn't notice. Is that explained anywhere?

It is not, I can add a comment somewhere before the assessments making that explicit.

amomchilov · 2022-06-17T01:38:00Z

Sources/Blessed/BlessError.swift

+    guard let plist = try? PropertyListSerialization.propertyList(from: data,
+                                                                 options: .mutableContainersAndLeaves,
+                                                                 format: nil) as? NSDictionary else {
+        return .notSatisfied(explanation: "The data embedded as the bundled helper tool's launchd property list is " +
+                                          "not a valid property list")
+    }
+    guard let plistLabel = plist["Label"] else {
+        return .notSatisfied(explanation: "The bundled helper tool's embedded launchd property list does not have a " +
+                                          "Label key")
+    }
+    guard label == plistLabel as? String else {
+        return .notSatisfied(explanation: "The bundled helper tool's launchd property list's value for the Label key " +
+                                           "does not match the label for the bundled helper tool\n" +
+                                           "Required value: \(label)\n" +
+                                           "Actual value: \(plistLabel)")
+    }


IMO using multi-line strings here works quite nice. They start on a new line anyway, so you alleviate the need to word break in the middle of short sentences:

Suggested change

guard let plist = try? PropertyListSerialization.propertyList(from: data,

options: .mutableContainersAndLeaves,

format: nil) as? NSDictionary else {

return .notSatisfied(explanation: "The data embedded as the bundled helper tool's launchd property list is " +

"not a valid property list")

}

guard let plistLabel = plist["Label"] else {

return .notSatisfied(explanation: "The bundled helper tool's embedded launchd property list does not have a " +

"Label key")

}

guard label == plistLabel as? String else {

return .notSatisfied(explanation: "The bundled helper tool's launchd property list's value for the Label key " +

"does not match the label for the bundled helper tool\n" +

"Required value: \(label)\n" +

"Actual value: \(plistLabel)")

}

guard let plist = try? PropertyListSerialization.propertyList(from: data,

options: .mutableContainersAndLeaves,

format: nil) as? NSDictionary else {

return .notSatisfied(explanation: """

The data embedded as the bundled helper tool's launchd property list is not a valid property list

""")

}

guard let plistLabel = plist["Label"] else {

return .notSatisfied(explanation: """

The bundled helper tool's embedded launchd property list is missing a value for the key "Label"

""")

}

guard label == plistLabel as? String else {

return .notSatisfied(explanation: """

The bundled helper tool's launchd property list's value for the "Label" key

does not match the label for the bundled helper tool.

Required value: \(label)

Actual value: \(plistLabel)

""")

}

Applies nicely to a few other places too

amomchilov · 2022-06-17T02:02:39Z

Sources/Blessed/BlessError.swift

+        for requirement in authorizedClientRequirements {
+            let evaluation = try? (try? Parser.parse(requirement: requirement))?.evaluateForCode(code)
+            explanation += "\n\(evaluation?.prettyDescription ?? "Failed to evaluate requirement.")"
+        }


Can this be simplified to a flatMap?

I think it could use a flatMap, but I don't see how it could be made simpler because the result would then be an array which then would need to be turned into a single string via joined?

reduce works nicely though

Ah, you get back a [Character]. Joining those is just a matter of running them through the string initializer, e.g.

String(["line 1\n", "line 2\n"].flatMap { "foo - " + $0 })

reduce also works, but it has n^2 time performance, so just be wary if you ever use this pattern in the future for large strings

In this specific case I expect n to most commonly be 1, occasionally 2 or perhaps 3.

More broadly though, why would it be O(n^2) as opposed to O(n)? It looks like it performs n iterations and allows for combining the result of one iteration with the next one. Is your comment about the cost of appending a string to another string? If so I'd expect that'd be a factor of how long the string is, not the number of elements in the collection for which reduce was called on.

Is your comment about the cost of appending a string to another string?

Yep

If so I'd expect that'd be a factor of how long the string is, not the number of elements in the collection for which reduce was called on.

It's a function of both the inputs strings' sizes, and their quantity. Larger strings (early on) means there's more to copy on each string reallocation, and a higher quantity means that more copies are necessary.

Definitely not a concern here, but just a warning for something to watch out for.

Actually, this makes me wonder: Are strings "right-sized" for the immediate need, or do they grow with extra capacity (e.g. capacity *= 1.5 on every realloc) similar to an Array?

Update, did some digging, yes String has slack: https://github.com/apple/swift/blob/0c67ce64874d83b2d4f8d73b899ee58f2a75527f/stdlib/public/core/StringStorage.swift#L457-L467

I guess this, plus the ability to mutate in-place (when uniquely referenced) makes sense why there isn't a StringBuilder like in Java (where appending even a single letter to a string would always require a full copy of the first part).

amomchilov · 2022-06-18T00:34:04Z

Sources/Blessed/BlessError.swift

+// The numbers in front of each function corresponds to the requirements described in LaunchdManager.bless(...)
+
+
+fileprivate struct HelperToolAssessor {


This turned out great!

And some other minor misc improvements

Introduces BlessError to provide detailed error explanation

2e71752

Instead of just passing through Apple's (lackluster) CFError value, `BlessedError` actually runs through each requirement and provides a detailed explanation for each one that fails to be met.

amomchilov approved these changes Jun 17, 2022

View reviewed changes

Josh Kaplan added 2 commits June 18, 2022 00:01

Addresses PR feedback, improves some error messages a little bit

a9abc18

Bumps the minimum version requirement for Required

5e36150

amomchilov reviewed Jun 18, 2022

View reviewed changes

Adds handling of kSMErrorJobMustBeEnabled error code

714e149

And some other minor misc improvements

jakaplan merged commit 50e98c0 into main Jun 20, 2022

jakaplan deleted the blessed-error branch June 21, 2022 13:28

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Introduces `BlessError` to provide detailed error explanation #14

Introduces `BlessError` to provide detailed error explanation #14

jakaplan commented Jun 16, 2022 •

edited

jakaplan commented Jun 16, 2022

amomchilov left a comment

amomchilov Jun 17, 2022

jakaplan Jun 17, 2022

amomchilov Jun 17, 2022

jakaplan Jun 17, 2022

amomchilov Jun 17, 2022

amomchilov Jun 17, 2022

jakaplan Jun 17, 2022

amomchilov Jun 17, 2022

jakaplan Jun 17, 2022

amomchilov Jun 17, 2022

jakaplan Jun 17, 2022

amomchilov Jun 18, 2022

jakaplan Jun 18, 2022

amomchilov Jun 17, 2022

amomchilov Jun 17, 2022

amomchilov Jun 17, 2022

jakaplan Jun 17, 2022 •

edited

jakaplan Jun 17, 2022

amomchilov Jun 18, 2022

jakaplan Jun 18, 2022

amomchilov Jun 19, 2022

amomchilov Jun 19, 2022

amomchilov Jun 18, 2022

		// The helper tool's embedded launchd property list must have an entry with `Label` as the key and the
		// value must be the filename of the helper tool.

		// The numbers in front of each function corresponds to the requirements described in LaunchdManager.bless(...)


		fileprivate struct HelperToolAssessor {

Introduces BlessError to provide detailed error explanation #14

Introduces BlessError to provide detailed error explanation #14

Conversation

jakaplan commented Jun 16, 2022 • edited

jakaplan commented Jun 16, 2022

amomchilov left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jakaplan Jun 17, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Introduces `BlessError` to provide detailed error explanation #14

Introduces `BlessError` to provide detailed error explanation #14

jakaplan commented Jun 16, 2022 •

edited

jakaplan Jun 17, 2022 •

edited