From 8ef268c7385bac990636eb3be46413f22f1a38dc Mon Sep 17 00:00:00 2001 From: lcallarec Date: Tue, 17 Nov 2020 09:08:05 +0100 Subject: [PATCH 1/4] Fix integer overflows --- assembly/Histogram.ts | 6 ++++-- package-lock.json | 2 +- src/Histogram.spec.ts | 27 +++++++++++++++++++++++++++ src/TypedArrayHistogram.spec.ts | 23 +++++++++++++++++++++++ src/TypedArrayHistogram.ts | 7 +++++-- 5 files changed, 60 insertions(+), 5 deletions(-) diff --git a/assembly/Histogram.ts b/assembly/Histogram.ts index 5375de8..689b385 100644 --- a/assembly/Histogram.ts +++ b/assembly/Histogram.ts @@ -589,9 +589,11 @@ export default class Histogram extends AbstractHistogramBase { // @ts-ignore const currentCount = unchecked(this.counts[index]); const newCount = currentCount + 1; - if (newCount < 0) { + if (newCount < currentCount) { + const bitSize = (sizeof() * 8); + const overflowAt = (currentCount + 1); throw new Error( - newCount.toString() + " would overflow short integer count" + overflowAt.toString() + " would overflow " + bitSize.toString() + "bits integer count" ); } // @ts-ignore diff --git a/package-lock.json b/package-lock.json index 69d2f50..e386a81 100644 --- a/package-lock.json +++ b/package-lock.json @@ -1,6 +1,6 @@ { "name": "hdr-histogram-js", - "version": "2.0.0", + "version": "2.0.1", "lockfileVersion": 1, "requires": true, "dependencies": { diff --git a/src/Histogram.spec.ts b/src/Histogram.spec.ts index 6367f2c..8e60ee9 100644 --- a/src/Histogram.spec.ts +++ b/src/Histogram.spec.ts @@ -557,3 +557,30 @@ describe("Histogram clearing support", () => { expect(histogram.mean).toBe(histogram2.mean); }); }); + +describe("WASM Histogram bucket size overflow", () => { + let wasmHistogram: Histogram; + beforeAll(initWebAssembly); + afterEach(() => wasmHistogram.destroy()); + + [8 as const, 16 as const].forEach( + (bitBucketSize) => { + it("should fail when recording more than 'bitBucketSize' times the same value", () => { + //given + wasmHistogram = build({ useWebAssembly: true, bitBucketSize }); + const overflowAt = (2**bitBucketSize) - 1; + + //when //then + try { + let i = 0; + for (i; i <= overflowAt; i++) { + wasmHistogram.recordValue(1); + } + fail(`should have failed due to ${bitBucketSize}bits integer overflow (bucket size : ${i})`); + } catch (e) { + //ok + } + }); + }) +}); + diff --git a/src/TypedArrayHistogram.spec.ts b/src/TypedArrayHistogram.spec.ts index 99493df..c68d674 100644 --- a/src/TypedArrayHistogram.spec.ts +++ b/src/TypedArrayHistogram.spec.ts @@ -67,3 +67,26 @@ import Float64Histogram from "./Float64Histogram"; }); } ); + + +describe("Histogram bucket size overflow", () => { + [Int8Histogram, Int16Histogram].forEach( + (Histogram) => { + it("should fail when recording more than 'maxBucketSize' times the same value", () => { + //given + const histogram = new Histogram(1, Number.MAX_SAFE_INTEGER, 3); + + //when //then + try { + let i = 0; + for (i; i <= histogram.maxBucketSize; i++) { + histogram.recordValue(1); + } + fail(`should have failed due to ${histogram._counts.BYTES_PER_ELEMENT * 8}bits integer overflow (bucket size: ${i})`); + } catch (e) { + //ok + expect(histogram.getCountAtIndex(1)).toBe(histogram.maxBucketSize); + } + }); + }) +}); diff --git a/src/TypedArrayHistogram.ts b/src/TypedArrayHistogram.ts index abf6f34..96ec0fc 100644 --- a/src/TypedArrayHistogram.ts +++ b/src/TypedArrayHistogram.ts @@ -18,6 +18,8 @@ class TypedArrayHistogram extends JsHistogram { _counts: TypedArray; _totalCount: number; + maxBucketSize: number; + constructor( private arrayCtr: new (size: number) => TypedArray, lowestDiscernibleValue: number, @@ -31,6 +33,7 @@ class TypedArrayHistogram extends JsHistogram { ); this._totalCount = 0; this._counts = new arrayCtr(this.countsArrayLength); + this.maxBucketSize = 2**(this._counts.BYTES_PER_ELEMENT * 8) - 1; } clearCounts() { @@ -40,8 +43,8 @@ class TypedArrayHistogram extends JsHistogram { incrementCountAtIndex(index: number) { const currentCount = this._counts[index]; const newCount = currentCount + 1; - if (newCount < 0) { - throw newCount + " would overflow short integer count"; + if (newCount > this.maxBucketSize) { + throw newCount + " would overflow " + this._counts.BYTES_PER_ELEMENT * 8 + "bits integer count"; } this._counts[index] = newCount; } From ba607cef016170d9a2a177fdfbcf1c3ab67049de Mon Sep 17 00:00:00 2001 From: lcallarec Date: Wed, 18 Nov 2020 13:37:19 +0100 Subject: [PATCH 2/4] fix integer overflows when adding & decoding histo --- assembly/Histogram.ts | 12 ++++++-- assembly/__tests__/Histogram.spec.ts | 16 +++++------ src/Histogram.spec.ts | 42 ++++++++++++++++++++++++---- src/TypedArrayHistogram.spec.ts | 42 ++++++++++++++++++++++++---- src/TypedArrayHistogram.ts | 11 +++----- 5 files changed, 96 insertions(+), 27 deletions(-) diff --git a/assembly/Histogram.ts b/assembly/Histogram.ts index 689b385..e0cd3e1 100644 --- a/assembly/Histogram.ts +++ b/assembly/Histogram.ts @@ -49,6 +49,8 @@ export default class Histogram extends AbstractHistogramBase { counts: T; totalCount: u64 = 0; + maxBucketSize: number; + constructor( lowestDiscernibleValue: u64, highestTrackableValue: u64, @@ -120,6 +122,8 @@ export default class Histogram extends AbstractHistogramBase { // @ts-ignore this.counts = instantiate(this.countsArrayLength); + this.maxBucketSize = 2 ** (sizeof() * 8) - 1; + this.leadingZeroCountBase = 64 - this.unitMagnitude - this.subBucketHalfCountMagnitude - 1; this.percentileIterator = new PercentileIterator(this, 1); @@ -609,8 +613,12 @@ export default class Histogram extends AbstractHistogramBase { // @ts-ignore const currentCount = unchecked(this.counts[index]); const newCount = currentCount + value; - if (newCount < 0) { - throw newCount + " would overflow short integer count"; + const u64NewCount = (currentCount + value) as number; + if (u64NewCount > this.maxBucketSize) { + const bitSize = (sizeof() * 8); + throw new Error( + u64NewCount.toString() + " would overflow " + bitSize.toString() + "bits integer count" + ); } // @ts-ignore unchecked((this.counts[index] = newCount)); diff --git a/assembly/__tests__/Histogram.spec.ts b/assembly/__tests__/Histogram.spec.ts index fdf960f..6127f82 100644 --- a/assembly/__tests__/Histogram.spec.ts +++ b/assembly/__tests__/Histogram.spec.ts @@ -317,18 +317,18 @@ describe("Histogram add & subtract", () => { // given const histogram = buildHistogram(); const histogram2 = buildHistogram(); - histogram.recordCountAtValue(2, 100); - histogram2.recordCountAtValue(1, 100); - histogram.recordCountAtValue(2, 200); - histogram2.recordCountAtValue(1, 200); - histogram.recordCountAtValue(2, 300); - histogram2.recordCountAtValue(1, 300); + histogram.recordCountAtValue(2, 10); + histogram2.recordCountAtValue(1, 10); + histogram.recordCountAtValue(2, 20); + histogram2.recordCountAtValue(1, 20); + histogram.recordCountAtValue(2, 30); + histogram2.recordCountAtValue(1, 30); const outputBefore = histogram.outputPercentileDistribution(); // when histogram.add, u8>(histogram2); - histogram.subtract, u8>(histogram2); + //histogram.subtract, u8>(histogram2); // then - expect(histogram.outputPercentileDistribution()).toBe(outputBefore); + //expect(histogram.outputPercentileDistribution()).toBe(outputBefore); }); it("should be equal when another histogram of lower precision is added then subtracted", () => { diff --git a/src/Histogram.spec.ts b/src/Histogram.spec.ts index 8e60ee9..a6079cc 100644 --- a/src/Histogram.spec.ts +++ b/src/Histogram.spec.ts @@ -3,8 +3,8 @@ import JsHistogram from "./JsHistogram"; import { NO_TAG } from "./Histogram"; import Int32Histogram from "./Int32Histogram"; import { initWebAssembly, WasmHistogram, initWebAssemblySync } from "./wasm"; -import Int8Histogram from "./Int8Histogram"; import Histogram from "./Histogram"; +import { decodeFromCompressedBase64 } from './encoding'; class HistogramForTests extends JsHistogram { //constructor() {} @@ -565,15 +565,15 @@ describe("WASM Histogram bucket size overflow", () => { [8 as const, 16 as const].forEach( (bitBucketSize) => { - it("should fail when recording more than 'bitBucketSize' times the same value", () => { + const maxBucketSize = (2 ** bitBucketSize) - 1; + it(`should fail when recording more than ${maxBucketSize} times the same value`, () => { //given wasmHistogram = build({ useWebAssembly: true, bitBucketSize }); - const overflowAt = (2**bitBucketSize) - 1; //when //then try { let i = 0; - for (i; i <= overflowAt; i++) { + for (i; i <= maxBucketSize; i++) { wasmHistogram.recordValue(1); } fail(`should have failed due to ${bitBucketSize}bits integer overflow (bucket size : ${i})`); @@ -581,6 +581,38 @@ describe("WASM Histogram bucket size overflow", () => { //ok } }); - }) + + //Cannot use a destroyed histogram error + it.skip(`should fail when adding two histograms when the same bucket count addition is greater than ${bitBucketSize}bits max integer value`, () => { + //given + const histogram1 = build({ useWebAssembly: true, bitBucketSize }); + histogram1.recordValueWithCount(1, maxBucketSize); + const histogram2 = build({ useWebAssembly: true, bitBucketSize }); + histogram2.recordValueWithCount(1, maxBucketSize); + + //when //then + try { + histogram2.add(histogram1); + fail(`should have failed due to ${bitBucketSize}bits integer overflow`); + } catch (e) { + //ok + } + }); + }); + //Cannot use a destroyed histogram error + it.skip("should fail when decoding an Int32 histogram with one bucket count greater than 16bits", () => { + //given + const int32Histogram = build({ useWebAssembly: true, bitBucketSize: 32 }) as WasmHistogram; + int32Histogram.recordValueWithCount(1, 2**32 - 1); + + //when //then + try { + const encodedInt32Histogram = int32Histogram.encodeIntoCompressedBase64(); + decodeFromCompressedBase64(encodedInt32Histogram, 16, true); + fail(`should have failed due to 16bits integer overflow`); + } catch (e) { + //ok + } + }); }); diff --git a/src/TypedArrayHistogram.spec.ts b/src/TypedArrayHistogram.spec.ts index c68d674..a4e852e 100644 --- a/src/TypedArrayHistogram.spec.ts +++ b/src/TypedArrayHistogram.spec.ts @@ -2,6 +2,7 @@ import Int8Histogram from "./Int8Histogram"; import Int16Histogram from "./Int16Histogram"; import Int32Histogram from "./Int32Histogram"; import Float64Histogram from "./Float64Histogram"; +import { decodeFromCompressedBase64 } from "./encoding"; [Int8Histogram, Int16Histogram, Int32Histogram, Float64Histogram].forEach( (Histogram) => { @@ -72,8 +73,10 @@ import Float64Histogram from "./Float64Histogram"; describe("Histogram bucket size overflow", () => { [Int8Histogram, Int16Histogram].forEach( (Histogram) => { - it("should fail when recording more than 'maxBucketSize' times the same value", () => { - //given + const maxBucketSize = (new Histogram(1, Number.MAX_SAFE_INTEGER, 3)).maxBucketSize; + const bitBucketSize = (new Histogram(1, Number.MAX_SAFE_INTEGER, 3))._counts.BYTES_PER_ELEMENT * 8; + it(`should fail when recording more than ${maxBucketSize} times the same value for a ${bitBucketSize}bits histogram`, () => { + //given; const histogram = new Histogram(1, Number.MAX_SAFE_INTEGER, 3); //when //then @@ -82,11 +85,40 @@ describe("Histogram bucket size overflow", () => { for (i; i <= histogram.maxBucketSize; i++) { histogram.recordValue(1); } - fail(`should have failed due to ${histogram._counts.BYTES_PER_ELEMENT * 8}bits integer overflow (bucket size: ${i})`); + fail(`should have failed due to ${bitBucketSize}bits integer overflow (bucket size: ${i})`); } catch (e) { //ok - expect(histogram.getCountAtIndex(1)).toBe(histogram.maxBucketSize); + expect(histogram.getCountAtIndex(1)).toBe(maxBucketSize); } }); - }) + it(`should fail when adding two histograms when the same bucket count addition is greater than ${bitBucketSize}bits max integer value`, () => { + //given + const histogram1 = new Histogram(1, Number.MAX_SAFE_INTEGER, 3); + histogram1.recordValueWithCount(1, maxBucketSize); + const histogram2 = new Histogram(1, Number.MAX_SAFE_INTEGER, 3); + histogram2.recordValueWithCount(1, maxBucketSize); + + //when //then + try { + histogram1.add(histogram2); + fail(`should have failed due to ${bitBucketSize}bits integer overflow`); + } catch (e) { + //ok + } + }); + }); + it("should fail when decoding an Int32 histogram with one bucket couunt greater than 16bits", () => { + //given + const int32Histogram = new Int32Histogram(1, Number.MAX_SAFE_INTEGER, 3); + int32Histogram.recordValueWithCount(1, 2**32 - 1); + + //when //then + try { + const encodedInt32Histogram = int32Histogram.encodeIntoCompressedBase64(); + decodeFromCompressedBase64(encodedInt32Histogram, 16); + fail(`should have failed due to bits integer overflow`); + } catch (e) { + //ok + } + }); }); diff --git a/src/TypedArrayHistogram.ts b/src/TypedArrayHistogram.ts index 96ec0fc..695edcb 100644 --- a/src/TypedArrayHistogram.ts +++ b/src/TypedArrayHistogram.ts @@ -52,18 +52,15 @@ class TypedArrayHistogram extends JsHistogram { addToCountAtIndex(index: number, value: number) { const currentCount = this._counts[index]; const newCount = currentCount + value; - if ( - newCount < Number.MIN_SAFE_INTEGER || - newCount > Number.MAX_SAFE_INTEGER - ) { - throw newCount + " would overflow integer count"; + if (newCount > this.maxBucketSize) { + throw newCount + " would overflow " + this._counts.BYTES_PER_ELEMENT * 8 + "bits integer count"; } this._counts[index] = newCount; } setCountAtIndex(index: number, value: number) { - if (value < Number.MIN_SAFE_INTEGER || value > Number.MAX_SAFE_INTEGER) { - throw value + " would overflow integer count"; + if (value > this.maxBucketSize) { + throw value + " would overflow " + this._counts.BYTES_PER_ELEMENT * 8 + "bits integer count"; } this._counts[index] = value; } From cc23b3a9fd663b19c338e007ad591aff190427a7 Mon Sep 17 00:00:00 2001 From: Laurent Callarec Date: Thu, 19 Nov 2020 12:24:20 +0100 Subject: [PATCH 3/4] Improve test readability --- src/TypedArrayHistogram.spec.ts | 18 ++++-------------- 1 file changed, 4 insertions(+), 14 deletions(-) diff --git a/src/TypedArrayHistogram.spec.ts b/src/TypedArrayHistogram.spec.ts index a4e852e..f1e1917 100644 --- a/src/TypedArrayHistogram.spec.ts +++ b/src/TypedArrayHistogram.spec.ts @@ -69,7 +69,6 @@ import { decodeFromCompressedBase64 } from "./encoding"; } ); - describe("Histogram bucket size overflow", () => { [Int8Histogram, Int16Histogram].forEach( (Histogram) => { @@ -99,26 +98,17 @@ describe("Histogram bucket size overflow", () => { histogram2.recordValueWithCount(1, maxBucketSize); //when //then - try { - histogram1.add(histogram2); - fail(`should have failed due to ${bitBucketSize}bits integer overflow`); - } catch (e) { - //ok - } + expect(() => histogram1.add(histogram2)).toThrow(); }); }); it("should fail when decoding an Int32 histogram with one bucket couunt greater than 16bits", () => { //given const int32Histogram = new Int32Histogram(1, Number.MAX_SAFE_INTEGER, 3); int32Histogram.recordValueWithCount(1, 2**32 - 1); + const encodedInt32Histogram = int32Histogram.encodeIntoCompressedBase64(); //when //then - try { - const encodedInt32Histogram = int32Histogram.encodeIntoCompressedBase64(); - decodeFromCompressedBase64(encodedInt32Histogram, 16); - fail(`should have failed due to bits integer overflow`); - } catch (e) { - //ok - } + expect(() => decodeFromCompressedBase64(encodedInt32Histogram, 16)).toThrow(); + }); }); From 74ebc8be423012cf8e8081a95b383264703e85de Mon Sep 17 00:00:00 2001 From: Laurent Callarec Date: Thu, 19 Nov 2020 12:33:30 +0100 Subject: [PATCH 4/4] Fix overflow when decoding WASM histogram --- assembly/Histogram.ts | 7 ++++ assembly/__tests__/Histogram.spec.ts | 4 +-- assembly/encoding.ts | 4 +-- src/Histogram.spec.ts | 52 ++++++++++++---------------- 4 files changed, 33 insertions(+), 34 deletions(-) diff --git a/assembly/Histogram.ts b/assembly/Histogram.ts index e0cd3e1..fbf92f0 100644 --- a/assembly/Histogram.ts +++ b/assembly/Histogram.ts @@ -605,6 +605,13 @@ export default class Histogram extends AbstractHistogramBase { } setCountAtIndex(index: i32, value: u64): void { + // @ts-ignore + if ((value) as number > this.maxBucketSize) { + const bitSize = (sizeof() * 8); + throw new Error( + value.toString() + " would overflow " + bitSize.toString() + "bits integer count" + ); + } // @ts-ignore unchecked((this.counts[index] = value)); } diff --git a/assembly/__tests__/Histogram.spec.ts b/assembly/__tests__/Histogram.spec.ts index 6127f82..31d1a65 100644 --- a/assembly/__tests__/Histogram.spec.ts +++ b/assembly/__tests__/Histogram.spec.ts @@ -326,9 +326,9 @@ describe("Histogram add & subtract", () => { const outputBefore = histogram.outputPercentileDistribution(); // when histogram.add, u8>(histogram2); - //histogram.subtract, u8>(histogram2); + histogram.subtract, u8>(histogram2); // then - //expect(histogram.outputPercentileDistribution()).toBe(outputBefore); + expect(histogram.outputPercentileDistribution()).toBe(outputBefore); }); it("should be equal when another histogram of lower precision is added then subtracted", () => { diff --git a/assembly/encoding.ts b/assembly/encoding.ts index a364516..a88138f 100644 --- a/assembly/encoding.ts +++ b/assembly/encoding.ts @@ -111,9 +111,9 @@ function fillCountsArrayFromSourceBuffer( const endPosition = sourceBuffer.position + lengthInBytes; while (sourceBuffer.position < endPosition) { let zerosCount: i32 = 0; - let count = ZigZagEncoding.decode(sourceBuffer); + let count = ZigZagEncoding.decode(sourceBuffer); if (count < 0) { - zerosCount = -count; + zerosCount = -count; dstIndex += zerosCount; // No need to set zeros in array. Just skip them. } else { self.setCountAtIndex(dstIndex++, count); diff --git a/src/Histogram.spec.ts b/src/Histogram.spec.ts index a6079cc..3797bc9 100644 --- a/src/Histogram.spec.ts +++ b/src/Histogram.spec.ts @@ -559,16 +559,13 @@ describe("Histogram clearing support", () => { }); describe("WASM Histogram bucket size overflow", () => { - let wasmHistogram: Histogram; beforeAll(initWebAssembly); - afterEach(() => wasmHistogram.destroy()); - [8 as const, 16 as const].forEach( (bitBucketSize) => { const maxBucketSize = (2 ** bitBucketSize) - 1; it(`should fail when recording more than ${maxBucketSize} times the same value`, () => { //given - wasmHistogram = build({ useWebAssembly: true, bitBucketSize }); + const wasmHistogram = build({ useWebAssembly: true, bitBucketSize }); //when //then try { @@ -577,42 +574,37 @@ describe("WASM Histogram bucket size overflow", () => { wasmHistogram.recordValue(1); } fail(`should have failed due to ${bitBucketSize}bits integer overflow (bucket size : ${i})`); - } catch (e) { + } catch(e) { //ok + } finally { + wasmHistogram.destroy(); } }); - //Cannot use a destroyed histogram error - it.skip(`should fail when adding two histograms when the same bucket count addition is greater than ${bitBucketSize}bits max integer value`, () => { - //given - const histogram1 = build({ useWebAssembly: true, bitBucketSize }); - histogram1.recordValueWithCount(1, maxBucketSize); - const histogram2 = build({ useWebAssembly: true, bitBucketSize }); - histogram2.recordValueWithCount(1, maxBucketSize); - - //when //then - try { - histogram2.add(histogram1); - fail(`should have failed due to ${bitBucketSize}bits integer overflow`); - } catch (e) { - //ok - } - }); + it(`should fail when adding two histograms when the same bucket count addition is greater than ${bitBucketSize}bits max integer value`, () => { + //given + const histogram1 = build({ useWebAssembly: true, bitBucketSize }); + histogram1.recordValueWithCount(1, maxBucketSize); + const histogram2 = build({ useWebAssembly: true, bitBucketSize }); + histogram2.recordValueWithCount(1, maxBucketSize); + + //when //then + expect(() => histogram2.add(histogram1)).toThrow(); + + histogram1.destroy(); + histogram2.destroy(); + }); }); - //Cannot use a destroyed histogram error - it.skip("should fail when decoding an Int32 histogram with one bucket count greater than 16bits", () => { + + it("should fail when decoding an Int32 histogram with one bucket count greater than 16bits in a smaller histogram", () => { //given const int32Histogram = build({ useWebAssembly: true, bitBucketSize: 32 }) as WasmHistogram; int32Histogram.recordValueWithCount(1, 2**32 - 1); + const encodedInt32Histogram = int32Histogram.encodeIntoCompressedBase64(); //when //then - try { - const encodedInt32Histogram = int32Histogram.encodeIntoCompressedBase64(); - decodeFromCompressedBase64(encodedInt32Histogram, 16, true); - fail(`should have failed due to 16bits integer overflow`); - } catch (e) { - //ok - } + expect(() => decodeFromCompressedBase64(encodedInt32Histogram, 16, true)).toThrow(); + int32Histogram.destroy(); }); });