Просмотр исходного кода

Fix curious assumption from the original C reader.

Reported as part of #323.

Logic in Go was fixed to respect the documented assumption, instead
of changing all call sites.

This is probably still broken in libyaml, but perhaps it doesn't
affect it because the accessed memory is still owned by the buffer.
Still, underlying logic seems somewhat bogus.
Gustavo Niemeyer 8 лет назад
Родитель
Сommit
1b2e8c1531
2 измененных файлов с 48 добавлено и 1 удалено
  1. 29 0
      decode_test.go
  2. 19 1
      readerc.go

+ 29 - 0
decode_test.go

@@ -1242,6 +1242,35 @@ func (t *textUnmarshaler) UnmarshalText(s []byte) error {
 	return nil
 }
 
+func (s *S) TestFuzzCrashers(c *C) {
+	cases := []string{
+		// runtime error: index out of range
+		"\"\\0\\\r\n",
+
+		// should not happen
+		"  0: [\n] 0",
+		"? ? \"\n\" 0",
+		"    - {\n000}0",
+		"0:\n  0: [0\n] 0",
+		"    - \"\n000\"0",
+		"    - \"\n000\"\"",
+		"0:\n    - {\n000}0",
+		"0:\n    - \"\n000\"0",
+		"0:\n    - \"\n000\"\"",
+
+		// runtime error: index out of range
+		" \ufeff\n",
+		"? \ufeff\n",
+		"? \ufeff:\n",
+		"0: \ufeff\n",
+		"? \ufeff: \ufeff\n",
+	}
+	for _, data := range cases {
+		var v interface{}
+		_ = yaml.Unmarshal([]byte(data), &v)
+	}
+}
+
 //var data []byte
 //func init() {
 //	var err error

+ 19 - 1
readerc.go

@@ -93,9 +93,18 @@ func yaml_parser_update_buffer(parser *yaml_parser_t, length int) bool {
 		panic("read handler must be set")
 	}
 
+	// [Go] This function was changed to guarantee the requested length size at EOF.
+	// The fact we need to do this is pretty awful, but the description above implies
+	// for that to be the case, and there are tests 
+
 	// If the EOF flag is set and the raw buffer is empty, do nothing.
 	if parser.eof && parser.raw_buffer_pos == len(parser.raw_buffer) {
-		return true
+		// [Go] ACTUALLY! Read the documentation of this function above.
+		// This is just broken. To return true, we need to have the
+		// given length in the buffer. Not doing that means every single
+		// check that calls this function to make sure the buffer has a
+		// given length is Go) panicking; or C) accessing invalid memory.
+		//return true
 	}
 
 	// Return if the buffer contains enough characters.
@@ -389,6 +398,15 @@ func yaml_parser_update_buffer(parser *yaml_parser_t, length int) bool {
 			break
 		}
 	}
+	// [Go] Read the documentation of this function above. To return true,
+	// we need to have the given length in the buffer. Not doing that means
+	// every single check that calls this function to make sure the buffer
+	// has a given length is Go) panicking; or C) accessing invalid memory.
+	// This happens here due to the EOF above breaking early.
+	for buffer_len < length {
+		parser.buffer[buffer_len] = 0
+		buffer_len++
+	}
 	parser.buffer = parser.buffer[:buffer_len]
 	return true
 }