You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@mynewt.apache.org by GitBox <gi...@apache.org> on 2020/01/24 10:22:20 UTC
[GitHub] [mynewt-core] de-nordic opened a new pull request #2165: FIx:
advance_internal should return error instead of using assert
de-nordic opened a new pull request #2165: FIx: advance_internal should return error instead of using assert
URL: https://github.com/apache/mynewt-core/pull/2165
Assert may evaluate to empty expression which means that an error removed
from call to extract_number may get missed. It is better to return it,
and let it be processed further.
Signed-off-by: Dominik Ermel <do...@nordicsemi.no>
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [mynewt-core] de-nordic commented on issue #2165: FIx:
advance_internal should return error instead of using assert
Posted by GitBox <gi...@apache.org>.
de-nordic commented on issue #2165: FIx: advance_internal should return error instead of using assert
URL: https://github.com/apache/mynewt-core/pull/2165#issuecomment-603168639
Abandoned.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [mynewt-core] sjanc removed a comment on issue #2165: FIx:
advance_internal should return error instead of using assert
Posted by GitBox <gi...@apache.org>.
sjanc removed a comment on issue #2165: FIx: advance_internal should return error instead of using assert
URL: https://github.com/apache/mynewt-core/pull/2165#issuecomment-596503934
hi, looks ok to me but you may have to rebase to fix CI issiues
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [mynewt-core] utzig commented on a change in pull request #2165:
FIx: advance_internal should return error instead of using assert
Posted by GitBox <gi...@apache.org>.
utzig commented on a change in pull request #2165: FIx: advance_internal should return error instead of using assert
URL: https://github.com/apache/mynewt-core/pull/2165#discussion_r370594942
##########
File path: encoding/tinycbor/src/cborparser.c
##########
@@ -279,7 +279,9 @@ static CborError advance_internal(CborValue *it)
{
uint64_t length;
CborError err = extract_number(it->parser, &it->offset, &length);
- assert(err == CborNoError);
+ if (err == CborNoError) {
+ return err;
+ }
Review comment:
I was trying to follow the unroll of this error to check that it is handled somewhere up in the call chain, it seems to be OK. But the same assert is also being checked inside `cbor_value_enter_container`, should not that one be fixed as well, for consistency?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [mynewt-core] de-nordic closed pull request #2165: FIx:
advance_internal should return error instead of using assert
Posted by GitBox <gi...@apache.org>.
de-nordic closed pull request #2165: FIx: advance_internal should return error instead of using assert
URL: https://github.com/apache/mynewt-core/pull/2165
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [mynewt-core] apache-mynewt-bot commented on issue #2165: FIx:
advance_internal should return error instead of using assert
Posted by GitBox <gi...@apache.org>.
apache-mynewt-bot commented on issue #2165: FIx: advance_internal should return error instead of using assert
URL: https://github.com/apache/mynewt-core/pull/2165#issuecomment-578080533
<!-- style-bot -->
## Style check summary
#### encoding/tinycbor/src/cborparser.c
<details>
```diff
@@ -302,7 +313,8 @@
* point values (SinglePrecisionFloat == Value32Bit and DoublePrecisionFloat ==
* Value64Bit).
*/
-uint64_t _cbor_value_decode_int64_internal(const CborValue *value)
+uint64_t
+_cbor_value_decode_int64_internal(const CborValue *value)
{
uint8_t val = value->parser->d->get8(value->parser->d, value->offset);
@@ -312,11 +324,12 @@
/* since the additional information can only be Value32Bit or Value64Bit,
* we just need to test for the one bit those two options differ */
assert((val & SmallValueMask) == Value32Bit || (val & SmallValueMask) == Value64Bit);
- if ((val & 1) == (Value32Bit & 1))
+ if ((val & 1) == (Value32Bit & 1)) {
return value->parser->d->get32(value->parser->d, value->offset + 1);
+ }
assert((val & SmallValueMask) == Value64Bit);
- return value->parser->d->get64(value->parser->d, value->offset + 1);
+ return value->parser->d->get64(value->parser->d, value->offset + 1);
}
/**
@@ -329,8 +342,9 @@
* threads iterating at the same time, but the object can be copied so multiple
* threads can iterate.
*/
-CborError cbor_parser_init(struct cbor_decoder_reader *d, int flags,
- CborParser *parser, CborValue *it)
+CborError
+cbor_parser_init(struct cbor_decoder_reader *d, int flags,
+ CborParser *parser, CborValue *it)
{
memset(parser, 0, sizeof(*parser));
parser->d = d;
@@ -400,19 +414,23 @@
*
* \sa cbor_value_at_end(), cbor_value_advance(), cbor_value_enter_container(), cbor_value_leave_container()
*/
-CborError cbor_value_advance_fixed(CborValue *it)
+CborError
+cbor_value_advance_fixed(CborValue *it)
{
assert(it->type != CborInvalidType);
assert(is_fixed_type(it->type));
- if (!it->remaining)
+ if (!it->remaining) {
return CborErrorAdvancePastEOF;
+ }
return advance_internal(it);
}
-static CborError advance_recursive(CborValue *it, int nestingLevel)
-{
- if (is_fixed_type(it->type))
+static CborError
+advance_recursive(CborValue *it, int nestingLevel)
+{
+ if (is_fixed_type(it->type)) {
return advance_internal(it);
+ }
if (!cbor_value_is_container(it)) {
size_t len = SIZE_MAX;
@@ -420,18 +438,21 @@
}
/* map or array */
- if (nestingLevel == CBOR_PARSER_MAX_RECURSIONS)
+ if (nestingLevel == CBOR_PARSER_MAX_RECURSIONS) {
return CborErrorNestingTooDeep;
+ }
CborError err;
CborValue recursed;
err = cbor_value_enter_container(it, &recursed);
- if (err)
+ if (err) {
return err;
+ }
while (!cbor_value_at_end(&recursed)) {
err = advance_recursive(&recursed, nestingLevel + 1);
- if (err)
+ if (err) {
return err;
+ }
}
return cbor_value_leave_container(it, &recursed);
}
@@ -448,11 +469,13 @@
*
* \sa cbor_value_at_end(), cbor_value_advance_fixed(), cbor_value_enter_container(), cbor_value_leave_container()
*/
-CborError cbor_value_advance(CborValue *it)
+CborError
+cbor_value_advance(CborValue *it)
{
assert(it->type != CborInvalidType);
- if (!it->remaining)
+ if (!it->remaining) {
return CborErrorAdvancePastEOF;
+ }
return advance_recursive(it, 0);
}
@@ -484,12 +507,14 @@
*
* \sa cbor_value_advance_fixed(), cbor_value_advance()
*/
-CborError cbor_value_skip_tag(CborValue *it)
+CborError
+cbor_value_skip_tag(CborValue *it)
{
while (cbor_value_is_tag(it)) {
CborError err = cbor_value_advance_fixed(it);
- if (err)
+ if (err) {
return err;
+ }
}
return CborNoError;
}
@@ -511,7 +536,8 @@
*
* \sa cbor_value_is_container(), cbor_value_leave_container(), cbor_value_advance()
*/
-CborError cbor_value_enter_container(const CborValue *it, CborValue *recursed)
+CborError
+cbor_value_enter_container(const CborValue *it, CborValue *recursed)
{
CborError err;
assert(cbor_value_is_container(it));
@@ -521,8 +547,9 @@
recursed->remaining = UINT32_MAX;
++recursed->offset;
err = preparse_value(recursed);
- if (err != CborErrorUnexpectedBreak)
+ if (err != CborErrorUnexpectedBreak) {
return err;
+ }
/* actually, break was expected here
* it's just an empty container */
++recursed->offset;
@@ -546,8 +573,9 @@
}
recursed->remaining *= 2;
}
- if (len != 0)
+ if (len != 0) {
return preparse_value(recursed);
+ }
}
/* the case of the empty container */
@@ -568,7 +596,8 @@
*
* \sa cbor_value_enter_container(), cbor_value_at_end()
*/
-CborError cbor_value_leave_container(CborValue *it, const CborValue *recursed)
+CborError
+cbor_value_leave_container(CborValue *it, const CborValue *recursed)
{
assert(cbor_value_is_container(it));
assert(recursed->type == CborInvalidType);
@@ -750,7 +779,8 @@
*
* \sa cbor_value_get_type(), cbor_value_is_valid(), cbor_value_is_integer(), cbor_value_get_int64()
*/
-CborError cbor_value_get_int64_checked(const CborValue *value, int64_t *result)
+CborError
+cbor_value_get_int64_checked(const CborValue *value, int64_t *result)
{
assert(cbor_value_is_integer(value));
uint64_t v = _cbor_value_extract_int64_helper(value);
@@ -765,12 +795,14 @@
* type).
*/
- if (unlikely(v > (uint64_t)INT64_MAX))
+ if (unlikely(v > (uint64_t)INT64_MAX)) {
return CborErrorDataTooLarge;
+ }
*result = v;
- if (value->flags & CborIteratorFlag_NegativeInteger)
+ if (value->flags & CborIteratorFlag_NegativeInteger) {
*result = -*result - 1;
+ }
return CborNoError;
}
@@ -789,7 +821,8 @@
* \sa cbor_value_get_type(), cbor_value_is_valid(), cbor_value_is_integer(), cbor_value_get_int64(),
* cbor_value_get_uint64(), cbor_value_get_int64_checked(), cbor_value_get_raw_integer()
*/
-CborError cbor_value_get_int_checked(const CborValue *value, int *result)
+CborError
+cbor_value_get_int_checked(const CborValue *value, int *result)
{
assert(cbor_value_is_integer(value));
uint64_t v = _cbor_value_extract_int64_helper(value);
@@ -805,14 +838,16 @@
*/
if (value->flags & CborIteratorFlag_NegativeInteger) {
- if (unlikely(v > (unsigned) -(INT_MIN + 1)))
+ if (unlikely(v > (unsigned) -(INT_MIN + 1))) {
return CborErrorDataTooLarge;
+ }
*result = v;
*result = -*result - 1;
} else {
- if (unlikely(v > (uint64_t)INT_MAX))
+ if (unlikely(v > (uint64_t)INT_MAX)) {
return CborErrorDataTooLarge;
+ }
*result = v;
}
@@ -899,7 +934,8 @@
*
* \sa cbor_value_get_string_length(), cbor_value_copy_string(), cbor_value_is_length_known()
*/
-CborError cbor_value_calculate_string_length(const CborValue *value, size_t *len)
+CborError
+cbor_value_calculate_string_length(const CborValue *value, size_t *len)
{
*len = SIZE_MAX;
return _cbor_value_copy_string(value, NULL, len, NULL);
@@ -911,7 +947,8 @@
* only. */
typedef uintptr_t (*IterateFunction)(struct cbor_decoder_reader *d, char *dst, int src_offset, size_t len);
-static uintptr_t iterate_noop(struct cbor_decoder_reader *d, char *dst, int src_offset, size_t len)
+static uintptr_t
+iterate_noop(struct cbor_decoder_reader *d, char *dst, int src_offset, size_t len)
{
(void)d;
(void)dst;
@@ -920,8 +957,9 @@
return true;
}
-static CborError iterate_string_chunks(const CborValue *value, char *buffer, size_t *buflen,
- bool *result, CborValue *next, IterateFunction func)
+static CborError
+iterate_string_chunks(const CborValue *value, char *buffer, size_t *buflen,
+ bool *result, CborValue *next, IterateFunction func)
{
assert(cbor_value_is_byte_string(value) || cbor_value_is_text_string(value));
@@ -931,14 +969,17 @@
if (cbor_value_is_length_known(value)) {
/* easy case: fixed length */
err = extract_length(value->parser, &offset, &total);
- if (err)
+ if (err) {
return err;
- if (total > (size_t)(value->parser->end - offset))
+ }
+ if (total > (size_t)(value->parser->end - offset)) {
return CborErrorUnexpectedEOF;
- if (total <= *buflen)
+ }
+ if (total <= *buflen) {
*result = !!func(value->parser->d, buffer, offset, total);
- else
+ } else {
*result = false;
+ }
offset += total;
} else {
/* chunked */
@@ -950,8 +991,9 @@
size_t chunkLen;
size_t newTotal;
- if (offset == value->parser->end)
+ if (offset == value->parser->end) {
return CborErrorUnexpectedEOF;
+ }
val = value->parser->d->get8(value->parser->d, offset);
@@ -961,23 +1003,28 @@
}
/* is this the right type? */
- if ((val & MajorTypeMask) != value->type)
+ if ((val & MajorTypeMask) != value->type) {
return CborErrorIllegalType;
+ }
err = extract_length(value->parser, &offset, &chunkLen);
- if (err)
+ if (err) {
return err;
-
- if (unlikely(add_check_overflow(total, chunkLen, &newTotal)))
+ }
+
+ if (unlikely(add_check_overflow(total, chunkLen, &newTotal))) {
return CborErrorDataTooLarge;
-
- if (chunkLen > (size_t)(value->parser->end - offset))
+ }
+
+ if (chunkLen > (size_t)(value->parser->end - offset)) {
return CborErrorUnexpectedEOF;
-
- if (*result && *buflen >= newTotal)
+ }
+
+ if (*result && *buflen >= newTotal) {
*result = !!func(value->parser->d, buffer + total, offset, chunkLen);
- else
+ } else {
*result = false;
+ }
offset += chunkLen;
total = newTotal;
@@ -990,7 +1037,7 @@
* because this is called by function pointer with an abstract
* reader. Since this is the output buffer, we can assume that if
* we have a valid buffer its ok to write a NULL here */
- if(buffer) {
+ if (buffer) {
*(buffer + total) = '\0';
}
}
@@ -1069,14 +1116,15 @@
* \sa cbor_value_dup_text_string(), cbor_value_copy_text_string(), cbor_value_get_string_length(), cbor_value_calculate_string_length()
*/
-CborError _cbor_value_copy_string(const CborValue *value, void *buffer,
- size_t *buflen, CborValue *next)
+CborError
+_cbor_value_copy_string(const CborValue *value, void *buffer,
+ size_t *buflen, CborValue *next)
{
bool copied_all;
- CborError err = iterate_string_chunks(value, (char*)buffer, buflen, &copied_all, next,
+ CborError err = iterate_string_chunks(value, (char *)buffer, buflen, &copied_all, next,
buffer ? (IterateFunction) value->parser->d->cpy : iterate_noop);
return err ? err :
- copied_all ? CborNoError : CborErrorOutOfMemory;
+ copied_all ? CborNoError : CborErrorOutOfMemory;
}
/**
@@ -1097,12 +1145,14 @@
*
* \sa cbor_value_skip_tag(), cbor_value_copy_text_string()
*/
-CborError cbor_value_text_string_equals(const CborValue *value, const char *string, bool *result)
+CborError
+cbor_value_text_string_equals(const CborValue *value, const char *string, bool *result)
{
CborValue copy = *value;
CborError err = cbor_value_skip_tag(©);
- if (err)
+ if (err) {
return err;
+ }
if (!cbor_value_is_text_string(©)) {
*result = false;
return CborNoError;
@@ -1185,42 +1235,50 @@
*
* \sa cbor_value_is_valid(), cbor_value_text_string_equals(), cbor_value_advance()
*/
-CborError cbor_value_map_find_value(const CborValue *map, const char *string, CborValue *element)
+CborError
+cbor_value_map_find_value(const CborValue *map, const char *string, CborValue *element)
{
assert(cbor_value_is_map(map));
size_t len = strlen(string);
CborError err = cbor_value_enter_container(map, element);
- if (err)
+ if (err) {
goto error;
+ }
while (!cbor_value_at_end(element)) {
/* find the non-tag so we can compare */
err = cbor_value_skip_tag(element);
- if (err)
+ if (err) {
goto error;
+ }
if (cbor_value_is_text_string(element)) {
bool equals;
size_t dummyLen = len;
err = iterate_string_chunks(element, CONST_CAST(char *, string), &dummyLen,
&equals, element, map->parser->d->cmp);
- if (err)
+ if (err) {
goto error;
- if (equals)
+ }
+ if (equals) {
return preparse_value(element);
+ }
} else {
/* skip this key */
err = cbor_value_advance(element);
- if (err)
+ if (err) {
goto error;
+ }
}
/* skip this value */
err = cbor_value_skip_tag(element);
- if (err)
+ if (err) {
goto error;
+ }
err = cbor_value_advance(element);
- if (err)
+ if (err) {
goto error;
+ }
}
/* not found */
@@ -1296,7 +1354,8 @@
*
* \sa cbor_value_get_type(), cbor_value_is_valid(), cbor_value_is_half_float(), cbor_value_get_float()
*/
-CborError cbor_value_get_half_float(const CborValue *value, void *result)
+CborError
+cbor_value_get_half_float(const CborValue *value, void *result)
{
assert(cbor_value_is_half_float(value));
```
</details>
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [mynewt-core] de-nordic commented on issue #2165: FIx:
advance_internal should return error instead of using assert
Posted by GitBox <gi...@apache.org>.
de-nordic commented on issue #2165: FIx: advance_internal should return error instead of using assert
URL: https://github.com/apache/mynewt-core/pull/2165#issuecomment-578073217
For example, in zephyr this evaluates to empty expression which will make code proceed as if in case of success.
Additionally it cause compilation to warn about unused err variable.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [mynewt-core] sjanc commented on a change in pull request #2165:
FIx: advance_internal should return error instead of using assert
Posted by GitBox <gi...@apache.org>.
sjanc commented on a change in pull request #2165: FIx: advance_internal should return error instead of using assert
URL: https://github.com/apache/mynewt-core/pull/2165#discussion_r389637997
##########
File path: encoding/tinycbor/src/cborparser.c
##########
@@ -279,7 +279,9 @@ static CborError advance_internal(CborValue *it)
{
uint64_t length;
CborError err = extract_number(it->parser, &it->offset, &length);
- assert(err == CborNoError);
+ if (err == CborNoError) {
+ return err;
+ }
Review comment:
hmm shouldn't if be != and not == ?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [mynewt-core] de-nordic commented on a change in pull request
#2165: FIx: advance_internal should return error instead of using assert
Posted by GitBox <gi...@apache.org>.
de-nordic commented on a change in pull request #2165: FIx: advance_internal should return error instead of using assert
URL: https://github.com/apache/mynewt-core/pull/2165#discussion_r370595796
##########
File path: encoding/tinycbor/src/cborparser.c
##########
@@ -279,7 +279,9 @@ static CborError advance_internal(CborValue *it)
{
uint64_t length;
CborError err = extract_number(it->parser, &it->offset, &length);
- assert(err == CborNoError);
+ if (err == CborNoError) {
+ return err;
+ }
Review comment:
Yes. It should.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [mynewt-core] sjanc commented on issue #2165: FIx: advance_internal
should return error instead of using assert
Posted by GitBox <gi...@apache.org>.
sjanc commented on issue #2165: FIx: advance_internal should return error instead of using assert
URL: https://github.com/apache/mynewt-core/pull/2165#issuecomment-596503934
hi, looks ok to me but you may have to rebase to fix CI issiues
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [mynewt-core] utzig commented on a change in pull request #2165:
FIx: advance_internal should return error instead of using assert
Posted by GitBox <gi...@apache.org>.
utzig commented on a change in pull request #2165: FIx: advance_internal should return error instead of using assert
URL: https://github.com/apache/mynewt-core/pull/2165#discussion_r370594942
##########
File path: encoding/tinycbor/src/cborparser.c
##########
@@ -279,7 +279,9 @@ static CborError advance_internal(CborValue *it)
{
uint64_t length;
CborError err = extract_number(it->parser, &it->offset, &length);
- assert(err == CborNoError);
+ if (err == CborNoError) {
+ return err;
+ }
Review comment:
I was trying to follow the unroll of this error up to check that it is handled somewhere up in the call chain, it seems to be OK. But the same assert is also being checked inside `cbor_value_enter_container`, should not that one be fixed as well, for consistency?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services