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

sys/ubjson: Write missing marker for i64 #11703

Merged
merged 1 commit into from
Jun 18, 2019

Conversation

fhessel
Copy link
Contributor

@fhessel fhessel commented Jun 14, 2019

Contribution description

The UBJSON writer skips the marker for large integer values (64 bit) when writing them. This makes the resulting UBJSON document unparsable for the receiver.

UBJSON in general uses a <type-marker>[value] syntax, and for 64 bit integers, one would expect an literal L followed by the 8 data bytes (Reference).

Testing procedure

The following demo application can be run on native to examine the issue in isolation. It prints a 64 bit value within an object structure.

main.c
#include <ctype.h>
#include <stdio.h>
#include "ubjson.h"

/** Callback to print the data to the console. */
int write_cb(ubjson_cookie_t *cookie, const void *buf, size_t len);

int main(void)
{
    uint64_t big_number = 0x0123456789abcdef;
    ubjson_cookie_t cookie;
    ubjson_write_init(&cookie, write_cb);
    ubjson_open_object(&cookie);
    ubjson_write_key(&cookie, "big_number", 10);
    ubjson_write_i64(&cookie, big_number);
    ubjson_close_object(&cookie);
    return 0;
}

int write_cb(ubjson_cookie_t *cookie, const void *buf, size_t len)
{
    (void)cookie;
    const uint8_t *tbuf = buf;
    for(size_t idx = 0; idx < len; idx++) {
        printf("0x%02x ", tbuf[idx]);
        putchar(isprint(tbuf[idx]) ? tbuf[idx] : '.');
        printf("\n");
    }
    return len;
}
Makefile
APPLICATION = ubjson-test-bool
BOARD ?= native
RIOTBASE ?= $(CURDIR)/../..
DEVELHELP ?= 1
QUIET ?= 1
USEMODULE += ubjson
include $(RIOTBASE)/Makefile.include

Before applying the fix, it will output:

0x7b 0x69 0x0a 0x62 0x69 0x67 0x5f 0x6e 0x75 0x6d 0x62 0x65 0x72 0x01 0x23 0x45 0x67 0x89 0xab 0xcd 0xef 0x7d 
{    i    .    b    i    g    _    n    u    m    b    e    r    .    #    E    g    .    .    .    .    }

With the fix, the values will be correct (note the additional L):

0x7b 0x69 0x0a 0x62 0x69 0x67 0x5f 0x6e 0x75 0x6d 0x62 0x65 0x72 0x4c 0x01 0x23 0x45 0x67 0x89 0xab 0xcd 0xef 0x7d 
{    i    .    b    i    g    _    n    u    m    b    e    r    L    .    #    E    g    .    .    .    .    }

Issues/PRs references

None.

@MrKevinWeiss MrKevinWeiss added Area: sys Area: System CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Impact: minor The PR is small in size and might only require a quick look of a knowledgeable reviewer Type: bug The issue reports a bug / The PR fixes a bug (including spelling errors) labels Jun 17, 2019
@MrKevinWeiss MrKevinWeiss self-requested a review June 17, 2019 09:02
@leandrolanzieri leandrolanzieri added Reviewed: 1-fundamentals The fundamentals of the PR were reviewed according to the maintainer guidelines Reviewed: 2-code-design The code design of the PR was reviewed according to the maintainer guidelines Reviewed: 3-testing The PR was tested according to the maintainer guidelines Reviewed: 4-code-style The adherence to coding conventions by the PR were reviewed according to the maintainer guidelines labels Jun 18, 2019
Copy link
Contributor

@leandrolanzieri leandrolanzieri left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One would think that this type of errors would be caught by the module's unit tests, it is not the case. : / Anyways, thanks for the fix! ACK.

@leandrolanzieri leandrolanzieri merged commit f32ab70 into RIOT-OS:master Jun 18, 2019
@fhessel fhessel deleted the fix-ubjson-i64 branch June 18, 2019 09:24
jcarrano added a commit to jcarrano/RIOT that referenced this pull request Jun 19, 2019
The ubjson module has a number of quality defects and is unsafe.
Considering CBOR is popular, standarized and supported in RIOT and that
the ubjson implementation is a home-grown one whose API will likely be
unfamiliar to new users, I propose to delete it.

This removal, of course, dows not have to be NOW. We can deprecate it for
one or two releases before.

What's wrong with this module?

- Unsafe: the parsing is done recursively. This is embedded in the API, so it
  is not possible to fix it without changing the API. A document with too much
  nesting can cause a stack overflow.
- Does not validate writing: it is possible to produce invalid output. From
  the docs:
  > The library won't complain if you write multiple values that are not
  > inside an array or object. The result will just not be properly serialized.
- Poorly tested. As shown by RIOT-OS#11702, RIOT-OS#11703 the tests were not even detecting
  that a False was stored as True.
- In line with the previous remark, see
  https://github.com/RIOT-OS/RIOT/blob/68dc5b0d6e0422df9c3653c2cb8021fc35974aae/tests/unittests/tests-ubjson/tests-ubjson.c#L66-L77
  Why is the following code in the unit tests??
  ```c
    irq_disable();
    sched_set_status(data->main_thread, STATUS_PENDING);
  ```
- RIOT-OS#2175 is still unfixed after 3.5 years.
- Code quality. The code has multiline macros that assign variables and
  return. See https://github.com/RIOT-OS/RIOT/blob/c3325148755c64a56b256253151a485498bcbc9b/sys/ubjson/ubjson-write.c#L34-L41

Can we mark it as deprecated this release and sweep it in the following one?
jcarrano added a commit to jcarrano/RIOT that referenced this pull request Aug 15, 2019
The ubjson module has a number of quality defects and is unsafe.
Considering CBOR is popular, standarized and supported in RIOT and that
the ubjson implementation is a home-grown one whose API will likely be
unfamiliar to new users, I propose to delete it.

This removal, of course, dows not have to be NOW. We can deprecate it for
one or two releases before.

What's wrong with this module?

- Unsafe: the parsing is done recursively. This is embedded in the API, so it
  is not possible to fix it without changing the API. A document with too much
  nesting can cause a stack overflow.
- Does not validate writing: it is possible to produce invalid output. From
  the docs:
  > The library won't complain if you write multiple values that are not
  > inside an array or object. The result will just not be properly serialized.
- Poorly tested. As shown by RIOT-OS#11702, RIOT-OS#11703 the tests were not even detecting
  that a False was stored as True.
- In line with the previous remark, see
  https://github.com/RIOT-OS/RIOT/blob/68dc5b0d6e0422df9c3653c2cb8021fc35974aae/tests/unittests/tests-ubjson/tests-ubjson.c#L66-L77
  Why is the following code in the unit tests??
  ```c
    irq_disable();
    sched_set_status(data->main_thread, STATUS_PENDING);
  ```
- RIOT-OS#2175 is still unfixed after 3.5 years.
- Code quality. The code has multiline macros that assign variables and
  return. See https://github.com/RIOT-OS/RIOT/blob/c3325148755c64a56b256253151a485498bcbc9b/sys/ubjson/ubjson-write.c#L34-L41

Can we mark it as deprecated this release and sweep it in the following one?
leandrolanzieri pushed a commit to leandrolanzieri/RIOT that referenced this pull request Dec 1, 2019
The ubjson module has a number of quality defects and is unsafe.
Considering CBOR is popular, standarized and supported in RIOT and that
the ubjson implementation is a home-grown one whose API will likely be
unfamiliar to new users, I propose to delete it.

This removal, of course, dows not have to be NOW. We can deprecate it for
one or two releases before.

What's wrong with this module?

- Unsafe: the parsing is done recursively. This is embedded in the API, so it
  is not possible to fix it without changing the API. A document with too much
  nesting can cause a stack overflow.
- Does not validate writing: it is possible to produce invalid output. From
  the docs:
  > The library won't complain if you write multiple values that are not
  > inside an array or object. The result will just not be properly serialized.
- Poorly tested. As shown by RIOT-OS#11702, RIOT-OS#11703 the tests were not even detecting
  that a False was stored as True.
- In line with the previous remark, see
  https://github.com/RIOT-OS/RIOT/blob/68dc5b0d6e0422df9c3653c2cb8021fc35974aae/tests/unittests/tests-ubjson/tests-ubjson.c#L66-L77
  Why is the following code in the unit tests??
  ```c
    irq_disable();
    sched_set_status(data->main_thread, STATUS_PENDING);
  ```
- RIOT-OS#2175 is still unfixed after 3.5 years.
- Code quality. The code has multiline macros that assign variables and
  return. See https://github.com/RIOT-OS/RIOT/blob/c3325148755c64a56b256253151a485498bcbc9b/sys/ubjson/ubjson-write.c#L34-L41

Can we mark it as deprecated this release and sweep it in the following one?
fjmolinas pushed a commit to fjmolinas/RIOT that referenced this pull request Dec 15, 2019
The ubjson module has a number of quality defects and is unsafe.
Considering CBOR is popular, standarized and supported in RIOT and that
the ubjson implementation is a home-grown one whose API will likely be
unfamiliar to new users, I propose to delete it.

This removal, of course, dows not have to be NOW. We can deprecate it for
one or two releases before.

What's wrong with this module?

- Unsafe: the parsing is done recursively. This is embedded in the API, so it
  is not possible to fix it without changing the API. A document with too much
  nesting can cause a stack overflow.
- Does not validate writing: it is possible to produce invalid output. From
  the docs:
  > The library won't complain if you write multiple values that are not
  > inside an array or object. The result will just not be properly serialized.
- Poorly tested. As shown by RIOT-OS#11702, RIOT-OS#11703 the tests were not even detecting
  that a False was stored as True.
- In line with the previous remark, see
  https://github.com/RIOT-OS/RIOT/blob/68dc5b0d6e0422df9c3653c2cb8021fc35974aae/tests/unittests/tests-ubjson/tests-ubjson.c#L66-L77
  Why is the following code in the unit tests??
  ```c
    irq_disable();
    sched_set_status(data->main_thread, STATUS_PENDING);
  ```
- RIOT-OS#2175 is still unfixed after 3.5 years.
- Code quality. The code has multiline macros that assign variables and
  return. See https://github.com/RIOT-OS/RIOT/blob/c3325148755c64a56b256253151a485498bcbc9b/sys/ubjson/ubjson-write.c#L34-L41

Can we mark it as deprecated this release and sweep it in the following one?
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: sys Area: System CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Impact: minor The PR is small in size and might only require a quick look of a knowledgeable reviewer Reviewed: 1-fundamentals The fundamentals of the PR were reviewed according to the maintainer guidelines Reviewed: 2-code-design The code design of the PR was reviewed according to the maintainer guidelines Reviewed: 3-testing The PR was tested according to the maintainer guidelines Reviewed: 4-code-style The adherence to coding conventions by the PR were reviewed according to the maintainer guidelines Type: bug The issue reports a bug / The PR fixes a bug (including spelling errors)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants