WebNN: Replace null characters in operand names for ORT backend [chromium/src : main]
Miao, Bin (Gerrit)
Miao, Bin added 5 comments![]( "Open in Gerrit")
Miao, BinPlease add a WPT with input name starting with null character, such as '\0', '\0input' etc.?
Done
Updated this CL to reach the comments. Please take another look, thanks a lot!
WebNN: Fix operand name validation for ORT backendMiao, BinThis title is not accurate for the latest PS.
Updated the title.
Fuzzing discovered that operand names starting with NULL byte (0x00)Miao, BinAs I commented in the issue https://issues.chromium.org/issues/470831369, it is not only fuzzer, but also input name starting with '\0' would meet this issue and cause context lost.
Updated the commit message.
if (name.contains(kNullCharacter)) {
base::ReplaceChars(name, kNullCharacter, kUnderscore, &effective_name);
} else {
effective_name = name;
}Miao, BinJust use `base::ReplaceChars`? It does nothing if there is no match.
```suggestion
std::string effective_name(name);
base::ReplaceChars(effective_name, kNullCharacter, kUnderscore, &effective_name);
```
Done
Related details
- Hu, Ningxin
- Rafael Cintron
- Xu, Mingming1
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Hu, Ningxin (Gerrit)
Hu, Ningxin voted and added 2 comments![]( "Open in Gerrit")
Votes added by Hu, Ningxin
| Code-Review | +1 |
2 comments
Input and output operand names containing null characters ('\0') causeIt is due to names starting with null character.
```suggestion
Input and output operand names starting with null character ('\0') cause
```
Related details
- Miao, Bin
- Rafael Cintron
- Xu, Mingming1
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Miao, Bin (Gerrit)
Miao, Bin added 1 comment![]( "Open in Gerrit")
Input and output operand names containing null characters ('\0') causeIt is due to names starting with null character.
```suggestion
Input and output operand names starting with null character ('\0') cause
```
- Rafael Cintron
- Xu, Mingming1
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Rafael Cintron (Gerrit)
Rafael Cintron added 1 comment![]( "Open in Gerrit")
[1] https://github.com/microsoft/onnxruntime/blob/7b5a93ef5f71ca58a1b6e4ae81b250e767756c68/onnxruntime/core/session/model_editor_c_api.cc#L29The ORT code you linked to has a comment which states that it does not allow empty or non-existent strings. However, you code replaces all null characters in non-empty strings with an underscore. This isn't exactly the same thing.
Does ORT allow null characters in the middle of a string or at the beginning of a non-empty strings? If it accidentally disallows a null character at the beginning of a non-empty string, might be best to replace only the first character with an underscore. Otherwise, we might be introducing name collisions with existing strings in a model.
Related details
- Miao, Bin
- Xu, Mingming1
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Miao, Bin (Gerrit)
Miao, Bin added 1 comment![]( "Open in Gerrit")
The ORT code you linked to has a comment which states that it does not allow empty or non-existent strings. However, you code replaces all null characters in non-empty strings with an underscore. This isn't exactly the same thing.
Does ORT allow null characters in the middle of a string or at the beginning of a non-empty strings? If it accidentally disallows a null character at the beginning of a non-empty string, might be best to replace only the first character with an underscore. Otherwise, we might be introducing name collisions with existing strings in a model.
Good point! I think name collisions are prevented by the joined unique operand id. `GetOperandName()` always appends the id, so:
```
'input\0a' (id=1) → 'input_a_1'
'input_a' (id=2) → 'input_a_2'
'input\0b' (id=3) → 'input_b_3'
```
All the effective name will be unique.
In fact, replacing all the "\0" characters was also for another reason. ORT's C API truncates at any '\0' and this happens before we append the operand id. Without full replacement, 'input\0a' and 'input\0b' would both become 'input' and it will cause `ORT status error code: 1 error message: Error: Duplicate definition-site for (input)`
Added a new WPT case with two inputs ('input\0a' and 'input\0b') to validate this scenario.
Similarly, this situation might occur when generating node names, because it uses the label provided by the user. I am designing a new WPT test case to verify this hypothesis. If it proves correct, I plan to fix this issue in the following CL. WDYT?
Related details
- Hu, Ningxin
- Rafael Cintron
- Xu, Mingming1
Hu, Ningxin (Gerrit)
Hu, Ningxin voted and added 2 comments![]( "Open in Gerrit")
Votes added by Hu, Ningxin
| Code-Review | +1 |
LGTM
Miao, BinThe ORT code you linked to has a comment which states that it does not allow empty or non-existent strings. However, you code replaces all null characters in non-empty strings with an underscore. This isn't exactly the same thing.
Does ORT allow null characters in the middle of a string or at the beginning of a non-empty strings? If it accidentally disallows a null character at the beginning of a non-empty string, might be best to replace only the first character with an underscore. Otherwise, we might be introducing name collisions with existing strings in a model.
Good point! I think name collisions are prevented by the joined unique operand id. `GetOperandName()` always appends the id, so:
```
'input\0a' (id=1) → 'input_a_1'
'input_a' (id=2) → 'input_a_2'
'input\0b' (id=3) → 'input_b_3'
```
All the effective name will be unique.In fact, replacing all the "\0" characters was also for another reason. ORT's C API truncates at any '\0' and this happens before we append the operand id. Without full replacement, 'input\0a' and 'input\0b' would both become 'input' and it will cause `ORT status error code: 1 error message: Error: Duplicate definition-site for (input)`
Added a new WPT case with two inputs ('input\0a' and 'input\0b') to validate this scenario.
Similarly, this situation might occur when generating node names, because it uses the label provided by the user. I am designing a new WPT test case to verify this hypothesis. If it proves correct, I plan to fix this issue in the following CL. WDYT?
I think name collisions are prevented by the joined unique operand id.
+1, the operand id is guaranteed to be unique by mojo.
Similarly, this situation might occur when generating node names, because it uses the label provided by the user.
- Miao, Bin
- Rafael Cintron
- Xu, Mingming1
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Rafael Cintron (Gerrit)
Rafael Cintron added 1 comment![]( "Open in Gerrit")
Miao, BinThe ORT code you linked to has a comment which states that it does not allow empty or non-existent strings. However, you code replaces all null characters in non-empty strings with an underscore. This isn't exactly the same thing.
Does ORT allow null characters in the middle of a string or at the beginning of a non-empty strings? If it accidentally disallows a null character at the beginning of a non-empty string, might be best to replace only the first character with an underscore. Otherwise, we might be introducing name collisions with existing strings in a model.
Hu, NingxinGood point! I think name collisions are prevented by the joined unique operand id. `GetOperandName()` always appends the id, so:
```
'input\0a' (id=1) → 'input_a_1'
'input_a' (id=2) → 'input_a_2'
'input\0b' (id=3) → 'input_b_3'
```
All the effective name will be unique.In fact, replacing all the "\0" characters was also for another reason. ORT's C API truncates at any '\0' and this happens before we append the operand id. Without full replacement, 'input\0a' and 'input\0b' would both become 'input' and it will cause `ORT status error code: 1 error message: Error: Duplicate definition-site for (input)`
Added a new WPT case with two inputs ('input\0a' and 'input\0b') to validate this scenario.
Similarly, this situation might occur when generating node names, because it uses the label provided by the user. I am designing a new WPT test case to verify this hypothesis. If it proves correct, I plan to fix this issue in the following CL. WDYT?
I think name collisions are prevented by the joined unique operand id.
+1, the operand id is guaranteed to be unique by mojo.
Similarly, this situation might occur when generating node names, because it uses the label provided by the user.
Having a separate issue makes sense to me.
OK. Can you please update the CL description and comment to reflect what's really happening?
Related details
- Miao, Bin
- Xu, Mingming1
Miao, Bin (Gerrit)
Miao, Bin added 1 comment![]( "Open in Gerrit")
Miao, BinThe ORT code you linked to has a comment which states that it does not allow empty or non-existent strings. However, you code replaces all null characters in non-empty strings with an underscore. This isn't exactly the same thing.
Does ORT allow null characters in the middle of a string or at the beginning of a non-empty strings? If it accidentally disallows a null character at the beginning of a non-empty string, might be best to replace only the first character with an underscore. Otherwise, we might be introducing name collisions with existing strings in a model.
Hu, NingxinGood point! I think name collisions are prevented by the joined unique operand id. `GetOperandName()` always appends the id, so:
```
'input\0a' (id=1) → 'input_a_1'
'input_a' (id=2) → 'input_a_2'
'input\0b' (id=3) → 'input_b_3'
```
All the effective name will be unique.In fact, replacing all the "\0" characters was also for another reason. ORT's C API truncates at any '\0' and this happens before we append the operand id. Without full replacement, 'input\0a' and 'input\0b' would both become 'input' and it will cause `ORT status error code: 1 error message: Error: Duplicate definition-site for (input)`
Added a new WPT case with two inputs ('input\0a' and 'input\0b') to validate this scenario.
Similarly, this situation might occur when generating node names, because it uses the label provided by the user. I am designing a new WPT test case to verify this hypothesis. If it proves correct, I plan to fix this issue in the following CL. WDYT?
Rafael CintronI think name collisions are prevented by the joined unique operand id.
+1, the operand id is guaranteed to be unique by mojo.
Similarly, this situation might occur when generating node names, because it uses the label provided by the user.
Having a separate issue makes sense to me.
OK. Can you please update the CL description and comment to reflect what's really happening?
- Rafael Cintron
- Xu, Mingming1
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Dwayne Robinson (Gerrit)
Dwayne Robinson added 1 comment![]( "Open in Gerrit")
- Miao, Bin
- Rafael Cintron
- Xu, Mingming1
Rafael Cintron (Gerrit)
Rafael Cintron voted Code-Review+1![]( "Open in Gerrit")
- Miao, Bin
- Xu, Mingming1
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Blink W3C Test Autoroller (Gerrit)
Message from Blink W3C Test Autoroller![]( "Open in Gerrit")
Exportable changes to web-platform-tests were detected in this CL and a pull request in the upstream repo has been made: https://github.com/web-platform-tests/wpt/pull/57024.
When this CL lands, the bot will automatically merge the PR on GitHub if the required GitHub checks pass; otherwise, ecosystem-infra@ team will triage the failures and may contact you.
WPT Export docs:
https://chromium.googlesource.com/chromium/src/+/main/docs/testing/web_platform_tests.md#Automatic-export-process
Related details
- Miao, Bin
- Xu, Mingming1
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Miao, Bin (Gerrit)
Miao, Bin voted Commit-Queue+2![]( "Open in Gerrit")
- Xu, Mingming1
Chromium LUCI CQ (Gerrit)
Chromium LUCI CQ submitted the change![]( "Open in Gerrit")
Change information
WebNN: Replace null characters in operand names for ORT backend
Input and output operand names starting with null characters ('\0')
cause ORT's CreateValueInfo API to fail with "name cannot be null or
empty string" error [1], leading to context lost.
Furthermore, ORT's C API truncates at any '\0' and this happens before
we append the operand id. For example, both 'input\0a_1' and
'input\0b_2' would become 'input', which will lead to “Duplicate
definition-site for (input)” error.
This CL replaces all '\0' characters with '_' in operand names to ensure
compatibility with ORT's C API (name collisions are prevented by the
joined unique operand id).
This CL also adds WPT tests to cover null characters in different
positions.
[1] https://github.com/microsoft/onnxruntime/blob/7b5a93ef5f71ca58a1b6e4ae81b250e767756c68/onnxruntime/core/session/model_editor_c_api.cc#L29
- M services/webnn/ort/graph_builder_ort.cc
- M third_party/blink/web_tests/external/wpt/webnn/conformance_tests/inputs-with-special-names.https.any.js
Code-Review: +1 by Hu, Ningxin, +1 by Rafael Cintron
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Blink W3C Test Autoroller (Gerrit)
Message from Blink W3C Test Autoroller![]( "Open in Gerrit")
The WPT PR for this CL has been merged upstream! https://github.com/web-platform-tests/wpt/pull/57024
Related details
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |