Repository: amzn/ion-go
Pull request #150 (2020-08-26 19:46:25)

Fix malformed Ion annotations tests
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.



codecov-commenter: # [Codecov](https://codecov.io/gh/amzn/ion-go/pull/150?src=pr&el=h1) Report
> Merging [#150](https://codecov.io/gh/amzn/ion-go/pull/150?src=pr&el=desc) into [master](https://codecov.io/gh/amzn/ion-go/commit/583eb3f07ef283e0d72a4cbbed7e0d59d1283f3a?el=desc) will **increase** coverage by `0.08%`.
> The diff coverage is `90.47%`.

[![Impacted file tree graph](https://codecov.io/gh/amzn/ion-go/pull/150/graphs/tree.svg?width=650&height=150&src=pr&token=OBDT1TNoNE)](https://codecov.io/gh/amzn/ion-go/pull/150?src=pr&el=tree)

```diff
@@            Coverage Diff             @@
##           master     #150      +/-   ##
==========================================
+ Coverage   76.50%   76.59%   +0.08%     
==========================================
  Files          24       24              
  Lines        5180     5216      +36     
==========================================
+ Hits         3963     3995      +32     
- Misses        731      733       +2     
- Partials      486      488       +2     
```


| [Impacted Files](https://codecov.io/gh/amzn/ion-go/pull/150?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [ion/bitstream.go](https://codecov.io/gh/amzn/ion-go/pull/150/diff?src=pr&el=tree#diff-aW9uL2JpdHN0cmVhbS5nbw==) | `82.62% <90.47%> (+0.42%)` | :arrow_up: |

------

[Continue to review full report at Codecov](https://codecov.io/gh/amzn/ion-go/pull/150?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/amzn/ion-go/pull/150?src=pr&el=footer). Last update [583eb3f...fd42ec3](https://codecov.io/gh/amzn/ion-go/pull/150?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).


Patch:
diff --git a/ion/bitstream.go b/ion/bitstream.go
index b389718..3720363 100644
--- a/ion/bitstream.go
+++ b/ion/bitstream.go
@@ -396,26 +396,39 @@ func (b *bitstream) ReadAnnotationIDs() ([]uint64, error) {
 		panic("not an annotation")
 	}
 
-	lengthValue, lengthOfLength, err := b.readVarUintLen(b.len)
+	annotFieldLength, lengthOfAnnotFieldLength, err := b.readVarUintLen(b.len)
 	if err != nil {
 		return nil, err
 	}
 
-	if b.len-lengthOfLength <= lengthValue {
+	if annotFieldLength == 0 {
+		// An annotation with zero length is illegal because at least one annotation must be present.
+		return nil, &SyntaxError{"malformed annotation: at least one annotation must be specified",
+			b.pos - lengthOfAnnotFieldLength}
+	}
+
+	remainingAnnotationLength := b.len - lengthOfAnnotFieldLength - annotFieldLength
+
+	if remainingAnnotationLength <= 0 {
 		// The size of the annotations is larger than the remaining free space inside the
 		// annotation container.
-		return nil, &SyntaxError{"malformed annotation", b.pos - lengthOfLength}
+		return nil, &SyntaxError{"malformed annotation", b.pos - lengthOfAnnotFieldLength}
 	}
 
 	var as []uint64
-	for lengthValue > 0 {
-		id, idlen, err := b.readVarUintLen(lengthValue)
+	for annotFieldLength > 0 {
+		id, idlen, err := b.readVarUintLen(annotFieldLength)
 		if err != nil {
 			return nil, err
 		}
 
 		as = append(as, id)
-		lengthValue -= idlen
+		annotFieldLength -= idlen
+	}
+
+	err = b.validateAnnotatedValue(remainingAnnotationLength)
+	if err != nil {
+		return nil, err
 	}
 
 	b.state = bssBeforeValue
@@ -424,6 +437,68 @@ func (b *bitstream) ReadAnnotationIDs() ([]uint64, error) {
 	return as, nil
 }
 
+func (b *bitstream) validateAnnotatedValue(remainingLength uint64) error {
+	tagByte, err := b.peekAtOffset(0)
+	if err != nil {
+		return err
+	}
+
+	code, length := parseTag(int(tagByte))
+
+	if length == 15 {
+		// Anything with length 15 is null and should only require one byte to represent it.
+		if remainingLength != 1 {
+			return &InvalidTagByteError{tagByte, b.pos}
+		}
+		return nil
+	}
+
+	if code == bitcodeNull {
+		// It is illegal for an annotation to wrap a NOP Pad.
+		return &SyntaxError{"an annotation cannot wrap a NOP Pad", b.pos}
+	} else if code == bitcodeAnnotation {
+		// We cannot have an annotation directly wrapping another annotation.
+		return &SyntaxError{"an annotation cannot be the enclosed value of another annotation", b.pos}
+	}
+
+	// Adjust remainingLength because we just processed the first byte of the annotated data.
+	remainingLength--
+
+	// If the above length is 14 or we have an ordered struct (indicated by struct with length 1),
+	// then we need to process additional bytes to figure out the full length.
+	if length == 0x0E || (code == bitcodeStruct && length == 1) {
+		val := uint64(0)
+		counter := 1
+
+		for {
+			c, err := b.peekAtOffset(counter)
+			if err != nil {
+				return err
+			}
+
+			counter++
+			remainingLength--
+
+			val <<= 7
+			val ^= uint64(c & 0x7F)
+
+			if (c & 0x80) != 0 {
+				length = val
+				break
+			}
+		}
+	}
+
+	// Confirm the computed length is consistent with the expected remaining length from the annotation wrapper.
+	if length != remainingLength {
+		msg := fmt.Sprintf("annotation wrapper indicates the enclosed value's length to be %d "+
+			"but the enclosed value claims to have length %d", remainingLength, length)
+		return &SyntaxError{msg, b.pos}
+	}
+
+	return nil
+}
+
 // ReadInt reads an integer value.
 func (b *bitstream) ReadInt() (interface{}, error) {
 	if b.code != bitcodeInt && b.code != bitcodeNegInt {
@@ -1002,6 +1077,16 @@ func (b *bitstream) skip(n uint64) error {
 	return nil
 }
 
+// PeekAtOffset returns the data at a certain offset without advancing the reader.
+func (b *bitstream) peekAtOffset(offset int) (byte, error) {
+	data, err := b.in.Peek(offset + 1)
+	if err != nil {
+		return 0, err
+	}
+
+	return data[offset], nil
+}
+
 // A bitnode represents a container value, including its type code and
 // the offset at which it (supposedly) ends.
 type bitnode struct {


Test patch:
diff --git a/ion/binaryreader_test.go b/ion/binaryreader_test.go
index 0f58c85..297e987 100644
--- a/ion/binaryreader_test.go
+++ b/ion/binaryreader_test.go
@@ -102,7 +102,7 @@ func TestReadMultipleLSTs(t *testing.T) {
 		0x71, 0x0B, // $11
 		0x71, 0x6F, // bar
 		0xE3, 0x81, 0x83, 0xDF, // $ion_symbol_table::null.struct
-		0xEE, 0x8F, 0x81, 0x83, 0xDD, // $ion_symbol_table::{
+		0xEE, 0x90, 0x81, 0x83, 0xDD, // $ion_symbol_table::{
 		0x86, 0x71, 0x03, // imports: $ion_symbol_table,
 		0x87, 0xB8, // symbols:[
 		0x83, 'f', 'o', 'o', // "foo"
diff --git a/ion/integration_test.go b/ion/integration_test.go
index 00673e3..7363ddb 100644
--- a/ion/integration_test.go
+++ b/ion/integration_test.go
@@ -100,14 +100,8 @@ var textRoundTripSkipList = []string{
 }
 
 var malformedIonsSkipList = []string{
-	"annotationLengthTooLongContainer.10n",
-	"annotationLengthTooLongScalar.10n",
-	"annotationLengthTooShortContainer.10n",
-	"annotationLengthTooShortScalar.10n",
-	"annotationNested.10n",
 	"annotationSymbolIDUnmapped.10n",
 	"annotationSymbolIDUnmapped.ion",
-	"emptyAnnotatedInt.10n",
 	"fieldNameSymbolIDUnmapped.10n",
 	"fieldNameSymbolIDUnmapped.ion",
 	"invalidVersionMarker_ion_0_0.ion",
@@ -123,7 +117,6 @@ var malformedIonsSkipList = []string{
 	"localSymbolTableWithMultipleSymbolsFields.ion",
 	"minLongWithLenTooSmall.10n",
 	"nopPadTooShort.10n",
-	"nopPadWithAnnotations.10n",
 	"nullDotCommentInt.ion",
 	"surrogate_1.ion",
 	"surrogate_10.ion",
@@ -326,11 +319,8 @@ func testInvalidReader(t *testing.T, r Reader) error {
 			}
 		}
 	}
-	if r.Err() != nil {
-		return r.Err()
-	}
 
-	return nil
+	return r.Err()
 }
 
 // Execute equivalency and non-equivalency tests, where true for eq means