-
Notifications
You must be signed in to change notification settings - Fork 30.3k
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
buffer: throw if string is not a valid HEX string #3773
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -451,11 +451,36 @@ size_t StringBytes::Write(Isolate* isolate, | |
return nbytes; | ||
} | ||
|
||
template <typename Type> | ||
bool IsValidHexString(Isolate* isolate, const Type* str, const size_t len) { | ||
for (size_t i = 0; i < len; i += 1) { | ||
if (!((str[i] >= '0' && str[i] <= '9') || | ||
(str[i] >= 'a' && str[i] <= 'f') || | ||
(str[i] >= 'A' && str[i] <= 'F'))) | ||
return false; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Suggest braces around the consequent. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is there any reason why this requires |
||
} | ||
return true; | ||
} | ||
|
||
bool IsValidHexString(Isolate* isolate, Local<String> string) { | ||
if (string->Length() % 2 != 0) | ||
return false; | ||
|
||
const char* str = nullptr; | ||
size_t n = 0; | ||
bool external = StringBytes::GetExternalParts(isolate, string, &str, &n); | ||
|
||
if (external) | ||
return IsValidHexString(isolate, str, n); | ||
|
||
String::Value value(string); | ||
return IsValidHexString(isolate, *value, value.length()); | ||
} | ||
|
||
bool StringBytes::IsValidString(Isolate* isolate, | ||
Local<String> string, | ||
enum encoding enc) { | ||
if (enc == HEX && string->Length() % 2 != 0) | ||
if (enc == HEX && IsValidHexString(isolate, string) == false) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
return false; | ||
// TODO(bnoordhuis) Add BASE64 check? | ||
return true; | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -741,6 +741,15 @@ for (let i = 0; i < 256; i++) { | |
assert.equal(b2, b4); | ||
} | ||
|
||
// invalid HEX strings should throw an error | ||
assert.throws(() => new Buffer('xx', 'hex'), /TypeError: Invalid hex string/); | ||
// following will fail even though it has zero as the first character, because | ||
// `x` is an invalid hexa-decimal value | ||
assert.throws(() => new Buffer('0x', 'hex'), /TypeError: Invalid hex string/); | ||
// following invalid HEX strings will fail because of the odd length | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. hex |
||
assert.throws(() => new Buffer('0', 'hex'), /TypeError: Invalid hex string/); | ||
assert.throws(() => new Buffer('000', 'hex'), /TypeError: Invalid hex string/); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Could you also add some test without the zeros or |
||
|
||
function buildBuffer(data) { | ||
if (Array.isArray(data)) { | ||
var buffer = new Buffer(data.length); | ||
|
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.
Seems rather wasteful to call IsValidString() and only act on the result when encoding == HEX.
EDIT: What I mean is that IsValidString() may be cheap now but that can change when more checks are added.
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.
I was about to send a follow-up or for base 64 strings
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.
No need to compare a known boolean value to false.
if (!…
would do.