os_dirent_t.FileName uses OS_MAX_PATH_LEN for array size

[ghost]

typedef struct
{
char FileName[OS_MAX_PATH_LEN];
} os_dirent_t;

It's probably the case that FileName should be of OS_MAX_FILE_NAME size instead.

The use case is to build a filename from a path and a filename from an os_dirent_t. This path, including the filename, would be OS_MAX_PATH_LEN.

[ghost]

This issue was filed by Steven Seeger @ GSFC

[skliper]

Link to code:

osal/src/os/inc/osapi-os-filesys.h

Lines 189 to 193 in 7d9c4c8

	/** @brief Directory entry */
	typedef struct
	{
	char FileName[OS_MAX_PATH_LEN];
	} os_dirent_t;

[skliper]

Are directory names under the same limit, or at least reported directory names from DirRead? I see sample config OS_MAX_FILE_NAME is 20, OS_MAX_PATH_LEN is 64. The default_osconfig.h doesn't really specify the file name length applies to directory names, maybe it should? OS_mkdir just takes a char *, no internal limiting in vxworks implementation.

[skliper]

Further digging - OS_mkdir limit is OS_MAX_PATH_LEN. OS_OpenCreate checks both OS_MAX_PATH_LEN and OS_MAX_FILE_NAME. Looks like in that context (directory name can be bigger than OS_MAX_FILE_NAME) the current definition is consistent. Plan to just add a comment to that effect to resolve this issue.

[jphickey]

I think the bug report is valid - definition of this struct should use OS_MAX_FILE_NAME, not OS_MAX_PATH_LEN.

OS_MAX_PATH_LEN is the maximum length of a full path/absolute file name which has both pathname component and a file name component whereas the OS_MAX_FILE_NAME is the limit for just the filename component. The "readdir" function returns just file names, so the latter is the better length.

Using OS_MAX_PATH_LEN just means the buffer is bigger than it needs to be. (better than being too small, I guess - there shouldn't be a risk of overrun here but it just uses extra memory).

[jphickey]

OS_mkdir and OS_open all accept absolute pathnames, hence why they check against the larger limit, whereas readdir is only reading entries within the currently open directory, and just returns names, not absolute paths.

[skliper]

But OS_mkdir doesn't limit individual directory name lengths so you could make a directory name longer than OS_MAX_FILE_NAME, but then the name would be truncated if you readdir...

[skliper]

There is no limit I could find other than OS_MAX_PATH_LEN on a directory name... if there is please provide a link to code.

[skliper]

In other words, OS_mkdir doesn't limit each individual directory name with OS_MAX_FILE_NAME just the absolute path. If it's just one deep, could create a directory name of that full length, so why would we limit readdir?

[jphickey]

Yeah, that's possibly another bug, then.... The helper function OS_check_name_length (not part of API, but used by a number of file-related APIs) should be confirming that the length between the final '/' character and the end of the string is not more than OS_MAX_FILE_NAME.

osal/src/os/shared/osapi-file.c

Lines 67 to 90 in 7d9c4c8

	static int32 OS_check_name_length(const char *path)
	{
	char* name_ptr;
	if (path == NULL)
	return OS_FS_ERR_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_FS_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_FS_SUCCESS;
	} /* end OS_check_name_length */

(Interestingly, it looks like this is using > comparison when it should be >=, so that might be another bug...)

Most file ops seem to employ this function, but its missing on some, which is probably not correct.

An alternative that might be simpler AND more complete -- This same test could probably be done as part of OS_TranslatePath, as I would think anything doing path translation (incl mkdir) should be checking this limit.

[skliper]

An alternative that might be simpler AND more complete...

Yes, sounds like a good change to me. Add to OS_TranslatePath and remove where now redundant (OS_rename, OS_OpenCreate, OS_remove). Then covers every file/dir access and limits dir names to OS_MAX_FILE_NAME (with the comparison update).

[jphickey]

After thinking about it a bit more, my only concern with doing this in OS_TranslatePath is if a file was created outside OSAL that didn't meet this limit (such as from the shell) then it would be become completely "non-accessible" from OSAL, even for removal or rename operations. But since due to the translations the name as to fit within OS_MAX_LOCAL_PATH_LEN regardless.... so maybe that is acceptable behavior.