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.

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

PackageIdentifiers are optional in InstallHistory.plist #6767

Merged
merged 2 commits into from
Nov 25, 2020
Merged

PackageIdentifiers are optional in InstallHistory.plist #6767

merged 2 commits into from
Nov 25, 2020

Conversation

directionless
Copy link
Member

When parsing /Library/Receipts/InstallHistory.plist not all entries have a PackageIdentifiers. Allow them to remain unset.

This opts to leave them unset, preserving the underlying data, and does not make assumptions about it

Fixes: #6766

@directionless
PackageIdentifiers are optional in InstallHistory.plist
56ce79c 
When parsing `/Library/Receipts/InstallHistory.plist` not all entries
have a PackageIdentifiers. This does not set them.

Fixes: osquery#6766
Smjert
Smjert requested changes Nov 25, 2020
View reviewed changes
Copy link
Member

@Smjert Smjert left a comment •
edited

Choose a reason for hiding this comment

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

Thank you for the PR, good catch!
I think there are a couple of small thing to fix otherwise it will return less rows than it should.

osquery/tables/system/darwin/packages.mm Show resolved Hide resolved
osquery/tables/system/darwin/packages.mm Outdated Show resolved Hide resolved
@directionless
Copy link
Member Author

The thing I'm most leery of here, is that this means package_id can suddenly be nil. This might confuse someone. On the other hand, we don't declare it as index column.

@Smjert
Copy link
Member

Smjert commented Nov 25, 2020

The thing I'm most leery of here, is that this means package_id can suddenly be nil. This might confuse someone. On the other hand, we don't declare it as index column.

But also, that's the data, I'm not sure what it can be substituted with?

@directionless
Copy link
Member Author

The thing I'm most leery of here, is that this means package_id can suddenly be nil. This might confuse someone. On the other hand, we don't declare it as index column.

But also, that's the data, I'm not sure what it can be substituted with?

Yep. That's why I wrote it as is. Data is what it is... But our old code mis-represented it to users.

Smjert
Smjert approved these changes Nov 25, 2020
View reviewed changes
Copy link
Member

@Smjert Smjert left a comment •
edited

Choose a reason for hiding this comment

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

Yep. That's why I wrote it as is. Data is what it is... But our old code mis-represented it to users.

I mean I was agreeing with you, I don't see how else we can go about this, unless those identifiers are recoverable by some other mean, so I think this PR is fine!

We can investigate later!

@directionless directionless merged commit f986e9c into osquery:master Nov 25, 2020
@directionless directionless deleted the seph/packages-issue-6766 branch November 25, 2020 15:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Smjert Smjert Smjert approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

package_install_history and big sur
2 participants
@directionless @Smjert