Compiled mapper: throw on a value not in the mappings instead of writing it - #176
Conversation
…ing it The compiled write/sizeOf mapper fell through to the raw value when it wasn't in the mappings (`mappings[value] || value`), so an unmapped name reached the underlying numeric type, serialized as NaN -> 0, and went out on the wire as a bogus packet. A packet name that doesn't exist in the current protocol state serialized to a single 0x00 byte with no body — a real packet id the peer then fails to decode. The interpreted mapper already throws here; make the compiled one match. The `|| value` fallback also meant a value legitimately mapped to 0 only worked by accident (0 is falsy, so the name itself was passed to the numeric type and serialized as NaN -> 0).
|
Please run benchmark before/after |
|
Benchmarks, Node v24.19.0, Apple Silicon.
That suite has very few mapper values in it, and my box was under enough background load that run-to-run variance exceeded the delta, so I also A/B'd the mapper codegen directly: both the old and the new PR is ~3-5% faster on the compiled write path in every run, which makes sense: the argument to the underlying numeric type is now always a number instead of a number-or-string union. Read is untouched. A/B scriptconst { Compiler: { ProtoDefCompiler } } = require('protodef')
function swapMappings (json) { const r = {}; for (const k in json) r[json[k]] = k; return r }
const master = {
Write: ['parametrizable', (c, m) => c.wrapCode('return ' + c.callType(`${JSON.stringify(swapMappings(m.mappings))}[value] || value`, m.type))],
SizeOf: ['parametrizable', (c, m) => c.wrapCode('return ' + c.callType(`${JSON.stringify(swapMappings(m.mappings))}[value] || value`, m.type))]
}
const pr = {
Write: ['parametrizable', (c, m) => {
let code = `const mapped = ${JSON.stringify(swapMappings(m.mappings))}[value]\n`
code += 'if (mapped === undefined) throw new Error(value + \' is not in the mappings value\')\n'
return c.wrapCode(code + 'return ' + c.callType('mapped', m.type))
}],
SizeOf: ['parametrizable', (c, m) => {
let code = `const mapped = ${JSON.stringify(swapMappings(m.mappings))}[value]\n`
code += 'if (mapped === undefined) throw new Error(value + \' is not in the mappings value\')\n'
return c.wrapCode(code + 'return ' + c.callType('mapped', m.type))
}]
}
const mappings = {}
for (let i = 0; i < 64; i++) mappings[i] = 'name_' + i
const types = {
m8: ['mapper', { type: 'u8', mappings }],
mvar: ['mapper', { type: 'varint', mappings }],
packet: ['container', [{ name: 'kind', type: 'm8' }, { name: 'id', type: 'mvar' }, { name: 'other', type: 'mvar' }]]
}
function build (variant) {
const c = new ProtoDefCompiler()
c.writeCompiler.addTypes({ mapper: variant.Write })
c.sizeOfCompiler.addTypes({ mapper: variant.SizeOf })
c.addTypesToCompile(types)
return c.compileProtoDefSync()
}
const A = build(master); const B = build(pr)
const value = { kind: 'name_5', id: 'name_0', other: 'name_63' }
if (!A.createPacketBuffer('packet', value).equals(B.createPacketBuffer('packet', value))) throw new Error('mismatch')
function cycle (p, ms) {
let n = 0; const end = performance.now() + ms
while (performance.now() < end) { for (let i = 0; i < 1000; i++) p.createPacketBuffer('packet', value); n += 1000 }
return n / ms * 1000
}
for (const p of [A, B]) cycle(p, 300) // warmup
const best = { master: 0, pr: 0 }; const all = { master: [], pr: [] }
for (let r = 0; r < 40; r++) {
const a = cycle(A, 100); const b = cycle(B, 100)
all.master.push(a); all.pr.push(b)
best.master = Math.max(best.master, a); best.pr = Math.max(best.pr, b)
}
const med = xs => xs.slice().sort((x, y) => x - y)[xs.length >> 1]
console.log(`master: best ${Math.round(best.master).toLocaleString()} ops/sec, median ${Math.round(med(all.master)).toLocaleString()} ops/sec`)
console.log(`PR: best ${Math.round(best.pr).toLocaleString()} ops/sec, median ${Math.round(med(all.pr)).toLocaleString()} ops/sec`) |
|
Approving. Has a risk to break nmp mineflayer flying squid so please check and roll back if it breaks |
The compiled write/sizeOf
mapperfalls through to the raw value when it isn't in the mappings (mappings[value] || value), so an unmapped name reaches the underlying numeric type and serializes asNaN→0. The interpreted mapper throws<value> is not in the mappings valueon the same input; this makes the compiled one match.Why it matters: in node-minecraft-protocol, writing a packet name that doesn't exist in the current protocol state (e.g. mineflayer's physics loop sending
positionwhile a Velocity server transfer has the client in the configuration state) serialized to a single0x00byte with no body. In the configuration state that's a real packet id (client_information), so the proxy fails to decode it and kicks with "An internal error occurred in your connection." With this change the write raises a serialization error instead of putting corrupt bytes on the wire.The
|| valuefallback also meant a value legitimately mapped to0only worked by accident:0is falsy, so the name was passed to the numeric type and happened to serialize asNaN→0. Covered by the new test.Read is left as is (it still returns the raw id for an unknown value), since consumers rely on receiving unknown packets rather than a parse error.
Heads-up for consumers: anything that was serializing a mapper from
undefined/an unmapped value and silently getting0now throws. Found two in the login path and fixed them ahead of this:particleStatusin the configuration-phasesettings)settings)