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

Fix an export name check exception #2806

Merged
merged 1 commit into from
Dec 1, 2023

Conversation

lum1n0us
Copy link
Collaborator

No description provided.

Copy link
Collaborator

@yamt yamt left a comment

Choose a reason for hiding this comment

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

this fix doesn't seem right.
if we want to support names including '\00', we should stop using strlen etc.

@lum1n0us
Copy link
Collaborator Author

lum1n0us commented Nov 23, 2023

we are thinking about using an extra length field to store the length of a string, in our case an unicode string.

It seems both import name and export name need to face gaps between cstyle name and unicode name:

  • an unicode name may start with a \0
  • an unicode name may not end with a \0

Definity, we need an extra field to store name's length and use binary operations to replace "<string.h>" operations when dealing with import names and export names.

Need to do include modifications about:

  • Store Wasm import names and export names in unicode string instead of c-style string
  • Process(mostly comparison) import names and export names in a way of binaries. It includes linking(w/ import names), calling from outside(w/ export names)
  • (Optional) handling unicode strings in REPL
  • (Partial implement) Store import name and export name in unicode string in AOT file
  • Use unicode strings in const string pool of both Wasm loader and AOT

It is requires by Spec. But, is it a good idea to support unicode string as import name and export name? A lot modification Vs. unusual benefit. We tend to put a limitation of import names and export names that only c-style names are allowed.

name in string name in binary support?
length bytes
"" 00 00 Y
"a" 01 61 Y
"\61" 01 61 N
"\00" 01 00 N
"\00\01" 02 00 01 N
"\01\00\02" 03 01 00 02 N
"\61\00" 02 61 00 N

@lum1n0us lum1n0us marked this pull request as draft November 23, 2023 02:23
@lum1n0us lum1n0us force-pushed the export_name_check branch 2 times, most recently from 9af0f77 to ee277b6 Compare November 24, 2023 09:28
@lum1n0us lum1n0us marked this pull request as ready for review November 30, 2023 05:27
Copy link
Contributor

@wenyongh wenyongh left a comment

Choose a reason for hiding this comment

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

LGTM

@wenyongh wenyongh merged commit 718f067 into bytecodealliance:main Dec 1, 2023
383 checks passed
victoryang00 pushed a commit to victoryang00/wamr-aot-gc-checkpoint-restore that referenced this pull request May 27, 2024
…ealliance#2806)

Leave it as a limitation when import/export name contains '\00' in wasm file.
p.s. bytecodealliance#2789
@lum1n0us lum1n0us deleted the export_name_check branch May 31, 2024 03:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants