[wasm] Emit webcil payloadSize/tableSize into boot config for streaming instantiation - #9
Conversation
…ng instantiation Source PR: dotnet#131658 Source head: 8a97f41
⛔ Shipwright · BlockedRecommendation: do not merge PR #9 · Tier
Findings (20)
Fireworks usage: 37,370 input · 2,802 output · 40,172 total tokens · $0.0101 · 29s · 0 fix iteration(s) Open the Shipwright check for full evidence and the audit bundle. Use |
![shipwright-agent[bot]](https://avatars.githubusercontent.com/u/140162738?s=60&v=4){kind=small}
| public void NonR2R_ReadsPayloadSize_WithZeroTableSize() | ||
| { | ||
| byte[] wasm = BuildWebcilInWasm(payloadSize: 0x1234, tableSize: null); | ||
|
|
There was a problem hiding this comment.
Shipwright · CRITICAL
The new test file references WebcilReader.TryReadWebcilInWasmSizes, but the diff does not add or modify that method in the WebcilReader.cs shown.
Impact: The new test file references WebcilReader.TryReadWebcilInWasmSizes, but the diff does not add or modify that method in the WebcilReader.cs shown. If the method does not already exist in the base branch, the test project will fail to compile, breaking CI. The diff only adds WebcilSizesModuleReader and the public TryReadWebcilInWasmSizes overloads are present in the shown WebcilReader.cs hunk, but the review evidence…
Suggested fix: Review the cited evidence, fix the risk if confirmed, and rerun Shipwright.
| int r = stream.Read(buffer, read, count - read); | ||
| if (r == 0) | ||
| return false; | ||
| read += r; |
There was a problem hiding this comment.
Shipwright · CRITICAL
WebcilSizesModuleReader.VisitSection sets shouldStop = true for the Data section, but if the base WasmModuleReader.Visit() does not honor shouldStop or the module contains multiple
Impact: WebcilSizesModuleReader.VisitSection sets shouldStop = true for the Data section, but if the base WasmModuleReader.Visit() does not honor shouldStop or the module contains multiple Data sections, later sections can overwrite HasSizes/PayloadSize/TableSize or fail after the first Data section. This can produce incorrect payload/table sizes in the boot config, causing runtime allocation/table-growth failures for valid…
Suggested fix: Review the cited evidence, fix the risk if confirmed, and rerun Shipwright.
| if (isWebcilInWasmAssembly) | ||
| { | ||
| if (!int.TryParse(resource.GetMetadata("PayloadSize"), NumberStyles.Integer, CultureInfo.InvariantCulture, out int ps) || ps <= 0) | ||
| { |
There was a problem hiding this comment.
Shipwright · CRITICAL
GenerateWasmBootJson parses TableSize with int.TryParse but ignores the result.
Impact: GenerateWasmBootJson parses TableSize with int.TryParse but ignores the result. If TableSize metadata is missing or malformed, ts defaults to 0 and the R2R image is recorded as non-R2R. The loader then skips table growth and R2R imports, leading to instantiation failure or incorrect indirect-call behavior at runtime.
Suggested fix: Review the cited evidence, fix the risk if confirmed, and rerun Shipwright.
| public void NonR2R_ReadsPayloadSize_WithZeroTableSize() | ||
| { | ||
| byte[] wasm = BuildWebcilInWasm(payloadSize: 0x1234, tableSize: null); | ||
|
|
There was a problem hiding this comment.
Shipwright · HIGH
The new test file WebcilInWasmSizesTests.cs references WebcilReader.TryReadWebcilInWasmSizes, but no implementation of this method is added or modified in the diff.
Impact: The new test file WebcilInWasmSizesTests.cs references WebcilReader.TryReadWebcilInWasmSizes, but no implementation of this method is added or modified in the diff. The tests will not compile unless the method already exists elsewhere, and the PR title indicates the reader is part of this change. This is a concrete build/test failure introduced by the diff.
Suggested fix: Fix the review finding before release.
| int r = stream.Read(buffer, read, count - read); | ||
| if (r == 0) | ||
| return false; | ||
| read += r; |
There was a problem hiding this comment.
Shipwright · HIGH
The WebcilSizesModuleReader.VisitSection method sets shouldStop = true for the Data section, but the base WasmModuleReader.Visit() may not stop after the first Data section.
Impact: The WebcilSizesModuleReader.VisitSection method sets shouldStop = true for the Data section, but the base WasmModuleReader.Visit() may not stop after the first Data section. If the module contains multiple Data sections (allowed by the wasm spec), the reader will continue visiting subsequent sections and may overwrite HasSizes/PayloadSize/TableSize or fail on a later Data section, producing incorrect sizes. The code…
Suggested fix: Fix the review finding before release.
| if (isWebcilInWasmAssembly) | ||
| { | ||
| if (!int.TryParse(resource.GetMetadata("PayloadSize"), NumberStyles.Integer, CultureInfo.InvariantCulture, out int ps) || ps <= 0) | ||
| { |
There was a problem hiding this comment.
Shipwright · HIGH
The code uses int.TryParse for TableSize without checking the result.
Impact: The code uses int.TryParse for TableSize without checking the result. If TableSize metadata is missing or invalid, ts defaults to 0, which is then stored in webcilSizes. This may cause the runtime to treat an R2R image as non-R2R, leading to incorrect table growth and potential runtime failure.
Suggested fix: Fix the review finding before release.
| // cultures, so qualify by culture to avoid collisions. It matches how | ||
| // BootJsonBuilderHelper resolves webcilSizes per (culture subfolder, route). | ||
| string webcilCulture = string.Equals("Culture", assetTraitName, StringComparison.OrdinalIgnoreCase) ? assetTraitValue : null; | ||
| string r2rSizeStoreKey = webcilCulture != null ? webcilCulture + "/" + resourceRoute : resourceRoute; |
There was a problem hiding this comment.
Shipwright · HIGH
The webcilSizes dictionary key format is documented as matching BootJsonBuilderHelper, but the helper's lookup uses subFolder + '/' + a.Key while GenerateWasmBootJson builds webcil
Impact: The webcilSizes dictionary key format is documented as matching BootJsonBuilderHelper, but the helper's lookup uses subFolder + '/' + a.Key while GenerateWasmBootJson builds webcilCulture + '/' + resourceRoute. The diff does not demonstrate that resourceRoute and a.Key are always identical, so a mismatch would silently omit payloadSize/tableSize from boot JSON and break the loader at runtime.
Suggested fix: Review the cited evidence, fix the risk if confirmed, and rerun Shipwright.
| // so the runtime loader can instantiate without parsing the wasm. payloadSize is | ||
| // emitted for every webcil; tableSize only for R2R. Identify them by the produced | ||
| // ".wasm" extension, excluding native wasm (dotnet.native.wasm) which is handled | ||
| // separately and is not a webcil module. The AttachWebcilSizes task has already |
There was a problem hiding this comment.
Shipwright · HIGH
The isWebcilInWasmAssembly check treats every .wasm asset that is not the native runtime wasm as webcil-in-wasm and requires PayloadSize metadata.
Impact: The isWebcilInWasmAssembly check treats every .wasm asset that is not the native runtime wasm as webcil-in-wasm and requires PayloadSize metadata. If any non-webcil .wasm asset reaches this path, the build logs an error and skips the resource, potentially breaking valid builds. The check should positively identify webcil-in-wasm assets rather than exclude only dotnet.native.wasm.
Suggested fix: Review the cited evidence, fix the risk if confirmed, and rerun Shipwright.
| } | ||
|
|
||
| protected override bool VisitSection(Section sec, out bool shouldStop) | ||
| { |
There was a problem hiding this comment.
Shipwright · HIGH
The data segment 0 reader accepts any segment length >= 4 and reads 8 bytes when length >= 8, without validating that the segment contains exactly the expected 4-byte payload size
Impact: The data segment 0 reader accepts any segment length >= 4 and reads 8 bytes when length >= 8, without validating that the segment contains exactly the expected 4-byte payload size or 8-byte payload+table size header. A malformed or unexpected segment layout can produce incorrect sizes that are then emitted into boot JSON.
Suggested fix: Review the cited evidence, fix the risk if confirmed, and rerun Shipwright.
| failureReason = ex.Message; | ||
| return false; | ||
| } | ||
| } |
There was a problem hiding this comment.
Shipwright · HIGH
ReadUInt32LE shifts byte values without first casting to uint.
Impact: ReadUInt32LE shifts byte values without first casting to uint. In C#, byte promotes to int, so a byte with the high bit set can sign-extend during the shift and produce an incorrect uint value for payloadSize or tableSize.
Suggested fix: Review the cited evidence, fix the risk if confirmed, and rerun Shipwright.
| failureReason = null; | ||
| try | ||
| { | ||
| using var reader = new WebcilSizesModuleReader(stream); |
There was a problem hiding this comment.
Shipwright · HIGH
TryReadWebcilInWasmSizes catches all exceptions and converts them into a false result with the exception message.
Impact: TryReadWebcilInWasmSizes catches all exceptions and converts them into a false result with the exception message. This can mask IO failures, OutOfMemoryException, or other critical errors as 'not a webcil-in-wasm image', allowing the build to proceed without required size metadata and deferring failure to runtime. The catch-all also risks leaking filesystem paths or internal exception details into build logs.
Suggested fix: Review the cited evidence, fix the risk if confirmed, and rerun Shipwright.
| } | ||
|
|
||
| protected override bool VisitSection(Section sec, out bool shouldStop) | ||
| { |
There was a problem hiding this comment.
Shipwright · MEDIUM
The TryReadWebcilInWasmSizes method reads the payload size from data segment 0, but the comment says 'The webcil payload lives in a later data segment, so only the small size heade
Impact: The TryReadWebcilInWasmSizes method reads the payload size from data segment 0, but the comment says 'The webcil payload lives in a later data segment, so only the small size header is read.' If the payload size is stored in data segment 0, the code reads only 4 or 8 bytes from the segment, but the segment may contain additional data. The code does not validate that the segment length is exactly 4 or 8 bytes, so it…
Suggested fix: Fix the review finding before release.
| // cultures, so qualify by culture to avoid collisions. It matches how | ||
| // BootJsonBuilderHelper resolves webcilSizes per (culture subfolder, route). | ||
| string webcilCulture = string.Equals("Culture", assetTraitName, StringComparison.OrdinalIgnoreCase) ? assetTraitValue : null; | ||
| string r2rSizeStoreKey = webcilCulture != null ? webcilCulture + "/" + resourceRoute : resourceRoute; |
There was a problem hiding this comment.
Shipwright · MEDIUM
The webcilSizes dictionary is keyed by r2rSizeStoreKey, which is either resourceRoute or webcilCulture + '/' + resourceRoute.
Impact: The webcilSizes dictionary is keyed by r2rSizeStoreKey, which is either resourceRoute or webcilCulture + '/' + resourceRoute. However, the comment says 'It matches how BootJsonBuilderHelper resolves webcilSizes per (culture subfolder, route).' If the BootJsonBuilderHelper uses a different key format (e.g., culture subfolder without a slash, or a different separator), the lookup will fail, and the sizes will not be e…
Suggested fix: Fix the review finding before release.
| // so the runtime loader can instantiate without parsing the wasm. payloadSize is | ||
| // emitted for every webcil; tableSize only for R2R. Identify them by the produced | ||
| // ".wasm" extension, excluding native wasm (dotnet.native.wasm) which is handled | ||
| // separately and is not a webcil module. The AttachWebcilSizes task has already |
There was a problem hiding this comment.
Shipwright · MEDIUM
The isWebcilInWasmAssembly check uses IsTargeting110OrLater() and fileExtension == '.wasm', but it does not verify that the asset is actually a webcil-in-wasm assembly.
Impact: The isWebcilInWasmAssembly check uses IsTargeting110OrLater() and fileExtension == '.wasm', but it does not verify that the asset is actually a webcil-in-wasm assembly. It only excludes native wasm. If there are other .wasm assets that are not webcil-in-wasm, the code will attempt to read PayloadSize metadata and log an error if missing, potentially failing the build for valid assets.
Suggested fix: Fix the review finding before release.
|
|
||
| private sealed class WebcilSizesModuleReader : WasmModuleReader | ||
| { | ||
| internal bool HasSizes { get; private set; } |
There was a problem hiding this comment.
Shipwright · MEDIUM
The WebcilSizesModuleReader.VisitSection method reads the data segment 0 using TryReadPassiveDataSegment, but it does not check if the segment is actually passive.
Impact: The WebcilSizesModuleReader.VisitSection method reads the data segment 0 using TryReadPassiveDataSegment, but it does not check if the segment is actually passive. The method name suggests it checks for passive segments, but the code does not verify the segment's mode. If the segment is active, the start offset may be incorrect, leading to reading wrong data.
Suggested fix: Fix the review finding before release.
| failureReason = ex.Message; | ||
| return false; | ||
| } | ||
| } |
There was a problem hiding this comment.
Shipwright · MEDIUM
The ReadUInt32LE method uses bitwise OR and shifts on bytes, but the bytes are signed in C#.
Impact: The ReadUInt32LE method uses bitwise OR and shifts on bytes, but the bytes are signed in C#. If the high bit of a byte is set, the shift operation may produce incorrect results due to sign extension. The code should cast to uint before shifting.
Suggested fix: Fix the review finding before release.
| byte[] notWasm = { 0x7f, 0x45, 0x4c, 0x46, 0x00, 0x00, 0x00, 0x00 }; | ||
|
|
||
| using var stream = new MemoryStream(notWasm); | ||
| bool ok = WebcilReader.TryReadWebcilInWasmSizes(stream, out _, out _, out string? failureReason); |
There was a problem hiding this comment.
Shipwright · LOW
The test comment says the segment is 'smaller than the 4-byte payload size' but the body writes only 2 bytes.
Impact: The test comment says the segment is 'smaller than the 4-byte payload size' but the body writes only 2 bytes. The test behavior is correct, but the comment is misleading about the exact size used.
Suggested fix: Review the cited evidence, fix the risk if confirmed, and rerun Shipwright.
| byte[] notWasm = { 0x7f, 0x45, 0x4c, 0x46, 0x00, 0x00, 0x00, 0x00 }; | ||
|
|
||
| using var stream = new MemoryStream(notWasm); | ||
| bool ok = WebcilReader.TryReadWebcilInWasmSizes(stream, out _, out _, out string? failureReason); |
There was a problem hiding this comment.
Shipwright · LOW
The test DataSegmentTooSmall_Fails constructs a data section with a passive segment of length 2, but the reader is expected to read a 4-byte payload size.
Impact: The test DataSegmentTooSmall_Fails constructs a data section with a passive segment of length 2, but the reader is expected to read a 4-byte payload size. The test asserts failure, which is correct, but the comment says 'smaller than the 4-byte payload size' while the body writes only 2 bytes. This is a documentation mismatch in the test comment, not a functional failure.
Suggested fix: Fix the review finding before release.
| public string cache { get; set; } | ||
| } | ||
|
|
||
| [DataContract] |
There was a problem hiding this comment.
Shipwright · LOW
The new WebcilAsset class inherits from GeneralAsset and adds nullable int?
Impact: The new WebcilAsset class inherits from GeneralAsset and adds nullable int? tableSize and payloadSize. However, the internal.ts AssetEntryInternal adds tableSize?: number and payloadSize?: number. The C# DataMember EmitDefaultValue=false will omit null values, but the TypeScript interface does not mark them as nullable, which is consistent with optional properties. No concrete defect is demonstrated.
Suggested fix: Fix the review finding before release.
| failureReason = null; | ||
| try | ||
| { | ||
| using var reader = new WebcilSizesModuleReader(stream); |
There was a problem hiding this comment.
Shipwright · LOW
The TryReadWebcilInWasmSizes method catches all exceptions and returns false with the exception message.
Impact: The TryReadWebcilInWasmSizes method catches all exceptions and returns false with the exception message. This may hide critical errors (e.g., OutOfMemoryException, IOException) and treat them as 'not a webcil-in-wasm image', causing the build to proceed without the required sizes and potentially fail at runtime.
Suggested fix: Fix the review finding before release.
1 participant
Split from dotnet#129634
Summary
The WASM loader previously instantiated each webcil-in-wasm assembly by buffering its bytes and calling the module's
getWebcilSize/getWebcilPayloadexports to discover the payload size at runtime. This records the payload size — and, for ReadyToRun images, the indirect-call table size — in the boot config so the loader can stream-instantiate the module directly.Changes
BootJsonData): a newWebcilAsset : GeneralAssettype carries optionalpayloadSize(emitted for every webcil-in-wasm assembly) andtableSize(R2R images only). The assembly / coreAssembly / lazyAssembly / satellite asset lists use this type; both fields useEmitDefaultValue = false, so non-webcil assets are unchanged. Only emitted fornet11.0+.WebcilReader.TryReadWebcilInWasmSizesreads payloadSize (+ tableSize for R2R) from data segment 0 of the produced webcil, reusing the existingWasmModuleReader. Covered by unit tests (WebcilInWasmSizesTests).AttachWebcilSizestask reads the sizes from the already-produced webcil files and attaches them asPayloadSize/TableSizemetadata on the resource items. New_AttachBuildWebcilSizes/_AttachPublishWebcilSizestargets run on every boot-config generation (cheap: only the small size header is read), so the metadata is present even on incremental builds where the convert task was skipped for unchanged assemblies.GenerateWasmBootJsonconsumes the metadata directly and errors if it is missing — there is no file-reading fallback andConvertDllsToWebcilno longer computes sizes.host/assets.ts,loader/assets.ts):instantiateWebcilModuletakespayloadSize/tableSizeand usesWebAssembly.instantiateStreamingwhen possible, with no byte buffering and no data-section parsing. Instantiation is wrapped so the payload buffer is freed on failure. For R2R images it wires the host ABI handshake (stack pointer, exception tag, indirect-call table + base, image base) and grows the table before instantiation.Notes
payloadSizeis emitted for all webcil-in-wasm assemblies;tableSizeonly appears once ReadyToRun webcil-in-wasm images are produced (separate follow-up work —R2RWebcilPathis consumed here but produced by that work).WebcilAssettype (payloadSize/tableSize) inpublic-api.ts.Note
This pull request description was generated with the assistance of GitHub Copilot.
Source merge-base:
c8f97975ab0d8ff84aebf176829e17f7c7052f03Source head:
8a97f41ac2be07799ae9199d3405d667f3197b64