From f9c53aa576d00c1ac7a05d8cdbb4a60319f4ea25 Mon Sep 17 00:00:00 2001 From: Lilleman auf Larv Date: Fri, 25 Sep 2026 19:02:07 +0200 Subject: [PATCH] Charge each held segment for its objects, at the measured cost, and file the held-message gap --- CHANGELOG.md | 6 +++--- src/reassembly.ts | 7 ++++--- test/session-extras.test.ts | 28 ++++++++++++++++------------ test/session.test.ts | 4 ++-- todo.md | 8 ++++++++ 5 files changed, 33 insertions(+), 20 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 01f74d3..f5857a8 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -35,9 +35,9 @@ - An `alert_notification` or an `outbind` from the peer is logged and left unanswered, as SMPP 3.4 gives neither a response. Each one used to emit `sessionError`, `"alert_notification" has no response command`. -- `maxOctets` charges every TLV a held segment carries for the memory it keeps, empty ones - included. A peer could hold megabytes per segment beyond the cap by sending thousands of empty - TLVs, which it counted as nothing. +- `maxOctets` charges each held segment, and every TLV it carries, for the memory it keeps beyond + its octets. A peer could hold around 67 times the cap with segments of empty fields, and megabytes + per segment with thousands of empty TLVs, both of which the cap counted as next to nothing. - `server()` refuses a `maxOctets` below 1 or not a whole number, `Infinity` included, like its other limits. `server({ maxOctets: 0 })` used to start and then refuse every multipart message. - `callback_num`, `callback_num_atag`, `callback_num_pres_ind`, `broadcast_area_identifier` and diff --git a/src/reassembly.ts b/src/reassembly.ts index 00d0409..c53d993 100644 --- a/src/reassembly.ts +++ b/src/reassembly.ts @@ -73,8 +73,9 @@ function detach(pduObj: PduObject): PduObject { return { ...pduObj, params, shortMessageOctets: octets, tlvs }; } -// Roughly the heap a TLV or listed occurrence costs beyond its value, so a PDU of empty ones is not free. -const tlvObjectOverhead = 200; +// Measured heap beyond the octets, so a segment of empty fields or empty TLVs is not free. +const segmentObjectOverhead = 1000; +const tlvObjectOverhead = 300; // A cstring param arrives as a string, and source_addr alone can carry most of a 1 MiB PDU. function sizeOf(value: ParamValue): number { @@ -84,7 +85,7 @@ function sizeOf(value: ParamValue): number { } function octetsOf(pduObj: PduObject): number { - let octets = 0; + let octets = segmentObjectOverhead; for (const value of Object.values(pduObj.params)) { octets += sizeOf(value); diff --git a/test/session-extras.test.ts b/test/session-extras.test.ts index 4add97b..d4b4314 100644 --- a/test/session-extras.test.ts +++ b/test/session-extras.test.ts @@ -1805,7 +1805,7 @@ describe('reassembly bounds', () => { assert.deepEqual(lost, [], 'the peer holds the only segment there was, so nothing was lost'); }); - // The two addresses and the TLV's object are 222 octets, so only the 14 it carries can overrun a cap of 230. + // The two addresses and the segment's and TLV's objects are 1322 octets, so only the 14 it carries can overrun 1330. test('counts a body carried in message_payload against the octet cap', () => { function collectPayload(maxOctets: number): Collected { const reassembler = new Reassembler({ @@ -1820,11 +1820,11 @@ describe('reassembly bounds', () => { return collectPdu(reassembler, payloadSegment(9, 1, 2)); } - assert.equal(collectPayload(230).kept, false, 'a TLV body the cap cannot hold is refused, not dropped later'); - assert.equal(collectPayload(240).kept, true); + assert.equal(collectPayload(1330).kept, false, 'a TLV body the cap cannot hold is refused, not dropped later'); + assert.equal(collectPayload(1340).kept, true); }); - test('counts every TLV and every occurrence of a repeatable one against the octet cap, empty ones included', () => { + test('counts the objects a segment and each of its TLVs hold against the octet cap, empty ones included', () => { function collectTlvs(maxOctets: number, tlvs: PduObject['tlvs']): Collected { const reassembler = new Reassembler({ log: silentLog, @@ -1854,7 +1854,11 @@ describe('reassembly bounds', () => { false, 'an empty occurrence still holds an object', ); - assert.equal(collectTlvs(30_000, unknownTags).kept, false, 'an empty tag still holds an object'); + // The segment itself is 1036 octets, so the tags must be charged 300 each to overrun 3,001,000. + assert.equal(collectTlvs(3_001_000, unknownTags).kept, false, 'an empty tag still holds an object'); + assert.equal(collectTlvs(3_002_000, unknownTags).kept, true); + assert.equal(collectTlvs(1_000, {}).kept, false, 'a segment holds objects beyond its 36 octets'); + assert.equal(collectTlvs(1_036, {}).kept, true); }); // The segments before it were answered ESME_ROK, so dropping those is not the same as refusing one. @@ -1863,8 +1867,8 @@ describe('reassembly bounds', () => { const reassembler = new Reassembler({ log: silentLog, max: 10, - // One segment is 36 octets, so the second overruns a group already holding the first. - maxOctets: 50, + // One segment is 1036 octets, so the second overruns a group already holding the first. + maxOctets: 1050, now: () => 0, onLost: one => { lost.push(one); }, timeout: 60_000, @@ -1928,9 +1932,9 @@ describe('reassembly bounds', () => { assert.equal(counted.size, 1, 'the second group evicted the first, as a UDH group would'); counted.clear(); - // A sar_* segment is the two 11-octet addresses, an 8-octet body and three 200-octet TLV objects. - assert.equal(collectSar(capped(620), 3, 1, 2).kept, false); - assert.equal(collectSar(capped(630), 3, 1, 2).kept, true); + // A sar_* segment is the two 11-octet addresses, an 8-octet body, its own object and three TLVs'. + assert.equal(collectSar(capped(1920), 3, 1, 2).kept, false); + assert.equal(collectSar(capped(1930), 3, 1, 2).kept, true); }); // Nothing else says a message the peer has already been answered for was thrown away. @@ -2046,8 +2050,8 @@ describe('reassembly bounds', () => { const reassembler = new Reassembler({ log: silentLog, max: 10, - // One segment is 36 octets: 14 of short_message plus the two 11-octet addresses. - maxOctets: 80, + // One segment is 1036 octets: 14 of short_message, the two 11-octet addresses and its object. + maxOctets: 2100, now: () => 0, onLost: () => undefined, timeout: 60_000, diff --git a/test/session.test.ts b/test/session.test.ts index 5e936ae..9aab5e6 100644 --- a/test/session.test.ts +++ b/test/session.test.ts @@ -910,8 +910,8 @@ describe('receiving', () => { // The refusal a submission gets is the one submit_sm_resp defines, whichever command carried it. test('refuses a data_sm segment a server has no room for with the submit code', async t => { - // The two addresses are 22 octets, so the 6-octet UDH and its text are what overrun 30. - const smpp = await startServer(t, { maxOctets: 30 }); + // The two addresses and the objects are 1322 octets, so the 6-octet UDH and its text are what overrun 1330. + const smpp = await startServer(t, { maxOctets: 1330 }); const { session } = await connect(t, smpp, { bindType: 'transmitter' }); assert.ok(session); diff --git a/todo.md b/todo.md index 843a501..2650bab 100644 --- a/todo.md +++ b/todo.md @@ -6,6 +6,14 @@ hard rules first — they constrain every item below. This is a working file that sets its own rules. The documentation conventions in AGENTS.md do not govern it, and nothing here is a source anything else may cite. +## Security + +- [ ] **Bound the heap `HeldMessages` keeps, not only its count.** It holds up to 1000 messages the + application has not answered for up to 300 s, each as the PDUs it arrived in, undetached and + uncharged: one 200 KB PDU of 50,000 empty unknown TLVs is 15 MB of heap, and a completed + reassembly is up to `maxOctets`. A peer faster than an application answering asynchronously + holds gigabytes per session. From the stability review of #27. + ## Status The rewrite is **feature complete and green**: the suite, lint and typecheck are clean on Node 18