Skip to content

test: add tests for invalid UTF-8 - #40351

Closed
git-srinivas wants to merge 2 commits into
nodejs:masterfrom
git-srinivas:body-mixin-text-to-return-usvstring
Closed

test: add tests for invalid UTF-8#40351
git-srinivas wants to merge 2 commits into
nodejs:masterfrom
git-srinivas:body-mixin-text-to-return-usvstring

Conversation

@git-srinivas

@git-srinivasgit-srinivas commented Oct 6, 2021

Copy link
Copy Markdown
Contributor

Fixes: #39804

  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • make lint passes

@nodejs-github-botnodejs-github-bot added the needs-ci PRs that need a full CI run. label Oct 6, 2021
@MesteeryMesteery added the stream Issues and PRs related to the stream subsystem. label Oct 6, 2021
@git-srinivas
git-srinivasforce-pushed the body-mixin-text-to-return-usvstring branch from 0995716 to 18827d1CompareOctober 7, 2021 02:57

@lpincalpinca left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

// Run with --expose-internals flagconstassert=require('assert');const{ toUSVString }=require('internal/util');constdecoder=newTextDecoder();constchunk=Buffer.from([0x66,0x6f,0x6f,0xed,0xa0,0x80]);// foo + U+D800conststr=decoder.decode(chunk);assert.strictEqual(toUSVString(str),'foo\ufffd');

TextDecoder() already replaces each byte of the surrogate code point with U+FFFD.

@lpinca

lpinca commented Oct 7, 2021

Copy link
Copy Markdown
Member

Hmm both Chrome and Safari work like this PR:

newBlob([newUint8Array([0x66,0x6f,0x6f,0xed,0xa0,0x80])]).text().then((str)=>str==='foo\ufffd\ufffd\ufffd').then(console.log);

but this is not consistent with

const{ toUSVString }=require('internal/util');toUSVString('foo\ud800');// returns 'foo\ufffd';

@git-srinivas

Copy link
Copy Markdown
ContributorAuthor
// Run with --expose-internals flagconstassert=require('assert');const{ toUSVString }=require('internal/util');constdecoder=newTextDecoder();constchunk=Buffer.from([0x66,0x6f,0x6f,0xed,0xa0,0x80]);// foo + U+D800conststr=decoder.decode(chunk);assert.strictEqual(toUSVString(str),'foo\ufffd');

TextDecoder() already replaces each byte of the surrogate code point with U+FFFD.

@lpinca Does it mean the issue 39804 is not valid?

@ronag

ronag commented Oct 8, 2021

