From d43050abd77dff663b081b675acf0c2fe7c4b0bc Mon Sep 17 00:00:00 2001 From: togrue Date: Tue, 27 Feb 2024 21:03:09 +0100 Subject: [PATCH] Change the FF_MakeNameCompliant behavior (#63) * Change the handling of invalid file-names to cause an error. Depending on the entry to create (directory/file), a different error code is returned. Description ----------- This changes the behavior of the FF_CreateDirent function to return an error in case of an invalid path. Filenames with the first character of 0xE5 are still modified. (Which might have some issues. TODO) Note: Invalid directory paths and invalid file paths now result in two different FreeRTOS+ return values. Is this the intended behavior or should only one error code be used? ``` case FF_ERR_FILE_INVALID_PATH: return pdFREERTOS_ERRNO_ENOTDIR; /* The path of the file was not found. */ case FF_ERR_DIR_INVALID_PATH: return pdFREERTOS_ERRNO_EINVAL; /* Could not find the directory specified by the path. */ ``` FF_MakeNameCompliant returns a boolean and not an error-code, because the function would need an additional parameter to select the right error code (FF_ERR_DIR_INVALID_PATH or FF_ERR_FILE_INVALID_PATH). Test Steps ----------- A invalid filepath "test:file.txt" resulted in setting the FF_ERR_FILE_INVALID_PATH. (Tested through breakpoint in prvFFErrorToErrno). Todo: - mkdir with invalid path - handling of names starting with 0xE5 Checklist: ---------- - [ ] I have tested my changes. No regression in existing tests. - [ ] I have modified and/or added unit-tests to cover the code changes in this Pull Request. Related Issue ----------- By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice. --------- Co-authored-by: Tobias Gruen --- ff_dir.c | 59 +++++++++++++++++++++++------------------ ff_file.c | 66 ++++++++++++++++++++++++++++------------------ include/ff_dir.h | 10 +++++++ include/ff_error.h | 1 + 4 files changed, 84 insertions(+), 52 deletions(-) diff --git a/ff_dir.c b/ff_dir.c index 04cf851..8ea3c5c 100644 --- a/ff_dir.c +++ b/ff_dir.c @@ -134,12 +134,6 @@ static BaseType_t FF_ValidShortChar( char cChar ); #endif /* if ( ffconfigUNICODE_UTF16_SUPPORT != 0 ) */ #endif /* if ( ffconfigLFN_SUPPORT != 0 ) */ -#if ( ffconfigUNICODE_UTF16_SUPPORT != 0 ) - static void FF_MakeNameCompliant( FF_T_WCHAR * pcName ); -#else - static void FF_MakeNameCompliant( char * pcName ); -#endif - #if ( FF_NOSTRCASECMP == 0 ) static portINLINE unsigned char prvToLower( unsigned char c ) { @@ -2875,40 +2869,48 @@ FF_Error_t FF_ExtendDirectory( FF_IOManager_t * pxIOManager, } /* FF_ExtendDirectory() */ /*-----------------------------------------------------------*/ -static const uint8_t forbiddenChrs[] = +/* *INDENT-OFF* */ +#if ( ffconfigUNICODE_UTF16_SUPPORT != 0 ) + static const FF_T_WCHAR forbiddenChars[] = +#else + static const uint8_t forbiddenChars[] = +#endif { /* Windows says: don't use these characters: '\/:*?"<>|' * " * / : < > ? '\' ? | */ 0x22, 0x2A, 0x2F, 0x3A, 0x3C, 0x3E, 0x3F, 0x5C, 0x7F, 0x7C }; +/* *INDENT-ON* */ /* *INDENT-OFF* */ #if ( ffconfigUNICODE_UTF16_SUPPORT != 0 ) - static void FF_MakeNameCompliant( FF_T_WCHAR * pcName ) + BaseType_t FF_IsNameCompliant( FF_T_WCHAR * pcName ) #else - static void FF_MakeNameCompliant( char * pcName ) + BaseType_t FF_IsNameCompliant( char * pcName ) #endif /* *INDENT-ON* */ { + BaseType_t xReturn = pdTRUE; BaseType_t index; - if( ( uint8_t ) pcName[ 0 ] == FF_FAT_DELETED ) /* Support Japanese KANJI symbol0xE5. */ + if( ( uint8_t ) pcName[ 0 ] == FF_FAT_DELETED ) /* Do not create a "deleted" entry 0xE5. */ { - pcName[ 0 ] = 0x05; + xReturn = pdFALSE; } for( ; *pcName; pcName++ ) { - for( index = 0; index < ( BaseType_t ) sizeof( forbiddenChrs ); index++ ) + for( index = 0; index < ( BaseType_t ) ( sizeof( forbiddenChars ) / sizeof( forbiddenChars[ 0 ] ) ); index++ ) { - if( *pcName == forbiddenChrs[ index ] ) + if( *pcName == forbiddenChars[ index ] ) { - *pcName = '_'; - break; + xReturn = pdFALSE; } } } -} /* FF_MakeNameCompliant() */ + + return xReturn; +} /* FF_IsNameCompliant() */ /*-----------------------------------------------------------*/ FF_Error_t FF_CreateDirent( FF_IOManager_t * pxIOManager, @@ -2941,15 +2943,6 @@ FF_Error_t FF_CreateDirent( FF_IOManager_t * pxIOManager, /* Round-up the number of LFN's needed: */ xLFNCount = ( BaseType_t ) ( ( NameLen + 12 ) / 13 ); - #if ( ffconfigUNICODE_UTF16_SUPPORT != 0 ) - { - FF_MakeNameCompliant( pxDirEntry->pcFileName ); /* Ensure we don't break the Dir tables. */ - } - #else - { - FF_MakeNameCompliant( pxDirEntry->pcFileName ); /* Ensure we don't break the Dir tables. */ - } - #endif memset( pucEntryBuffer, 0, sizeof( pucEntryBuffer ) ); #if ( ffconfigLFN_SUPPORT != 0 ) @@ -3127,7 +3120,15 @@ FF_Error_t FF_CreateDirent( FF_IOManager_t * pxIOManager, STRNCPY( xMyFile.pcFileName, pcFileName, ffconfigMAX_FILENAME - 1 ); xMyFile.pcFileName[ ffconfigMAX_FILENAME - 1 ] = 0; - xMyFile.ulObjectCluster = FF_CreateClusterChain( pxIOManager, &xError ); + if( FF_IsNameCompliant( xMyFile.pcFileName ) == pdFALSE ) + { + xError = FF_createERR( FF_ERR_FILE_INVALID_PATH, FF_CREATEFILE ); + } + + if( FF_isERR( xError ) == pdFALSE ) + { + xMyFile.ulObjectCluster = FF_CreateClusterChain( pxIOManager, &xError ); + } if( FF_isERR( xError ) == pdFALSE ) { @@ -3269,6 +3270,12 @@ FF_Error_t FF_CreateDirent( FF_IOManager_t * pxIOManager, break; } + if( FF_IsNameCompliant( pcDirName ) == pdFALSE ) + { + xError = FF_createERR( FF_ERR_DIR_INVALID_PATH, FF_MKDIR ); + break; + } + xFindParams.ulDirCluster = FF_FindDir( pxIOManager, pcPath, ( uint16_t ) xIndex, &xError ); if( FF_isERR( xError ) ) diff --git a/ff_file.c b/ff_file.c index 3ad033a..87bb31e 100644 --- a/ff_file.c +++ b/ff_file.c @@ -868,6 +868,7 @@ static FF_FILE * prvAllocFileHandle( FF_IOManager_t * pxIOManager, * @retval FF_ERR_FILE_DESTINATION_EXISTS if the destination file exists. * @retval FF_ERR_FILE_COULD_NOT_CREATE_DIRENT if dirent creation failed (fatal error!). * @retval FF_ERR_FILE_DIR_NOT_FOUND if destination directory was not found. + * @retval FF_ERR_FILE_INVALID_PATH if destination path is invalid. * @retval FF_ERR_FILE_SOURCE_NOT_FOUND if the source file was not found. * **/ @@ -886,11 +887,11 @@ static FF_FILE * prvAllocFileHandle( FF_IOManager_t * pxIOManager, #endif /* *INDENT-ON* */ { - FF_Error_t xError; + FF_Error_t xError = FF_ERR_NONE; FF_FILE * pSrcFile, * pxDestFile; FF_DirEnt_t xMyFile; uint8_t ucEntryBuffer[ 32 ]; - UBaseType_t xIndex; + size_t uxIndex = 0U; uint32_t ulDirCluster = 0ul; FF_FetchContext_t xFetchContext; @@ -906,12 +907,46 @@ static FF_FILE * prvAllocFileHandle( FF_IOManager_t * pxIOManager, } #if ( ffconfigREMOVABLE_MEDIA != 0 ) - else if( ( pxIOManager->ucFlags & FF_IOMAN_DEVICE_IS_EXTRACTED ) != 0 ) + else if( ( pxIOManager->ucFlags & FF_IOMAN_DEVICE_IS_EXTRACTED ) != 0U ) { xError = FF_createERR( FF_ERR_IOMAN_DRIVER_NOMEDIUM, FF_MOVE ); } #endif /* ffconfigREMOVABLE_MEDIA */ - else + + if( FF_isERR( xError ) == pdFALSE ) + { + uxIndex = ( size_t ) STRLEN( szDestinationFile ); + + /* Find the base name. */ + while( uxIndex != 0U ) + { + if( ( szDestinationFile[ uxIndex ] == '\\' ) || ( szDestinationFile[ uxIndex ] == '/' ) ) + { + break; + } + + uxIndex--; + } + + /* Copy the base name of the destination file. */ + STRNCPY( xMyFile.pcFileName, ( szDestinationFile + uxIndex + 1 ), ffconfigMAX_FILENAME - 1 ); + xMyFile.pcFileName[ ffconfigMAX_FILENAME - 1 ] = 0; + + /* Now check if the target base name is compliant. */ + if( FF_IsNameCompliant( xMyFile.pcFileName ) == pdFALSE ) + { + xError = FF_createERR( FF_ERR_FILE_INVALID_PATH, FF_MOVE ); + } + + if( uxIndex == 0U ) + { + /* Give the directory part a minimum + * length of 1, to get '/' at least. */ + uxIndex = 1U; + } + } + + if( FF_isERR( xError ) == pdFALSE ) { /* Check destination file doesn't exist! */ pxDestFile = FF_Open( pxIOManager, szDestinationFile, FF_MODE_READ, &xError ); @@ -969,30 +1004,9 @@ static FF_FILE * prvAllocFileHandle( FF_IOManager_t * pxIOManager, xMyFile.ulObjectCluster = pSrcFile->ulObjectCluster; xMyFile.usCurrentItem = 0; - xIndex = ( UBaseType_t ) STRLEN( szDestinationFile ); - - while( xIndex != 0 ) - { - if( ( szDestinationFile[ xIndex ] == '\\' ) || ( szDestinationFile[ xIndex ] == '/' ) ) - { - break; - } - - xIndex--; - } - - /* Copy the base name of the destination file. */ - STRNCPY( xMyFile.pcFileName, ( szDestinationFile + xIndex + 1 ), ffconfigMAX_FILENAME - 1 ); - xMyFile.pcFileName[ ffconfigMAX_FILENAME - 1 ] = 0; - - if( xIndex == 0 ) - { - xIndex = 1; - } - /* Find the (cluster of the) directory in which the target file will be located. * It must exist before calling FF_Move(). */ - ulDirCluster = FF_FindDir( pxIOManager, szDestinationFile, ( uint16_t ) xIndex, &xError ); + ulDirCluster = FF_FindDir( pxIOManager, szDestinationFile, ( uint16_t ) uxIndex, &xError ); } } } diff --git a/include/ff_dir.h b/include/ff_dir.h index 7169f7d..4a23624 100644 --- a/include/ff_dir.h +++ b/include/ff_dir.h @@ -172,6 +172,16 @@ int8_t FF_PushEntry( FF_IOManager_t * pxIOManager, uint8_t * buffer, void * pParam ); +/* + * FF_IsNameCompliant is a function that checks if a given path name + * contains any legal characters only. + */ +#if ( ffconfigUNICODE_UTF16_SUPPORT != 0 ) + BaseType_t FF_IsNameCompliant( FF_T_WCHAR * pcName ); +#else + BaseType_t FF_IsNameCompliant( char * pcName ); +#endif + static portINLINE BaseType_t FF_isEndOfDir( const uint8_t * pucEntryBuffer ) { return pucEntryBuffer[ 0 ] == ( uint8_t ) 0; diff --git a/include/ff_error.h b/include/ff_error.h index 18ff2f1..c2e2286 100644 --- a/include/ff_error.h +++ b/include/ff_error.h @@ -118,6 +118,7 @@ #define FF_MKDIR ( ( 12 << FF_FUNCTION_SHIFT ) | FF_MODULE_DIR ) #define FF_TRAVERSE ( ( 13 << FF_FUNCTION_SHIFT ) | FF_MODULE_DIR ) #define FF_FINDDIR ( ( 14 << FF_FUNCTION_SHIFT ) | FF_MODULE_DIR ) +#define FF_CREATEFILE ( ( 15 << FF_FUNCTION_SHIFT ) | FF_MODULE_DIR ) /*----- FF_FILE - The FreeRTOS+FAT file handling routines. */ #define FF_GETMODEBITS ( ( 1 << FF_FUNCTION_SHIFT ) | FF_MODULE_FILE )