-
Notifications
You must be signed in to change notification settings - Fork 7
feat: add a native machine ID library COMPASS-8443 #527
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
Conversation
4a49185
to
39fa004
Compare
39fa004
to
c6ca238
Compare
00b0bea
to
931af28
Compare
8d8bed8
to
b8e5a65
Compare
@@ -0,0 +1,103 @@ | |||
#!/usr/bin/env node | |||
/* eslint-disable no-console */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is more for fun, I was curious what the difference would be
b8e5a65
to
fb33312
Compare
3e45d00
to
fb33312
Compare
59cb919
to
657c3b7
Compare
packages/machine-id/binding.cc
Outdated
if (uuidProperty) | ||
{ | ||
char buffer[128]; | ||
if (CFStringGetCString((CFStringRef)uuidProperty, buffer, sizeof(buffer), kCFStringEncodingUTF8)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Likely we want to return an empty string here to mark an error. While it's true that the empty constructor of std::string must initialise an empty string, it's best if we early return to avoid further mistakes as buffer contains undefined contents and might be tempting to use them.
if (CFStringGetCString((CFStringRef)uuidProperty, buffer, sizeof(buffer), kCFStringEncodingUTF8)) | |
if (!CFStringGetCString((CFStringRef)uuidProperty, buffer, sizeof(buffer), kCFStringEncodingUTF8)) { | |
// clean up too | |
return ""; | |
} |
packages/machine-id/binding.cc
Outdated
if (file.is_open()) | ||
{ | ||
std::string line; | ||
if (std::getline(file, line)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Likely we want to check the failbit in file here too. There might be sometimes that there is an overflow on the output string due to surpassing the amount of max bytes that fit in the string (unlikely) or there is not enough memory (more likely).
HKEY_LOCAL_MACHINE, | ||
"SOFTWARE\\Microsoft\\Cryptography", | ||
0, | ||
KEY_QUERY_VALUE | KEY_WOW64_64KEY, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This enforces to use always the 64 bit registry in Windows. Is that what we want to do? Windows dropped 32 bit support 5 years ago, so this is likely doing nothing and I would just use KEY_QUERY_VALUE
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ah one thing I forgot mention is that one goal of it this is to match Atlas CLI's (Go-based) device ID library so this copies the implementation from https://github.com/denisbrodbeck/machineid/blob/master/id_windows.go which does do this, assuming |
isn't different in C++
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah the operator does the same, the only thing is that this logic can break old 32 bit machines and don't do anything on 64 bit machines.
However if we want to do the same thing just in case I'm fine keeping this logic as it is.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah consistency seems preferable but we can revisit if this ends up problematic. basically having no machine ID from both the Go and Node libraries on the same machine seems preferable to only one of them producing output as it skews our analytics to imply i.e. lower % use Atlas CLI
packages/machine-id/binding.cc
Outdated
|
||
if (result == ERROR_SUCCESS && valueType == REG_SZ) | ||
{ | ||
machineGuid = value; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is dangerous. Windows does not ensure that the value is a null-terminated string, so this can overflow. We need to use the output valueSize: https://learn.microsoft.com/en-us/windows/win32/api/winreg/nf-winreg-regqueryvalueexa
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Leaving it approved so you can merge when the comments have been resolved.
Great job!
bf8c4ae
to
60cc9ce
Compare
Adds a native device ID library for MacOS, Linux, and Windows. 100x+ faster implementation without depending on child process spawning.
Meant as a replacement for node-machine-id and since this is a popular library and usecase, I also put some effort to make this package more tidy and useful for the larger community.