H A D | msgbuf.cpp | 2ff8cf89 Fri May 17 00:50:46 CDT 2024 Andrew Jeffery <andrew@codeconstruct.com.au> msgbuf: Remove use of ssize_t for overflow tracking
There are a few concerns with the use of ssize_t in this context:
1. It's defined by POSIX and not C, and I'd prefer we not require POSIX concepts where we can avoid it 2. ssize_t is defined over [-1, SSIZE_MAX] - it is not defined to have the range of a regular signed type.
The source of both these statements is The Open Group Base Specifications Issue 7, 2018 edition. IEEE Std 1003.1-2017 (Revision of IEEE Std 1003.1-2008)
The second point directly contradicts how I was trying to use ssize_t in the msgbuf implementation. As a result, switch the type of `remaining` to intmax_t. Usually intmax_t is a problem child, but it's not used in any public API, and it has the semantics I wanted by contrast to the definition of ssize_t.
Note that we add assert() calls where we know the value of remaining must be negative. Without the addition of the `assert()` calls in the underflow checks, clang-analyzer gets tripped up by not being able to prove `INTMAX_MIN + (intmax_t)sizeof(uint16_t) < 0`:
``` ../src/platform.c:17:18: error: The left operand of '+' is a garbage value [clang-analyzer-core.UndefinedBinaryOperatorResult,-warnings-as-errors] 17 | if (ctx->length + sizeof(*ctx) < lower) { | ^ ../src/platform.c:2445:6: note: 'rc' is 0 2445 | if (rc) { | ^~ ../src/platform.c:2445:2: note: Taking false branch 2445 | if (rc) { | ^ ../src/platform.c:2449:7: note: Calling 'pldm_msgbuf_extract_value_pdr_hdr' 2449 | rc = pldm_msgbuf_extract_value_pdr_hdr(buf, &hdr); | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ ../src/msgbuf/platform.h:17:2: note: Calling 'pldm__msgbuf_extract_uint16' 17 | pldm_msgbuf_extract(ctx, hdr->length); | ^ ../src/msgbuf/../msgbuf.h:517:2: note: expanded from macro 'pldm_msgbuf_extract' 517 | _Generic((dst), \ | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ 518 | uint8_t: pldm__msgbuf_extract_uint8, \ | ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ 519 | int8_t: pldm__msgbuf_extract_int8, \ | ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ 520 | uint16_t: pldm__msgbuf_extract_uint16, \ | ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ 521 | int16_t: pldm__msgbuf_extract_int16, \ | ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ 522 | uint32_t: pldm__msgbuf_extract_uint32, \ | ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ 523 | int32_t: pldm__msgbuf_extract_int32, \ | ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ 524 | real32_t: pldm__msgbuf_extract_real32)(ctx, (void *)&(dst)) | ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ ../src/msgbuf/../msgbuf.h:341:7: note: 'ctx' is non-null 341 | if (!ctx || !ctx->cursor || !dst) { | ^~~ ../src/msgbuf/../msgbuf.h:341:6: note: Left side of '||' is false 341 | if (!ctx || !ctx->cursor || !dst) { | ^ ../src/msgbuf/../msgbuf.h:341:20: note: Field 'cursor' is non-null 341 | if (!ctx || !ctx->cursor || !dst) { | ^ ../src/msgbuf/../msgbuf.h:341:6: note: Left side of '||' is false 341 | if (!ctx || !ctx->cursor || !dst) { | ^ ../src/msgbuf/../msgbuf.h:341:31: note: 'dst' is non-null 341 | if (!ctx || !ctx->cursor || !dst) { | ^~~ ../src/msgbuf/../msgbuf.h:341:2: note: Taking false branch 341 | if (!ctx || !ctx->cursor || !dst) { | ^ ../src/msgbuf/../msgbuf.h:347:2: note: Taking true branch 347 | if (ctx->remaining < INTMAX_MIN + (intmax_t)sizeof(ldst)) { | ^ ../src/msgbuf/../msgbuf.h:348:3: note: Returning without writing to '*dst' 348 | return PLDM_ERROR_INVALID_LENGTH; | ^ ../src/msgbuf/platform.h:17:2: note: Returning from 'pldm__msgbuf_extract_uint16' 17 | pldm_msgbuf_extract(ctx, hdr->length); | ^ ../src/msgbuf/../msgbuf.h:517:2: note: expanded from macro 'pldm_msgbuf_extract' 517 | _Generic((dst), \ | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ 518 | uint8_t: pldm__msgbuf_extract_uint8, \ | ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ 519 | int8_t: pldm__msgbuf_extract_int8, \ | ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ 520 | uint16_t: pldm__msgbuf_extract_uint16, \ | ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ 521 | int16_t: pldm__msgbuf_extract_int16, \ | ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ 522 | uint32_t: pldm__msgbuf_extract_uint32, \ | ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ 523 | int32_t: pldm__msgbuf_extract_int32, \ | ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ 524 | real32_t: pldm__msgbuf_extract_real32)(ctx, (void *)&(dst)) | ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ ../src/msgbuf/platform.h:19:2: note: Returning without writing to 'hdr->length' 19 | return pldm_msgbuf_validate(ctx); | ^ ../src/platform.c:2449:7: note: Returning from 'pldm_msgbuf_extract_value_pdr_hdr' 2449 | rc = pldm_msgbuf_extract_value_pdr_hdr(buf, &hdr); | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ ../src/platform.c:2450:6: note: 'rc' is 0 2450 | if (rc) { | ^~ ../src/platform.c:2450:2: note: Taking false branch 2450 | if (rc) { | ^ ../src/platform.c:2454:7: note: Calling 'pldm_platform_pdr_hdr_validate' 2454 | rc = pldm_platform_pdr_hdr_validate( | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ 2455 | &hdr, PLDM_PDR_NUMERIC_EFFECTER_PDR_MIN_LENGTH, | ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ 2456 | pdr_data_length); | ~~~~~~~~~~~~~~~~ ../src/platform.c:17:18: note: The left operand of '+' is a garbage value 17 | if (ctx->length + sizeof(*ctx) < lower) { | ~~~~~~~~~~~ ^ ```
Change-Id: Idbe5a14455ad677a39c8f535eddd9c2ce471c783 Signed-off-by: Andrew Jeffery <andrew@codeconstruct.com.au>
|
H A D | msgbuf.h | 2ff8cf89 Fri May 17 00:50:46 CDT 2024 Andrew Jeffery <andrew@codeconstruct.com.au> msgbuf: Remove use of ssize_t for overflow tracking
There are a few concerns with the use of ssize_t in this context:
1. It's defined by POSIX and not C, and I'd prefer we not require POSIX concepts where we can avoid it 2. ssize_t is defined over [-1, SSIZE_MAX] - it is not defined to have the range of a regular signed type.
The source of both these statements is The Open Group Base Specifications Issue 7, 2018 edition. IEEE Std 1003.1-2017 (Revision of IEEE Std 1003.1-2008)
The second point directly contradicts how I was trying to use ssize_t in the msgbuf implementation. As a result, switch the type of `remaining` to intmax_t. Usually intmax_t is a problem child, but it's not used in any public API, and it has the semantics I wanted by contrast to the definition of ssize_t.
Note that we add assert() calls where we know the value of remaining must be negative. Without the addition of the `assert()` calls in the underflow checks, clang-analyzer gets tripped up by not being able to prove `INTMAX_MIN + (intmax_t)sizeof(uint16_t) < 0`:
``` ../src/platform.c:17:18: error: The left operand of '+' is a garbage value [clang-analyzer-core.UndefinedBinaryOperatorResult,-warnings-as-errors] 17 | if (ctx->length + sizeof(*ctx) < lower) { | ^ ../src/platform.c:2445:6: note: 'rc' is 0 2445 | if (rc) { | ^~ ../src/platform.c:2445:2: note: Taking false branch 2445 | if (rc) { | ^ ../src/platform.c:2449:7: note: Calling 'pldm_msgbuf_extract_value_pdr_hdr' 2449 | rc = pldm_msgbuf_extract_value_pdr_hdr(buf, &hdr); | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ ../src/msgbuf/platform.h:17:2: note: Calling 'pldm__msgbuf_extract_uint16' 17 | pldm_msgbuf_extract(ctx, hdr->length); | ^ ../src/msgbuf/../msgbuf.h:517:2: note: expanded from macro 'pldm_msgbuf_extract' 517 | _Generic((dst), \ | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ 518 | uint8_t: pldm__msgbuf_extract_uint8, \ | ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ 519 | int8_t: pldm__msgbuf_extract_int8, \ | ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ 520 | uint16_t: pldm__msgbuf_extract_uint16, \ | ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ 521 | int16_t: pldm__msgbuf_extract_int16, \ | ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ 522 | uint32_t: pldm__msgbuf_extract_uint32, \ | ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ 523 | int32_t: pldm__msgbuf_extract_int32, \ | ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ 524 | real32_t: pldm__msgbuf_extract_real32)(ctx, (void *)&(dst)) | ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ ../src/msgbuf/../msgbuf.h:341:7: note: 'ctx' is non-null 341 | if (!ctx || !ctx->cursor || !dst) { | ^~~ ../src/msgbuf/../msgbuf.h:341:6: note: Left side of '||' is false 341 | if (!ctx || !ctx->cursor || !dst) { | ^ ../src/msgbuf/../msgbuf.h:341:20: note: Field 'cursor' is non-null 341 | if (!ctx || !ctx->cursor || !dst) { | ^ ../src/msgbuf/../msgbuf.h:341:6: note: Left side of '||' is false 341 | if (!ctx || !ctx->cursor || !dst) { | ^ ../src/msgbuf/../msgbuf.h:341:31: note: 'dst' is non-null 341 | if (!ctx || !ctx->cursor || !dst) { | ^~~ ../src/msgbuf/../msgbuf.h:341:2: note: Taking false branch 341 | if (!ctx || !ctx->cursor || !dst) { | ^ ../src/msgbuf/../msgbuf.h:347:2: note: Taking true branch 347 | if (ctx->remaining < INTMAX_MIN + (intmax_t)sizeof(ldst)) { | ^ ../src/msgbuf/../msgbuf.h:348:3: note: Returning without writing to '*dst' 348 | return PLDM_ERROR_INVALID_LENGTH; | ^ ../src/msgbuf/platform.h:17:2: note: Returning from 'pldm__msgbuf_extract_uint16' 17 | pldm_msgbuf_extract(ctx, hdr->length); | ^ ../src/msgbuf/../msgbuf.h:517:2: note: expanded from macro 'pldm_msgbuf_extract' 517 | _Generic((dst), \ | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ 518 | uint8_t: pldm__msgbuf_extract_uint8, \ | ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ 519 | int8_t: pldm__msgbuf_extract_int8, \ | ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ 520 | uint16_t: pldm__msgbuf_extract_uint16, \ | ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ 521 | int16_t: pldm__msgbuf_extract_int16, \ | ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ 522 | uint32_t: pldm__msgbuf_extract_uint32, \ | ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ 523 | int32_t: pldm__msgbuf_extract_int32, \ | ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ 524 | real32_t: pldm__msgbuf_extract_real32)(ctx, (void *)&(dst)) | ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ ../src/msgbuf/platform.h:19:2: note: Returning without writing to 'hdr->length' 19 | return pldm_msgbuf_validate(ctx); | ^ ../src/platform.c:2449:7: note: Returning from 'pldm_msgbuf_extract_value_pdr_hdr' 2449 | rc = pldm_msgbuf_extract_value_pdr_hdr(buf, &hdr); | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ ../src/platform.c:2450:6: note: 'rc' is 0 2450 | if (rc) { | ^~ ../src/platform.c:2450:2: note: Taking false branch 2450 | if (rc) { | ^ ../src/platform.c:2454:7: note: Calling 'pldm_platform_pdr_hdr_validate' 2454 | rc = pldm_platform_pdr_hdr_validate( | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ 2455 | &hdr, PLDM_PDR_NUMERIC_EFFECTER_PDR_MIN_LENGTH, | ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ 2456 | pdr_data_length); | ~~~~~~~~~~~~~~~~ ../src/platform.c:17:18: note: The left operand of '+' is a garbage value 17 | if (ctx->length + sizeof(*ctx) < lower) { | ~~~~~~~~~~~ ^ ```
Change-Id: Idbe5a14455ad677a39c8f535eddd9c2ce471c783 Signed-off-by: Andrew Jeffery <andrew@codeconstruct.com.au>
|