[wasmfx] Fix continuation signature hash collisions [v8/v8 : main]
Thibaud Michaud (Gerrit)
Thibaud Michaud added 1 comment![]( "Open in Gerrit")
Related details
- Clemens Backes
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Clemens Backes (Gerrit)
Clemens Backes voted and added 6 comments![]( "Open in Gerrit")
Votes added by Clemens Backes
| Code-Review | +1 |
6 comments
Ah, unfortunate that we need a different hash. But makes sense. A few comments to make this all a bit shorter and nicer, otherwise LGTM.
class ContinuationSignatureHasher {Just `using ContinuationSignatureHasher = SignatureHasher;`?
for (size_t i = 0; i < sig->return_count(); ++i) {
hasher.Add(GetMachineRepresentation(sig->GetReturn(i)));
}
for (size_t i = 0; i < sig->parameter_count(); ++i) {
hasher.Add(GetMachineRepresentation(sig->GetParam(i)));
}Just iterate over `sig->all()` (one (range-based) loop instead of two)?
hasher.Add(sig->return_count());
hasher.Add(sig->parameter_count());Shouldn't one of them be enough?
class ContinuationSignatureHasher {This class needs a comment explaining the difference to `SignatureHasher` and why we need that.
CanonicalTypeIndex expected_csig_index =
env_->module->canonical_type_id(imm.cont_type->contfun_typeindex());
const CanonicalSig* expected_csig =
GetTypeCanonicalizer()->LookupFunctionSignature(expected_csig_index);
uint64_t expected_hash = ContinuationSignatureHasher::Hash(expected_csig);Now that we need to compute a new hash anyway it's probably nicer to just use the module-specific type (saves the lookup via the type canonicalizer).
Related details
- Thibaud Michaud
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Thibaud Michaud (Gerrit)
Thibaud Michaud added 5 comments![]( "Open in Gerrit")
Just `using ContinuationSignatureHasher = SignatureHasher;`?
Done
for (size_t i = 0; i < sig->return_count(); ++i) {
hasher.Add(GetMachineRepresentation(sig->GetReturn(i)));
}
for (size_t i = 0; i < sig->parameter_count(); ++i) {
hasher.Add(GetMachineRepresentation(sig->GetParam(i)));
}Just iterate over `sig->all()` (one (range-based) loop instead of two)?
We also use this with the recently introduced `VectorSignature` which stores parameters and returns separately and does not have an `all()` method.
hasher.Add(sig->return_count());
hasher.Add(sig->parameter_count());Shouldn't one of them be enough?
Done
This class needs a comment explaining the difference to `SignatureHasher` and why we need that.
Done
CanonicalTypeIndex expected_csig_index =
env_->module->canonical_type_id(imm.cont_type->contfun_typeindex());
const CanonicalSig* expected_csig =
GetTypeCanonicalizer()->LookupFunctionSignature(expected_csig_index);
uint64_t expected_hash = ContinuationSignatureHasher::Hash(expected_csig);Now that we need to compute a new hash anyway it's probably nicer to just use the module-specific type (saves the lookup via the type canonicalizer).
- Clemens Backes
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Clemens Backes (Gerrit)
Clemens Backes voted and added 2 comments![]( "Open in Gerrit")
Votes added by Clemens Backes
| Code-Review | +0 |
2 comments
Oh, wait, there is a more serious problem with this change:
Even though the old hasher is called `SignatureHasher`, it does not actually compute a hash, but basically a bitfield containing all information. Hence it's collision-free.
Our hashes are not cryptographically safe, so for an attacker it would be simple enough to still cause confusion here.
Adding Jakob as well. This needs more discussion and probably a fundamentally different approach.
What if we compute a "canonical generic signature" where each type is replaced by the most generic compatible type, and then store the canonical signature ID of that type? It needs relocation though. Maybe someone has a better idea.
for (size_t i = 0; i < sig->return_count(); ++i) {
hasher.Add(GetMachineRepresentation(sig->GetReturn(i)));
}
for (size_t i = 0; i < sig->parameter_count(); ++i) {
hasher.Add(GetMachineRepresentation(sig->GetParam(i)));
}Thibaud MichaudJust iterate over `sig->all()` (one (range-based) loop instead of two)?
We also use this with the recently introduced `VectorSignature` which stores parameters and returns separately and does not have an `all()` method.
- Jakob Kummerow
- Thibaud Michaud
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |