Skip to content

Commit

Permalink
c: use proper type coercion in consume
Browse files Browse the repository at this point in the history
On 32bit platforms `size_t` is essentially `uint32_t` (or at times
even meager `uint16_t`). Loading `uint64_t` field value into `size_t` on
these platforms would truncate the high bits and leave only the low 32
(16) bits in place. This leads to various interesting errors in
downstream modules. See:

- nodejs/llhttp#110
- nodejs/undici#803

This patch makes all field loads go into their respective types.
Truncation doesn't happen in this case because C coercion rules will
cast both types to the largest necessary datatype to hold either of
them.
  • Loading branch information
indutny committed May 12, 2021
1 parent 888a0ce commit 0ea76ed
Show file tree
Hide file tree
Showing 2 changed files with 40 additions and 1 deletion.
21 changes: 20 additions & 1 deletion src/implementation/c/node/consume.ts
Original file line number Diff line number Diff line change
Expand Up @@ -8,12 +8,31 @@ export class Consume extends Node<frontend.node.Consume> {
const ctx = this.compilation;

const index = ctx.stateField(this.ref.field);
const ty = ctx.getFieldType(this.ref.field);

let fieldTy: string;
if (ty === 'i64') {
fieldTy = 'uint64_t';
} else if (ty === 'i32') {
fieldTy = 'uint32_t';
} else if (ty === 'i16') {
fieldTy = 'uint16_t';
} else if (ty === 'i8') {
fieldTy = 'uint8_t';
} else {
throw new Error(
`Unsupported type ${ty} of field ${this.ref.field} for consume node`);
}

out.push('size_t avail;');
out.push('size_t need;');
out.push(`${fieldTy} need;`);

out.push('');
out.push(`avail = ${ctx.endPosArg()} - ${ctx.posArg()};`);
out.push(`need = ${index};`);

// Note: `avail` or `need` are going to coerced to the largest
// datatype needed to hold either of the values.
out.push('if (avail >= need) {');
out.push(` p += need;`);
out.push(` ${index} = 0;`);
Expand Down
20 changes: 20 additions & 0 deletions test/consume-test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -46,4 +46,24 @@ describe('llparse/consume', () => {
const binary = await build(p, start, 'consume-i64');
await binary.check('3aaa2bb1a01b', 'off=4\noff=7\noff=9\noff=10\noff=12\n');
});

it('should consume bytes with untruncated i64 field', async () => {
p.property('i64', 'to_consume');

const start = p.node('start');
const consume = p.consume('to_consume');

start
.select(
NUM_SELECT,
p.invoke(p.code.mulAdd('to_consume', { base: 10 }), start)
)
.skipTo(consume);

consume
.otherwise(printOff(p, start));

const binary = await build(p, start, 'consume-untruncated-i64');
await binary.check('4294967297.xxxxxxxx', '\n');
});
});

0 comments on commit 0ea76ed

Please sign in to comment.