Fix: Packed attribute missing for FF_Part_t causing buggy memcpy and alignment issues

[Adithya65]

Fix: Packed attribute missing for FF_Part_t causing buggy memcpy and alignment issues

Description

The structure FF_Part_t was used with memcpy to copy partition data (pxPartitions + xPartNr into p) in ff_ioman.c, but it contains bitfields:

typedef struct _SPart
{
uint32_t ulStartLBA;
uint32_t ulSectorCount;
uint32_t ucActive : 8,
ucPartitionID : 8,
bIsExtended : 1;
} FF_Part_t;

Test Steps

This is a general fix; no additional tests are required.

Checklist:

• I have tested my changes. No regression in existing tests[NOT TESTED].
• I have modified and/or added unit-tests to cover the code changes in this Pull Request.

Related Issue

None opened

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

[ActoryOu]

Hi @Adithya65,
Could you help fix the formatting issue in CI?

[htibosch]

Thank you for this PR.
I'm always very hesitant to use the packed attribute. What goes wrong in memcpy()? Alignment problem? Is this the only location where memcpy() gets into trouble?

  /* Store this partition for the caller */
  FF_Part_t * p = &pPartsFound->pxPartitions[ pPartsFound->iCount++ ];

  /* Copy the whole structure */
  memcpy( p, pxPartitions + xPartNr, sizeof( *p ) );

  /* and make LBA absolute to sector-0. */
  p->ulStartLBA += ulThisSector;
  p->bIsExtended = pdTRUE;

For instance, I would rather copy the bytes in an alignment-safe loop. That avoids that the proposed change will affect a lot of other code.

[htibosch]

Sorry, transforming from bitfields to uint_8 is good and I guess it will avoid the crash.
For completeness, I would show the unused (alignment) byte as well:

-            ucActive : 8,       /* FF_FAT_PTBL_ACTIVE */
-            ucPartitionID : 8,  /* FF_FAT_PTBL_ID */
-            bIsExtended : 1;
+            uint8_t ucActive;       /* FF_FAT_PTBL_ACTIVE */
+            uint8_t ucPartitionID;  /* FF_FAT_PTBL_ID */
+            uint8_t bIsExtended;
+            uint8_t bIsUnused;      /* Byte added to make the length a multiple of 32 byte. */

and without the packed attribute.
Thanks, Hein

[Adithya65]

Hi @htibosch , I will update those changes.Do you want me to raise a fresh PR.

[htibosch]

This is ff_ioman,c with some minor format changes: ff_ioman.zip

-}         /* extern "C" */
+    } /* extern "C" */
     #endif
 
 #endif /* ifndef _FF_IOMAN_H_ */
+

I thought we had a tool that source formats a file on github directly? Maybe only in the +TCP repo?

uint8_t bIsUnused;      /* Byte added to make the length a multiple of 32 byte. */

The nice thing of this filler byte is that it can be assigned a new meaning/function without the risk of changing the size of the struct.

[htibosch]

Thank you @Adithya65 for this PR. Thanks @ActoryOu for the suggestion to turn the bitfields into uint8_t fields.

I approve it.