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

Use registry to get timezone #52046

Merged
merged 7 commits into from Apr 12, 2019

Conversation

@twangboy
Copy link
Contributor

commented Mar 7, 2019

What does this PR do?

Uses the registry to get the timezone (reverts #51095)
Handles null characters in the registry.
Adds tests to handle null characters in the registry.

Registry is faster than shelling out commands:

timeit: cmd - 1000 runs
9.4037297
timeit: reg - 1000 runs
0.0563001

Since this is used for grains and is run often we need this to be fast.

What issues does this PR fix or reference?

#50667
#51940

Tests written?

Yes

Commits signed with GPG?

Yes

twangboy added 2 commits Mar 7, 2019
Reverts PR 51095
Checks for null characters
@twangboy

This comment has been minimized.

Copy link
Contributor Author

commented Mar 7, 2019

@afischer-opentext-com @arizvisa This should take care of the issue with null values. Please let me know if this solves the issues you are having.

@twangboy twangboy self-assigned this Mar 7, 2019
@twangboy twangboy requested a review from saltstack/team-windows Mar 7, 2019
@afischer-opentext-com

This comment has been minimized.

Copy link
Contributor

commented Mar 8, 2019

Doesn't it make sense to use the command as a fallback in case the registry lookup fails? That way we always would have a legit result.

@twangboy

This comment has been minimized.

Copy link
Contributor Author

commented Mar 8, 2019

The transition to tzutil was prompted because in some systems (Win 8 and below) the registry contained some strange data (#51455 (comment), #50667, #51940). In all cases the correct value was followed by a null character and then some additional 'junk' data. This PR will take everything up to the first null character as the timezone in those cases.

The transition to tzutil was causing incorrect results as well in instances where DST was disabled. The tzutil appends some data to the end in that case which couldn't map to the lookup dict. This was causing the second issue for some folks... or would in the next release.

@arizvisa

This comment has been minimized.

Copy link

commented Mar 8, 2019

Yeah despite this specifically being related to a timezone, really it's just the semantics of the registry apis that's confusing. Fetching the Timezone from the registry is probably safe.

The oversight is that Windows allows you to write arbitrary data to any the registry types. So types such as REG_SZ (and even REG_DWORD), lets you write anything you want (its parameters are a pointer to your data and a length) due to the type really just being an enumeration and not a constraint.

The REG_SZ parameter even mentions that it must be null-terminated in the api documentation.
(https://docs.microsoft.com/en-us/windows/desktop/api/Winreg/nf-winreg-regsetvalueexa). When you get to languages that allow null-bytes in their "string", if the caller includes all the data returned from the api, a trailing null-byte(s) can be returned.

So the proper workflow is, if you want to read arbitrary data from the registry. There's no need to process anything. However.. if you want to read a "string" from the registry, it is up to you to terminate it at the null byte (unless your programming language already does it for you).

@arizvisa

This comment has been minimized.

Copy link

commented Mar 8, 2019

Ftr, with the prior comment this implies that any place that a string is intended to be fetched from the registry has the potential to include null bytes at the end. As twangboy mentioned, this manifests itself in the timezone on some versions of Windows which can include more than one trailng null byte.

Actually, after posting the prior comment I went to check the size of the timezone which turns out to be a common/constant size. I'm willing to bet the code that Windows was using for writing the timezone looks like:

char timezone[1024];
// initialize and populate timezone buffer
hr = RegSetValueEx(hKey, "TimeZoneInformation", 0, REG_SZ, timezone, sizeof(timezone));
...
twangboy added 4 commits Mar 11, 2019
@thatch45 thatch45 merged commit dd7a4ba into saltstack:2018.3 Apr 12, 2019
10 checks passed
10 checks passed
WIP Ready for review
Details
continuous-integration/jenkins/pr-merge This commit looks good
Details
jenkins/pr/docs The docs job has passed
Details
jenkins/pr/lint Python lint test has passed
Details
jenkins/pr/py2-centos-7 The py2-centos-7 job has passed
Details
jenkins/pr/py2-ubuntu-1604 The py2-ubuntu-1604 job has passed
Details
jenkins/pr/py2-windows-2016 The py2-windows-2016 job has passed
Details
jenkins/pr/py3-centos-7 The py3-centos-7 job has passed
Details
jenkins/pr/py3-ubuntu-1604 The py3-ubuntu-1604 job has passed
Details
jenkins/pr/py3-windows-2016 The py3-windows-2016 job has passed
Details
@twangboy twangboy deleted the twangboy:use_reg_timezone branch Apr 17, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.