Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Dag object that could be read in 0.4.17 can no longer be read in 0.4.18 #5776

Closed
rklaehn opened this issue Nov 14, 2018 · 7 comments
Closed
Labels
kind/bug A bug in existing code (including security flaws)

Comments

@rklaehn
Copy link

rklaehn commented Nov 14, 2018

Version information:

$ ipfs version --all
go-ipfs version: 0.4.18-
Repo version: 7
System version: amd64/darwin
Golang version: go1.11.1

Type:

Bug

Description:

A block that could be read as using ipfs dag get in versions up to 0.4.17 no longer can be read as as a dag object in 0.4.18.

Before:

$ ipfs version --all
go-ipfs version: 0.4.17-
Repo version: 7
System version: amd64/darwin
Golang version: go1.10.3
$ ipfs dag get zdpuArHM9moUGtzo47sHeQ5qS4jiuQApyMnFFLoBDdYeouwHy
{"block":{"/":"zdpuAvwmPV8AGBKeeyW8vc8rX5Yd1TzFaeiA6uyxAtzoWCC2E"},"index":{"edge.ax.sf.Terminal":["theTerminal"],"edge.ax.sf.UiSession":["4E815E63","Some Body"]},"max":26,"min":0,"prev":null}

After:

$ ipfs version --all
go-ipfs version: 0.4.18-
Repo version: 7
System version: amd64/darwin
Golang version: go1.11.1

$ ipfs dag get zdpuArHM9moUGtzo47sHeQ5qS4jiuQApyMnFFLoBDdYeouwHy
Error: Invalid majorByte: 0xf7

The block:

hexdump

$ ipfs block get zdpuArHM9moUGtzo47sHeQ5qS4jiuQApyMnFFLoBDdYeouwHy | hexdump
0000000 a5 63 6d 61 78 18 1a 63 6d 69 6e 00 64 70 72 65
0000010 76 f7 65 62 6c 6f 63 6b d8 2a 58 25 00 01 71 12
0000020 20 9c 59 ab c0 3a 31 13 ae 24 54 be 45 5b 5d ea
0000030 09 17 7b 6f 62 24 d7 08 93 e7 74 17 06 79 12 98
0000040 db 65 69 6e 64 65 78 a2 73 65 64 67 65 2e 61 78
0000050 2e 73 66 2e 54 65 72 6d 69 6e 61 6c 81 6b 74 68
0000060 65 54 65 72 6d 69 6e 61 6c 74 65 64 67 65 2e 61
0000070 78 2e 73 66 2e 55 69 53 65 73 73 69 6f 6e 82 68
0000080 34 45 38 31 35 45 36 33 69 53 6f 6d 65 20 42 6f
0000090 64 79                                          
0000092

base64

$ ipfs block get zdpuArHM9moUGtzo47sHeQ5qS4jiuQApyMnFFLoBDdYeouwHy | base64
pWNtYXgYGmNtaW4AZHByZXb3ZWJsb2Nr2CpYJQABcRIgnFmrwDoxE64kVL5FW13qCRd7b2Ik1wiT53QXBnkSmNtlaW5kZXiic2VkZ2UuYXguc2YuVGVybWluYWyBa3RoZVRlcm1pbmFsdGVkZ2UuYXguc2YuVWlTZXNzaW9ugmg0RTgxNUU2M2lTb21lIEJvZHk=
@Stebalien Stebalien added kind/bug A bug in existing code (including security flaws) P0 Critical: Tackled by core team ASAP labels Nov 14, 2018
@Stebalien
Copy link
Member

Upstream bug report: polydawn/refmt#43

@Stebalien
Copy link
Member

So, it looks like you ended up encoding undefined. In CBOR, undefined is supposed to be used for values that fail to encode... How did you create this block?

@Stebalien
Copy link
Member

dignifiedquire/borc#36?

@rklaehn
Copy link
Author

rklaehn commented Nov 14, 2018

This is a js app that is using go-ipfs as storage and uses it via the REST api on port 5001. Due to various issues (very long story) we transitioned from using the dag api of go-ipfs to doing the conversion from JSON to CBOR inside javascript and using the block api.

So here is the code I am using:

import * as dagCbor from 'ipld-dag-cbor'

const encode = <T>(value: T): Observable<Buffer> =>
  new Observable<Buffer>(subscriber => {
    dagCbor.util.serialize(value, (err: any, serialized: Buffer) => {
      if (err !== null) {
        subscriber.error(err)
      } else {
        subscriber.next(serialized)
        subscriber.complete()
      }
    })
  })

from package.json:

    "ipld-dag-cbor": "^0.12.1",

@rklaehn
Copy link
Author

rklaehn commented Nov 14, 2018

So for whatever reason I got a js object which is { block: "whatever", next: undefined }.

The cbor encoder should either refuse to encode this at all (which would be very inconvenient but at least clear), or should treat { block: "whatever", next: undefined } as if next was not present at all, so identical to encoding { block: "whatever" }. This seems to be what JSON.stringify does...

$ node
> JSON.stringify({ block: 'whatever', next: undefined })
'{"block":"whatever"}'

Instead borc uses some guard value for "can not encode this", which yields incorrect CBOR.

Is that a correct summary?

@Stebalien
Copy link
Member

Instead borc uses some guard value for "can not encode this", which yields incorrect CBOR.

Not quite. borc encoded JavaScript's undefined to CBOR's undefined; the final output is technically "valid" CBOR. However, CBOR's undefined is really only supposed to be used to indicate a failure to encode so users can partially encode values. It's not meant to be equivalent to JavaScript's undefined.

Spec:

   In some CBOR-based protocols, the simple value (Section 2.3) of
   Undefined might be used by an encoder as a substitute for a data item
   with an encoding problem, in order to allow the rest of the enclosing
   data items to be encoded without harm.

@Stebalien Stebalien removed the P0 Critical: Tackled by core team ASAP label Nov 19, 2018
Stebalien added a commit to ipfs/go-ipld-cbor that referenced this issue Nov 30, 2018
See: ipfs/kubo#5776

This won't round-trip undef, it'll just convert it to null (which is what most
decoders do anyways).
@Stebalien
Copy link
Member

(fixed)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/bug A bug in existing code (including security flaws)
Projects
None yet
Development

No branches or pull requests

2 participants