cbor: bounds checking on read and no_copy string deserialization #5203

OlegHahm · 2016-03-30T12:42:03Z

Since @luciotorre seems to be not working on #3843, I adopted parts of it. I refactored that commits and cut out the parts that I didn't understand or which seem unrelated.

@krf, can you take a brief look?

cgundogan · 2016-03-30T12:44:56Z

I can imagine the compiler to complain for 8-and 16-bits architectures without luciotorre@2df5b2b

OlegHahm · 2016-03-30T12:47:11Z

I can imagine the compiler to complain for 8-and 16-bits architectures without luciotorre/RIOT@2df5b2b

But this is untouched code - shouldn't the CI have whined about this before?

cgundogan · 2016-03-30T12:50:29Z

But this is untouched code - shouldn't the CI have whined about this before?

yeah weird .. nevermind then

cgundogan · 2016-04-01T13:02:01Z

@OlegHahm commented 2 days ago in #3843
Sorry, feature freeze.

OlegHahm · 2016-05-18T10:53:51Z

Feature freeze is over.

kYc0o · 2016-07-22T14:38:13Z

tests/unittests/tests-cbor/tests-cbor.c

                        new_TestFixture(test_byte_string_invalid),
                        new_TestFixture(test_unicode_string),
+                        new_TestFixture(test_unicode_string_no_copy),
                        new_TestFixture(test_unicode_string_invalid),
                        new_TestFixture(test_array),
                        new_TestFixture(test_array_indefinite),


This function names respect our coding conventions?

This is part of embUnit's API (sys/include/embUnit/TestCaller.h).

kYc0o · 2016-07-22T14:38:56Z

Can you rebase and remake the tests? Anyways it won't make it for this release so I'll postpone it.

OlegHahm · 2016-07-22T14:44:01Z

What do you mean by "remake the tests"?

kYc0o · 2016-07-22T15:09:01Z

I thought you have made some test for this PR, I mean, testing functionality. Anyways it's marked for the next release. But if you think this can be merged soon let's do it :)

BytesGalore · 2016-07-25T11:33:29Z

sys/cbor/cbor.c

@@ -319,10 +335,14 @@ static size_t decode_int(const cbor_stream_t *s, size_t offset, uint64_t *val)

    *val = 0; /* clear val first */

+    CBOR_ENSURE_SIZE_READ(s, offset + 1);


is there a case where the cbor_stream_t *s can be NULL?
The question also applies for all other cases where we pass a pointer to a macro.

I know we check the case here, but in other cases we don't (e.g. [encode][decode]_bytes(...))

Valid point, but IMO out of scope of this PR. I would propose to open a follow-up PR that adds some assertions to check for null pointers. Okay?

miri64 · 2016-10-31T17:41:07Z

Postponed due to feature freeze

miri64 · 2017-01-17T09:54:06Z

@OlegHahm can you please rebase?

OlegHahm · 2017-01-17T10:47:34Z

Rebased. @BytesGalore's comment should IMO be addressed in a separate PR.

miri64 · 2017-01-17T11:02:48Z

sys/cbor/cbor.c

@@ -92,6 +96,18 @@
 #endif

 #ifndef CBOR_NO_FLOAT
+
+/* pack to force aligned access on ARMv7 (buggy GCC) */
+#pragma GCC diagnostic error "-Wcast-align"


Quoting yourself in #2189 (comment)

Concerning the pragmas: they should be avoided wherever possible. @Kijewski, any way to get rid of them?

I'm not sure I understand the context of the comment, the pragma and the packed attribute, so please provide better context.

Not sure, I understand it myself - I'm only a caretaker of this PR. But I will try my best.

(sorry, forgot that you adapted the PR :-/)

No worries - your comment is valid anyway.

However, after thinking about this a little bit, my conclusion is:
1.) I would rephrase my own comment from #2198 to: "pragmas that may lead to different output on different compilers should be avoided wherever possible".
2.) I don't quite understand which bug in GCC this refers to.
3.) The pragma would only add one more switch to GCC which IMO should not create a problem.

It is not ignored - on the contrary, it will raise an error.

Ahh, I like that. 👍 How much would break if we'd enable -Wcast-align globally?

Everything. ;-)

