Fix buffer overrun in fatfs_utf16_inode_str_2_utf8

[zweger]

I noticed valgrind reporting errors on an exFAT image:

==2797== Conditional jump or move depends on uninitialised value(s)
==2797==    at 0x5D9BF4B: tsk_UTF16toUTF8 (tsk_unicode.c:163)
==2797==    by 0x5DD0C9C: fatfs_utf16_inode_str_2_utf8 (fatfs_utils.c:296)
==2797==    by 0x5D951BA: exfatfs_copy_file_inode (exfatfs_meta.c:1251)
==2797==    by 0x5DCF096: exfatfs_inode_lookup (exfatfs_meta.c:1683)
==2797==    by 0x5DA957B: tsk_fs_dir_walk_lcl (fs_dir.c:577)
==2797==    by 0x5DA9BB0: tsk_fs_dir_walk.part.5 (fs_dir.c:803)

This fixes those errors.

[bcarrier]

Thanks. But, I'm not sure that is the correct fix. My impression of the main change here is that it is defining the end of the UTF16 buffer based on bytes versus UTF16 characters. So, it's a factor of 2 difference. The API docs on that method are not specific about what the metrics of a_src_len are (bytes or characters) and a quick grep of the code shows that we seem to be using it differently.

I think that a_src_len should be number of characters in a_src (not number of bytes). In which case, the code in fatfs_utf16_inode_str_2_utf8() is correct, but some of the callers are passing in the wrong value.

For example, in extfats_meta.c on line 1251, we pass in sizeof(utf16_name), which is the number of bytes in the char buffer.

Did valgrind report the stack trace of who called it?

[zweger]

I believe you are correct. exfatfs_copy_file_name_inode, exfatfs_copy_vol_label_inode, exfatfs_parse_vol_label_dentry, and exfatfs_parse_file_name_dentry all use "characters". exfatfs_copy_file_inode appears to be the only caller of fatfs_utf16_inode_str_2_utf8 which uses bytes.

That's a problem, because fatfs_utf16_inode_str_2_utf8 is required to convert to bytes to calculate an end address for tsk_UTF16toUTF8. It's not possible for a caller to calculate the byte length from the number of characters, because UTF-16 is variable-width. Even if every UTF-16 character were exactly 2 bytes, the existing implementation of fatfs_utf16_inode_str_2_utf8 will only work on systems where a short is 2 bytes.

Do you know if exFAT/FAT actually only supports UCS-2? Or if the "number of UTF-16 characters" field is actually a "half the number of bytes" field? That would allow for trivial conversions.

I would prefer to modify the other callers to pass the length in bytes, but that could be harder than it sounds. What are your thoughts?

I tried to omit my application code from the trace, but was a little overzealous:

