From b88f757a4c856660ca83888201287b250fd0a03b Mon Sep 17 00:00:00 2001 From: Marko Lahma Date: Thu, 27 Aug 2026 18:27:30 +0300 Subject: [PATCH] Interop: an index outside a wrapped collection is refused, not handed to the collection MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Fixes #3422. A plain ObjectWrapper over a bounded CLR collection - a host type with a Count and an `int this[int]`, and none of the three interfaces that would give it an array-like view - took every index-shaped key to the reflected indexer, which hands the index straight to the collection. `x[3]`, `x["3"] = 9`, `x[-1] = 9` and `Reflect.set(x, 3, 9)` were all the CLR's own ArgumentOutOfRangeException out of Evaluate, invisible to a script try/catch and to a host catch (JavaScriptException) alike, while `3 in x` answered true for a position that could not be read at all. here is conditioned on two facts at once, because the same lane serves shapes where an out-of-range key is the point: the target must be array-like (a Count, and not a dictionary, so `d[99] = "x"` on a Dictionary still adds), and the member being resolved must be an integer-keyed indexer (so a string-keyed indexer on a collection still answers for its own key). Such a key now reads undefined, writes as the ordinary [[Set]] refusal, is absent from `in` and hasOwnProperty, and deletes as true. ElementKey/ClassifyElementKey move from ArrayLikeWrapper up to ObjectWrapper unchanged: one definition of "index-shaped key" is the point of #3384, and a second copy would drift. docs/v5-migration.md §4.56 carries the embedder-facing form. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_014W5mbjGhyvgAS4pivXoc4S --- .../HostCollectionIndexBoundsTests.cs | 203 ++++++++++++++++ Jint/Runtime/Interop/AGENTS.md | 1 + .../Interop/ObjectWrapper.Specialized.cs | 90 ------- Jint/Runtime/Interop/ObjectWrapper.cs | 222 +++++++++++++++++- docs/v5-migration.md | 96 ++++++-- 5 files changed, 500 insertions(+), 112 deletions(-) create mode 100644 Jint.Tests.PublicInterface/HostCollectionIndexBoundsTests.cs diff --git a/Jint.Tests.PublicInterface/HostCollectionIndexBoundsTests.cs b/Jint.Tests.PublicInterface/HostCollectionIndexBoundsTests.cs new file mode 100644 index 0000000000..cd40d6cebe --- /dev/null +++ b/Jint.Tests.PublicInterface/HostCollectionIndexBoundsTests.cs @@ -0,0 +1,203 @@ +#nullable enable + +using System.Collections; +using System.Globalization; +using Jint.Native; +using Jint.Runtime; + +namespace Jint.Tests.PublicInterface; + +/// +/// A host collection that has a count and an indexer of its own, and none of the three interfaces that would +/// give it an array-like view. It is wrapped plainly, so every index-shaped key resolves the +/// reflected indexer — which takes whatever index it parsed out of the key straight to the collection. +/// +/// +/// +/// #3384 gave an ArrayLikeWrapper ownership of every index-shaped key, so an out-of-range write is +/// refused rather than handed over. The refusal lives in the array-like view, and this shape does not get one: +/// a plain still went to the indexer and let the collection's +/// own out of Evaluate, where neither a script +/// try/catch nor a host catch (JavaScriptException) can see it (#3422). +/// +/// +/// The guard cannot be the array-like one, because a plain wrapper's indexer lane also serves +/// Dictionary<int, string>, where d[99] = "x" is a legitimate add. Both halves are pinned +/// here. +/// +/// +public class HostCollectionIndexBoundsTests +{ + /// Keys naming a position a three-element window cannot address, in both spellings. + public static IEnumerable UnaddressableIndices => ["3", "'3'", "10", "-1", "'-1'", "'08'", "'+3'"]; + + [TestCaseSource(nameof(UnaddressableIndices))] + public void ReadingAnIndexTheCollectionCannotAddressAnswersUndefined(string key) + { + var engine = CreateEngine(out _); + + engine.Evaluate($"x[{key}]").Should().Be(JsValue.Undefined, + "there is no element at {0}, and a hole reads undefined rather than raising the collection's own exception", key); + } + + [TestCaseSource(nameof(UnaddressableIndices))] + public void WritingAnIndexTheCollectionCannotAddressIsTheOrdinarySetRefusal(string key) + { + var engine = CreateEngine(out var window); + + engine.Execute($"x[{key}] = 9"); + engine.Evaluate($"Reflect.set(x, {key}, 9)").AsBoolean().Should().BeFalse(); + + var thrown = Caught.Exception(() => engine.Execute($"'use strict'; x[{key}] = 9;")); + thrown.Should().BeOfType("a refused [[Set]] in strict mode is a TypeError"); + ((JavaScriptException) thrown!).Error.Get("name").AsString().Should().Be("TypeError"); + + window.Should().Equal(new[] { 1, 2, 3 }, "nothing reached the collection"); + } + + [TestCaseSource(nameof(UnaddressableIndices))] + public void EveryExistenceLaneAgreesThatSuchAnIndexIsAbsent(string key) + { + var engine = CreateEngine(out _); + + engine.Evaluate($"[({key} in x), x.hasOwnProperty({key}), x.propertyIsEnumerable({key}), Object.getOwnPropertyDescriptor(x, {key}) !== undefined].join(',')") + .AsString().Should().Be("false,false,false,false", + "{0} is not a position of the collection, so no lane may report it as one", key); + } + + [TestCaseSource(nameof(UnaddressableIndices))] + public void DeletingSuchAnIndexSucceedsWithoutTouchingTheCollection(string key) + { + var engine = CreateEngine(out var window); + + engine.Evaluate($"delete x[{key}]").AsBoolean().Should().BeTrue("OrdinaryDelete returns true for a property that is not there"); + window.Should().Equal(1, 2, 3); + } + + /// + /// The contrast that says the guard is a bound and not a ban on integer keys: every position the + /// collection does have still reads and writes through its own indexer. + /// + [TestCase("0", 1d)] + [TestCase("'0'", 1d)] + [TestCase("2", 3d)] + [TestCase("'2'", 3d)] + public void APositionTheCollectionHasIsUnaffected(string key, double expected) + { + var engine = CreateEngine(out var window); + + engine.Evaluate($"x[{key}]").AsNumber().Should().Be(expected); + engine.Evaluate($"{key} in x").AsBoolean().Should().BeTrue(); + engine.Evaluate($"x.hasOwnProperty({key})").AsBoolean().Should().BeTrue(); + + engine.Execute($"x[{key}] = 9"); + window[int.Parse(key.Trim('\''), CultureInfo.InvariantCulture)].Should().Be(9); + } + + /// Named CLR members of the same target keep resolving; only index-shaped keys are the view's. + [Test] + public void NamedMembersAreUnaffected() + { + var engine = CreateEngine(out _); + + engine.Evaluate("x.Count").AsNumber().Should().Be(3); + engine.Evaluate("x.length").AsNumber().Should().Be(3); + engine.Evaluate("Array.prototype.join.call(x, '-')").AsString().Should().Be("1-2-3"); + } + + /// + /// The other half of the trade-off. A wrapper's indexer lane also serves a dictionary, where a key outside + /// what it currently holds is a legitimate add and refusing it would be the bug. `IsArrayLike` is what + /// separates the two, and it is `false` for a dictionary. + /// + [Test] + public void AnIntegerKeyedDictionaryStillAdds() + { + var engine = new Engine(options => + { + options.AllowClr(); + options.Interop.AllowWrite = true; + }); + var dictionary = new Dictionary { [1] = "one" }; + engine.SetValue("x", dictionary); + + engine.Execute("x[99] = 'ninety-nine'"); + + dictionary.Should().ContainKey(99).WhoseValue.Should().Be("ninety-nine"); + engine.Evaluate("x[99]").AsString().Should().Be("ninety-nine"); + engine.Evaluate("x[1]").AsString().Should().Be("one"); + } + + /// + /// A string-keyed indexer on an array-like target is not the collection's positions, and the check reads + /// the resolved accessor's index parameter rather than guessing from the key, so it stays out of the way. + /// headers["3"] is the header named 3, whatever the count happens to be. + /// + [Test] + public void AStringKeyedIndexerOnAnArrayLikeTargetStillAnswers() + { + var engine = new Engine(options => + { + options.AllowClr(); + options.Interop.AllowWrite = true; + }); + engine.SetValue("x", new NamedValues { ["3"] = "the third", ["etag"] = "abc" }); + + engine.Evaluate("x['3']").AsString().Should().Be("the third"); + engine.Evaluate("x['etag']").AsString().Should().Be("abc"); + } + + private static Engine CreateEngine(out Window window) + { + window = new Window(1, 2, 3); + var engine = new Engine(options => + { + options.AllowClr(); + options.Interop.AllowWrite = true; + }); + engine.SetValue("x", window); + return engine; + } + + /// + /// Bounded and indexed, and neither an , an nor a + /// non-generic — so no array-like view is built for it and every part of that is + /// load-bearing. is what gives it a Count, and the declared + /// indexer is what the reflected member lane resolves for an index-shaped key. + /// + public sealed class Window : IReadOnlyCollection + { + private readonly List _items; + + public Window(params int[] items) => _items = [.. items]; + + public int this[int index] { get => _items[index]; set => _items[index] = value; } + + public int Count => _items.Count; + + public IEnumerator GetEnumerator() => _items.GetEnumerator(); + + IEnumerator IEnumerable.GetEnumerator() => _items.GetEnumerator(); + } + + /// + /// An array-like target whose indexer is keyed by , the shape a + /// NameValueCollection has. + /// + public sealed class NamedValues : IReadOnlyCollection + { + private readonly Dictionary _values = new(StringComparer.Ordinal); + + public string this[string name] + { + get => _values.TryGetValue(name, out var value) ? value : string.Empty; + set => _values[name] = value; + } + + public int Count => _values.Count; + + public IEnumerator GetEnumerator() => _values.Values.GetEnumerator(); + + IEnumerator IEnumerable.GetEnumerator() => _values.Values.GetEnumerator(); + } +} diff --git a/Jint/Runtime/Interop/AGENTS.md b/Jint/Runtime/Interop/AGENTS.md index a7e19c806d..a11c2e3e34 100644 --- a/Jint/Runtime/Interop/AGENTS.md +++ b/Jint/Runtime/Interop/AGENTS.md @@ -65,6 +65,7 @@ rest of the list is split across the files indexed from the repository-root [`AG - **A wrapper's prototype comes from the engine's *running* realm, so building it is realm-scoped work.** `JsValue.FromObject` reaches `ObjectInstance`'s base constructor, `TypeReference.CreateTypeReference` reaches `TypeReferencePrototype`, `DelegateWrapper` reads it outright — all three take `engine.Realm.Intrinsics`, which is `ExecutionContext.Realm` and therefore whichever realm happens to be current at the call. That is right for `Engine.SetValue`, where the realm being written *is* the current one, and wrong for anything projecting into a second realm: `ShadowRealm.SetValue` converted against the principal realm and installed on the shadow realm's global, so a host object handed to a realm was not `instanceof Object` inside it ([#3325](https://github.com/sebastienros/jint/issues/3325)). Jint has exactly one realm-scoped construction path — push that realm's `ExecutionContext` for the duration (`ShadowRealm.EnterRealm`, and `ShadowRealmImportValue` before it) — and no lighter one; `Engine._realmInConstruction` is not it, being neither nestable nor exception-safe. Three host-facing base constructors deliberately do **not** follow the running realm: `ClrFunction(Engine, …)`, `HostFunction` and `Constructor(Engine, string)` pin `engine._originalIntrinsics`, because a function a host builds against an `Engine` belongs to the realm the surrounding script can reach whatever was running when it was built ([#2893](https://github.com/sebastienros/jint/pull/2893)). The two rules do not conflict — one is about a realm named by the caller, the other about a realm that merely happened to be current — but a member built through those constructors *inside* a wrapper (its `toJSON`, its `Symbol.dispose`) still lands in the principal realm, which realm-scoping the wrapper does not change. - **Array-like is not indexable, and `TypeDescriptor.IsArrayLike` is the weaker of the two.** It means the target has a `Count`, nothing more: `ICollection`, `ICollection` and `IReadOnlyCollection` are count-and-copy contracts with no index in them, so `Queue`, `Stack`, `LinkedList`, `SortedSet` and `HashSet` are all array-like with no element at index 0. Any lane that *reads by index* must gate on `ObjectWrapper.HasIndexedElements` instead — `ArrayOperations.For` gated on array-likeness and a bare `ICollection` and handed `IndexWrappedOperations` a target it then hard-cast to `IList`, which is [#3302](https://github.com/sebastienros/jint/issues/3302): a raw `InvalidCastException` out of `Evaluate` for every `Array.prototype` generic over a `Queue`. Falling through to `ObjectOperations` is not a consolation prize — it asks the *object*, so a host collection that really does have an integer indexer still gets its elements through the reflected accessor, and only a genuinely index-less one reads `undefined` per index, which is what an array-like with no index properties means. The same rule governs `ObjectWrapper.HasOriginalIterator`: a wrapper's `Symbol.iterator` is never the array iterator (it enumerates the CLR target), so the index-reading fast path it enables for array destructuring may only stand in for `GetIterator` where index reads reproduce what enumeration yields — which is exactly `HasIndexedElements`. - **Overload scoring's last rule is the converter's own answer, and it has to be.** `InteropHelper.CalculateMethodParameterScore` rates an argument against a parameter with a dozen structural rules, and everything they did not recognize scored a blanket 100 — "will rarely succeed". `FindBestMatch` discards only a *negative* score, so 100 is a match, and a candidate the argument can never bind to is the *best* one whenever it is the only one. That is survivable where the caller retries — `MethodInfoFunction.TryCall` asks `converter.TryConvert` per candidate and moves on when it declines — and not where the caller takes the first match and stops, which `JintBinaryExpression.TryOperatorOverloading` and `TypeReference`'s constructor selection both do: `'s' + v` selected `op_Addition(T, T)` and died converting the string instead of concatenating, for any host type whose only `+` is `(T, T)` ([#3407](https://github.com/sebastienros/jint/issues/3407)). The last rule now asks the installed `ClrTypeConverter` — the very one `MethodDescriptor.Call` will use — so the score cannot claim a conversion the call then fails to perform, and only what it *confirms* keeps the 100. Three things follow. **What the 100 was protecting is three shapes no structural rule can see**: a JS function to a delegate parameter, an enum parameter given a number outside its defined members, and a conversion operator declared on the **target** type — the operator scan above it reads the *argument's* type only, while `DefaultTypeConverter.TryCastWithOperators` reads both. Each is pinned in `Jint.Tests.PublicInterface/HostOverloadScoringTests.cs`, and deleting the probe fails exactly those three and nothing else in the suite. A parameter type still carrying **open** type parameters (`T`, `Func`) is the one thing the converter cannot be asked — the closed type does not exist until `MethodInfoFunction.ResolveMethod` builds it, and handing an open one over is an `ArgumentException` rather than an answer — so those keep the undecided 100. And the probe *performs* the conversion rather than predicting it, so a user-defined `op_Implicit` on a candidate that is then selected runs twice; the `CanChangeType` rule above it already converts speculatively, so that is this function's established cost rather than a new one, but it is why nothing here may assume a conversion happens once. +- **An index-shaped key on a wrapped bounded collection is the wrapper's to answer, in every lane.** The reflected indexer parses an index out of whatever key it is handed and takes it to the collection, so an out-of-range `x[3] = 9` was the CLR's own `ArgumentOutOfRangeException` out of `Evaluate` — invisible to a script `try`/`catch` and to a host `catch (JavaScriptException)` alike. `ArrayLikeWrapper` owns those keys now ([#3384](https://github.com/sebastienros/jint/issues/3384)), *including* the descriptor lane ([#3423](https://github.com/sebastienros/jint/issues/3423)): `Get`, `Set`, `HasProperty`, `Delete`, `GetOwnProperty`, `ProbeOwnProperty`, `DefineOwnProperty` and both key enumerations all answer from `Length`, and a lane added later that does not is a lane where `in` and `hasOwnProperty` contradict each other — which they may not, `OrdinaryHasProperty` being defined in terms of `[[GetOwnProperty]]`. A **plain** `ObjectWrapper` over a bounded target has no view and reads the target's own count instead ([#3422](https://github.com/sebastienros/jint/issues/3422)); that check is deliberately conditioned on two facts at once, because the same lane serves `Dictionary`, where `d[99] = "x"` is a legitimate add, and a string-keyed indexer on a collection, where `x["3"]` names a key rather than a position. `ObjectWrapper.ClassifyElementKey` is the one definition of "index-shaped key" and lives on the base class for that reason; a second copy is the thing that would drift. ### `[JsAccessible]`: the generated lane, and why it is equivalent rather than merely fast diff --git a/Jint/Runtime/Interop/ObjectWrapper.Specialized.cs b/Jint/Runtime/Interop/ObjectWrapper.Specialized.cs index 8c4eb41e69..cb6c58db1d 100644 --- a/Jint/Runtime/Interop/ObjectWrapper.Specialized.cs +++ b/Jint/Runtime/Interop/ObjectWrapper.Specialized.cs @@ -122,96 +122,6 @@ protected ArrayLikeWrapper( // type descriptor does not recognize as integer-indexed internal sealed override bool HasIndexedElements => true; - /// - /// What a property key names on this view. Every element lane below is keyed on the index - /// rather than on how script spelled it: x[3] and x["3"] are one property key, because - /// ToPropertyKey turns both into the - /// String "3". Answering the two spellings from different lanes is answering one question - /// twice, and the lane that used to serve the string spelling was the reflected indexer, which reaches - /// the collection with whatever index it parses out of the key (#3384). - /// - private enum ElementKey - { - /// Not a position of this view: a member name, a symbol, a fractional number, or any string key of a dictionary-shaped target. - None, - - /// A position the target could address. May be at or past . - Position, - - /// Index-shaped but never a position of this view: negative, non-canonical ("08", "+3"), or past what the target can address. - OutOfBand, - } - - /// - /// The highest position an element lane will address, one below so that - /// growing to index + 1 cannot overflow. - /// - private const int MaxPosition = int.MaxValue - 1; - - private ElementKey ClassifyElementKey(JsValue property, out int index) - { - index = 0; - - if (property is JsNumber number) - { - var value = number._value; - if (!TypeConverter.IsIntegralNumber(value)) - { - return ElementKey.None; - } - - if (value < 0 || value > MaxPosition) - { - return ElementKey.OutOfBand; - } - - index = (int) value; - return ElementKey.Position; - } - - // a symbol names no position, and a dictionary-shaped target (e.g. Newtonsoft's JObject: both - // IDictionary and IList<_>) answers a string key from its own keys rather than by index - if (property is not JsString jsString || _typeDescriptor.IsDictionary) - { - return ElementKey.None; - } - - var member = jsString.ToString(); - var parsed = ArrayInstance.ParseArrayIndex(member); - if (parsed != uint.MaxValue) - { - if (parsed > MaxPosition) - { - return ElementKey.OutOfBand; - } - - index = (int) parsed; - return ElementKey.Position; - } - - // One character rules out every ordinary member name that reaches this view — "length", "Count", - // "push" — which would otherwise pay a whole long.TryParse only to be rejected by it. - if (member.Length == 0 || !IsIntegerShapedStart(member[0])) - { - return ElementKey.None; - } - - // an integer-shaped but non-canonical key ("-1", "08") is not a position of the view either, and - // must not fall through to the reflected indexer, which parses one out of it and reaches the - // collection with an index the collection rejects - return long.TryParse(member, NumberStyles.Integer, CultureInfo.InvariantCulture, out _) - ? ElementKey.OutOfBand - : ElementKey.None; - } - - /// - /// Whether can begin something parses: a digit, - /// a sign, or the leading white space it tolerates. - /// - [MethodImpl(MethodImplOptions.AggressiveInlining)] - private static bool IsIntegerShapedStart(char c) - => (uint) (c - '0') <= 9 || c == '-' || c == '+' || char.IsWhiteSpace(c); - /// /// Whether spells a position this view does not have — index-shaped, and /// either outside [0, Length) or never addressable at all (negative, non-canonical, past what the diff --git a/Jint/Runtime/Interop/ObjectWrapper.cs b/Jint/Runtime/Interop/ObjectWrapper.cs index b32a089a9a..17180dd483 100644 --- a/Jint/Runtime/Interop/ObjectWrapper.cs +++ b/Jint/Runtime/Interop/ObjectWrapper.cs @@ -3,6 +3,7 @@ using System.Diagnostics.CodeAnalysis; using System.Globalization; using System.Reflection; +using System.Runtime.CompilerServices; using Jint.Native; using Jint.Native.Array; using Jint.Native.Iterator; @@ -405,6 +406,196 @@ private static EnumerableSnapshotFactory ResolveEnumerableSnapshotFactory( internal override bool IsIntegerIndexedArray => _typeDescriptor.IsIntegerIndexed; + /// + /// What a property key names on a wrapped indexed collection. Every element lane is keyed on the + /// index rather than on how script spelled it: x[3] and x["3"] are one property + /// key, because ToPropertyKey turns both + /// into the String "3". Answering the two spellings from different lanes is answering one + /// question twice, and the lane that used to serve the string spelling was the reflected indexer, which + /// reaches the collection with whatever index it parses out of the key (#3384). + /// + /// + /// It lives on the base class rather than on because a plain wrapper over + /// a bounded collection asks the same question of the same keys — it simply has no array-like view to + /// answer it (#3422). A second definition of "index-shaped key" is the thing that would drift. + /// + private protected enum ElementKey + { + /// Not a position: a member name, a symbol, a fractional number, or any string key of a dictionary-shaped target. + None, + + /// A position the target could address. May be at or past the collection's current count. + Position, + + /// Index-shaped but never a position: negative, non-canonical ("08", "+3"), or past what the target can address. + OutOfBand, + } + + /// + /// The highest position an element lane will address, one below so that + /// growing to index + 1 cannot overflow. + /// + private protected const int MaxPosition = int.MaxValue - 1; + + private protected ElementKey ClassifyElementKey(JsValue property, out int index) + { + index = 0; + + if (property is JsNumber number) + { + var value = number._value; + if (!TypeConverter.IsIntegralNumber(value)) + { + return ElementKey.None; + } + + if (value < 0 || value > MaxPosition) + { + return ElementKey.OutOfBand; + } + + index = (int) value; + return ElementKey.Position; + } + + // a symbol names no position, and a dictionary-shaped target (e.g. Newtonsoft's JObject: both + // IDictionary and IList<_>) answers a string key from its own keys rather than by index + if (property is not JsString jsString || _typeDescriptor.IsDictionary) + { + return ElementKey.None; + } + + var member = jsString.ToString(); + var parsed = ArrayInstance.ParseArrayIndex(member); + if (parsed != uint.MaxValue) + { + if (parsed > MaxPosition) + { + return ElementKey.OutOfBand; + } + + index = (int) parsed; + return ElementKey.Position; + } + + // One character rules out every ordinary member name that reaches a wrapper — "length", "Count", + // "push" — which would otherwise pay a whole long.TryParse only to be rejected by it. + if (member.Length == 0 || !IsIntegerShapedStart(member[0])) + { + return ElementKey.None; + } + + // an integer-shaped but non-canonical key ("-1", "08") is not a position either, and must not fall + // through to the reflected indexer, which parses one out of it and reaches the collection with an + // index the collection rejects + return long.TryParse(member, NumberStyles.Integer, CultureInfo.InvariantCulture, out _) + ? ElementKey.OutOfBand + : ElementKey.None; + } + + /// + /// Whether can begin something parses: a digit, + /// a sign, or the leading white space it tolerates. + /// + [MethodImpl(MethodImplOptions.AggressiveInlining)] + private static bool IsIntegerShapedStart(char c) + => (uint) (c - '0') <= 9 || c == '-' || c == '+' || char.IsWhiteSpace(c); + + /// + /// Whether spells a position cannot address, resolving + /// the member lane itself. For callers that have already resolved it, see the overload below. + /// + private bool IsUnaddressablePosition(JsValue property) + { + if (!_typeDescriptor.IsArrayLike) + { + return false; + } + + var key = ClassifyElementKey(property, out var index); + return key != ElementKey.None + && IsUnaddressablePosition(key, index, ResolveMemberAccessor(property.ToString(), MemberResolutionRequirement.None)); + } + + /// + /// Whether spells a position cannot address, on a + /// wrapper whose element lane is . + /// + /// + /// Two facts have to hold at once and neither is enough alone. The target must be bounded: + /// means it has a Count and is not a dictionary, and a + /// dictionary is exactly where a key outside what it holds is a legitimate add — d[99] = "x" on a + /// Dictionary<int, string> must keep working. And the member being resolved must be an + /// indexer keyed by an integer, so that a string-keyed indexer on an array-like target — a + /// NameValueCollection asked for x["3"] — goes on answering for its own key. + /// + private bool IsUnaddressablePosition(JsValue property, ReflectionAccessor accessor) + { + if (!_typeDescriptor.IsArrayLike) + { + return false; + } + + var key = ClassifyElementKey(property, out var index); + return key != ElementKey.None && IsUnaddressablePosition(key, index, accessor); + } + + private bool IsUnaddressablePosition(ElementKey key, int index, ReflectionAccessor accessor) + { + if (accessor is not IndexerAccessor indexer + || !IsIntegerIndexParameter(indexer.FirstIndexParameter.ParameterType)) + { + return false; + } + + // a negative, non-canonical or unaddressable index is a position no collection can hold, so it + // needs no count read to be refused + return key == ElementKey.OutOfBand + || (TryGetTargetCount(out var count) && (uint) index >= (uint) count); + } + + /// + /// Whether an indexer parameter of this type addresses a position. Anything else — a + /// , an enum, a host key type — names something that is not one, and is none of the + /// bound check's business. + /// + private static bool IsIntegerIndexParameter(Type parameterType) + => parameterType == typeof(int) + || parameterType == typeof(long) + || parameterType == typeof(short) + || parameterType == typeof(sbyte) + || parameterType == typeof(uint) + || parameterType == typeof(ulong) + || parameterType == typeof(ushort) + || parameterType == typeof(byte); + + /// + /// The number of positions currently has. Only ever asked of an array-like target, + /// which is what says there is a count to read at all; means it could not be + /// read, and the caller then refuses nothing. + /// + private bool TryGetTargetCount(out int count) + { + if (Target is ICollection collection) + { + count = collection.Count; + _engine.CheckAmortizedConstraintsAtHostBoundary(); + return true; + } + + // ICollection and IReadOnlyCollection carry a Count with no non-generic ICollection behind it. + // LengthProperty is the same member the wrapper's own "length" forwarder reads. + if (_typeDescriptor.LengthProperty?.GetValue(Target) is int length) + { + _engine.CheckAmortizedConstraintsAtHostBoundary(); + count = length; + return true; + } + + count = 0; + return false; + } + public override bool Set(JsValue property, JsValue value, JsValue receiver) { // check if we can take shortcuts for empty object, no need to generate properties @@ -429,6 +620,16 @@ public override bool Set(JsValue property, JsValue value, JsValue receiver) // can try utilize fast path var accessor = ResolveMemberAccessor(member, MemberResolutionRequirement.Writable); + if (IsUnaddressablePosition(property, accessor)) + { + // The indexer below takes the index straight to the collection, which is where the + // CLR's own ArgumentOutOfRangeException came from - invisible to a script try/catch and + // to a host catch (JavaScriptException) alike. There is no position to write to, so the + // answer owed is the ordinary [[Set]] refusal: silent outside strict mode, a TypeError + // inside it (#3422). + return false; + } + if (ReferenceEquals(accessor, ConstantValueAccessor.NullAccessor)) { // The writable-filtered resolution answers with the same NullAccessor for "no such @@ -505,6 +706,14 @@ public override bool Set(JsValue property, JsValue value, JsValue receiver) return written; } + // A number key and its string spelling are one property key, and the lane below resolves the same + // reflected indexer by member name - so the bound the string lane above just applied has to be + // applied here too, before SetSlow reaches it (#3422). + if (!property.IsSymbol() && IsUnaddressablePosition(property)) + { + return false; + } + return SetSlow(property, value); } @@ -766,7 +975,10 @@ public override JsValue Get(JsValue property, JsValue receiver) if (protoResult.IsUndefined() && property is JsString && !_typeDescriptor.IsDictionary - && _engine.Options.Interop.ThrowOnUnresolvedMember) + && _engine.Options.Interop.ThrowOnUnresolvedMember + // an index outside a bounded collection is a hole, not a member the host forgot to declare: + // GetOwnProperty refused it rather than failing to resolve it (#3422) + && !IsUnaddressablePosition(property)) { throw TypeResolver.CreateMissingMemberException(_engine, ClrType, property.ToString()); } @@ -1072,6 +1284,14 @@ private PropertyDescriptor GetOwnProperty(JsValue property, MemberResolutionRequ } } + if (IsUnaddressablePosition(property, accessor)) + { + // Reading the indexer at this position is the collection's own ArgumentOutOfRangeException, and + // reporting a descriptor for it is what made `3 in x` and hasOwnProperty answer true for a + // position that cannot be read at all. There is no element here, so there is no property (#3422). + return PropertyDescriptor.Undefined; + } + // A member resolving purely to registered extension methods must not shadow a same-named // Array.prototype native on an indexed array-like view (#2976): return Undefined WITHOUT // caching, so the outer Get falls through to the attached prototype and a later mutation of diff --git a/docs/v5-migration.md b/docs/v5-migration.md index 955a8c8f20..c4e28cfca0 100644 --- a/docs/v5-migration.md +++ b/docs/v5-migration.md @@ -2471,27 +2471,6 @@ else. A host implementing `ICldrProvider` **from scratch** rather than deriving has one more member to write; deriving costs nothing, and [5.2](#52-changing-one-locale-datum-is-one-override) is why that is the shape the interface is documented for. ### 4.48 The blocking promise drain is bounded on the engine's clock, not on the wall clock ([#3406](https://github.com/sebastienros/jint/issues/3406)) - -`Engine.DrainEventLoopUntil` — what `UnwrapIfPromise` blocks in, and what a synchronous `Modules.Import` -waits in — used to arm its deadline from `DateTime.UtcNow`. It now arms it from a monotonic timestamp, read -through `Options.Constraints.TimeProvider` on `net8.0` and later and from `Stopwatch` everywhere else. Three -consequences, none of which changes a signature: - -- **A system-clock step no longer moves a promise budget.** An NTP correction forwards used to cut a - `PromiseTimeout` short and one backwards used to stretch it; neither does now. This is the rule the pump's - own ceiling has always followed (`Engine.Pump.ElapsedSince`), and the drain was the one wait that did not. -- **`Options.Constraints.TimeProvider` now governs `Options.Constraints.PromiseTimeout` as well as - `LimitExecutionTime`.** A host that supplied a clock to make its execution-timeout tests exact will find - the promise budget measured against the same clock — so a *frozen* clock now keeps a blocking unwrap - pending until the host advances it, where before it timed out on real time. If that is not wanted, leave - `TimeProvider` at `TimeProvider.System` (the default) and register `OperationDeadlineConstraint` with its - own clock instead; it takes one through its own constructor. The web-API timers are unaffected — they keep - their own clock in `Options.WebApi.Timers.TimeProvider`, because they are a WHATWG feature rather than an - execution budget. -- **A `TimeProvider` reporting a non-positive `TimestampFrequency` is rejected when the engine is built**, - rather than only when `LimitExecutionTime` happens to be registered. It was always rejected by - `ConstraintClock.Resolve` with a named `ArgumentException`; that clock is now resolved for every engine, so - the diagnostic arrives where the clock was supplied instead of at whichever budget first tried to use it. ### 4.49 `Duration.prototype.round` reckons in the calendar its `relativeTo` carries ([#3450](https://github.com/sebastienros/jint/issues/3450)) `round` read the calendar off a `PlainDate` `relativeTo` and then wrote `"iso8601"` into both calendar @@ -2658,6 +2637,81 @@ Two consequences worth naming: `Object.getOwnPropertyDescriptor` reporting an out-of-range index as present. Nothing changes for in-range indices, for named CLR members, or for a dictionary-shaped target such as Newtonsoft's `JObject`, whose index-shaped string keys are dictionary keys and still answer from its own key set. +### 4.56 An index outside a wrapped collection is refused, not handed to the collection ([#3422](https://github.com/sebastienros/jint/issues/3422)) + +[§4.40](#440-an-index-on-a-host-collection-is-one-property-however-it-is-spelled-3384) gave the array-like +*view* ownership of every index-shaped key. A host collection that has a `Count` and an indexer of its own, +but none of the three interfaces that produce such a view, does not get one — and every index-shaped key on it +resolved the reflected indexer, which takes whatever index it parsed out of the key straight to the +collection: + +```csharp +sealed class Window : IReadOnlyCollection +{ + public int this[int index] { get => _items[index]; set => _items[index] = value; } + public int Count => _items.Count; + // ... +} +``` + +```js +// 4.16.x, three elements, options.Interop.AllowWrite = true +x[3] // ArgumentOutOfRangeException out of Evaluate — a read +x["3"] = 9 // ArgumentOutOfRangeException +x[-1] = 9 // ArgumentOutOfRangeException +Reflect.set(x, 3, 9) // ArgumentOutOfRangeException +3 in x // true, for a position that cannot be read at all +``` + +None of those is a `JavaScriptException`, so neither a script `try`/`catch` nor a host +`catch (JavaScriptException)` could see them. In 5.x the wrapper answers such a key itself: + +```js +// 5.x +x[3] // undefined +x["3"] = 9 // sloppy: silently ignored; strict: TypeError +x[-1] = 9 // same +3 in x // false +x.hasOwnProperty(3) // false +delete x[3] // true — there was nothing to delete +``` + +The refusal is conditioned on **two** facts, because the same lane serves shapes where an out-of-range key is +the point: + +- the target is **bounded** — it has a `Count` and is not a dictionary. `d[99] = "x"` on a + `Dictionary` is a legitimate add and is unchanged, in both spellings; +- the member being resolved is an **integer-keyed** indexer. A string-keyed indexer on a collection — a + `NameValueCollection` asked for `x["3"]` — still answers for its own key, whatever the count is. + +Positions the collection does have are unchanged, and so is every named CLR member. + +**What could break:** a `catch (ArgumentOutOfRangeException)` around `Evaluate` written to absorb an +out-of-range index on such a target no longer fires — the read is `undefined` and the write is refused. Code +reading `n in x` or `x.hasOwnProperty(n)` as "the indexer would accept `n`" gets `false` for out-of-range `n` +instead of `true`. + +`Engine.DrainEventLoopUntil` — what `UnwrapIfPromise` blocks in, and what a synchronous `Modules.Import` +waits in — used to arm its deadline from `DateTime.UtcNow`. It now arms it from a monotonic timestamp, read +through `Options.Constraints.TimeProvider` on `net8.0` and later and from `Stopwatch` everywhere else. Three +consequences, none of which changes a signature: + +- **A system-clock step no longer moves a promise budget.** An NTP correction forwards used to cut a + `PromiseTimeout` short and one backwards used to stretch it; neither does now. This is the rule the pump's + own ceiling has always followed (`Engine.Pump.ElapsedSince`), and the drain was the one wait that did not. +- **`Options.Constraints.TimeProvider` now governs `Options.Constraints.PromiseTimeout` as well as + `LimitExecutionTime`.** A host that supplied a clock to make its execution-timeout tests exact will find + the promise budget measured against the same clock — so a *frozen* clock now keeps a blocking unwrap + pending until the host advances it, where before it timed out on real time. If that is not wanted, leave + `TimeProvider` at `TimeProvider.System` (the default) and register `OperationDeadlineConstraint` with its + own clock instead; it takes one through its own constructor. The web-API timers are unaffected — they keep + their own clock in `Options.WebApi.Timers.TimeProvider`, because they are a WHATWG feature rather than an + execution budget. +- **A `TimeProvider` reporting a non-positive `TimestampFrequency` is rejected when the engine is built**, + rather than only when `LimitExecutionTime` happens to be registered. It was always rejected by + `ConstraintClock.Resolve` with a named `ArgumentException`; that clock is now resolved for every engine, so + the diagnostic arrives where the clock was supplied instead of at whichever budget first tried to use it. + ## 5. New in v5 Everything in the table below is opt-in: nothing in it is installed unless the host asks for it, so