[riot-notifications] [RIOT-OS/RIOT] sys/ubjson: remove module. (#11724)

Juan I Carrano notifications at github.com
Wed Jun 19 18:09:03 CEST 2019


### Summary

**NOTE**: Marking as waiting because we need to deprecate it before removing.

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 #11702, #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);
  ```
- #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?


### Testing procedure

AFAIK there is nothing _in RIOT_ making use of this, so there is nothing to test. There might be third party code, so that's why we should first deprecate it.

### Issues/PRs references

See #2175, #11702, #11703
You can view, comment on, or merge this pull request online at:

  https://github.com/RIOT-OS/RIOT/pull/11724

-- Commit Summary --

  * sys/ubjson: remove module.

-- File Changes --

    D sys/include/ubjson.h (581)
    D sys/ubjson/Makefile (1)
    D sys/ubjson/ubjson-internal.h (62)
    D sys/ubjson/ubjson-read.c (392)
    D sys/ubjson/ubjson-write.c (189)
    D tests/unittests/tests-ubjson/Makefile (1)
    D tests/unittests/tests-ubjson/Makefile.include (2)
    D tests/unittests/tests-ubjson/test-ubjson-empty-array.c (127)
    D tests/unittests/tests-ubjson/test-ubjson-empty-object.c (130)
    D tests/unittests/tests-ubjson/tests-ubjson.c (116)
    D tests/unittests/tests-ubjson/tests-ubjson.h (54)

-- Patch Links --

https://github.com/RIOT-OS/RIOT/pull/11724.patch
https://github.com/RIOT-OS/RIOT/pull/11724.diff

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/RIOT-OS/RIOT/pull/11724
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.riot-os.org/pipermail/notifications/attachments/20190619/3c9cc14b/attachment-0001.html>


More information about the notifications mailing list