/home/oleg/git/RIOT/sys/include/net/gnrc/ipv6/netif.h: In function 'gnrc_ipv6_netif_addr_get' /home/oleg/git/RIOT/core/include/kernel_defines.h:53:14: error: cast increases required alignment of target type [-Werror=cast-align] ((TYPE *) ((char *) __m____ - offsetof(TYPE, MEMBER))); \

@Kijewski, okay to leave as is?

Okay, if this is only concerning GCC, I'm okay with this special case.

miri64 · 2017-01-18T17:31:54Z

Jenkins reports

cbor.c:108:6: error: ISO C99 doesn't support unnamed structs/unions [-Werror=pedantic]

But let's see what Murdock has to say also.

miri64 · 2017-01-18T17:34:30Z

sys/cbor/cbor.c

+        uint16_t u16;
+        uint32_t u32;
+        uint64_t u64;
+    };


Union needs a member name (that's what Jenkins is complaining about)

miri64 · 2017-01-18T17:41:18Z

Please add the following patch for the other errors reported by Jenkins:

diff --git a/sys/cbor/cbor.c b/sys/cbor/cbor.c
index f3c7995..9319480 100644
--- a/sys/cbor/cbor.c
+++ b/sys/cbor/cbor.c
@@ -95,19 +95,18 @@
 #define NAN (0.0/0.0)
 #endif
 
-#ifndef CBOR_NO_FLOAT
-
 /* pack to force aligned access on ARMv7 (buggy GCC) */
 #pragma GCC diagnostic error "-Wcast-align"
-typedef struct cast_align_u8 {
+typedef struct  __attribute__((packed)) {
     unsigned char u8;
     union {
         uint16_t u16;
         uint32_t u32;
         uint64_t u64;
     };
-} __attribute__((packed)) cast_align_u8_t;
+} cast_align_u8_t;
 
+#ifndef CBOR_NO_FLOAT
 /**
  * Convert float @p x to network format
  */

OlegHahm · 2017-01-18T17:56:48Z

updated

miri64

Waiting for @Kijewski's approval, but in general: ACK, please squash.

OlegHahm · 2017-01-18T18:33:38Z

squashed

miri64 · 2017-01-18T18:46:02Z

sys/cbor/cbor.c

@@ -521,7 +578,7 @@ size_t cbor_deserialize_float(const cbor_stream_t *stream, size_t offset, float
    unsigned char *data = &stream->data[offset];

    if (*data == CBOR_FLOAT32) {
-        *val = ntohf(*(uint32_t *)(data + 1));
+        *val = ntohf(((cast_align_u8_t *)data)->u32);


miri64 · 2017-01-18T18:46:18Z

sys/cbor/cbor.c

    if (CBOR_TYPE(stream, offset) != CBOR_7 || !val) {
        return 0;
    }

    unsigned char *data = &stream->data[offset];

    if (*data == CBOR_FLOAT64) {
-        *val = ntohd(*(uint64_t *)(data + 1));
+        CBOR_ENSURE_SIZE_READ(stream, offset + 9);
+        *val = ntohd(((cast_align_u8_t *)data)->u64);


miri64

Please resquash

This can happen due to cast using buggy GCC on ARMv7 Credit to our shy french pal

OlegHahm · 2017-01-18T19:40:41Z

resquashed

cgundogan · 2017-01-18T20:24:37Z

on my machine:

RIOT/tests/unittests % make tests-cbor
Building application "unittests" for "native" with MCU "native".
RIOT/sys/cbor/cbor.c:108:6: error: ISO C99 doesn’t support unnamed structs/unions [-Werror=pedantic]
     };

miri64 · 2017-01-18T20:50:27Z

@cgundogan that was fixed... Are you sure you have the most current version of @OlegHahm's branch?

cgundogan · 2017-01-18T20:53:25Z

Are you sure you have the most current version of @OlegHahm's branch?

yup, just checked. Problem persists for me.

miri64 · 2017-01-18T20:55:24Z

But neither is the line you are referring to still at 108, nor is it unnamed

cgundogan · 2017-01-18T21:01:18Z

weird. I did a clean checkout and now it notices the different commit history. Thanks for the pointer, seems to be OK.

OlegHahm · 2017-01-19T09:45:13Z

@Kijewski, are you okay with the pragma?

Kijewski · 2017-01-19T11:17:52Z

I'm okay with the pragma.

OlegHahm assigned cgundogan Mar 30, 2016

OlegHahm added Type: enhancement The issue suggests enhanceable parts / The PR enhances parts of the codebase / documentation Area: network Area: Networking labels Mar 30, 2016

OlegHahm added this to the Release 2016.04 milestone Mar 30, 2016

cgundogan modified the milestones: Release 2016.07, Release 2016.04 Apr 1, 2016

kYc0o reviewed Jul 22, 2016
View reviewed changes

kYc0o modified the milestones: Release 2016.10, Release 2016.07 Jul 22, 2016

BytesGalore reviewed Jul 25, 2016
View reviewed changes

OlegHahm mentioned this pull request Sep 28, 2016

cbor: Bounds checking on read and no_copy string deserialization #3843

Closed

miri64 modified the milestones: Release 2017.01, Release 2016.10 Oct 31, 2016

OlegHahm added the Community: Hack'n'ACK candidate This PR is a candidate for review and discussion during one of RIOT's monthly Hack'n'ACK parties label Jan 15, 2017

OlegHahm force-pushed the cbor_bound_checking branch from 470b01a to 123e56d Compare January 17, 2017 10:44

miri64 reviewed Jan 17, 2017

View reviewed changes

miri64 added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Jan 18, 2017

miri64 requested changes Jan 18, 2017

View reviewed changes

sys/cbor/cbor.c

uint16_t u16;

uint32_t u32;

uint64_t u64;

};

Copy link

Member

miri64 Jan 18, 2017

Choose a reason for hiding this comment

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

Union needs a member name (that's what Jenkins is complaining about)

miri64 approved these changes Jan 18, 2017

View reviewed changes

Lucio Torre added 5 commits January 18, 2017 19:32

cbor: introduce bounds checking on read

1c64d45

cbor: add zero copy string deserialization

d783d78

cbor: check return value for zero

f6db569

cbor: introduce cbor_serialize_byte_stringl

04fc0b3

cbor: fix typo

5f26b76

OlegHahm force-pushed the cbor_bound_checking branch from 1c8c2cf to 6820478 Compare January 18, 2017 18:33

miri64 requested changes Jan 18, 2017

View reviewed changes

miri64 approved these changes Jan 18, 2017

View reviewed changes

cbor: Packed struct to bypass unaligned stack

63bcccf

This can happen due to cast using buggy GCC on ARMv7 Credit to our shy french pal

OlegHahm force-pushed the cbor_bound_checking branch from bbf9f70 to 63bcccf Compare January 18, 2017 19:40

OlegHahm merged commit b9b6e49 into RIOT-OS:master Jan 19, 2017

OlegHahm deleted the cbor_bound_checking branch January 19, 2017 11:21

		@@ -319,10 +335,14 @@ static size_t decode_int(const cbor_stream_t s, size_t offset, uint64_t val)

		val = 0; / clear val first */

		CBOR_ENSURE_SIZE_READ(s, offset + 1);

cbor: bounds checking on read and no_copy string deserialization #5203

cbor: bounds checking on read and no_copy string deserialization #5203

Conversation

OlegHahm commented Mar 30, 2016

cgundogan commented Mar 30, 2016

OlegHahm commented Mar 30, 2016

cgundogan commented Mar 30, 2016

cgundogan commented Apr 1, 2016

OlegHahm commented May 18, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kYc0o commented Jul 22, 2016

OlegHahm commented Jul 22, 2016

kYc0o commented Jul 22, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

miri64 commented Oct 31, 2016

miri64 commented Jan 17, 2017

OlegHahm commented Jan 17, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

miri64 commented Jan 18, 2017

Choose a reason for hiding this comment

miri64 commented Jan 18, 2017

OlegHahm commented Jan 18, 2017

miri64 left a comment

Choose a reason for hiding this comment

OlegHahm commented Jan 18, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

miri64 left a comment

Choose a reason for hiding this comment

OlegHahm commented Jan 18, 2017

cgundogan commented Jan 18, 2017

miri64 commented Jan 18, 2017

cgundogan commented Jan 18, 2017

miri64 commented Jan 18, 2017

cgundogan commented Jan 18, 2017

OlegHahm commented Jan 19, 2017

Kijewski commented Jan 19, 2017 • edited

Kijewski commented Jan 19, 2017 •

edited