From 3fdd39832c63940a4048d8b886d97634531756a9 Mon Sep 17 00:00:00 2001 From: Jacob Hageman Date: Fri, 17 Apr 2020 09:50:54 -0400 Subject: [PATCH] Fix #344, Consistent directory entry size limit Changes FileName in os_dirent_t from OS_MAX_PATH_LEN to OS_MAX_FILE_NAME, and moves OS_check_name_length into OS_TranslatePath so it is consistantly applied everywhere. Also fixes the length checks in OS_check_name_length to account for terminating null. Unit tests updated to match new directory name limit. Coverage tests updated to match simplified logic. --- src/bsp/mcp750-vxworks/config/osconfig.h | 2 +- src/bsp/pc-linux/config/osconfig.h | 2 +- src/bsp/pc-rtems/config/osconfig.h | 2 +- src/os/inc/osapi-os-filesys.h | 2 +- src/os/portable/os-impl-posix-dirs.c | 4 +- src/os/shared/osapi-file.c | 65 ++------------ src/os/shared/osapi-filesys.c | 49 +++++++++-- src/os/vxworks/osfileapi.c | 4 +- src/tests/file-api-test/file-api-test.c | 85 +++++++++++-------- .../shared/src/coveragetest-file.c | 26 ++---- .../shared/src/coveragetest-filesys.c | 23 +++-- src/ut-stubs/osapi-utstub-filesys.c | 2 +- 12 files changed, 128 insertions(+), 138 deletions(-) diff --git a/src/bsp/mcp750-vxworks/config/osconfig.h b/src/bsp/mcp750-vxworks/config/osconfig.h index eb4cf6d6b..ea1c4f72b 100644 --- a/src/bsp/mcp750-vxworks/config/osconfig.h +++ b/src/bsp/mcp750-vxworks/config/osconfig.h @@ -42,7 +42,7 @@ #define OS_MAX_API_NAME 20 /* -** The maximum length for a file name +** The maximum length (including null terminator) for a file name */ #define OS_MAX_FILE_NAME 20 diff --git a/src/bsp/pc-linux/config/osconfig.h b/src/bsp/pc-linux/config/osconfig.h index 665531a8e..6d96e223d 100644 --- a/src/bsp/pc-linux/config/osconfig.h +++ b/src/bsp/pc-linux/config/osconfig.h @@ -41,7 +41,7 @@ #define OS_MAX_API_NAME 20 /* -** The maximum length for a file name +** The maximum length (including null terminator) for a file name */ #define OS_MAX_FILE_NAME 20 diff --git a/src/bsp/pc-rtems/config/osconfig.h b/src/bsp/pc-rtems/config/osconfig.h index 5a33fcac1..8d30f6f7a 100644 --- a/src/bsp/pc-rtems/config/osconfig.h +++ b/src/bsp/pc-rtems/config/osconfig.h @@ -41,7 +41,7 @@ #define OS_MAX_API_NAME 20 /* -** The maximum length for a file name +** The maximum length (including null terminator) for a file name */ #define OS_MAX_FILE_NAME 20 diff --git a/src/os/inc/osapi-os-filesys.h b/src/os/inc/osapi-os-filesys.h index 2d7f922b1..b0f9aa3ce 100644 --- a/src/os/inc/osapi-os-filesys.h +++ b/src/os/inc/osapi-os-filesys.h @@ -190,7 +190,7 @@ enum /** @brief Directory entry */ typedef struct { - char FileName[OS_MAX_PATH_LEN]; + char FileName[OS_MAX_FILE_NAME]; } os_dirent_t; #ifndef OSAL_OMIT_DEPRECATED diff --git a/src/os/portable/os-impl-posix-dirs.c b/src/os/portable/os-impl-posix-dirs.c index daae3f075..b1139de52 100644 --- a/src/os/portable/os-impl-posix-dirs.c +++ b/src/os/portable/os-impl-posix-dirs.c @@ -133,8 +133,8 @@ int32 OS_DirRead_Impl(uint32 local_id, os_dirent_t *dirent) return OS_ERROR; } - strncpy(dirent->FileName, de->d_name, OS_MAX_PATH_LEN - 1); - dirent->FileName[OS_MAX_PATH_LEN - 1] = 0; + strncpy(dirent->FileName, de->d_name, sizeof(dirent->FileName) - 1); + dirent->FileName[sizeof(dirent->FileName) - 1] = 0; return OS_SUCCESS; } /* end OS_DirRead_Impl */ diff --git a/src/os/shared/osapi-file.c b/src/os/shared/osapi-file.c index 6bf13dd7b..0ea3c4501 100644 --- a/src/os/shared/osapi-file.c +++ b/src/os/shared/osapi-file.c @@ -53,41 +53,7 @@ enum }; OS_stream_internal_record_t OS_stream_table[OS_MAX_NUM_OPEN_FILES]; - -/*---------------------------------------------------------------- - * - * Function: OS_check_name_length - * - * Purpose: Local helper routine, not part of OSAL API. - * Validates that the path length is within spec and - * contains at least one directory separator (/) char. - * - *-----------------------------------------------------------------*/ -static int32 OS_check_name_length(const char *path) -{ - char* name_ptr; - - if (path == NULL) - return OS_INVALID_POINTER; - - if (strlen(path) > OS_MAX_PATH_LEN) - return OS_FS_ERR_PATH_TOO_LONG; - - /* checks to see if there is a '/' somewhere in the path */ - name_ptr = strrchr(path, '/'); - if (name_ptr == NULL) - return OS_ERROR; - - /* strrchr returns a pointer to the last '/' char, so we advance one char */ - name_ptr = name_ptr + 1; - - if( strlen(name_ptr) > OS_MAX_FILE_NAME) - return OS_FS_ERR_NAME_TOO_LONG; - - return OS_SUCCESS; - -} /* end OS_check_name_length */ /**************************************************************************************** FILE API @@ -126,16 +92,9 @@ static int32 OS_OpenCreate(uint32 *filedes, const char *path, int32 flags, int32 char local_path[OS_MAX_LOCAL_PATH_LEN]; /* - ** check if the name of the file is too long - */ - return_code = OS_check_name_length(path); - if (return_code == OS_SUCCESS) - { - /* - ** Translate the path - */ - return_code = OS_TranslatePath(path, (char *)local_path); - } + * Translate the path + */ + return_code = OS_TranslatePath(path, (char *)local_path); if (return_code == OS_SUCCESS) { @@ -455,14 +414,10 @@ int32 OS_remove (const char *path) int32 return_code; char local_path[OS_MAX_LOCAL_PATH_LEN]; - return_code = OS_check_name_length(path); + return_code = OS_TranslatePath(path, local_path); if (return_code == OS_SUCCESS) { - return_code = OS_TranslatePath(path, local_path); - if (return_code == OS_SUCCESS) - { - return_code = OS_FileRemove_Impl(local_path); - } + return_code = OS_FileRemove_Impl(local_path); } return return_code; @@ -485,15 +440,7 @@ int32 OS_rename (const char *old, const char *new) char old_path[OS_MAX_LOCAL_PATH_LEN]; char new_path[OS_MAX_LOCAL_PATH_LEN]; - return_code = OS_check_name_length(old); - if (return_code == OS_SUCCESS) - { - return_code = OS_check_name_length(new); - } - if (return_code == OS_SUCCESS) - { - return_code = OS_TranslatePath(old, old_path); - } + return_code = OS_TranslatePath(old, old_path); if (return_code == OS_SUCCESS) { return_code = OS_TranslatePath(new, new_path); diff --git a/src/os/shared/osapi-filesys.c b/src/os/shared/osapi-filesys.c index 0d781ec24..985d492ff 100644 --- a/src/os/shared/osapi-filesys.c +++ b/src/os/shared/osapi-filesys.c @@ -66,6 +66,40 @@ extern const OS_VolumeInfo_t OS_VolumeTable[]; #define OS_COMPAT_VOLTAB_SIZE 0 #endif + +/*---------------------------------------------------------------- + * + * Function: OS_check_name_length + * + * Purpose: Local helper routine, not part of OSAL API. + * Validates that the path length is within spec and + * contains at least one directory separator (/) char. + * + *-----------------------------------------------------------------*/ +static int32 OS_check_name_length(const char *path) +{ + char* name_ptr; + + /* NULL check is done in calling function */ + + if (strlen(path) >= OS_MAX_PATH_LEN) + return OS_FS_ERR_PATH_TOO_LONG; + + /* checks to see if there is a '/' somewhere in the path */ + name_ptr = strrchr(path, '/'); + if (name_ptr == NULL) + return OS_FS_ERR_PATH_INVALID; + + /* strrchr returns a pointer to the last '/' char, so we advance one char */ + name_ptr = name_ptr + 1; + + if( strlen(name_ptr) >= OS_MAX_FILE_NAME) + return OS_FS_ERR_NAME_TOO_LONG; + + return OS_SUCCESS; + +} /* end OS_check_name_length */ + /*---------------------------------------------------------------- * @@ -1061,18 +1095,19 @@ int32 OS_TranslatePath(const char *VirtualPath, char *LocalPath) return OS_INVALID_POINTER; } - SysMountPointLen = 0; - VirtPathLen = strlen(VirtualPath); - VirtPathBegin = VirtPathLen; - /* - ** Check to see if the path is too long + ** Check length */ - if (VirtPathLen >= OS_MAX_PATH_LEN) + return_code = OS_check_name_length(VirtualPath); + if (return_code != OS_SUCCESS) { - return OS_FS_ERR_PATH_TOO_LONG; + return return_code; } + SysMountPointLen = 0; + VirtPathLen = strlen(VirtualPath); + VirtPathBegin = VirtPathLen; + /* ** All valid Virtual paths must start with a '/' character */ diff --git a/src/os/vxworks/osfileapi.c b/src/os/vxworks/osfileapi.c index 902a0b01d..a9e6b94c2 100644 --- a/src/os/vxworks/osfileapi.c +++ b/src/os/vxworks/osfileapi.c @@ -176,8 +176,8 @@ int32 OS_DirRead_Impl(uint32 local_id, os_dirent_t *dirent) return OS_ERROR; } - strncpy(dirent->FileName, de->d_name, OS_MAX_PATH_LEN - 1); - dirent->FileName[OS_MAX_PATH_LEN - 1] = 0; + strncpy(dirent->FileName, de->d_name, sizeof(dirent->FileName) - 1); + dirent->FileName[sizeof(dirent->FileName) - 1] = 0; return OS_SUCCESS; } /* end OS_DirRead_Impl */ diff --git a/src/tests/file-api-test/file-api-test.c b/src/tests/file-api-test/file-api-test.c index 867e34cd3..502292746 100644 --- a/src/tests/file-api-test/file-api-test.c +++ b/src/tests/file-api-test/file-api-test.c @@ -95,55 +95,65 @@ void TestCreatRemove(void) char maxfilename[OS_MAX_PATH_LEN]; char longfilename [OS_MAX_PATH_LEN + 10]; int status; + int fd; + int i; - strncpy(filename,"/drive0/test11chars", sizeof(filename) - 1); - filename[sizeof(filename) - 1] = 0; + /* Short file name */ + strncpy(filename,"/drive0/a", sizeof(filename)); - /* make the two really long filenames */ - - /* 1 '/' and 40 'm' */ - strncpy(maxfilename,"/drive0/mmmmmmmmmmmmmmmmmmmm", sizeof(maxfilename) - 1); - maxfilename[sizeof(maxfilename) - 1] = 0; + /* Create max file name (OS_MAX_FILE_NAME including terminating null) */ + strncpy(maxfilename,"/drive0/", sizeof(maxfilename)); + for (i = strlen(maxfilename); i < (OS_MAX_FILE_NAME - 1); i++) + { + maxfilename[i] = 'm'; + } - /* 1 '/' and 41 'l' */ - strncpy(longfilename,"/drive0/lllllllllllllllllllllllllllllllllllllllll", sizeof(longfilename) - 1); - longfilename[sizeof(longfilename) - 1] = 0; + /* Create longer than max file name */ + strncpy(longfilename,"/drive0/", sizeof(longfilename)); + for (i = strlen(longfilename); i < (sizeof(longfilename) - 1); i++) + { + longfilename[i] = 'm'; + } - /* create a file of reasonable length (but over 8 chars) */ - status = OS_creat(filename,OS_READ_WRITE); - UtAssert_True(status >= OS_SUCCESS, "status after creat = %d",(int)status); + /* create a file with short name */ + fd = OS_creat(filename,OS_READ_WRITE); + UtAssert_True(fd >= 0, "fd after creat short name length file = %d",(int)fd); /* close the first file */ - status = OS_close(status); - UtAssert_True(status == OS_SUCCESS, "status after close = %d",(int)status); + status = OS_close(fd); + UtAssert_True(status == OS_SUCCESS, "status after close short name length file = %d",(int)status); - /* create a file of OS_max_file_name size */ - status = OS_creat(maxfilename,OS_READ_WRITE); - UtAssert_True(status >= OS_SUCCESS, "status after creat = %d",(int)status); + /* create a file with max name size */ + fd = OS_creat(maxfilename,OS_READ_WRITE); + UtAssert_True(fd >= 0, "fd after creat max name length file = %d",(int)fd); /* close the second file */ - status = OS_close(status); - UtAssert_True(status == OS_SUCCESS, "status after close = %d",(int)status); + status = OS_close(fd); + UtAssert_True(status == OS_SUCCESS, "status after close max name length file = %d",(int)status); - /* try removing the file from the drive */ + /* remove the file from the drive */ status = OS_remove(filename); - UtAssert_True(status == OS_SUCCESS, "status after remove = %d",(int)status); + UtAssert_True(status == OS_SUCCESS, "status after short name length file remove = %d",(int)status); - /* try removing the file from the drive */ + /* remove the file from the drive */ status = OS_remove(maxfilename); - UtAssert_True(status == OS_SUCCESS, "status after remove = %d",(int)status); + UtAssert_True(status == OS_SUCCESS, "status after remove max name length file = %d",(int)status); - /* try removing the file from the drive. Should Fail */ + /* try creating with file name too big, should fail */ + status = OS_creat(longfilename,OS_READ_WRITE); + UtAssert_True(status < OS_SUCCESS, "status after create file name too long = %d",(int)status); + + /* try removing with file name too big. Should Fail */ status = OS_remove(longfilename); - UtAssert_True(status < OS_SUCCESS, "status after remove = %d",(int)status); + UtAssert_True(status < OS_SUCCESS, "status after remove file name too long = %d",(int)status); - /* try removing the file from the drive. Should Fail */ - status = OS_remove("/drive0/FileNotOnDrive"); - UtAssert_True(status < OS_SUCCESS, "status after remove = %d",(int)status); + /* try removing file that no longer exists. Should Fail */ + status = OS_remove(filename); + UtAssert_True(status < OS_SUCCESS, "status after remove file doesn't exist = %d",(int)status); /* Similar to previous but with a bad mount point (gives different error code) */ - status = OS_remove("/FileNotOnDrive"); - UtAssert_True(status == OS_FS_ERR_PATH_INVALID, "status after remove = %d",(int)status); + status = OS_remove("/x"); + UtAssert_True(status == OS_FS_ERR_PATH_INVALID, "status after remove bad mount = %d",(int)status); } /*--------------------------------------------------------------------------------------- @@ -781,13 +791,16 @@ void TestStat(void) char filename1[OS_MAX_PATH_LEN]; char dir1 [OS_MAX_PATH_LEN]; + char dir1slash[OS_MAX_PATH_LEN]; char buffer1 [OS_MAX_PATH_LEN]; os_fstat_t StatBuff; int32 fd1; int size; - strcpy(dir1,"/drive0/ThisIsALongDirectoryName"); - strcpy(filename1,"/drive0/ThisIsALongDirectoryName/MyFile1"); + strcpy(dir1,"/drive0/DirectoryName"); + strcpy(dir1slash, dir1); + strcat(dir1slash, "/"); + strcpy(filename1,"/drive0/DirectoryName/MyFile1"); strcpy(buffer1,"111111111111"); /* make the directory */ @@ -812,13 +825,13 @@ void TestStat(void) /* ** Make the stat calls */ - status = OS_stat( "/drive0/ThisIsALongDirectoryName/",&StatBuff); + status = OS_stat(dir1slash,&StatBuff); UtAssert_True(status == OS_SUCCESS, "status after stat 1 = %d",(int)status); - status = OS_stat( "/drive0/ThisIsALongDirectoryName",&StatBuff); + status = OS_stat(dir1,&StatBuff); UtAssert_True(status == OS_SUCCESS, "status after stat 2 = %d",(int)status); - status = OS_stat( "/drive0/ThisIsALongDirectoryName/MyFile1",&StatBuff); + status = OS_stat(filename1,&StatBuff); UtAssert_True(status == OS_SUCCESS, "status after stat 3 = %d",(int)status); /* Clean up */ diff --git a/src/unit-test-coverage/shared/src/coveragetest-file.c b/src/unit-test-coverage/shared/src/coveragetest-file.c index a435a9c15..119591b8f 100644 --- a/src/unit-test-coverage/shared/src/coveragetest-file.c +++ b/src/unit-test-coverage/shared/src/coveragetest-file.c @@ -52,29 +52,15 @@ void Test_OS_creat(void) * int32 OS_creat (const char *path, int32 access) */ int32 actual = OS_creat("/cf/file", OS_READ_WRITE); - UtAssert_True(actual >= 0, "OS_creat() (%ld) >= 0", (long)actual); actual = OS_creat("/cf/file", OS_READ_ONLY); UtAssert_True(actual == OS_ERROR, "OS_creat() (%ld) == OS_ERROR", (long)actual); + UT_SetForceFail(UT_KEY(OS_TranslatePath), OS_ERROR); actual = OS_creat(NULL, OS_WRITE_ONLY); - UtAssert_True(actual == OS_INVALID_POINTER, "OS_creat() (%ld) == OS_INVALID_POINTER", (long)actual); - - UT_SetForceFail(UT_KEY(OCS_strlen), 2 + OS_MAX_PATH_LEN); - actual = OS_creat("/file", OS_WRITE_ONLY); - UtAssert_True(actual == OS_FS_ERR_PATH_TOO_LONG, "OS_creat() (%ld) == OS_FS_ERR_PATH_TOO_LONG", (long)actual); - UT_ClearForceFail(UT_KEY(OCS_strlen)); - - UT_SetForceFail(UT_KEY(OCS_strrchr), -1); - actual = OS_creat("/file", OS_WRITE_ONLY); UtAssert_True(actual == OS_ERROR, "OS_creat() (%ld) == OS_ERROR", (long)actual); - UT_ClearForceFail(UT_KEY(OCS_strrchr)); - - UT_SetDeferredRetcode(UT_KEY(OCS_strlen), 2, 2 + OS_MAX_FILE_NAME); - actual = OS_creat("/file", OS_WRITE_ONLY); - UtAssert_True(actual == OS_FS_ERR_NAME_TOO_LONG, "OS_creat() (%ld) == OS_FS_ERR_NAME_TOO_LONG", (long)actual); - UT_ClearForceFail(UT_KEY(OCS_strlen)); + UT_ClearForceFail(UT_KEY(OS_TranslatePath)); } @@ -307,13 +293,11 @@ void Test_OS_cp(void) actual = OS_cp("/cf/file1", "/cf/file2"); UtAssert_True(actual == expected, "OS_cp() (%ld) == -555", (long)actual); - UT_SetDeferredRetcode(UT_KEY(OCS_strrchr), 1, -1); - expected = OS_ERROR; - actual = OS_cp("/cf/file1", "/cf/file2"); - UtAssert_True(actual == expected, "OS_cp() (%ld) == OS_INVALID_POINTER", (long)actual); - UT_SetDeferredRetcode(UT_KEY(OCS_strrchr), 2, -1); + UT_SetForceFail(UT_KEY(OS_TranslatePath), OS_INVALID_POINTER); + expected = OS_INVALID_POINTER; actual = OS_cp("/cf/file1", "/cf/file2"); UtAssert_True(actual == expected, "OS_cp() (%ld) == OS_INVALID_POINTER", (long)actual); + UT_ClearForceFail(UT_KEY(OS_TranslatePath)); } diff --git a/src/unit-test-coverage/shared/src/coveragetest-filesys.c b/src/unit-test-coverage/shared/src/coveragetest-filesys.c index d134347ac..1c889145a 100644 --- a/src/unit-test-coverage/shared/src/coveragetest-filesys.c +++ b/src/unit-test-coverage/shared/src/coveragetest-filesys.c @@ -480,32 +480,43 @@ void Test_OS_TranslatePath(void) UtAssert_True(actual == expected, "OS_TranslatePath() (%ld) == OS_FS_ERR_PATH_TOO_LONG", (long)actual); UT_ClearForceFail(UT_KEY(OCS_strlen)); + /* Invalid no '/' */ expected = OS_FS_ERR_PATH_INVALID; actual = OS_TranslatePath("invalid",LocalBuffer); UtAssert_True(actual == expected, "OS_TranslatePath() (%ld) == OS_FS_ERR_PATH_INVALID", (long)actual); + UT_SetDeferredRetcode(UT_KEY(OCS_strlen), 2, OS_MAX_FILE_NAME + 1); + expected = OS_FS_ERR_NAME_TOO_LONG; + actual = OS_TranslatePath("/cf/test",LocalBuffer); + UtAssert_True(actual == expected, "OS_TranslatePath(/cf/test) (%ld) == OS_FS_ERR_NAME_TOO_LONG", (long)actual); + + /* Invalid no leading '/' */ + expected = OS_FS_ERR_PATH_INVALID; + actual = OS_TranslatePath("invalid/",LocalBuffer); + UtAssert_True(actual == expected, "OS_TranslatePath() (%ld) == OS_FS_ERR_PATH_INVALID", (long)actual); + UT_SetForceFail(UT_KEY(OS_ObjectIdGetBySearch), OS_ERR_NAME_NOT_FOUND); actual = OS_TranslatePath("/cf/test",LocalBuffer); UtAssert_True(actual == expected, "OS_TranslatePath() (%ld) == OS_FS_ERR_PATH_INVALID", (long)actual); UT_ClearForceFail(UT_KEY(OS_ObjectIdGetBySearch)); - UT_SetDeferredRetcode(UT_KEY(OCS_strlen), 3, OS_MAX_PATH_LEN + 10); + /* VirtPathLen < VirtPathBegin */ + UT_SetDeferredRetcode(UT_KEY(OCS_strlen), 5, OS_MAX_PATH_LEN); expected = OS_FS_ERR_PATH_INVALID; actual = OS_TranslatePath("/cf/test",LocalBuffer); UtAssert_True(actual == expected, "OS_TranslatePath(/cf/test) (%ld) == OS_FS_ERR_PATH_INVALID", (long)actual); - UT_SetForceFail(UT_KEY(OCS_memcpy), OS_ERROR); - UT_SetDeferredRetcode(UT_KEY(OCS_strlen), 1, OS_MAX_PATH_LEN - 1); - UT_SetDeferredRetcode(UT_KEY(OCS_strlen), 1, OS_MAX_LOCAL_PATH_LEN - 1); - UT_SetDeferredRetcode(UT_KEY(OCS_strlen), 1, 1); + /* (SysMountPointLen + VirtPathLen) > OS_MAX_LOCAL_PATH_LEN */ + UT_SetDeferredRetcode(UT_KEY(OCS_strlen), 4, OS_MAX_LOCAL_PATH_LEN); expected = OS_FS_ERR_PATH_TOO_LONG; actual = OS_TranslatePath("/cf/test",LocalBuffer); UtAssert_True(actual == expected, "OS_TranslatePath(/cf/test) (%ld) == OS_FS_ERR_PATH_TOO_LONG", (long)actual); - + OS_filesys_table[1].flags = 0; expected = OS_ERR_INCORRECT_OBJ_STATE; actual = OS_TranslatePath("/cf/test",LocalBuffer); UtAssert_True(actual == expected, "OS_TranslatePath(/cf/test) (%ld) == OS_ERR_INCORRECT_OBJ_STATE", (long)actual); + } void Test_OS_FileSys_FindVirtMountPoint(void) diff --git a/src/ut-stubs/osapi-utstub-filesys.c b/src/ut-stubs/osapi-utstub-filesys.c index 2e2d40cc4..1f3c368ff 100644 --- a/src/ut-stubs/osapi-utstub-filesys.c +++ b/src/ut-stubs/osapi-utstub-filesys.c @@ -189,7 +189,7 @@ int32 OS_TranslatePath( const char *VirtualPath, char *LocalPath) status = UT_DEFAULT_IMPL(OS_TranslatePath); - if (status == OS_SUCCESS && + if (status == OS_SUCCESS && VirtualPath != NULL && LocalPath != NULL && UT_Stub_CopyToLocal(UT_KEY(OS_TranslatePath), LocalPath, OS_MAX_LOCAL_PATH_LEN) == 0) { strncpy(LocalPath, VirtualPath, OS_MAX_LOCAL_PATH_LEN);