==2797== Memcheck, a memory error detector
==2797== Copyright (C) 2002-2015, and GNU GPL'd, by Julian Seward et al.
==2797== Using Valgrind-3.11.0 and LibVEX; rerun with -h for copyright info
==2797== Command: 
==2797== 
==2797== Conditional jump or move depends on uninitialised value(s)
==2797==    at 0x5D9BF4B: tsk_UTF16toUTF8 (tsk_unicode.c:163)
==2797==    by 0x5DD0C9C: fatfs_utf16_inode_str_2_utf8 (fatfs_utils.c:296)
==2797==    by 0x5D951BA: exfatfs_copy_file_inode (exfatfs_meta.c:1251)
==2797==    by 0x5DCF096: exfatfs_inode_lookup (exfatfs_meta.c:1683)
==2797==    by 0x5DA957B: tsk_fs_dir_walk_lcl (fs_dir.c:577)
==2797==    by 0x5DA9BB0: tsk_fs_dir_walk.part.5 (fs_dir.c:803)
==2797==    by 0x5E0386A: TskAuto::findFilesInFsInt(TSK_FS_INFO*, unsigned long) (auto.cpp:545)
==2797==    by 0x5E038E6: TskAuto::findFilesInFsRet(long, TSK_FS_TYPE_ENUM) (auto.cpp:372)
==2797==    by 0x5E03A88: TskAuto::vsWalkCb(TSK_VS_INFO*, TSK_VS_PART_INFO const*, void*) (auto.cpp:258)
==2797==    by 0x5DA04D8: tsk_vs_part_walk (mm_part.c:276)
==2797==    by 0x5E03B74: TskAuto::findFilesInVs(long, TSK_VS_TYPE_ENUM) (auto.cpp:309)
==2797== 
==2797== Conditional jump or move depends on uninitialised value(s)
==2797==    at 0x5D9BF5F: tsk_UTF16toUTF8 (tsk_unicode.c:190)
==2797==    by 0x5DD0C9C: fatfs_utf16_inode_str_2_utf8 (fatfs_utils.c:296)
==2797==    by 0x5D951BA: exfatfs_copy_file_inode (exfatfs_meta.c:1251)
==2797==    by 0x5DCF096: exfatfs_inode_lookup (exfatfs_meta.c:1683)
==2797==    by 0x5DA957B: tsk_fs_dir_walk_lcl (fs_dir.c:577)
==2797==    by 0x5DA9BB0: tsk_fs_dir_walk.part.5 (fs_dir.c:803)
==2797==    by 0x5E0386A: TskAuto::findFilesInFsInt(TSK_FS_INFO*, unsigned long) (auto.cpp:545)
==2797==    by 0x5E038E6: TskAuto::findFilesInFsRet(long, TSK_FS_TYPE_ENUM) (auto.cpp:372)
==2797==    by 0x5E03A88: TskAuto::vsWalkCb(TSK_VS_INFO*, TSK_VS_PART_INFO const*, void*) (auto.cpp:258)
==2797==    by 0x5DA04D8: tsk_vs_part_walk (mm_part.c:276)
==2797==    by 0x5E03B74: TskAuto::findFilesInVs(long, TSK_VS_TYPE_ENUM) (auto.cpp:309)
==2797== 
==2797== Conditional jump or move depends on uninitialised value(s)
==2797==    at 0x5D9BFBC: tsk_UTF16toUTF8 (tsk_unicode.c:203)
==2797==    by 0x5DD0C9C: fatfs_utf16_inode_str_2_utf8 (fatfs_utils.c:296)
==2797==    by 0x5D951BA: exfatfs_copy_file_inode (exfatfs_meta.c:1251)
==2797==    by 0x5DCF096: exfatfs_inode_lookup (exfatfs_meta.c:1683)
==2797==    by 0x5DA957B: tsk_fs_dir_walk_lcl (fs_dir.c:577)
==2797==    by 0x5DA9BB0: tsk_fs_dir_walk.part.5 (fs_dir.c:803)
==2797==    by 0x5E0386A: TskAuto::findFilesInFsInt(TSK_FS_INFO*, unsigned long) (auto.cpp:545)
==2797==    by 0x5E038E6: TskAuto::findFilesInFsRet(long, TSK_FS_TYPE_ENUM) (auto.cpp:372)
==2797==    by 0x5E03A88: TskAuto::vsWalkCb(TSK_VS_INFO*, TSK_VS_PART_INFO const*, void*) (auto.cpp:258)
==2797==    by 0x5DA04D8: tsk_vs_part_walk (mm_part.c:276)
==2797==    by 0x5E03B74: TskAuto::findFilesInVs(long, TSK_VS_TYPE_ENUM) (auto.cpp:309)
==2797== 
==2797== Conditional jump or move depends on uninitialised value(s)
==2797==    at 0x5D9BFC4: tsk_UTF16toUTF8 (tsk_unicode.c:206)
==2797==    by 0x5DD0C9C: fatfs_utf16_inode_str_2_utf8 (fatfs_utils.c:296)
==2797==    by 0x5D951BA: exfatfs_copy_file_inode (exfatfs_meta.c:1251)
==2797==    by 0x5DCF096: exfatfs_inode_lookup (exfatfs_meta.c:1683)
==2797==    by 0x5DA957B: tsk_fs_dir_walk_lcl (fs_dir.c:577)
==2797==    by 0x5DA9BB0: tsk_fs_dir_walk.part.5 (fs_dir.c:803)
==2797==    by 0x5E0386A: TskAuto::findFilesInFsInt(TSK_FS_INFO*, unsigned long) (auto.cpp:545)
==2797==    by 0x5E038E6: TskAuto::findFilesInFsRet(long, TSK_FS_TYPE_ENUM) (auto.cpp:372)
==2797==    by 0x5E03A88: TskAuto::vsWalkCb(TSK_VS_INFO*, TSK_VS_PART_INFO const*, void*) (auto.cpp:258)
==2797==    by 0x5DA04D8: tsk_vs_part_walk (mm_part.c:276)
==2797==    by 0x5E03B74: TskAuto::findFilesInVs(long, TSK_VS_TYPE_ENUM) (auto.cpp:309)
==2797== 
==2797== Conditional jump or move depends on uninitialised value(s)
==2797==    at 0x5D9BE97: tsk_UTF16toUTF8 (tsk_unicode.c:170)
==2797==    by 0x5DD0C9C: fatfs_utf16_inode_str_2_utf8 (fatfs_utils.c:296)
==2797==    by 0x5D951BA: exfatfs_copy_file_inode (exfatfs_meta.c:1251)
==2797==    by 0x5DCF096: exfatfs_inode_lookup (exfatfs_meta.c:1683)
==2797==    by 0x5DA957B: tsk_fs_dir_walk_lcl (fs_dir.c:577)
==2797==    by 0x5DA98D1: tsk_fs_dir_walk_lcl (fs_dir.c:698)
==2797==    by 0x5DA98D1: tsk_fs_dir_walk_lcl (fs_dir.c:698)
==2797==    by 0x5DA98D1: tsk_fs_dir_walk_lcl (fs_dir.c:698)
==2797==    by 0x5DA98D1: tsk_fs_dir_walk_lcl (fs_dir.c:698)
==2797==    by 0x5DA9BB0: tsk_fs_dir_walk.part.5 (fs_dir.c:803)
==2797==    by 0x5E0386A: TskAuto::findFilesInFsInt(TSK_FS_INFO*, unsigned long) (auto.cpp:545)
==2797==    by 0x5E038E6: TskAuto::findFilesInFsRet(long, TSK_FS_TYPE_ENUM) (auto.cpp:372)
==2797== 
==2797== 
==2797== More than 10000000 total errors detected.  I'm not reporting any more.
==2797== Final error counts will be inaccurate.  Go fix your program!
==2797== Rerun with --error-limit=no to disable this cutoff.  Note
==2797== that errors may occur in your program without prior warning from
==2797== Valgrind, because errors are no longer being displayed.
==2797==

[zweger]

I'm closing this PR as it appears this issue isn't as simple as I thought it was.