Count and detach a repeatable TLV's occurrences in reassembly, and let only byte or integer tags repeat

src/defs/tlvs.ts

@@ -2,17 +2,14 @@ import type { ParamValue, TlvValue, WireType } from './types.ts';
 import type { Result } from '../result.ts';
 import { tlv } from './types.ts';
 
-export type TlvDefinition = {
-	id: number;
-	multiple?: boolean;
-	tag: string;
-	type: WireType;
-};
+/** Only a tag read as octets or as a number may repeat, since its occurrences are listed as one of those. */
+type Definition<Tag> = { id: number; multiple?: false; tag: Tag; type: WireType<Buffer | number | string> }
+	| { id: number; multiple: true; tag: Tag; type: WireType<Buffer> | WireType<number> };
+
+export type TlvDefinition = Definition<string>;
 
 /** The constraint keys every definition to its own name, so a `tag` that drifts fails to compile. */
-const tlvSpecs = <T extends { [K in keyof T]: { id: number; multiple?: boolean; tag: K; type: WireType } }>(
-	definitions: T,
-): T => definitions;
+const tlvSpecs = <T extends { [K in keyof T]: Definition<K> }>(definitions: T): T => definitions;
 
 // Ordered by tag id, mirroring the SMPP 5.0 TLV table.
 const specs = tlvSpecs({
@@ -98,7 +95,7 @@ for (const definition of Object.values<TlvDefinition>(specs)) {
 }
 
 /** Fallback for tags this table does not know: keep the raw octets. */
-export const tlvDefault: WireType = tlv.buffer;
+export const tlvDefault: WireType<Buffer> = tlv.buffer;
 
 export type Tlv = {
 	tagId: number;
@@ -182,3 +179,68 @@ function writeTlv(tagId: number, type: WireType, value: ParamValue): Result<{ ch
 
 	return written.err ? { err: written.err } : { chunk };
 }
+
+type Occurrence = { definition: TlvDefinition | undefined; tagId: number; value: Buffer | number | string };
+
+function readTlv(pdu: Buffer, offset: number): Result<{ octets: number; occurrence: Occurrence }> {
+	const tagId = pdu.readUInt16BE(offset);
+	const tagLength = pdu.readUInt16BE(offset + 2);
+
+	if (offset + 4 + tagLength > pdu.length) {
+		return { err: new Error(`TLV ${String(tagId)} runs past the end of the PDU`) };
+	}
+
+	const definition = tlvsById[tagId];
+	const read = (definition?.type ?? tlvDefault).read(pdu, offset + 4, tagLength);
+
+	if (read.err) return { err: read.err };
+
+	return { occurrence: { definition, tagId, value: read.value }, octets: 4 + tagLength };
+}
+
+/** Keyed by tag name, a repeatable tag listing every occurrence in wire order and any other keeping its last. */
+function keyedTlvs(occurrences: Occurrence[]): Record<string, Tlv> {
+	const repeated = new Map<string, { tagId: number; values: Occurrence['value'][] }>();
+	const tlvs: Record<string, Tlv> = {};
+
+	for (const { definition, tagId, value } of occurrences) {
+		const key = definition?.tag ?? tagId.toString();
+
+		if (definition?.multiple === true) {
+			const entry = repeated.get(key) ?? { tagId, values: [] };
+
+			entry.values.push(value);
+			repeated.set(key, entry);
+		} else {
+			tlvs[key] = { tagId, tagName: definition?.tag, tagValue: value };
+		}
+	}
+
+	for (const [key, { tagId, values }] of repeated) {
+		const buffers = values.filter(value => Buffer.isBuffer(value));
+
+		tlvs[key] = {
+			tagId,
+			tagName: key,
+			tagValue: buffers.length === values.length ? buffers : values.filter(value => typeof value === 'number'),
+		};
+	}
+
+	return tlvs;
+}
+
+export function parseTlvs(pdu: Buffer, start: number): Result<{ offset: number; tlvs: Record<string, Tlv> }> {
+	const occurrences: Occurrence[] = [];
+	let offset = start;
+
+	while (offset + 4 <= pdu.length) {
+		const read = readTlv(pdu, offset);
+
+		if (read.err) return { err: read.err };
+
+		occurrences.push(read.occurrence);
+		offset += read.octets;
+	}
+
+	return { offset, tlvs: keyedTlvs(occurrences) };
+}

src/defs/types.ts

@@ -17,6 +17,31 @@ export type ParamValue = Buffer | DestAddress[] | UnsuccessSme[] | number | stri
 /** A tag defined `multiple` holds every occurrence, in wire order; any other tag holds one value. */
 export type TlvValue = Buffer | Buffer[] | number | number[] | string;
 
+/** Octets a value holds, counting a string by its length. */
+export function tlvOctets(value: TlvValue): number {
+	if (Buffer.isBuffer(value)) return value.length;
+	if (typeof value === 'string') return value.length;
+	if (typeof value === 'number') return 0;
+
+	let octets = 0;
+
+	for (const one of value) {
+		octets += typeof one === 'number' ? 0 : one.length;
+	}
+
+	return octets;
+}
+
+/** A value holding no view into the PDU it was read from. */
+export function detachedTlv(value: TlvValue): TlvValue {
+	if (Buffer.isBuffer(value)) return Buffer.from(value);
+	if (!Array.isArray(value)) return value;
+
+	const buffers = value.filter(one => Buffer.isBuffer(one));
+
+	return buffers.length === value.length ? buffers.map(one => Buffer.from(one)) : value;
+}
+
 /**
  * One field on the wire. `read` reports how many octets it consumed so callers never have to
  * re-derive a length that could disagree with what was actually written.

src/pdu.ts

@@ -1,6 +1,6 @@
 import type { CommandDefinition, CommandName, PduParams, PduParamsInput } from './defs/commands.ts';
 import type { ErrorName } from './defs/errors.ts';
-import type { ParamValue, TlvValue } from './defs/types.ts';
+import type { ParamValue } from './defs/types.ts';
 import type { PduHeader } from './pdu-refusal.ts';
 import type { Result, VoidResult } from './result.ts';
 import type { Tlv, TlvInput } from './defs/tlvs.ts';
@@ -10,7 +10,7 @@ import { hasUdh } from './defs/constants.ts';
 import { decodeMessage, encodeBody } from './message.ts';
 import { errorNameById, errors, isErrorName } from './defs/errors.ts';
 import { paramNumber, valueText } from './defs/types.ts';
-import { tagIdOf, tlvDefault, tlvs, tlvsById, writeTlvs } from './defs/tlvs.ts';
+import { parseTlvs, tagIdOf, tlvs, writeTlvs } from './defs/tlvs.ts';
 
 /** The highest sequence number this library hands out; SMPP 3.4 4.7.1 reserves 0x7fffffff. */
 export const maxSeqNr = 2147483646;
@@ -262,63 +262,6 @@ export function objToPdu<C extends CommandName>(obj: PduObjectInput<C>): Result<
 	);
 }
 
-type Repeats = { buffers: Map<string, Buffer[]>; numbers: Map<string, number[]> };
-
-/** A repeatable tag's occurrences so far, `value` appended; a value no TLV type reads is undefined. */
-function tlvValue(value: ParamValue, key: string, multiple: boolean | undefined, repeats: Repeats): TlvValue | undefined {
-	if (Buffer.isBuffer(value)) return multiple === true ? appended(repeats.buffers, key, value) : value;
-	if (typeof value === 'number') return multiple === true ? appended(repeats.numbers, key, value) : value;
-	if (typeof value === 'string' && multiple !== true) return value;
-
-	return undefined;
-}
-
-function appended<T>(lists: Map<string, T[]>, key: string, value: T): T[] {
-	const list = lists.get(key) ?? [];
-
-	list.push(value);
-	lists.set(key, list);
-
-	return list;
-}
-
-function readTlv(pdu: Buffer, offset: number, repeats: Repeats): Result<{ key: string; octets: number; tlv: Tlv }> {
-	const tagId = pdu.readUInt16BE(offset);
-	const tagLength = pdu.readUInt16BE(offset + 2);
-
-	if (offset + 4 + tagLength > pdu.length) {
-		return { err: new Error(`TLV ${String(tagId)} runs past the end of the PDU`) };
-	}
-
-	const definition = tlvsById[tagId];
-	const read = (definition?.type ?? tlvDefault).read(pdu, offset + 4, tagLength);
-
-	if (read.err) return { err: read.err };
-
-	const key = definition?.tag ?? tagId.toString();
-	const tagValue = tlvValue(read.value, key, definition?.multiple, repeats);
-
-	if (tagValue === undefined) return { err: new Error(`TLV ${String(tagId)} read as a value no TLV holds`) };
-
-	return { key, octets: 4 + tagLength, tlv: { tagId, tagName: definition?.tag, tagValue } };
-}
-
-function parseTlvs(pdu: Buffer, start: number): Result<{ offset: number; tlvs: Record<string, Tlv> }> {
-	const repeats: Repeats = { buffers: new Map(), numbers: new Map() };
-	const tlvs: Record<string, Tlv> = {};
-	let offset = start;
-
-	while (offset + 4 <= pdu.length) {
-		const read = readTlv(pdu, offset, repeats);
-
-		if (read.err) return { err: read.err };
-
-		tlvs[read.key] = read.tlv;
-		offset += read.octets;
-	}
-
-	return { offset, tlvs };
-}
 
 function readParams(
 	cmdName: CommandName,

src/reassembly.ts

@@ -6,7 +6,7 @@ import type { Tlv } from './defs/tlvs.ts';
 import { ExpiringGroups } from './expiring-groups.ts';
 import { decodeMessage } from './message.ts';
 import { messageOctets } from './message-body.ts';
-import { paramNumber, paramText } from './defs/types.ts';
+import { detachedTlv, paramNumber, paramText, tlvOctets } from './defs/types.ts';
 import { uuidv7 } from './uuid.ts';
 
 /** A concatenated message given up on, whose segments the peer has already been answered for. */
@@ -62,9 +62,7 @@ function detach(pduObj: PduObject): PduObject {
 	}
 
 	for (const [name, tlv] of Object.entries(pduObj.tlvs)) {
-		tlvs[name] = Buffer.isBuffer(tlv.tagValue)
-			? { ...tlv, tagValue: Buffer.from(tlv.tagValue) }
-			: tlv;
+		tlvs[name] = { ...tlv, tagValue: detachedTlv(tlv.tagValue) };
 	}
 
 	// short_message holds the same octets wherever it was not decoded, so one copy covers both.
@@ -76,7 +74,7 @@ function detach(pduObj: PduObject): PduObject {
 }
 
 // A cstring param arrives as a string, and source_addr alone can carry most of a 1 MiB PDU.
-function sizeOf(value: unknown): number {
+function sizeOf(value: ParamValue): number {
 	if (Buffer.isBuffer(value)) return value.length;
 
 	return typeof value === 'string' ? value.length : 0;
@@ -90,7 +88,7 @@ function octetsOf(pduObj: PduObject): number {
 	}
 
 	for (const tlv of Object.values(pduObj.tlvs)) {
-		octets += sizeOf(tlv.tagValue);
+		octets += tlvOctets(tlv.tagValue);
 	}
 
 	return octets;

test/session-extras.test.ts

@@ -1824,6 +1824,27 @@ describe('reassembly bounds', () => {
 		assert.equal(collectPayload(40).kept, true);
 	});
 
+	// One segment is 36 octets before its two 10-octet callback numbers.
+	test('counts every occurrence of a repeatable TLV against the octet cap', () => {
+		function collectCallbacks(maxOctets: number): Collected {
+			const reassembler = new Reassembler({
+				log: silentLog,
+				max: 10,
+				maxOctets,
+				now: () => 0,
+				onLost: () => undefined,
+				timeout: 60_000,
+			});
+			const carried = segment(9, 1, 2);
+			const tagValue = [Buffer.alloc(10, 0x31), Buffer.alloc(10, 0x32)];
+
+			return collectPdu(reassembler, { ...carried, tlvs: { callback_num: { tagId: 0x0381, tagName: 'callback_num', tagValue } } });
+		}
+
+		assert.equal(collectCallbacks(50).kept, false);
+		assert.equal(collectCallbacks(60).kept, true);
+	});
+
 	// The segments before it were answered ESME_ROK, so dropping those is not the same as refusing one.
 	test('reports the answered segments of a group that overruns the cap mid-message', () => {
 		const lost: LostGroup[] = [];

todo.md

@@ -189,6 +189,15 @@ and is also what the panel ranked hardest — two methods, one answer.
 
 ### Correctness, ahead of everything below
 
+- [ ] **Settle what a repeated tag not marked `multiple` reads as, and pin it in a test.** A vendor
+      tag or a known single-value tag a peer sends twice keeps the last occurrence and drops the
+      rest silently, which goal 3 argues against; listing it would change every such tag's shape.
+      From the architecture review of #25.
+
+- [ ] **Refuse a TLV input naming one tag under both its spellings.** `broadcast_area_identifier`
+      and `failed_broadcast_area_identifier` in one `tlvs` record both write, so the peer receives
+      the union of two lists the caller may have meant as one. From the architecture review of #25.
+
 - [ ] **Test that a multipart send which errors never fires `messageDlr`.** Goal 2 now says so and
       README promises it; `session-extras.test.ts` covers a drop *after* the send, not one during it.