platform: Fix LE encoding of present_reading
The following portion of the test implementation demonstrates
significant confusion in the handling of endiannes:
```
uint32_t presentReading = 305441741;
...
sensorData->present_reading[3] =
static_cast<uint8_t>(htole32(presentReading) & (0x000000ff));
sensorData->present_reading[2] =
static_cast<uint8_t>((htole32(presentReading) & (0x0000ff00)) >> 8);
sensorData->present_reading[1] =
static_cast<uint8_t>((htole32(presentReading) & (0x00ff0000)) >> 16);
sensorData->present_reading[0] =
static_cast<uint8_t>((htole32(presentReading) & (0xff000000)) >> 24);
```
First up, plugging 305441741 into a calculator an converting it to
hexadecimal representation reveals the the value 0x1234ABCD. So let's
just state that directly, as it's an easily recognisable pattern.
From there the test implementation tried to encode the value 0x1234ABCD
in little-endian format into the present_reading byte buffer. `struct
pldm_sensor_event_numeric_sensor_state` is allocated as if the
present_reading member was a flexible array member, extending its range
to 4 bytes, taking care of the space allocation concerns (modulo
undefined behaviour).
Analysing the implementation we have to start with the following
concerns:
1. The host representation of the value 0x1234ABCD
2. The architectural representation of the value 0x1234ABCD
3. The behaviour of htole32()
On point 1, value constants defined in the source are represented in the
usual positional notation convention: Most-Significant (MS) digit on the
left, Least-Significant (LS) digit on the right. This source
representation is independent of any architectural representation in the
storage for the value. As a forward reference, any operations on values
in the source are in accordance with the value's type, and do not
concern endianness, only the positional notation.
The architectural representation is required because we have a
byte-addressable memory system and a multi-byte object. As above, when
dealt with as coherent type the object's value behaves as if it has the
properties of positional notation. The ordering of the byte quantities
composing the value only become visible when viewing the storage of the
object as byte sequence:
```
Big Endian MS Little Endian
+-------------- 12 --------------+
| +----------- 34 -----------+ |
| | +-------- AB --------+ | |
| | | +----- CD -----+ | | |
| | | | LS | | | |
v v v v || v v v v
| MS | 12 34 AB CD | LS || LS | CD AB 34 12 | MS | Byte Value
| | 0 1 2 3 | || | 0 1 2 3 | | Byte Address
```
On point 3, the post-condition of htole32() is that if the storage of
the 32-bit unsigned integer result is viewed as a byte array then the
value represented in the array is encoded as little-endian. Whether any
re-arrangement of the argument's representation is required is a
function of the architecture of the host. If the host is big-endian, a
byte-swap will occur, and inspecting the value as its canonical type
will yield a value significantly different from that which was provided
as an argument. On the other hand, if the host is little-endian, then no
byte-swap will occur, as the value is already encoded in the required
fashion. The value of the result when used as its canonical type is the
same as the input value.
Concretely:
+-----------------------+-----------------------------------+
| | Result |
| Source |------------------+----------------|
| Operation | BE Architecture | LE Achitecture |
|-----------------------|------------------+----------------|
| `htole32(0x1234ABCD)` | 0xCDAB3412 | 0x1234ABCD |
| `htobe32(0x1234ABCD)` | 0x1234ABCD | 0xCDAB3412 |
+-----------------------+------------------+----------------+
As a result, any operations on values, aside from endian conversion,
must always be done in terms of the host endianness. Failing that the
behaviour is beyond the semantics of the C specification as there are
multiple possible results.
The test implementation in question violates this requirement. The
following sub-expression (and others) is an operation whose operands
include a value of specific endianness and a value of host endianness:
```
htole32(presentReading) & (0x000000ff)
```
Having covered the problematic sub-expression we need to expand our view
to the assignment into the `present_reading` member of `struct
pldm_sensor_event_numeric_sensor_state`:
```
sensorData->present_reading[3] =
static_cast<uint8_t>(htole32(presentReading) & (0x000000ff));
...
```
We find that the values selected by the sub-expression are then encoded
into an array that is later interpreted to contain a 32-bit
little-endian value. Under the assumption that we're running on a
little-endian host, htole32() becomes an identity function, yielding
presentReading. Rewriting:
```
sensorData->present_reading[3] =
static_cast<uint8_t>(presentReading & (0x000000ff));
...
```
The mask thus selects the Least-Sigificant-Byte (LSB) of
`presentReading`, however this LSB value is inserted into the
present_reading array at the index of the Most-Significant-Byte (MSB) of
the little-endian encoded value (index 3).
Throw away all the trickery and just encode the value into the
`present_reading` member directly, in the correct fashion.
Also fix the implementation of decode_numeric_sensor_data() which
demonstrated similar confusion.
Signed-off-by: Andrew Jeffery <andrew@aj.id.au>
Change-Id: I71990aa594da054bdef0ce42b137bfe44502831b
diff --git a/src/platform.c b/src/platform.c
index 0abd6e8..69e2730 100644
--- a/src/platform.c
+++ b/src/platform.c
@@ -1217,33 +1217,56 @@
switch (*sensor_data_size) {
case PLDM_SENSOR_DATA_SIZE_UINT8:
- case PLDM_SENSOR_DATA_SIZE_SINT8:
if (sensor_data_length !=
PLDM_SENSOR_EVENT_NUMERIC_SENSOR_STATE_8BIT_DATA_LENGTH) {
return PLDM_ERROR_INVALID_LENGTH;
}
*present_reading = present_reading_ptr[0];
break;
- case PLDM_SENSOR_DATA_SIZE_UINT16:
- case PLDM_SENSOR_DATA_SIZE_SINT16:
+ case PLDM_SENSOR_DATA_SIZE_SINT8:
+ if (sensor_data_length !=
+ PLDM_SENSOR_EVENT_NUMERIC_SENSOR_STATE_8BIT_DATA_LENGTH) {
+ return PLDM_ERROR_INVALID_LENGTH;
+ }
+ *present_reading = (uint32_t)(int32_t)present_reading_ptr[0];
+ break;
+ case PLDM_SENSOR_DATA_SIZE_UINT16: {
+ uint16_t val_le;
+
if (sensor_data_length !=
PLDM_SENSOR_EVENT_NUMERIC_SENSOR_STATE_16BIT_DATA_LENGTH) {
return PLDM_ERROR_INVALID_LENGTH;
}
- *present_reading = le16toh(present_reading_ptr[1] |
- (present_reading_ptr[0] << 8));
+
+ memcpy(&val_le, present_reading_ptr, sizeof(val_le));
+ *present_reading = (uint32_t)(le16toh(val_le));
break;
+ }
+ case PLDM_SENSOR_DATA_SIZE_SINT16: {
+ uint16_t val_le;
+
+ if (sensor_data_length !=
+ PLDM_SENSOR_EVENT_NUMERIC_SENSOR_STATE_16BIT_DATA_LENGTH) {
+ return PLDM_ERROR_INVALID_LENGTH;
+ }
+
+ memcpy(&val_le, present_reading_ptr, sizeof(val_le));
+ *present_reading = (uint32_t)(int32_t)(le16toh(val_le));
+ break;
+ }
case PLDM_SENSOR_DATA_SIZE_UINT32:
- case PLDM_SENSOR_DATA_SIZE_SINT32:
+ case PLDM_SENSOR_DATA_SIZE_SINT32: {
+ uint32_t val_le;
+
if (sensor_data_length !=
PLDM_SENSOR_EVENT_NUMERIC_SENSOR_STATE_32BIT_DATA_LENGTH) {
return PLDM_ERROR_INVALID_LENGTH;
}
- *present_reading = le32toh(present_reading_ptr[3] |
- (present_reading_ptr[2] << 8) |
- (present_reading_ptr[1] << 16) |
- (present_reading_ptr[0] << 24));
+
+ memcpy(&val_le, present_reading_ptr, sizeof(val_le));
+ *present_reading = le32toh(val_le);
break;
+ }
default:
return PLDM_ERROR_INVALID_DATA;
}