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: Do not invert bool when writing it #11702

Merged
merged 1 commit into from
Jun 17, 2019

Conversation

fhessel
Copy link
Contributor

@fhessel fhessel commented Jun 14, 2019

Contribution description

The UBJSON writer inverts boolean values when writing the output. This does not seem to be the intended behavior. This PR reverts the inversion.

UBJSON in general uses a <type-marker>[value] syntax, and for boolean values, there are two dedicated markers for true (T) and false (F). Those are interchanged when writing.

Testing procedure

The following demo application can be run on native to examine the issue in isolation:

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)
{
    ubjson_cookie_t cookie;
    ubjson_write_init(&cookie, write_cb);
    ubjson_open_object(&cookie);
    ubjson_write_key(&cookie, "bool_true", 9);
    ubjson_write_bool(&cookie, true);
    ubjson_write_key(&cookie, "bool_false", 10);
    ubjson_write_bool(&cookie, false);
    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 0x09 0x62 0x6f 0x6f 0x6c 0x5f 0x74 0x72 0x75 0x65 0x46 0x69 0x0a 0x62 0x6f 0x6f 0x6c 0x5f 0x66 0x61 0x6c 0x73 0x65 0x54 0x7d
{    i    .    b    o    o    l    _    t    r    u    e    F    i    .    b    o    o    l    _    f    a    l    s    e    T    }

With the fix, the values will be correct (note the different markers after bool_true and bool_false):

0x7b 0x69 0x09 0x62 0x6f 0x6f 0x6c 0x5f 0x74 0x72 0x75 0x65 0x54 0x69 0x0a 0x62 0x6f 0x6f 0x6c 0x5f 0x66 0x61 0x6c 0x73 0x65 0x46 0x7d
{    i    .    b    o    o    l    _    t    r    u    e    T    i    .    b    o    o    l    _    f    a    l    s    e    F    }

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
@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: 4-code-style The adherence to coding conventions by the PR were reviewed according to the maintainer guidelines labels Jun 17, 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.

Good catch, thanks for fixing this. The change makes sense. ACK!

@leandrolanzieri leandrolanzieri added the Reviewed: 3-testing The PR was tested according to the maintainer guidelines label Jun 17, 2019
@leandrolanzieri leandrolanzieri merged commit 3f348f5 into RIOT-OS:master Jun 17, 2019
@fhessel fhessel deleted the fix-ubjson-bool branch June 17, 2019 15:21
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