Skip to content

buffer: add buffer.isUtf8 for utf8 validation #45947

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

Merged
nodejs-github-bot merged 6 commits into from
Dec 25, 2022
Merged

buffer: add buffer.isUtf8 for utf8 validation #45947

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
be10b36
buffer: add buffer.isUtf8 for utf8 validation
anonrig Dec 22, 2022
bcb19ec
fixup! buffer: add buffer.isUtf8 for utf8 validation
anonrig Dec 23, 2022
6c8ac38
fixup! buffer: add buffer.isUtf8 for utf8 validation
anonrig Dec 23, 2022
103e807
fixup! buffer: add buffer.isUtf8 for utf8 validation
anonrig Dec 23, 2022
b590f06
fixup! buffer: add buffer.isUtf8 for utf8 validation
anonrig Dec 24, 2022
e940f59
fixup! buffer: add buffer.isUtf8 for utf8 validation
anonrig Dec 24, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
fixup! buffer: add buffer.isUtf8 for utf8 validation
  • Loading branch information
@anonrig
anonrig committed Dec 24, 2022
commit b590f061edbbca3568f07869dc461da4717a807e
6 changes: 4 additions & 2 deletions src/node_buffer.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1226,11 +1226,13 @@ static void EncodeInto(const FunctionCallbackInfo<Value>& args) {
static void IsUtf8(const FunctionCallbackInfo<Value>& args) {
Environment* env = Environment::GetCurrent(args);
CHECK_EQ(args.Length(), 1);
CHECK(args[0]->IsTypedArray() || args[0]->IsArrayBuffer());
CHECK(args[0]->IsTypedArray() || args[0]->IsArrayBuffer() ||
args[0]->IsSharedArrayBuffer());
ArrayBufferViewContents<char> abv(args[0]);

if (abv.WasDetached()) {
return node::THROW_ERR_BUFFER_CONTEXT_NOT_AVAILABLE(env->isolate());
return node::THROW_ERR_INVALID_STATE(
env, "Cannot validate on a detached buffer");
}
Copy link
Copy Markdown
Member

@jasnell jasnell Dec 25, 2022

Choose a reason for hiding this comment

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

I know it's after the fact but why does the buffer being detached matter here? It would be otherwise indistinguishable from zero-length which we should just return false for anyway.

Copy link
Copy Markdown
Member Author

@anonrig anonrig Dec 25, 2022

Choose a reason for hiding this comment

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

Detached buffers create false sense of UTF8 validation, if there isn’t an error in here, since there is no way of accessing the underlying data store, and validating for UTF-8, I believe this error is valid.


args.GetReturnValue().Set(simdutf::validate_utf8(abv.data(), abv.length()));
Expand Down
1 change: 1 addition & 0 deletions src/node_errors.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -68,6 +68,7 @@ void OOMErrorHandler(const char* location, const v8::OOMDetails& details);
V(ERR_INVALID_ARG_TYPE, TypeError) \
V(ERR_INVALID_OBJECT_DEFINE_PROPERTY, TypeError) \
V(ERR_INVALID_MODULE, Error) \
V(ERR_INVALID_STATE, Error) \
V(ERR_INVALID_THIS, TypeError) \
V(ERR_INVALID_TRANSFER_OBJECT, TypeError) \
V(ERR_MEMORY_ALLOCATION_FAILED, Error) \
Expand Down
14 changes: 13 additions & 1 deletion test/parallel/test-buffer-isutf8.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,9 +9,21 @@ const encoder = new TextEncoder();

assert.strictEqual(isUtf8(encoder.encode('hello')), true);
assert.strictEqual(isUtf8(encoder.encode('ğ')), true);
assert.strictEqual(isUtf8(Buffer.from([])), true);
Copy link
Copy Markdown
Member

@jasnell jasnell Dec 25, 2022

Choose a reason for hiding this comment

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

Why does a zero length buffer return true? I would expect this to be false.

Copy link
Copy Markdown
Member Author

@anonrig anonrig Dec 25, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Because it does not include an invalid code point. Is there a similar Node function that has a different behavior?

Copy link
Copy Markdown
Member

@jasnell jasnell Dec 25, 2022

Choose a reason for hiding this comment

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

But the stated description of the API is, "This function is used to check if input contains UTF-8 code points"... An empty buffer does not contain UTF-8 code points so it really can't return true. Other methods we have that accept ArrayBuffer or TypedArray, with the exception of Web Streams which have specifically defined handling for detached, will treat those as indistinguishable from a zero-length input.

Copy link
Copy Markdown
Member Author

@anonrig anonrig Dec 25, 2022

Choose a reason for hiding this comment

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

Hm.. Thats correct. What do you recommend?

Copy link
Copy Markdown
Member

@jasnell jasnell Dec 25, 2022

Choose a reason for hiding this comment

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

I would just follow up with an additional pr that returned false for zero-length, removing the detached check and error entirely.

Copy link
Copy Markdown
Member

@lpinca lpinca Dec 25, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

In my opinion it should not be changed. It should return true. It's like the empty string which is valid UTF-8. I would be surprised if isUtf8(encoder.encode(''); returns false.

To avoid confusion the documentation can be updated like this "This function returns false if the input contains invalid UTF-8 code points, else true".

Copy link
Copy Markdown
Member

@jasnell jasnell Dec 25, 2022

Choose a reason for hiding this comment

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

The challenge there is that with that logic the empty buffer would pass any encoding check. isASCII? Yes. isUTF16le? Yes. IsUTF32be? Yes. Is Shift-JIS? Yes.... Which just simply isn't useful. If you want the inverse check, isInvalidUtf8() then implement that.

Copy link
Copy Markdown
Member Author

@anonrig anonrig Dec 25, 2022

Choose a reason for hiding this comment

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

I created a pull request: #45973

Copy link
Copy Markdown
Member

@lemire lemire Dec 25, 2022

Choose a reason for hiding this comment

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

The UTF-8 RFC specifies...

An octet sequence is valid UTF-8 only if it matches the
   following syntax, which is derived from the rules for encoding UTF-8
   and is expressed in the ABNF of [[RFC2234](https://www.rfc-editor.org/rfc/rfc2234)].

   UTF8-octets = *( UTF8-char )
...

Reference: https://www.rfc-editor.org/rfc/rfc3629

So UTF-8 explicitly, by its ABNF, includes the empty string.

Note that, in general, from a non-empty buffer alone, we cannot determine uniquely the character encoding. A BOM may help but UTF-8 is BOM-less.

A string of bytes may be interpreted under different encodings... and in some cases, it is by design. Thus, for example, ASCII buffers are always valid UTF-8 and Latin1 buffers (by design).

Copy link
Copy Markdown
Member

@lpinca lpinca Dec 25, 2022

Choose a reason for hiding this comment

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

The challenge there is that with that logic the empty buffer would pass any encoding check.

Yes, I think it makes sense and that is how it works in some other popular programming languages.


// Invalid UTF-8
assert.strictEqual(isUtf8(Buffer.from([0xf8])), false);
Comment thread
anonrig marked this conversation as resolved.
Outdated

// CESU-8
assert.strictEqual(isUtf8(encoder.encode('\u0045\u0205\u10400')), true);
assert.strictEqual(isUtf8(encoder.encode('aé日')), true);
Comment thread
anonrig marked this conversation as resolved.
Outdated

// Two byte overlong encoding
assert.strictEqual(isUtf8(encoder.encode('\u0000')), true);

// WTF-8
assert.strictEqual(isUtf8(encoder.encode('\uD800\uDFFF')), true);

[
null,
undefined,
Expand All @@ -34,7 +46,7 @@ assert.strictEqual(isUtf8(encoder.encode('aé日')), true);
assert.throws(
() => { isUtf8(arrayBuffer); },
{
code: 'ERR_BUFFER_CONTEXT_NOT_AVAILABLE'
code: 'ERR_INVALID_STATE'
}
);
}