Add CBOR parsing primitives by werwurm · Pull Request #72 · aurora-opensource/libnat20

werwurm · 2025-09-03T05:03:45Z

Add primitives for parsing CBOR headers, extracting slices of
octet and text strings, and recursively skipping unknown fields.

seidelrj

like Copilot, I'm also confused about n20_cbor_read_iterate_item. It's not in the header either.

include/nat20/cbor.h

src/core/test/cbor.cpp

Similar to n20_stream_t which offers safe writing to a buffer n20_istream_t offers a safe way to read from a buffer lowering the risk for out of bounds access.

Add primitives for parsing CBOR headers, extracting slices of octet and text strings, and recursively skipping unknown fields.

Copilot

Pull Request Overview

This PR adds CBOR parsing primitives to support reading and parsing CBOR-encoded data. The changes implement functions for reading CBOR headers and recursively skipping CBOR items, complementing the existing CBOR write functionality.

Adds n20_cbor_read_header function to parse CBOR type and value information from an input stream
Adds n20_cbor_read_skip_item function to recursively skip over CBOR items including nested structures
Comprehensive test coverage for both functions with various CBOR types and edge cases

Reviewed Changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 4 comments.

File	Description
include/nat20/cbor.h	Adds function declarations for the new CBOR reading primitives with documentation
src/core/cbor.c	Implements the CBOR header reading and item skipping functions
src/core/test/cbor.cpp	Extensive test suite covering various CBOR types, edge cases, and error conditions

_{Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.}

Copilot · 2025-09-10T18:37:27Z

src/core/cbor.c

