Repository: amzn/ion-js (ts)

Issue:
LST discards duplicate symbols in import, invalidating SIDs
When `ion-js` reads binary Ion data that includes a local symbol table, it checks to see whether each string being added to the table is already present. If it is, [it skips that string](https://github.com/amzn/ion-js/blob/217feef0ca0cd462f6909ead17f386579fdded0a/src/IonLocalSymbolTable.ts#L62-L63). The symbol is not added and the number of entries in the symbol table no longer aligns with the table that was used to encode the data. This behavior is incorrect and causes Errors to be thrown when reading Ion written by libraries other than `ion-js`.

For example, this Ion text:

```js
{
  files: [
    {name: "img1", format: "JPG", bytes: 552},
    {name: "img2", format: "PNG", bytes: 716},
    {name: "img3", format: "GIF", bytes: 1091},
    {name: "notes", format: "TXT", bytes: 480},
  ]
}
```

contains the symbols `files`, `name`, `format`, and `bytes`. 

A writer encoding that data into binary might (unwisely) choose to add `name` to the symbol table again even though [it's already in the system symbol table as SID `$4`](http://amzn.github.io/ion-docs/docs/symbols.html#system-symbols), creating a new SID `$11`.

Ignoring the duplicate symbol means that every starting at SID `$11` will map to incorrect text or to no text at all, leading to corrupted data and exceptions like the following:

```
Error: Symbol $20 greater than maxID.
    at BinaryReader.getSymbolString (src/IonBinaryReader.ts:249:23)
    at BinaryReader.fieldName (src/IonBinaryReader.ts:128:21)
    at _loadStruct (src/dom/index.ts:118:29)
    at _loadValue (src/dom/index.ts:108:38)
    at Object.load (src/dom/index.ts:68:44)
```
LocalSymbolTable treats Object properties as symbols
Follow-on to #645, which centered on `SharedSymbolTable`.

`LocalSymbolTable` [stores its symbols in an `Object`](https://github.com/amzn/ion-js/blob/b3abd8a58a2fc8eb1b2538cdfe767303dbfcc585/src/IonLocalSymbolTable.ts#L27) (not a `Map`). Unfortunately, it instantiates it using an empty `Object` literal:
```typescript
private index: SymbolIndex = {};
```
This creates an `Object` with the `Object` prototype, which includes a variety of properties including `toString`, `toLocaleString`, `valueOf`, etc. When `LocalSymbolTable` to checks to see whether a given string is in the table, it returns false positive for these `Object` properties.

This can be fixed by instead instantiating it with [`Object.create(null)`](https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Object/create), which explicitly specifies a `null` prototype.

Pull request description:
Bugfix for issue 644
*Issue #, if available:* #644, #649 

*Description of changes:*

* Duplicate symbols are no longer discarded during LST initialization (Fixes #644)
* LST's symbol index no longer includes `Object` prototype properties like `toString` (Fixes #649)

These were done in separate commits for an easier review process.

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.


Patch:
diff --git a/src/IonLocalSymbolTable.ts b/src/IonLocalSymbolTable.ts
index 06c3b80..fad89a6 100644
--- a/src/IonLocalSymbolTable.ts
+++ b/src/IonLocalSymbolTable.ts
@@ -24,7 +24,7 @@ import { getSystemSymbolTableImport } from "./IonSystemSymbolTable";
 export class LocalSymbolTable {
   private readonly _import: Import;
   private readonly offset: number;
-  private index: SymbolIndex = {};
+  private index: SymbolIndex = Object.create(null);
 
   constructor(theImport: Import | null, symbols: (string | null)[] = []) {
     if (theImport === null) {
@@ -35,7 +35,7 @@ export class LocalSymbolTable {
     this.offset = this._import.offset + this._import.length;
 
     for (const symbol_ of symbols) {
-      this.addSymbol(symbol_);
+      this.assignSymbolId(symbol_);
     }
   }
 
@@ -72,6 +72,23 @@ export class LocalSymbolTable {
     return symbolId;
   }
 
+  // Used during table initialization. Unlike `addSymbol`, `assignSymbolId` will not discard strings that are already
+  // in the symbol table. Ignoring duplicate symbols during table construction can cause symbol IDs that have already
+  // been used to encode data to become invalid. For example, if a stream uses symbols "foo", "bar", "baz" to encode
+  // its data even though "baz" was also defined in an imported table, discarding "baz" will cause data already encoded
+  // with that ID to become corrupted.
+  private assignSymbolId(symbol: string | null): number {
+    // Push the text onto the end of our array of strings no matter what
+    const symbolId = this.offset + this.symbols.length;
+    this.symbols.push(symbol);
+    // If this text isn't already in our index, go ahead and add it.
+    if (symbol !== null && this.getSymbolId(symbol) === undefined) {
+      this.index[symbol] = symbolId;
+    }
+    // Return the string's index in the symbol table, even if it isn't the lowest one.
+    return symbolId;
+  }
+
   getSymbolText(symbolId: number): string | null {
     if (symbolId > this.maxId) {
       throw new Error(


Test patch:
diff --git a/test/IonLocalSymbolTable.ts b/test/IonLocalSymbolTable.ts
index d59c9d2..c0c0dc9 100644
--- a/test/IonLocalSymbolTable.ts
+++ b/test/IonLocalSymbolTable.ts
@@ -16,7 +16,7 @@
 import {assert} from 'chai';
 import {LocalSymbolTable} from "../src/IonLocalSymbolTable";
 import {Import} from "../src/IonImport";
-import {getSystemSymbolTableImport} from "../src/IonSystemSymbolTable";
+import {getSystemSymbolTable, getSystemSymbolTableImport} from "../src/IonSystemSymbolTable";
 import {Catalog, defaultLocalSymbolTable, SharedSymbolTable} from "../src/Ion";
 
 let defaultCatalog = function () {
@@ -151,5 +151,18 @@ describe('Local symbol table', () => {
         // It is, however, a property (an accessor) on the 'Map' data type.
         // Asking for its symbol ID should return undefined.
         assert.isUndefined(symbolTable.getSymbolId("size"));
+        // Same test for 'toString', which isn't a 'Map' property, but exists on JS Objects instantiated with the
+        // `{}` literal instead of `Object.create(null)`.
+        assert.isUndefined(symbolTable.getSymbolId('toString'));
+    });
+
+    // See https://github.com/amzn/ion-js/issues/649
+    it('Symbol tables do not discard duplicate text during instantiation (Issue #649)', () => {
+        const extraSymbols = ["foo", "foo", "bar", "bar"];
+        const symbolTable = new LocalSymbolTable(getSystemSymbolTableImport(), extraSymbols);
+        assert.equal(
+            symbolTable.maxId,
+            getSystemSymbolTable().numberOfSymbols + extraSymbols.length
+        );
     });
 });