incorrect size of remote characteristic database not detected

[Shockel]

I was working from the ANCS example and the way it stores the remote characteristics in flash. I expanded the example to add the time service, and was now storing additional data for the same peer. When connecting to a known peer, I was expecting the code to fail on first attempt, because the data in the DB should be too small (ie not holding the newly added time server characteristics). From my interpretation, the code is actually attempting to do so, but deep inside pds_peer_data_read() prevents that (more below).

From the ANCS example, this is how the DB is loaded:

        case PM_EVT_BONDED_PEER_CONNECTED:
        {
            if (p_evt->peer_id != PM_PEER_ID_INVALID)
            {
                uint32_t data_len = sizeof(m_peer_srv_buf);
                ret = pm_peer_data_remote_db_load(p_evt->peer_id, m_peer_srv_buf, &data_len);
                if (ret == NRF_ERROR_NOT_FOUND)
                {
                    ...snip...
                }
                else
                {
                    // Check if the load was successful.
                    ASSERT(data_len == sizeof(m_peer_srv_buf));
                    APP_ERROR_CHECK(ret);
                    NRF_LOG_INFO("Remote Database loaded from flash.");

From the call pm_peer_data_remote_db_load(p_evt->peer_id, m_peer_srv_buf, &data_len); I expected data_len to come back with the data length in the entry from flash, but it didn't. If I debug, the above call ultimately calls pds_peer_data_read() which doesn't change data_len. And by function definition I don't think it actually could.

So data_len remains unchanged, and the

ASSERT(data_len == sizeof(m_peer_srv_buf));

will always succeed. It's a rare case, but maybe there's a bug here ?

[Shockel]

And already as a continuation of the above, now that for the first time I want to store

static ble_gatt_db_srv_t   m_peer_srv_buf[3]

via

pm_peer_data_remote_db_store(), exactly as in the ANCS example just with 3 elements in the array, an assertion fails because the array size (of 3*118) is not a multiple of 4 (which succeeds in the ANCS example since the element size is 2*118.

See if (ALIGN_NUM(4, length) != length) in pm_peer_data_store()

Thanks for you help / thoughts.

[Simon]

Couldn't you simply align the number to a multiple of four, increase the length to the nearest (larger) multiple of 4. E.g. if the length is 202, then the new length will be 202+2=204 (you can use ALIGN_NUM() to achieve this). In this case, two random bytes after the array m_peer_srv_buf[3] will be stored into flash.

I think this is a valid solution. Could you test it out and give me feedback? 

So data_len remains unchanged, and the

ASSERT(data_len == sizeof(m_peer_srv_buf));

will always succeed. It's a rare case, but maybe there's a bug here ?

This seems like a bug to me as well. I will report it and get back to you.

Best regards,

Simon

[Shockel]

Simon,

it may not be the proper way, but as a quick fix one can actually store a few extra bytes by

ret = pm_peer_data_remote_db_store(peer_id, (ble_gatt_db_srv_t *)m_peer_srv_buf, ALIGN_NUM(4, sizeof(m_peer_srv_buf), NULL);

and read-back without the ALIGN_NUM, since pm_peer_data_remote_db_load() will copy the lesser of the amount stored in flash and the requested data.

As to the bug, also note that the documentation in peer_manager.h states incorrectly a return value in p_len, which it doesn't return (I believe).

/**@brief Function for retrieving stored data of a peer.
*
* @note The length of the provided buffer must be a multiple of 4.
*
* @param[in] peer_id Peer ID to get data for.
* @param[in] data_id Which type of data to read.
* @param[out] p_data Where to put the retrieved data. The documentation for
* @ref pm_peer_data_id_t specifies what data type each data ID is stored as.
* @param[in,out] p_len In: The length in bytes of @p p_data.
* Out: The length in bytes of the read data, if the read was successful.
*
* @retval NRF_SUCCESS If the data was read successfully.
* @retval NRF_ERROR_INVALID_PARAM If the data type or the peer ID was invalid or unallocated.
* @retval NRF_ERROR_NULL If a pointer parameter was NULL.
* @retval NRF_ERROR_NOT_FOUND If no stored data was found for this peer ID/data ID combination.
* @retval NRF_ERROR_DATA_SIZE If the provided buffer was not large enough. The data is still
* copied, filling the provided buffer. Note that this error can
* occur even if loading the same size as was stored, because the
* underlying layers round the length up to the nearest word (4 bytes)
* when storing.
* @retval NRF_ERROR_INVALID_STATE If the Peer Manager is not initialized.
*/
ret_code_t pm_peer_data_load(pm_peer_id_t peer_id,
pm_peer_data_id_t data_id,
void * p_data,
uint32_t * p_len);

[Shockel]

Simon, all,

I actually have to correct the statement above. It is NOT advisable.

To recap, along the ANCS example, I was storing an array of m_peer_srv_buf into flash to retain the services of the remote peer in flash. As I increased the example app from m_peer_srv_buf [2] to three elements m_peer_srv_buf[3], it broke because pm_peer_data_remote_db_store MUST use array sizes of multiples of four. And m_peer_srv_buf[3] isn't.

You suggested to simply store two extra bytes, which would access two "illegal" bytes at the end of the array. Which seemed ok, because when reading the data back in via pm_peer_data_remote_db_load() it would realize that the data in flash is two bytes larger than the provided buffer, and it would truncate nicely back to my m_peer_srv_buf[3].

What I am realizing now is, that deep inside pm_peer_data_remote_db_load(), the FDS record will not be closed properly (due to an early return in line 444 of peer_data_storage.c .

The easiest fix is to add another element to the m_peer_srv_buf[] array and simply store always an even number.

Maybe to safe others the headache, the sample app should have a nicer workaround to this entire problem ?!