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

Introduce import.meta hook for module script #26544

Merged
merged 1 commit into from May 17, 2020

Conversation

@CYBAI
Copy link
Collaborator

CYBAI commented May 16, 2020


  • ./mach build -d does not report any errors
  • ./mach test-tidy does not report any errors
  • These changes fix #26542
  • There are tests for these changes under module/import-meta folder.
@highfive
Copy link

highfive commented May 16, 2020

Heads up! This PR modifies the following files:

@highfive
Copy link

highfive commented May 16, 2020

warning Warning warning

  • These commits modify unsafe code. Please review it carefully!
@CYBAI
Copy link
Collaborator Author

CYBAI commented May 16, 2020

r? @jdm

@highfive highfive assigned jdm and unassigned asajeffrey May 16, 2020
Copy link
Member

jdm left a comment

Ensuring the resulting strings don't contain random memory should help!

components/script/script_module.rs Outdated Show resolved Hide resolved
components/script/script_module.rs Outdated Show resolved Hide resolved
@CYBAI CYBAI force-pushed the CYBAI:module-metadata-hook branch from c1a720b to 84e396e May 17, 2020
@CYBAI
Copy link
Collaborator Author

CYBAI commented May 17, 2020

@jdm fixing with what you pointed out, we can pass the test now!

Ensuring the resulting strings don't contain random memory should help!

but, I still don't understand what you mean for contain random memory here 🤔
does that mean, while using JS_NewStringCopyZ, it's possible to return an empty string which might cause different memory in each run?

btw, how can I know when I'll need a null-terminated string 🤔 ? thanks!

@CYBAI CYBAI changed the title [WIP] Introduce import.meta hook for module script Introduce import.meta hook for module script May 17, 2020
@CYBAI CYBAI requested a review from jdm May 17, 2020
@jdm
Copy link
Member

jdm commented May 17, 2020

If an API expects a null-terminated string and we pass a string constant without one, whatever values are present in memory following the string contents will be included until there's a null value encountered. That's why the test failed, because we ended setting a property named something like "urlZzta3#';(84, x?&".

In general, any C aoi that accepts a chat* argument that is intended to be a name or string value should be null terminated unless there's a length argument included. In the string copy API in particular, the Z suffix stands for "zero", which means null-terminated. The N suffix stands for "length" and accepts an argument that includes an end pointer.

@jdm
Copy link
Member

jdm commented May 17, 2020

I suspect this needs a fmt, but otherwise it looks fine.

@CYBAI
Copy link
Collaborator Author

CYBAI commented May 17, 2020

If an API expects a null-terminated string and we pass a string constant without one, whatever values are present in memory following the string contents will be included until there's a null value encountered. That's why the test failed, because we ended setting a property named something like "urlZzta3#';(84, x?&".

In general, any C aoi that accepts a chat* argument that is intended to be a name or string value should be null terminated unless there's a length argument included. In the string copy API in particular, the Z suffix stands for "zero", which means null-terminated. The N suffix stands for "length" and accepts an argument that includes an end pointer.

I see! That's very clear and I understand now! Thanks! :D

I suspect this needs a fmt, but otherwise it looks fine.

Wait me a minute! Looks like I need to upgrade my local fmt 🤔

@CYBAI
Copy link
Collaborator Author

CYBAI commented May 17, 2020

oh, it's not a fmt error from rustfmt :P

@CYBAI CYBAI force-pushed the CYBAI:module-metadata-hook branch from 84e396e to 6bc6240 May 17, 2020
@jdm
Copy link
Member

jdm commented May 17, 2020

@bors-servo
Copy link
Contributor

bors-servo commented May 17, 2020

📌 Commit 6bc6240 has been approved by jdm

@bors-servo
Copy link
Contributor

bors-servo commented May 17, 2020

Testing commit 6bc6240 with merge 5a99b5e...

bors-servo added a commit that referenced this pull request May 17, 2020
Introduce import.meta hook for module script

<!-- Please describe your changes on the following line: -->

---
- [x] `./mach build -d` does not report any errors
- [x] `./mach test-tidy` does not report any errors
- [x] These changes fix #26542
- [x] There are tests for these changes under `module/import-meta` folder.
@bors-servo
Copy link
Contributor

bors-servo commented May 17, 2020

💔 Test failed - status-taskcluster

@jdm
Copy link
Member

jdm commented May 17, 2020

@bors-servo
Copy link
Contributor

bors-servo commented May 17, 2020

Testing commit 6bc6240 with merge 619e0bc...

@bors-servo
Copy link
Contributor

bors-servo commented May 17, 2020

☀️ Test successful - status-taskcluster
Approved by: jdm
Pushing 619e0bc to master...

@bors-servo bors-servo merged commit 619e0bc into servo:master May 17, 2020
2 checks passed
2 checks passed
Community-TC (pull_request) TaskGroup: success
Details
homu Test successful
Details
@CYBAI CYBAI deleted the CYBAI:module-metadata-hook branch May 17, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

5 participants
You can’t perform that action at this time.