Libcbor integrate #1101

TingDaoK · 2024-04-06T00:24:35Z

Issue #, if available:

Description of changes:

Integrate with libcbor to provide a cbor encode/decode interface based on aws-c-common. Major decision made:
- Decoding will have peek_type/get_next_*, and peek type will cache the next element, while get_next_* will consume the cached one, or just decode the next one if no cache was stored.
- Separate type for indefinite map/array for easier handling
- Separate type for indefinite string/bytes to avoid additional copy (The chunks has to be copied if we want to concatenate them)
Debatable: have the cbor in aws-c-common or aws-c-sdkutils, I put it in aws-c-common mostly for:
- common holds all the external 3rd-party lib for now
- right now, cbor only required by SDKs, but it's still a generic concept not limited to aws.
TODO:
- ~~Conversion from double to float is tricky, numbers like "1.1" cannot be represented by double or float.~~
- ~~Tags, should we be helpful to convert the tag value of timestamp (probably big number in the future) for the user? Or, let user to read the tag value and decide what to do with it??~~
- ~~Error handling. Error code, logging, documentation, etc.~~
- ~~Fuzz test from our side as well to make sure not undefined behavior~~
- Consider:
  - Add separate encoding for float
  - Add decode number to float, decode number to double
  - Add helper to handle indefinite string/bytes

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

This reverts commit 43ac8e5.

codecov-commenter · 2024-04-06T00:27:40Z

Codecov Report

Attention: Patch coverage is 92.10526% with 18 lines in your changes missing coverage. Please review.

Project coverage is 83.46%. Comparing base (e620a23) to head (fd7e45b).

Files	Patch %	Lines
source/cbor.c	91.74%	18 Missing ⚠️

Additional details and impacted files

@@              Coverage Diff              @@
##           cbor-main    #1101      +/-   ##
=============================================
+ Coverage      83.12%   83.46%   +0.33%     
=============================================
  Files             56       57       +1     
  Lines           5755     5982     +227     
=============================================
+ Hits            4784     4993     +209     
- Misses           971      989      +18

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

graebm

I only looked at the encoder part of the public header. Apologies for naive comments, since I haven't looked at the decoder yet, or any implementation details...

include/aws/common/cbor.h

graebm · 2024-04-24T17:49:06Z

include/aws/common/cbor.h

+ * @return AWS_OP_SUCCESS successfully consumed the next element and get the result, otherwise AWS_OP_ERR.
+ */
+AWS_COMMON_API
+int aws_cbor_decoder_pop_next_array_start(struct aws_cbor_decoder *decoder, uint64_t *out_size);


how does this handle indefinite length? document it

still curious

thought we talked about it.

AWS_CBOR_TYPE_INDEF_BYTES_START, AWS_CBOR_TYPE_INDEF_TEXT_START, AWS_CBOR_TYPE_INDEF_ARRAY_START, AWS_CBOR_TYPE_INDEF_MAP_START,

We have separate types for the indef types. I can add notes to the documents.

include/aws/common/cbor.h

graebm · 2024-05-20T20:33:37Z

include/aws/common/cbor.h

