shamblett / cbor

A CBOR implementation for Dart
MIT License
36 stars 16 forks source link

Invalid decoding of arrays #9

Closed DanielSoCra closed 4 years ago

DanielSoCra commented 4 years ago

Given the following cbor encoded data: Hex String: 81A301A1010103A101010CA10101 Expected output: [{1: {1: 1}, 3: {1: 1}, 12: {1: 1}}]

When decoding like this:

import 'package:convert/convert.dart';
import 'package:cbor/cbor.dart' as cbor;

void main() {
  String hexString = "84a301a1010103a101010ca10101";
  List<int> bytes = hex.decode(hexString);

  final cbor.Cbor inst = cbor.Cbor();
  inst.decodeFromList(bytes);

  List<dynamic> decoded = inst.getDecodedData();
  print(decoded);
}

The output is this: [[], {1: {1: 1}, 3: {1: 1}, 12: {1: 1}}]

So instead of wrapping the outer map with an array, the array gets prepended as empty. This happens with similar, more complex data aswell.

Did I miss something when decoding?

shamblett commented 4 years ago

I'll have more detailed look at this but just pasting your test string into the CBOR playground online CBOR test page gives

Out of bytes to decode (need at least 1 byte more)

Screensjot attached. Ok, so what tool do you use to decode the above string?

cbor

Michael-2020 commented 4 years ago

There is an error in the example. The correct example is:

import 'package:convert/convert.dart';
import 'package:cbor/cbor.dart' as cbor;

void main() {
  String hexString = "81a301a1010103a101010ca10101";
  List<int> bytes = hex.decode(hexString);

  final cbor.Cbor inst = cbor.Cbor();
  inst.decodeFromList(bytes);

  List<dynamic> decoded = inst.getDecodedData();
  print(decoded);
}

The only difference is: hexString starts with 81 (not 84). The output is

[[], {1: {1: 1}, 3: {1: 1}, 12: {1: 1}}]

This is not correct. CBOR Playground says:

[{1: {1: 1}, 3: {1: 1}, 12: {1: 1}}]
Michael-2020 commented 4 years ago

The smallest example I found which may indicate the same issue is:

import 'package:convert/convert.dart';
import 'package:cbor/cbor.dart' as cbor;

void main() {
  // [{1: {2: 3}}]
  String hexString = "81a101a10203";
  List<int> bytes = hex.decode(hexString);

  final cbor.Cbor inst = cbor.Cbor();
  inst.decodeFromList(bytes);

  List<dynamic> decoded = inst.getDecodedData();
  print(decoded);
  // wrong result: [[], {1: {2: 3}}]
}
DanielSoCra commented 4 years ago

@Michael-2020 thank you for verifiying, indeed I forgot to change the hex string in the example, it should start with 81 instead of 84, since the array only contains 1 element., not 4.

` ///cbor_listener_stack.dart Line 450

  if (entry.complete) {
    _stack.push(item);
  } else {
    // List or Map
    if (entry.type == dartTypes.dtList) {
      if (item.isIncompleteList() || item.isIncompleteMap()) {
        _stack.push(item);
      } else {
        entry.data.add(item.data);
        if (entry.data.length == entry.targetSize) {
          entry.complete = true;
          // Item can be ignored.
          item.ignore = true;
          // Recurse for nested lists
          final DartItem item1 = _stack.pop();
          _appendImpl(item1);
        }
      }
    } 

` When stepping through the example, I found

item.isIncompleteList() || item.isIncompleteMap() evaluates to true (but should be false) when the map should be inserted to the array. Instead, it gets pushed to the stack.

When I remove this:

          if (item.isIncompleteList() || item.isIncompleteMap()) {
            _stack.push(item);
          } 

I get this output: [[{}], 1, {2: 3}]

I can have a more in depth look on monday.

shamblett commented 4 years ago

Ok, merging this and testing it now gives an output of

[[{1: {1: 1}, 3: {1: 1}, 12: {1: 1}}]]

we now have the real outer list inside another list, rather than the original fault of having the empty list at the beginning. All other tests pass so no other effects.

DanielSoCra commented 4 years ago

I was to quick with the pull request.

I have tested another structure and there seems to be a bug with maps and lists in general.

If you take this hexdump:

81A301A1010203A10181010CA10102

Screenshot 2020-01-06 at 16 17 21

which is the representation of the following data: [{1: {1: 2}, 3: {1: [1]}, 12: {1: 2}}]

we now have this output (with the pull request): [[{1: {1: 2}, 3: {1: [1]}}], {1: [1]}, 12, {1: 2}] and this output without the pull request: [[], {1: {1: 2}, 3: {1: [1]}}, {1: [1]}, 12, {1: 2}]

The difference is, that the first two of 3 members of the map (hex A3) get wrapped inside the array (hex 81).

This seems to be because there is an array inside the second map. If you put the array inside the first map, only the first map gets wrapped inside the array.

PS: Thank you for taking the effort to solve this!

shamblett commented 4 years ago

Looking at this in greater detail I've come to the conclusion that although I could probably fix these two examples, the first is actually OK with the pull request if you take the first entry of the returned list that is

[[{1: {1: 1}, 3: {1: 1}, 12: {1: 1}}]]

is correct if you take the inner list only. The second one however is quite broken so I want to revamp how we stack/decode nested data in toto. This will take longer than fixing this issue but I think its worth it in the long run. I don't really have a feel of when this will be ready at the moment, I'll update as needed.

shamblett commented 4 years ago

OK, finally republished at 2.1.0. I've reworked the item stack, we now have a separate decode stack to process the DartItems when we have completely received them all rather than trying to decode on arrival. This doesn't help with streaming but then again we are not claiming to be streaming capable.

DanielSoCra commented 4 years ago

Thank you for the effort. I will be able to test this until the end of next week.

DanielSoCra commented 4 years ago

Everything is working fine and as expected so far! Thank you again for the quick & sophisticated support!