From 1aba1cf496026bf5194353a62b007d10761fc041 Mon Sep 17 00:00:00 2001 From: Patrick Cook <114708437+cookpate@users.noreply.github.com> Date: Tue, 14 Nov 2023 11:10:30 -0800 Subject: [PATCH 1/8] Fix FAT32 format check #55 Only the first two bytes were considered when reading the end-of-chain value from the FAT table. Extend to 4 bytes and relax requirement for first 3 bits. Make requirement for FAT12 more strict because all valid, non-reserved cluster bits must be set. --- ff_ioman.c | 32 ++++++++++++++------------------ 1 file changed, 14 insertions(+), 18 deletions(-) diff --git a/ff_ioman.c b/ff_ioman.c index 299860b..2796f9f 100644 --- a/ff_ioman.c +++ b/ff_ioman.c @@ -705,18 +705,21 @@ static uint8_t ucAssumeFATType = 0; * * In practice however, this does not always seem to be a correct assumption. * - * The first 12 or 16 bits in the FAT table may also help to determine the + * The end-of-chain value in the FAT table may also help to determine the * correct FAT-type: * - * FAT-12: ( firstWord & 0x3FF ) == 0x3F8 ) - * FAT-16: ( firstWord == 0xFFF8 ) + * endOfChain = bits 4-32 at beginning of table + * FAT-12: endOfChain == 0x0FF8 + * FAT-16: endOfChain == 0xFFF8 + * FAT-32: endOfChain == 0x0FFFFFF8 */ static FF_Error_t prvDetermineFatType( FF_IOManager_t * pxIOManager ) { FF_Partition_t * pxPartition; FF_Buffer_t * pxBuffer; - uint32_t ulFirstWord = 0ul; + /* final cluster sentinel value */ + uint32_t ulEndOfChain = 0ul; FF_Error_t xError = FF_ERR_NONE; pxPartition = &( pxIOManager->xPartition ); @@ -751,7 +754,7 @@ static FF_Error_t prvDetermineFatType( FF_IOManager_t * pxIOManager ) } else { - ulFirstWord = ( uint32_t ) FF_getShort( pxBuffer->pucBuffer, 0x0000 ); + ulEndOfChain = FF_getLong( pxBuffer->pucBuffer, 0x0000 ) & 0xFFFFFFF8ul; xError = FF_ReleaseBuffer( pxIOManager, pxBuffer ); } } @@ -764,7 +767,7 @@ static FF_Error_t prvDetermineFatType( FF_IOManager_t * pxIOManager ) /* FAT12 */ pxPartition->ucType = FF_T_FAT12; #if ( ffconfigFAT_CHECK != 0 ) - if( ( ulFirstWord & 0x3FF ) != 0x3F8 ) + if( ulEndOfChain == 0x0FF8 ) { xError = FF_createERR( FF_ERR_IOMAN_NOT_FAT_FORMATTED, FF_DETERMINEFATTYPE ); } @@ -783,20 +786,20 @@ static FF_Error_t prvDetermineFatType( FF_IOManager_t * pxIOManager ) pxPartition->ucType = FF_T_FAT16; #if ( ffconfigFAT_CHECK != 0 ) { - if( ulFirstWord == 0xFFF8 ) + if( ulEndOfChain == 0xFFF8 ) { xError = FF_ERR_NONE; } else { - if( ( ulFirstWord & 0x3FF ) != 0x3F8 ) + if( ulEndOfChain != 0x0FF8 ) { FF_PRINTF( "Part at %lu is probably a FAT12\n", pxIOManager->xPartition.ulFATBeginLBA ); } else { FF_PRINTF( "Partition at %lu has strange FAT data %08lX\n", - pxIOManager->xPartition.ulFATBeginLBA, ulFirstWord ); + pxIOManager->xPartition.ulFATBeginLBA, ulEndOfChain ); } xError = FF_createERR( FF_ERR_IOMAN_INVALID_FORMAT, FF_DETERMINEFATTYPE ); @@ -809,16 +812,9 @@ static FF_Error_t prvDetermineFatType( FF_IOManager_t * pxIOManager ) /* FAT 32! */ pxPartition->ucType = FF_T_FAT32; #if ( ffconfigFAT_CHECK != 0 ) - if( ( ( ulFirstWord & 0x0FFFFFF8 ) != 0x0FFFFFF8 ) && - ( ( ulFirstWord & 0x0FFFFFF8 ) != 0x0FFFFFF0 ) ) + if( ulEndOfChain != 0x0FFFFFF8 ) { - /* _HT_ - * I had an SD-card which worked well in Linux/W32 - * but FreeRTOS+FAT returned me this error - * So for me I left out this check (just issue a warning for now) - */ - FF_PRINTF( "prvDetermineFatType: firstWord %08lX\n", ulFirstWord ); - xError = FF_ERR_NONE; /* FF_ERR_IOMAN_NOT_FAT_FORMATTED; */ + xError = FF_createERR( FF_ERR_IOMAN_NOT_FAT_FORMATTED, FF_DETERMINEFATTYPE ); } #endif /* ffconfigFAT_CHECK */ xError = FF_ERR_NONE; From 9e1161bdf7d191956f91a1dabcd6fb11f04aeb2d Mon Sep 17 00:00:00 2001 From: Patrick Cook <114708437+cookpate@users.noreply.github.com> Date: Tue, 14 Nov 2023 11:24:40 -0800 Subject: [PATCH 2/8] Fix inverted FAT12 logic --- ff_ioman.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/ff_ioman.c b/ff_ioman.c index 2796f9f..6697e28 100644 --- a/ff_ioman.c +++ b/ff_ioman.c @@ -767,7 +767,7 @@ static FF_Error_t prvDetermineFatType( FF_IOManager_t * pxIOManager ) /* FAT12 */ pxPartition->ucType = FF_T_FAT12; #if ( ffconfigFAT_CHECK != 0 ) - if( ulEndOfChain == 0x0FF8 ) + if( ulEndOfChain != 0x0FF8 ) { xError = FF_createERR( FF_ERR_IOMAN_NOT_FAT_FORMATTED, FF_DETERMINEFATTYPE ); } @@ -792,7 +792,7 @@ static FF_Error_t prvDetermineFatType( FF_IOManager_t * pxIOManager ) } else { - if( ulEndOfChain != 0x0FF8 ) + if( ulEndOfChain == 0x0FF8 ) { FF_PRINTF( "Part at %lu is probably a FAT12\n", pxIOManager->xPartition.ulFATBeginLBA ); } From 867bced46b7e7b05cdf9f57ce493ac70ace30b92 Mon Sep 17 00:00:00 2001 From: Patrick Cook <114708437+cookpate@users.noreply.github.com> Date: Tue, 14 Nov 2023 11:44:59 -0800 Subject: [PATCH 3/8] Add defaulted heap selection to CMakeListst.txt --- CMakeLists.txt | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/CMakeLists.txt b/CMakeLists.txt index 7694f2d..5519b31 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -89,6 +89,12 @@ elseif((FREERTOS_PLUS_FAT_PORT STREQUAL "A_CUSTOM_PORT") AND (NOT TARGET freerto " freertos_plus_fat)") endif() +# This library requires access to a heap +# FreeRTOS/FreeRTOS-Kernel previously defaulted to heap4.c +if(NOT DEFINED FREERTOS_HEAP) + message(STATUS "FREERTOS_HEAP not set, setting FREERTOS_HEAP=4") + set(FREERTOS_HEAP 4) +endif() # Select the appropriate Build Test configuration # This is only used when freertos_config is not defined, otherwise the build test will be performed From e451679107a7822954b0888ca627f507e3682e83 Mon Sep 17 00:00:00 2001 From: Patrick Cook Date: Mon, 27 Nov 2023 16:17:08 -0800 Subject: [PATCH 4/8] Add suggested fixes and clarifications on-behalf-of: Hein Tibosch hein_tibosch@yahoo.es --- ff_ioman.c | 72 ++++++++++++++++++++++++++++++++++++------------------ 1 file changed, 48 insertions(+), 24 deletions(-) diff --git a/ff_ioman.c b/ff_ioman.c index 6697e28..472f8d8 100644 --- a/ff_ioman.c +++ b/ff_ioman.c @@ -708,18 +708,18 @@ static uint8_t ucAssumeFATType = 0; * The end-of-chain value in the FAT table may also help to determine the * correct FAT-type: * - * endOfChain = bits 4-32 at beginning of table - * FAT-12: endOfChain == 0x0FF8 - * FAT-16: endOfChain == 0xFFF8 - * FAT-32: endOfChain == 0x0FFFFFF8 + * ulFirstCluster = the first 32-bit of the FAT. + * FAT-12: endOfChain == 0x00000FF8 - 12 bits + * FAT-16: endOfChain == 0x0000FFF8 - 16 bits + * FAT-32: endOfChain == 0x0FFFFFF8 - 32 bits */ static FF_Error_t prvDetermineFatType( FF_IOManager_t * pxIOManager ) { FF_Partition_t * pxPartition; FF_Buffer_t * pxBuffer; - /* final cluster sentinel value */ - uint32_t ulEndOfChain = 0ul; + /* The first 32-bits of the FAT. */ + uint32_t ulFirstCluster = 0U; FF_Error_t xError = FF_ERR_NONE; pxPartition = &( pxIOManager->xPartition ); @@ -754,7 +754,8 @@ static FF_Error_t prvDetermineFatType( FF_IOManager_t * pxIOManager ) } else { - ulEndOfChain = FF_getLong( pxBuffer->pucBuffer, 0x0000 ) & 0xFFFFFFF8ul; + /* Read the first 4 bytes at offset 0. */ + ulFirstCluster = FF_getLong( pxBuffer->pucBuffer, 0U ); xError = FF_ReleaseBuffer( pxIOManager, pxBuffer ); } } @@ -767,13 +768,23 @@ static FF_Error_t prvDetermineFatType( FF_IOManager_t * pxIOManager ) /* FAT12 */ pxPartition->ucType = FF_T_FAT12; #if ( ffconfigFAT_CHECK != 0 ) - if( ulEndOfChain != 0x0FF8 ) + /* Keep bits 4..11 */ + + /* MS-DOS/PC DOS 3.3 and higher treats a value of 0xFF0 on FAT12. + * See https://en.wikipedia.org/wiki/Design_of_the_FAT_file_system + */ + ulFirstCluster &= 0xFF0U; + + if( ulFirstCluster != 0xFF0U ) { xError = FF_createERR( FF_ERR_IOMAN_NOT_FAT_FORMATTED, FF_DETERMINEFATTYPE ); + FF_PRINTF( "FAT_CHECK: FAT12 Partition has unexpected FAT data %04lX\n", + ulFirstCluster ); } else #endif /* ffconfigFAT_CHECK */ { + /* FAT12 entry OK. */ xError = FF_ERR_NONE; } } @@ -785,26 +796,30 @@ static FF_Error_t prvDetermineFatType( FF_IOManager_t * pxIOManager ) /* FAT 16 */ pxPartition->ucType = FF_T_FAT16; #if ( ffconfigFAT_CHECK != 0 ) + { + /* Keep bits 4..15 */ + ulFirstCluster &= 0xFFF8U; + + if( ulFirstCluster == 0xFFF8U ) { - if( ulEndOfChain == 0xFFF8 ) + /* FAT16 entry OK. */ + xError = FF_ERR_NONE; + } + else + { + if( ( ulFirstCluster & 0xFF8U ) == 0xFF8U ) { - xError = FF_ERR_NONE; + FF_PRINTF( "FAT_CHECK: FAT16 Part at %lu is probably a FAT12\n", pxIOManager->xPartition.ulFATBeginLBA ); } else { - if( ulEndOfChain == 0x0FF8 ) - { - FF_PRINTF( "Part at %lu is probably a FAT12\n", pxIOManager->xPartition.ulFATBeginLBA ); - } - else - { - FF_PRINTF( "Partition at %lu has strange FAT data %08lX\n", - pxIOManager->xPartition.ulFATBeginLBA, ulEndOfChain ); - } - - xError = FF_createERR( FF_ERR_IOMAN_INVALID_FORMAT, FF_DETERMINEFATTYPE ); + FF_PRINTF( "FAT_CHECK: FAT16 Partition has unexpected FAT data %08lX\n", + ulFirstCluster ); } + + xError = FF_createERR( FF_ERR_IOMAN_INVALID_FORMAT, FF_DETERMINEFATTYPE ); } + } #endif /* ffconfigFAT_CHECK */ } else @@ -812,12 +827,21 @@ static FF_Error_t prvDetermineFatType( FF_IOManager_t * pxIOManager ) /* FAT 32! */ pxPartition->ucType = FF_T_FAT32; #if ( ffconfigFAT_CHECK != 0 ) - if( ulEndOfChain != 0x0FFFFFF8 ) + /* Keep bits 4..27 */ + ulFirstCluster &= 0x0FFFFFF8UL; + + if( ulFirstCluster != 0x0FFFFFF8UL ) { - xError = FF_createERR( FF_ERR_IOMAN_NOT_FAT_FORMATTED, FF_DETERMINEFATTYPE ); + FF_PRINTF( "FAT_CHECK: FAT32 Partition at %lu has unexpected FAT data %08lX\n", + pxIOManager->xPartition.ulFATBeginLBA, ulFirstCluster ); + xError = FF_ERR_IOMAN_NOT_FAT_FORMATTED; } + else #endif /* ffconfigFAT_CHECK */ - xError = FF_ERR_NONE; + { + /* FAT32 entry OK. */ + xError = FF_ERR_NONE; + } } } From be0ff4108c1ba0a3aef6ef8626a174305d596c05 Mon Sep 17 00:00:00 2001 From: Soren Ptak Date: Tue, 28 Nov 2023 14:59:27 -0500 Subject: [PATCH 5/8] Run uncrustify on ff_ioman.c --- ff_ioman.c | 38 +++++++++++++++++++------------------- 1 file changed, 19 insertions(+), 19 deletions(-) diff --git a/ff_ioman.c b/ff_ioman.c index 472f8d8..70dbabd 100644 --- a/ff_ioman.c +++ b/ff_ioman.c @@ -654,7 +654,7 @@ int32_t FF_BlockWrite( FF_IOManager_t * pxIOManager, if( ( slRetVal == 0ul ) && ( pxIOManager->xBlkDevice.fnpWriteBlocks != NULL ) ) { do - { /* Make sure we don't execute a NULL. */ + { /* Make sure we don't execute a NULL. */ if( ( xSemLocked == pdFALSE ) && ( ( pxIOManager->ucFlags & FF_IOMAN_BLOCK_DEVICE_IS_REENTRANT ) == pdFALSE ) ) { @@ -796,30 +796,30 @@ static FF_Error_t prvDetermineFatType( FF_IOManager_t * pxIOManager ) /* FAT 16 */ pxPartition->ucType = FF_T_FAT16; #if ( ffconfigFAT_CHECK != 0 ) - { - /* Keep bits 4..15 */ - ulFirstCluster &= 0xFFF8U; - - if( ulFirstCluster == 0xFFF8U ) - { - /* FAT16 entry OK. */ - xError = FF_ERR_NONE; - } - else { - if( ( ulFirstCluster & 0xFF8U ) == 0xFF8U ) + /* Keep bits 4..15 */ + ulFirstCluster &= 0xFFF8U; + + if( ulFirstCluster == 0xFFF8U ) { - FF_PRINTF( "FAT_CHECK: FAT16 Part at %lu is probably a FAT12\n", pxIOManager->xPartition.ulFATBeginLBA ); + /* FAT16 entry OK. */ + xError = FF_ERR_NONE; } else { - FF_PRINTF( "FAT_CHECK: FAT16 Partition has unexpected FAT data %08lX\n", - ulFirstCluster ); - } + if( ( ulFirstCluster & 0xFF8U ) == 0xFF8U ) + { + FF_PRINTF( "FAT_CHECK: FAT16 Part at %lu is probably a FAT12\n", pxIOManager->xPartition.ulFATBeginLBA ); + } + else + { + FF_PRINTF( "FAT_CHECK: FAT16 Partition has unexpected FAT data %08lX\n", + ulFirstCluster ); + } - xError = FF_createERR( FF_ERR_IOMAN_INVALID_FORMAT, FF_DETERMINEFATTYPE ); + xError = FF_createERR( FF_ERR_IOMAN_INVALID_FORMAT, FF_DETERMINEFATTYPE ); + } } - } #endif /* ffconfigFAT_CHECK */ } else @@ -1528,7 +1528,7 @@ FF_Error_t FF_Mount( FF_Disk_t * pxDisk, } if( pxPartition->ulSectorsPerFAT == 0 ) - { /* FAT32 */ + { /* FAT32 */ pxPartition->ulSectorsPerFAT = FF_getLong( pxBuffer->pucBuffer, FF_FAT_32_SECTORS_PER_FAT ); pxPartition->ulRootDirCluster = FF_getLong( pxBuffer->pucBuffer, FF_FAT_ROOT_DIR_CLUSTER ); memcpy( pxPartition->pcVolumeLabel, pxBuffer->pucBuffer + FF_FAT_32_VOL_LABEL, sizeof( pxPartition->pcVolumeLabel ) - 1 ); From 44c705140d2ccbf16b990b2180ca8ddd8332a58a Mon Sep 17 00:00:00 2001 From: Soren Ptak Date: Tue, 28 Nov 2023 15:13:09 -0500 Subject: [PATCH 6/8] Use uncrustify .66 instead .69 --- ff_ioman.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/ff_ioman.c b/ff_ioman.c index 70dbabd..31b15e0 100644 --- a/ff_ioman.c +++ b/ff_ioman.c @@ -654,7 +654,7 @@ int32_t FF_BlockWrite( FF_IOManager_t * pxIOManager, if( ( slRetVal == 0ul ) && ( pxIOManager->xBlkDevice.fnpWriteBlocks != NULL ) ) { do - { /* Make sure we don't execute a NULL. */ + { /* Make sure we don't execute a NULL. */ if( ( xSemLocked == pdFALSE ) && ( ( pxIOManager->ucFlags & FF_IOMAN_BLOCK_DEVICE_IS_REENTRANT ) == pdFALSE ) ) { @@ -1528,7 +1528,7 @@ FF_Error_t FF_Mount( FF_Disk_t * pxDisk, } if( pxPartition->ulSectorsPerFAT == 0 ) - { /* FAT32 */ + { /* FAT32 */ pxPartition->ulSectorsPerFAT = FF_getLong( pxBuffer->pucBuffer, FF_FAT_32_SECTORS_PER_FAT ); pxPartition->ulRootDirCluster = FF_getLong( pxBuffer->pucBuffer, FF_FAT_ROOT_DIR_CLUSTER ); memcpy( pxPartition->pcVolumeLabel, pxBuffer->pucBuffer + FF_FAT_32_VOL_LABEL, sizeof( pxPartition->pcVolumeLabel ) - 1 ); From 40c1806654781ad91b231b421ce605672516f8fe Mon Sep 17 00:00:00 2001 From: Soren Ptak Date: Tue, 28 Nov 2023 15:13:56 -0500 Subject: [PATCH 7/8] Make the formatting workflow print the git diff for now --- .github/workflows/ci.yml | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index ea56938..52ce045 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -21,15 +21,18 @@ jobs: - name: Run Uncrustify run: | uncrustify --version - find . -iname "*.[hc]" -exec uncrustify --check -c tools/uncrustify.cfg {} + + find . -iname "*.[hc]" -exec uncrustify --no-backup --replace --if-changed -c tools/uncrustify.cfg {} + if [ "$?" = "0" ]; then exit 0 else echo -e "\033[31;1;43mFormatting check (using Uncrustify) failed...\033[0m" echo -e "\033[32;3mTo have the code uncrustified for you, please comment '/bot run uncrustify' (without the quotes) on the Pull Request.\033[0m" + git diff --color=always exit 1 fi - name: Check For Trailing Whitespace + shell: bash + if: success() || failure() run: | set +e grep --exclude="README.md" -rnI -e "[[:blank:]]$" . From 1034630dcda391e83dfe975f1d7a64f4d09d6f53 Mon Sep 17 00:00:00 2001 From: Soren Ptak Date: Tue, 28 Nov 2023 16:30:43 -0500 Subject: [PATCH 8/8] Revert back to upstream workflow file --- .github/workflows/ci.yml | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 52ce045..ea56938 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -21,18 +21,15 @@ jobs: - name: Run Uncrustify run: | uncrustify --version - find . -iname "*.[hc]" -exec uncrustify --no-backup --replace --if-changed -c tools/uncrustify.cfg {} + + find . -iname "*.[hc]" -exec uncrustify --check -c tools/uncrustify.cfg {} + if [ "$?" = "0" ]; then exit 0 else echo -e "\033[31;1;43mFormatting check (using Uncrustify) failed...\033[0m" echo -e "\033[32;3mTo have the code uncrustified for you, please comment '/bot run uncrustify' (without the quotes) on the Pull Request.\033[0m" - git diff --color=always exit 1 fi - name: Check For Trailing Whitespace - shell: bash - if: success() || failure() run: | set +e grep --exclude="README.md" -rnI -e "[[:blank:]]$" .