+ *  - 31 - AWS_CBOR_TYPE_BREAK
+ *  - result of value not supported.
+ */
+enum aws_cbor_element_type {


maybe just simple enum aws_cbor_type, since all the actual values are just like "AWS_CBOR_TYPE_UINT" anyway and don't bother with the term "element"

include/aws/common/cbor.h

graebm · 2024-05-20T20:45:17Z

include/aws/common/cbor.h

+ * @return AWS_OP_SUCCESS successfully consumed the next element and get the result, otherwise AWS_OP_ERR.
+ */
+AWS_COMMON_API
+int aws_cbor_decoder_pop_next_array_start(struct aws_cbor_decoder *decoder, uint64_t *out_size);


still curious

include/aws/common/macros.h

source/cbor.c

graebm · 2024-05-20T23:46:36Z

source/cbor.c

+int aws_cbor_decoder_consume_next_element(struct aws_cbor_decoder *decoder) {
+    enum aws_cbor_element_type out_type = 0;
+    if (aws_cbor_decoder_peek_type(decoder, &out_type)) {


should we be checking decoder->error_code first thing? or is that always handled first-thing by the helper functions?

aws_cbor_decoder_peek_type will check the error code as the first thing.

source/cbor.c

graebm · 2024-05-21T16:45:50Z

source/cbor.c

+            /* TODO: How to check overflow? */
+            double ms_double = timestamp_secs * 1000.0;
+            /* Round it up instead of type cast incase of overflow */
+            int64_t timestamp_ms = llround(ms_double);


I don't know off the top of my head the right way to check, but here's the docs:
https://en.cppreference.com/w/c/numeric/math/round

graebm · 2024-05-21T16:47:00Z

source/cbor.c

+            break;
+        }
+        default:
+            AWS_LOGF_ERROR(AWS_LS_COMMON_CBOR, "The cbor data is malformed to decode.");


better error message about how the type isn't expected in a timestamp

same as the encoding function, I am currently more leaning towards on removing this helper.

graebm · 2024-05-21T17:19:30Z

source/cbor.c

+    switch (decoder->cached_context.type) {
+        case AWS_CBOR_TYPE_TAG:
+            /* Read the next data item */
+            /* TODO: error check for the tag content?? */


Should we do this before commit, or will there be a follow-up?

arguably... It's complicate to 100% ensure the tag value is legal.

https://www.rfc-editor.org/rfc/rfc8949.html#tags

We need to check for all those tag values and check those data as well, but we are not even reading those data. Not sure it's worth the effort to ensure we just want to ignores the data item.

eg: if someone sends a bigfloat with an array with 2 string.

Oh, I have a notes here, maybe I'll just remove this TODO.

include/aws/common/cbor.h

source/cbor.c

TingDaoK added 18 commits March 28, 2024 13:30

import libcbor

2182033

try disable the warning

4d2e689

more warning

ad77931

don't export anything

43ac8e5

Revert "don't export anything"

172aee1

This reverts commit 43ac8e5.

define cbor export to do nothing

83bd5e6

oh gosh, we are not building it...

a6cdd6a

update the script

466c532

ignore one more warnings

54b2edd

ignore the unknown pragmas for windows as well

335d6ac

conversion warnings

2f74c49

update the comments from the script

ea5dad0

WIP

ba29627

encoder and decoder for nicer api

ad19e38

address comments

2a2712c

more

e7df03b

Merge branch 'import-libcbor' into libcbor-intergrate

a031467

impl of encoder and decoder.

fafd1fb

TingDaoK added 11 commits April 8, 2024 15:06

more tests to add

f1ee3c5

oops, half done

a2cff4f

more test and type cast

a945806

disable clang format for external lib

fe4a98b

exclude the files from external

7b6d691

macro format

d6f31dc

more

ae754c1

support both tag directly and helper

ba171bf

convert before check for overflow

fc49a46

woops

3721d1b

add documentation and logs and error code

a2afab6

TingDaoK added 2 commits April 15, 2024 15:09

renaming

3859f27

add another fuzz test

6ee5227

DmitriyMusatkin approved these changes Apr 18, 2024

View reviewed changes

graebm reviewed Apr 23, 2024

View reviewed changes

TingDaoK added 5 commits April 23, 2024 17:06

address comments

1d957a4

more warnings

587aeab

clang is different

7335306

import

fd917a1

Merge branch 'main' into import-libcbor

1214454

graebm changed the title ~~Libcbor intergrate~~ Libcbor integrate Apr 24, 2024

graebm reviewed Apr 24, 2024

View reviewed changes

TingDaoK added 3 commits April 24, 2024 14:26

address comments

e824bbe

woops

a604924

Merge branch 'import-libcbor' into libcbor-intergrate

64cf7f3

Base automatically changed from import-libcbor to cbor-main May 20, 2024 16:12

TingDaoK added 2 commits May 20, 2024 09:13

Merge branch 'cbor-main' into libcbor-intergrate

e1695ae

comments missed

97589ab

graebm reviewed May 20, 2024

View reviewed changes

graebm reviewed May 21, 2024

View reviewed changes

TingDaoK added 6 commits May 21, 2024 15:56

comments addressed

1c69aa7

add notes for handling indefinite length

af478e4

more address comment

7afee4d

reset

2e073de

Merge branch 'cbor-main' into libcbor-intergrate

809a296

renaming and more

fd7e45b

graebm approved these changes Jun 5, 2024

View reviewed changes

TingDaoK merged commit a3feb8f into cbor-main Jun 6, 2024

TingDaoK deleted the libcbor-intergrate branch June 6, 2024 22:45

This was referenced Jun 19, 2024

Cbor support #1127

Closed

cbor support #1131

Merged

Libcbor integrate #1101

Libcbor integrate #1101

Uh oh!

Conversation

TingDaoK commented Apr 6, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov-commenter commented Apr 6, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

graebm left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

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!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

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!

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!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

TingDaoK commented Apr 6, 2024 •

edited

Loading

codecov-commenter commented Apr 6, 2024 •

edited

Loading