diff --git a/CHANGELOG.md b/CHANGELOG.md index d8c15a9..193be3f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -41,8 +41,8 @@ tuned low**: it now holds several times fewer segments, and an incomplete message evicted over the cap is lost, since its segments were already answered. - Unanswered `sms` messages are capped at 64 MiB per session by the `maxOctets` charge, beside the - 1000-message cap: the oldest is dropped with a warning, as over the count. A peer faster than an - application answering asynchronously could hold gigabytes per session. + 1000-message cap: the oldest is dropped with a warning, as over the count. The library used to + hold up to 1000 messages of any size for an application that answered none of them. - `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/session-options.ts b/src/session-options.ts index 3351b92..0e2ad3b 100644 --- a/src/session-options.ts +++ b/src/session-options.ts @@ -128,7 +128,7 @@ export const defaults = { heldMessageTimeout: 300_000, maxDlrMerges: 1000, maxHeldMessages: 1000, - maxHeldOctets: defaultMaxOctets, + maxHeldOctets: 64 * 1024 * 1024, maxOutstanding: 10, maxReassembly: 1000, reassemblyTimeout: 300_000, diff --git a/test/session-extras.test.ts b/test/session-extras.test.ts index 7c8f8ad..f210725 100644 --- a/test/session-extras.test.ts +++ b/test/session-extras.test.ts @@ -29,7 +29,6 @@ import { client } from '../src/client.ts'; import { closeAfter, closeListenerAfter } from './teardown.ts'; import { concatOf } from '../src/concat.ts'; import { consts } from '../src/defs/constants.ts'; -import { detach } from '../src/retained-pdu.ts'; import { errors } from '../src/defs/errors.ts'; import { paramNumber, paramText } from '../src/defs/types.ts'; import { server } from '../src/server.ts'; @@ -1504,7 +1503,8 @@ describe('held message bounds', () => { // submitPdu() holds 1026 octets by the maxOctets charge: its object, and the three text fields. test('drops the message held longest once the octets held pass the cap', () => { - const held = new HeldMessages({ log: silentLog, max: 10, maxOctets: 2100, timeout: 10_000 }); + let now = 0; + const held = new HeldMessages({ log: silentLog, max: 10, maxOctets: 2100, now: () => now, timeout: 10_000 }); const oldest = message(1); held.hold(oldest); @@ -1517,6 +1517,26 @@ describe('held message bounds', () => { assert.equal(held.size, 2); assert.equal(held.has(oldest), false); + // A message that leaves any other way gives its octets back, so two still fit afterwards. + const answered = message(4); + + held.hold(answered); + held.release(answered); + held.hold(message(5)); + assert.equal(held.size, 2, 'after a release'); + + now = 20_000; + held.sweep(); + now = 0; + held.hold(message(6)); + held.hold(message(7)); + assert.equal(held.size, 2, 'after a sweep'); + + held.clear(); + held.hold(message(8)); + held.hold(message(9)); + assert.equal(held.size, 2, 'after a clear'); + held.clear(); }); @@ -1534,13 +1554,30 @@ describe('held message bounds', () => { held.clear(); }); - test('holds a message detached from the chunk it was read from', () => { + test('holds a message detached from the chunk it was read from', async t => { + const session = new Session({ sock: new net.Socket() }); + + closeAfter(t, session); + session.boundAs = 'transceiver'; + + const incoming = new IncomingRequests({ + dlrMerger: new DlrMerger({ log: silentLog, max: 10, timeout: 10_000 }), + log: silentLog, + sendPastDrain: () => Promise.resolve({ err: new Error('never sent') }), + session, + }); const chunk = Buffer.alloc(64 * 1024); const carried = submitPdu(1); - const retained = detach({ ...carried, params: { ...carried.params, short_message: chunk.subarray(16, 20) } }); + let received: Sms | undefined; - assert.ok(Buffer.isBuffer(retained.params.short_message)); - assert.notEqual(retained.params.short_message.buffer, chunk.buffer); + session.on('sms', sms => { received = sms; }); + await incoming.handle({ ...carried, params: { ...carried.params, short_message: chunk.subarray(16, 20) } }); + + const retained = received?.pduObjs[0]?.params.short_message; + + assert.ok(Buffer.isBuffer(retained)); + assert.notEqual(retained.buffer, chunk.buffer); + incoming.clear(); }); test('gives up on a message the application never answers', () => { diff --git a/todo.md b/todo.md index 843a501..1cac818 100644 --- a/todo.md +++ b/todo.md @@ -6,6 +6,17 @@ 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 what a peer can make the application hold, not only the library.** An `sms` the + application is still answering pins its PDUs through `Sms.pduObjs` and its `sendResp`/`sendDlr` + closures, so the held-message caps free nothing while it works, and nothing slows the peer: a + peer faster than an application answering asynchronously (a DB write per message) grows the + heap without bound. Each eviction by the caps also stops the drain waiting for a message still + being answered, so `close()` can cut it off and the peer re-sends it. Flow control is a wire + change under goal 4 and the maintainer's call: answer `ESME_RTHROTTLED`, or stop reading the + socket, once the held count or octets are at the cap. From the stability review of #28. + ## Status The rewrite is **feature complete and green**: the suite, lint and typecheck are clean on Node 18