Repository navigation
Issue 80 fix - #83
Open
Neoky wants to merge 1 commit into
Open
Issue 80 fix#83Neoky wants to merge 1 commit into
Neoky wants to merge 1 commit into
Conversation
…casting to long The remainder, bitwise and shift operators reached their integer operands by casting a double straight to long, which is undefined whenever the value does not fit, and they shifted by an unmasked count. UBSan reports float-cast-overflow and invalid-shift across nine sites in do_op. ECMAScript specifies ToInt32/ToUint32 for these operators and a shift count masked to five bits, so this is the conversion the language already requires. Three sites needed more than a substitution: % is a floating point remainder in JS and the cast was of the quotient, << has to go through the unsigned type, and == compared as integers, which made 1.5 === 1.7 true. ToUint32 reads the exponent and mantissa directly rather than calling fmod() and trunc(), because elk links no math library on its targets.
Neoky
force-pushed
the
fix/toint32-operator-coercion
branch
from
October 16, 2026 19:44
1991a64 to
bf6c685
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Hi, I'm Neoky, and I'm a cyber researcher. I've been developing a new tool to check for type safety and it found some issues with elk that I've patched.
do_op: apply the ToInt32/ToUint32 coercion the language specifies
Fixes #80.
do_op()converts operands with(long)before computing. That is undefined in Cwhenever the value is out of range (C11 6.3.1.4) or a shift count is negative or
≥ the operand width (C11 6.5.7), and for
===/!==it is also simply wrong —(long)1.5 == (long)1.7, so1.5 === 1.7is currentlytrue.ECMAScript specifies these operators:
&,|,^,~,<<,>>applyToInt32/ToUint32 (a defined modulo-2^32 conversion), the shift operators mask the
count to its low five bits, and
===compares doubles. This is that coercion, nothardening added on top.
Changes
&|^~<<>>go throughto_int32/to_uint32; shift counts aremasked to 5 bits, and
<<goes through the unsigned type widened to 64 bits sointeger promotion cannot overflow an
int.===/!==compare as doubles, like the relational operators already do.%keeps computinga - b * trunc(a / b); only the undefined(long)cast isreplaced. Operands that would yield a NaN are refused, the way
TOK_DIValreadyrefuses a zero divisor — see the note on Type Confusion: Positive NaN is misinterpreted as T_CODEREF #72 below.
Behavior changes
These are user-visible. Each is the answer the specification gives:
2147483648 | 02147483648-21474836484294967296 | 0429496729601e21 | 0-9.2233720368547758e+18-559939584~2147483648-214748364921474836471 << 312147483648-21474836481 << 32429496729611.5 === 1.7truefalse0.5 === 0truefalse5 % 05ERROR: bad % operandsUndefined behavior is also removed where the observed output does not change —
1 >> (0 - 1)and1 << 66both print the same value before and after, but onlythe second is defined.
No new dependency
elk links no math library on any target, so
ToUint32reads the exponent andmantissa out of the double rather than calling
fmod()/trunc().nmshows nofmod,trunc,poworsqrtin the object. The bit version was checked againstfmod(trunc(d), 2^32)over two million random doubles plus the boundary cases, withno difference.
That is also why
%is not an exact remainder:fmod()would be, andfmod()meanslibm. If you would rather have exact
%and accept-lm, it is a one-line change —happy to switch it.
Testing
make -C test testandmake -C test test++pass. C++ warning count is unchangedat 1 (the pre-existing
-Wdangling-pointeronjs->cstk = &resinjs_eval).uno,vc2017,vc22,mingw.make -C test testfails atunit_test.c:193on master as well. ASan relocates locals to its fake stack, sothe
cssprobe insetlwm()((char *) js->cstk - (char *) &n) returns 0 and thec > 0assertion fires.ASAN_OPTIONS=detect_stack_use_after_return=0restores it(
css0 → 2112). Not caused by this PR; glad to send it as a separate one-liner.Not addressed
The value representation, which is #71 and #72:
is_nan()requires the sign bit tobe clear, so a positive NaN or inf decodes as
T_CODEREFrather than as a number, and1e308 * 10evaluates to an empty object.%refusing NaN-producing operands worksaround that rather than fixing it; a real fix needs a representation that can hold NaN.