Copy link
Copy Markdown
Member
```js
// Run with --expose-internals flag
const assert = require('assert');
const { toUSVString } = require('internal/util');
const decoder = new TextDecoder();
const chunk = Buffer.from([0x66, 0x6f, 0x6f, 0xed, 0xa0, 0x80]); // foo + U+D800
const str = decoder.decode(chunk);
assert.strictEqual(toUSVString(str), 'foo\ufffd');

TextDecoder() already replaces each byte of the surrogate code point with U+FFFD.

@lpinca Does it mean the issue 39804 is not valid?

Does your tests pass without the fix?

@git-srinivas

Copy link
Copy Markdown
ContributorAuthor

yes. I ran test cases with --expose-internals flag. All of my test cases are passing without the fix.

@ronag

Copy link
Copy Markdown
Member

Then I don't think this needs fixing. I would propose you remove the changes but keep the tests. We should still merge the tests.

@git-srinivas

Copy link
Copy Markdown
ContributorAuthor
// Run with --expose-internals flagconstassert=require('assert');const{ toUSVString }=require('internal/util');constdecoder=newTextDecoder();constchunk=Buffer.from([0x66,0x6f,0x6f,0xed,0xa0,0x80]);// foo + U+D800conststr=decoder.decode(chunk);assert.strictEqual(toUSVString(str),'foo\ufffd');

TextDecoder() already replaces each byte of the surrogate code point with U+FFFD.

@lpinca@ronag
I am trying to understand this example to write some good testcases.
May i know how surrogate U+D800 is encoded as [0xed, 0xa0, 0x80]?

I tried with Buffer.from('\ud800','utf8') and what i get is <Buffer ef bf bd> .

@lpinca

lpinca commented Oct 12, 2021

Copy link
Copy Markdown
Member

All surrogates are 3 bytes and in this range:

byte 1 = ED
byte 2 = [A0 - BF]
byte 3 = [80 - BF]

I tried with Buffer.from('\ud800','utf8') and what i get is <Buffer ef bf bd> .

That's because Buffer.from() replaces invalid UTF-8 byte sequences with U+FFFD.

@git-srinivas
git-srinivasforce-pushed the body-mixin-text-to-return-usvstring branch from 18827d1 to 5fa492aCompareOctober 13, 2021 15:08
Comment on lines 76 to 88

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This does not make much sense because passthrough.write() and passthrough.end() will call Buffer.from() when the chunk is written. If anything, chunks should be Buffers or Uint8Arrays.

Comment on lines 203 to 201

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Similar to the one above this does not make much sense to me. When decoder.decode(chunk) is called, chunk is already U+FFFD (valid UTF-8).

@git-srinivas
git-srinivasforce-pushed the body-mixin-text-to-return-usvstring branch from 4fb8c8a to e5857baCompareOctober 21, 2021 19:45
Comment threadtest/parallel/test-whatwg-encoding-custom-textdecoder.js Outdated
@lpinca

Copy link
Copy Markdown
Member

@git-srinivas if you can please

  1. Update PR title to update the actual scope of the PR
  2. Squash commits and use a proper commit title and body as per point 1.

@git-srinivas

Copy link
Copy Markdown
ContributorAuthor

Sure @lpinca I'll do the changes

@git-srinivas
git-srinivasforce-pushed the body-mixin-text-to-return-usvstring branch 2 times, most recently from 22de91f to 50b943bCompareOctober 22, 2021 12:16
@git-srinivasgit-srinivas changed the title lib: body mixin text to return usvstringtest: ensure incomplete utf-8 byte sequence is converted to usv stringOct 22, 2021
@lpinca

Copy link
Copy Markdown
Member

@git-srinivas can I suggest something like this?

test: add tests for invalid UTF-8
Verify that `Blob.prototype.text()`, `streamConsumers.text()` and
`TextDecoder.prototype.decode()` work as expected with invalid UTF-8.
Fixes: https://github.com/nodejs/node/issues/39804

I find the current commit message a bit misleading becausetoUSVString() works differently as per #40351 (comment).

Thank you.

Verify that `Blob.prototype.text()`, `streamConsumers.text()` and
`TextDecoder.prototype.decode()` work as expected with invalid UTF-8.
Fixes: nodejs#39804
@git-srinivas
git-srinivasforce-pushed the body-mixin-text-to-return-usvstring branch from 50b943b to 75c69afCompareOctober 23, 2021 02:44
@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

@lpinca

lpinca commented Nov 15, 2021

Copy link
Copy Markdown
Member

Landed in dc35aef.

@lpincalpinca closed this Nov 15, 2021
lpinca pushed a commit that referenced this pull request Nov 15, 2021
Verify that `Blob.prototype.text()`, `streamConsumers.text()` and
`TextDecoder.prototype.decode()` work as expected with invalid UTF-8.
PR-URL: #40351
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Robert Nagy <ronagy@icloud.com>
targos pushed a commit that referenced this pull request Nov 21, 2021
Verify that `Blob.prototype.text()`, `streamConsumers.text()` and
`TextDecoder.prototype.decode()` work as expected with invalid UTF-8.
PR-URL: #40351
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Robert Nagy <ronagy@icloud.com>
@targostargos mentioned this pull request Nov 26, 2021
danielleadams pushed a commit that referenced this pull request Jan 30, 2022
Verify that `Blob.prototype.text()`, `streamConsumers.text()` and
`TextDecoder.prototype.decode()` work as expected with invalid UTF-8.
PR-URL: #40351
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Robert Nagy <ronagy@icloud.com>
danielleadams pushed a commit that referenced this pull request Feb 1, 2022
Verify that `Blob.prototype.text()`, `streamConsumers.text()` and
`TextDecoder.prototype.decode()` work as expected with invalid UTF-8.
PR-URL: #40351
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Robert Nagy <ronagy@icloud.com>
@danielleadamsdanielleadams mentioned this pull request Feb 1, 2022
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

author readyPRs that have at least one approval, no pending requests for changes, and a CI started.bufferIssues and PRs related to the buffer subsystem.encodingIssues and PRs related to the TextEncoder and TextDecoder APIs.streamIssues and PRs related to the stream subsystem.testIssues and PRs related to the tests.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Body Mixin text() is supposed to return USVString

7 participants

@git-srinivas@lpinca@ronag@nodejs-github-bot@jasnell@cjihrig@Mesteery