Use checked casts in ConvertBasicEnum#4568
Conversation
The signed/unsigned casts were unchecked, so a slicec bug that emitted an out-of-range enumerator value would silently wrap to a valid-looking but wrong C# enum value. Wrap each cast in checked so the failure becomes a loud OverflowException at code-gen time instead of a runtime encode/decode mismatch in user code. The Int64 branch needs special handling for long.MinValue, whose absolute value (2^63) doesn't fit in long.
There was a problem hiding this comment.
Pull request overview
Updates ConvertBasicEnum in ZeroC.Slice.Symbols to prevent silent wrap/truncation when converting Slice compiler enum enumerator values into C# integral values, addressing audit finding #4501.
Changes:
- Use
checkedcasts for Int8/16/32 and UInt8/16/32 enum underlying types to throw on out-of-range values instead of wrapping. - Add explicit handling for
long.MinValuewhen converting signed 64-bit enumerators (abs = 2^63 with negative flag) to avoid rejecting a legitimate value.
| BuiltinKind.Int8 => CreateBasicEnum( | ||
| raw, | ||
| module, | ||
| builtin, | ||
| (abs, isNegative) => isNegative ? (sbyte)-(long)abs : (sbyte)abs), | ||
| (abs, isNegative) => checked((sbyte)(isNegative ? -(long)abs : (long)abs))), | ||
| BuiltinKind.UInt8 => CreateBasicEnum( | ||
| raw, | ||
| module, | ||
| builtin, | ||
| (abs, _) => (byte)abs), | ||
| (abs, _) => checked((byte)abs)), |
There was a problem hiding this comment.
These conversions now throw OverflowException on out-of-range enumerator values, but the exception will currently bubble out of Generator.RunAsync without any enum/enumerator context. Consider catching OverflowException around the enum-value conversion (ideally where the enumerator identifier is available) and rethrowing an InvalidOperationException with a clear message (e.g., enum name, enumerator name, underlying type, and the raw absolute/negative values).
There was a problem hiding this comment.
This can only happen if slicec handles us an invalid value, failing is enough.
NeedsHashSetValidation computed contiguity as count - 1 < max - min using INumber<T> arithmetic, which wraps unchecked. For sparse enums whose declared min/max span (or near-span) the underlying type's full range, max - min overflowed to a negative value, the test returned false, and the generator emitted the range-check branch. The resulting "value is >= min and <= max" then accepted every underlying-type value as valid — silently letting invalid wire data become valid enum values. Promote the subtraction to BigInteger so the contiguity test stays correct regardless of underlying-type size or the declared bounds.
InsertCreativityHere
left a comment
InsertCreativityHere
left a comment
There was a problem hiding this comment.
Looks good to me!
But also, since all these Slice-Compiler Definitions are new and tweakable, and now that we have an implementation to compare against, might be worth looking back into the idea of:
enum EnumeratorValue {
Signed(v: int64)
Unsigned(v: uint64)
}
And to see if this makes the conversion logic simpler than the:
struct Enumerator {
entityInfo: EntityInfo
absoluteValue: uint64
hasNegativeValue: bool
}
Not clear to me if the extra type would actually make the conversion simpler or not.
| private ISymbol ConvertBasicEnum(Compiler.BasicEnum raw, Module module) | ||
| { | ||
| Builtin builtin = _builtins[raw.Underlying]; | ||
| return builtin.Kind switch |
There was a problem hiding this comment.
Unrelated to this PR, but could we do something like:
var func = builtin.Kind switch {
BuiltinKind.Int8 => ...
BuiltinKind.UInt8 => ...
};
return CreateBasicEnum(raw, module, builtin, func);
Instead of repeating the call to CreateBasicEnum in ever case? Because the only thing that actually changes between each case is the lambda we pass, everything else is the same right?
|
This also fixes #4569 right? |
Fix #4501
Fix #4569