+    *n = 0;
+
+    uint8_t additional_bytes = 1 << (additional_info - 24);
+    for (int i = 0; i < additional_bytes; i++) {


Variable type mismatch: additional_bytes is uint8_t but i is int in the loop condition. This could cause warnings or unexpected behavior. Change i to uint8_t or size_t for consistency.

Suggested change

for (int i = 0; i < additional_bytes; i++) {

for (uint8_t i = 0; i < additional_bytes; i++) {

Copilot · 2025-09-10T18:37:28Z

src/core/cbor.c

+
+    switch (type) {
+        case n20_cbor_type_array_e:
+            for (size_t i = 0; i < n; i++) {


Potential integer overflow: n is uint64_t but loop variable i is size_t. On 32-bit systems, this could cause infinite loops if n exceeds SIZE_MAX. Add a bounds check or use a safer iteration approach.

we have the check above to make sure n is <= SIZE_MAX before we get to this for loop.

Copilot · 2025-09-10T18:37:28Z

src/core/cbor.c

+            for (size_t i = 0; i < n; i++) {
+                if (!n20_cbor_read_skip_item(s)) {
+                    return false;
+                }
+            }
+            break;
+        case n20_cbor_type_map_e:
+            for (size_t i = 0; i < n; i++) {


Potential integer overflow: n is uint64_t but loop variable i is size_t. On 32-bit systems, this could cause infinite loops if n exceeds SIZE_MAX. Add a bounds check or use a safer iteration approach.

Suggested change

for (size_t i = 0; i < n; i++) {

if (!n20_cbor_read_skip_item(s)) {

return false;

}

}

break;

case n20_cbor_type_map_e:

for (size_t i = 0; i < n; i++) {

if (n > SIZE_MAX) {

/* Too many items to skip safely on this platform. */

return false;

}

for (size_t i = 0; i < (size_t)n; i++) {

if (!n20_cbor_read_skip_item(s)) {

return false;

}

}

break;

case n20_cbor_type_map_e:

if (n > SIZE_MAX) {

/* Too many items to skip safely on this platform. */

return false;

}

for (size_t i = 0; i < (size_t)n; i++) {

Copilot · 2025-09-10T18:37:28Z

src/core/cbor.c

+            break;
+        case n20_cbor_type_bytes_e:
+        case n20_cbor_type_string_e: {
+            uint8_t const *slice = n20_istream_get_slice(s, n);


Potential integer overflow: n is uint64_t but n20_istream_get_slice likely expects size_t. On 32-bit systems, this could cause truncation or incorrect behavior if n exceeds SIZE_MAX. Add a bounds check before the call.

Suggested change

uint8_t const *slice = n20_istream_get_slice(s, n);

if (n > SIZE_MAX) {

return false;

}

uint8_t const *slice = n20_istream_get_slice(s, (size_t)n);

n20_istream_get_slice will return NULL if the requested size is too large. The NULL check below is the correct behavoir.

Copilot

Pull Request Overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated 4 comments.

_{Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.}

Copilot · 2025-09-10T18:55:08Z

src/core/cbor.c

+    if (additional_info < 24 || additional_info > 27) {
+        *n = additional_info;
+        return true;
+    }
+
+    *n = 0;
+
+    uint8_t additional_bytes = 1 << (additional_info - 24);
+    for (uint8_t i = 0; i < additional_bytes; i++) {
+        uint8_t byte = 0;
+        if (!n20_istream_get(s, &byte)) {
+            return false;
+        }
+        *n = (*n << 8) | byte;
+    }
+
+    return true;


The condition on line 121 will incorrectly handle additional_info values 28-31. These should be treated as special cases (28-30 are invalid, 31 indicates indefinite length), but the current logic assigns them directly to *n which is incorrect.

Suggested change

if (additional_info < 24 || additional_info > 27) {

*n = additional_info;

return true;

}

*n = 0;

uint8_t additional_bytes = 1 << (additional_info - 24);

for (uint8_t i = 0; i < additional_bytes; i++) {

uint8_t byte = 0;

if (!n20_istream_get(s, &byte)) {

return false;

}

*n = (*n << 8) | byte;

}

return true;

if (additional_info < 24) {

*n = additional_info;

return true;

} else if (additional_info >= 24 && additional_info <= 27) {

*n = 0;

uint8_t additional_bytes = 1 << (additional_info - 24);

for (uint8_t i = 0; i < additional_bytes; i++) {

uint8_t byte = 0;

if (!n20_istream_get(s, &byte)) {

return false;

}

*n = (*n << 8) | byte;

}

return true;

} else if (additional_info >= 28 && additional_info <= 30) {

// Invalid/reserved additional_info values

return false;

} else if (additional_info == 31) {

// Indefinite length

*n = (uint64_t)-1;

return true;

}

I think copilot is on to something here. Right now, if you get an additional_data > 27, the current code will mark the header as valid, and return n as a value between 28-31. This will mess up n20_cbor_read_skip_item. E.g., if you had an array type, and additional_data=28, we would say the array had length 28 and skip that many items. We should also return an error on value 31 since we don't support it.

Yes I saw that. I have to think about this because. I tried to avoid returning error codes here. I may have to rethink this stance.

Copilot · 2025-09-10T18:55:08Z

src/core/cbor.c

+        case n20_cbor_type_array_e:
+            if (n > SIZE_MAX) {
+                /* Prevent overflow in the loop counter. */
+                return false;
+            }


The check n > SIZE_MAX is always false since n is a uint64_t and SIZE_MAX is typically also 64-bit on 64-bit systems. This check should use a more appropriate limit or handle the case where n cannot fit in a size_t for the loop counter.

These checks are really irrelevant because the loops will break when the buffer underruns, and because there are no zero sized cbor items to skip there is no real chance spin indefinitely.

Copilot · 2025-09-10T18:55:08Z

src/core/cbor.c

+            if (n > SIZE_MAX) {
+                /* Prevent overflow in the loop counter. */


Same issue as with arrays - the check n > SIZE_MAX is ineffective. Additionally, for maps, the actual number of items to skip is 2 * n (key-value pairs), so the overflow check should account for this multiplication.

Suggested change

if (n > SIZE_MAX) {

/* Prevent overflow in the loop counter. */

if (n > SIZE_MAX / 2) {

/* Prevent overflow in the loop counter (2 * n items to skip). */

This does not apply, because we are actually skipping two items per iteration. Also, because an item is at least one byte in size, the underlying stream will necessarily underrun before the counter can loop.

Copilot · 2025-09-10T18:55:09Z

src/core/cbor.c

+            if (n > SIZE_MAX) {
+                /* Prevent uncaught truncation. */
+                return false;
+            }


The same SIZE_MAX comparison issue exists here. This check will not prevent truncation when casting uint64_t to size_t on systems where they are the same size.

seidelrj · 2025-09-19T17:15:32Z

src/core/cbor.c

+    if (additional_info < 24 || additional_info > 27) {
+        *n = additional_info;
+        return true;
+    }
+
+    *n = 0;
+
+    uint8_t additional_bytes = 1 << (additional_info - 24);
+    for (uint8_t i = 0; i < additional_bytes; i++) {
+        uint8_t byte = 0;
+        if (!n20_istream_get(s, &byte)) {
+            return false;
+        }
+        *n = (*n << 8) | byte;
+    }
+
+    return true;


I think copilot is on to something here. Right now, if you get an additional_data > 27, the current code will mark the header as valid, and return n as a value between 28-31. This will mess up n20_cbor_read_skip_item. E.g., if you had an array type, and additional_data=28, we would say the array had length 28 and skip that many items. We should also return an error on value 31 since we don't support it.

seidelrj · 2025-09-19T17:18:34Z

src/core/cbor.c

+
+    switch (type) {
+        case n20_cbor_type_array_e:
+            for (size_t i = 0; i < n; i++) {


we have the check above to make sure n is <= SIZE_MAX before we get to this for loop.

github-actions · 2025-09-19T22:15:17Z

LCOV of commit `598c17e` during lcov-test-coverage-report #35

Summary coverage rate:
  lines......: 95.2% (1794 of 1884 lines)
  functions..: 99.4% (162 of 163 functions)
  branches...: 83.5% (1000 of 1197 branches)

Files changed coverage rate: n/a

Copilot

Pull Request Overview

Copilot reviewed 5 out of 5 changed files in this pull request and generated 4 comments.

_{Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.}

Copilot · 2025-09-20T00:52:52Z

src/core/test/cbor.cpp

 #include <variant>
 #include <vector>

+#include "gtest/gtest.h"


This include is redundant as gtest/gtest.h is already included at line 17. Remove the duplicate include.

Suggested change

#include "gtest/gtest.h"

Copilot · 2025-09-20T00:52:53Z

src/core/test/cbor.cpp

+#include <sys/types.h>

 #include <cstdint>
+#include <initializer_list>


[nitpick] The includes sys/types.h and initializer_list appear to be unused in the visible code. Consider removing them unless they are used elsewhere in the file.

Suggested change

#include <sys/types.h>

#include <cstdint>

#include <initializer_list>

#include <cstdint>

Copilot · 2025-09-20T00:52:53Z

src/core/test/cbor.cpp

+#include <sys/types.h>

 #include <cstdint>
+#include <initializer_list>


[nitpick] The includes sys/types.h and initializer_list appear to be unused in the visible code. Consider removing them unless they are used elsewhere in the file.

Suggested change

#include <sys/types.h>

#include <cstdint>

#include <initializer_list>

#include <cstdint>

Copilot · 2025-09-20T00:52:53Z

src/core/cbor.c

+            if (n > SIZE_MAX) {
+                /* Prevent overflow in the loop counter. */
+                return false;
+            }


This overflow check has the same issue as above. The condition n > SIZE_MAX will never be true on systems where uint64_t and size_t have the same width. Consider using if (n > SIZE_MAX / 2) or implementing a different overflow prevention strategy.

n20_istream_get_slice now returns a boolean and fills an n20_slice_t structure, which allows for more consistent control flow. This patch also adds the n20_istream_get_string_slice variant, which fills an n20_string_slice_t structure instead. Complete test coverage for stream module.

…rser

seidelrj · 2025-09-23T18:16:59Z

src/core/cbor.c


-    if (additional_info < 24 || additional_info > 27) {
+    if (additional_info > 27) {
+        /* Reserved additional info value. And this code does


Suggested change

/* Reserved additional info value. And this code does

/* Reserved additional info value. And this code does not

seidelrj · 2025-09-23T18:22:51Z

include/nat20/stream.h

+ * @brief Alias for @ref n20_istream_s
+ */
+typedef struct n20_istream_s n20_istream_t;



Suggested change

* specified size and returns a true if the stream is in a good state

* specified size and returns true if the stream is in a good state

seidelrj · 2025-09-23T18:23:35Z

include/nat20/stream.h

+ * the slice_out structure will point to the slice of the stream
+ * buffer beginning at the current read position and of size @p size.
+ *
+ * @param s The input stream to read from.


Suggested change

/** @brief Gets a buffer slice from the input stream.

/** @brief Gets a string slice from the input stream.

…rser

…urm/cbor_parser

werwurm requested review from Copilot and seidelrj September 3, 2025 05:47

This comment was marked as outdated.

Sign in to view

seidelrj requested changes Sep 3, 2025

View reviewed changes

include/nat20/cbor.h Outdated Show resolved Hide resolved

include/nat20/cbor.h Outdated Show resolved Hide resolved

src/core/test/cbor.cpp Outdated Show resolved Hide resolved

src/core/test/cbor.cpp Show resolved Hide resolved

werwurm requested a review from seidelrj September 8, 2025 21:59

werwurm added 6 commits September 10, 2025 08:07

Istream implementation

bdc1727

Similar to n20_stream_t which offers safe writing to a buffer n20_istream_t offers a safe way to read from a buffer lowering the risk for out of bounds access.

fixup comments and istream initialization

d6d0e63

Add CBOR parsing primitives

b01b638

Add primitives for parsing CBOR headers, extracting slices of octet and text strings, and recursively skipping unknown fields.

fix lint

cd3bf39

fix lint

92cbb49

address comments

088e415

werwurm force-pushed the werwurm/istream branch from f9c6e37 to d6d0e63 Compare September 10, 2025 18:12

werwurm force-pushed the werwurm/cbor_parser branch from 96ca001 to 088e415 Compare September 10, 2025 18:13

address comments.

f6aedbe

werwurm requested a review from Copilot September 10, 2025 18:36

Copilot AI reviewed Sep 10, 2025

View reviewed changes

werwurm added 3 commits September 10, 2025 11:37

fix lint

b11c093

Merge branch 'werwurm/istream' into werwurm/cbor_parser

3c62485

address comments.

558e393

werwurm requested a review from Copilot September 10, 2025 18:53

Copilot AI reviewed Sep 10, 2025

View reviewed changes

seidelrj requested changes Sep 19, 2025

View reviewed changes

werwurm added 2 commits September 19, 2025 15:10

Merge branch 'main' into werwurm/istream

45efc1f

Merge branch 'werwurm/istream' into werwurm/cbor_parser

1902e99

werwurm requested a review from a team as a code owner September 19, 2025 22:13

werwurm changed the base branch from werwurm/istream to main September 19, 2025 22:14

Address comments + complete cbor test coverage + fixup docs

e25320b

werwurm requested review from Copilot and seidelrj September 20, 2025 00:51

Copilot AI reviewed Sep 20, 2025

View reviewed changes

werwurm added 4 commits September 19, 2025 19:28

fix lint

8dc77c6

Merge branch 'werwurm/revisit_istream_get_slice' into werwurm/cbor_pa…

503b345

…rser

get slice cbor fixup

3a67c97

werwurm changed the base branch from main to werwurm/revisit_istream_get_slice September 20, 2025 02:39

remove unused/redundant includes

18f45fb

seidelrj requested changes Sep 23, 2025

View reviewed changes

werwurm added 6 commits September 23, 2025 20:15

address comments

da697a7

address comments

4e081fc

Merge branch 'werwurm/revisit_istream_get_slice' into werwurm/cbor_pa…

0a8fdb4

…rser

Merge branch 'main' into werwurm/revisit_istream_get_slice

5f6ddfb

Merge branch 'refs/heads/werwurm/revisit_istream_get_slice' into werw…

3f51d7c

…urm/cbor_parser

Merge branch 'main' into werwurm/cbor_parser

598c17e

werwurm changed the base branch from werwurm/revisit_istream_get_slice to main September 25, 2025 16:24

werwurm requested a review from seidelrj September 25, 2025 16:24

seidelrj approved these changes Sep 25, 2025

View reviewed changes

werwurm merged commit 277a90e into main Sep 25, 2025
7 checks passed

	for (int i = 0; i < additional_bytes; i++) {
	for (uint8_t i = 0; i < additional_bytes; i++) {

		if (n > SIZE_MAX) {
		/* Prevent overflow in the loop counter. */

	/* Reserved additional info value. And this code does
	/* Reserved additional info value. And this code does not

	* specified size and returns a true if the stream is in a good state
	* specified size and returns true if the stream is in a good state

	/** @brief Gets a buffer slice from the input stream.
	/** @brief Gets a string slice from the input stream.

Conversation

werwurm commented Sep 3, 2025

Uh oh!

This comment was marked as outdated.

Uh oh!

seidelrj left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Copilot AI Sep 10, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Sep 10, 2025

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI Sep 10, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Sep 10, 2025

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Uh oh!

Copilot AI Sep 10, 2025

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI Sep 10, 2025

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI Sep 10, 2025

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI Sep 10, 2025

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

github-actions bot commented Sep 19, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

LCOV of commit 598c17e during lcov-test-coverage-report #35

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Uh oh!

Copilot AI Sep 20, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Sep 20, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Sep 20, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Sep 20, 2025

Choose a reason for hiding this comment

github-actions bot commented Sep 19, 2025 •

edited

Loading

LCOV of commit `598c17e` during lcov-test-coverage-report #35