Fix parsing of Windows shortcut (.lnk) files in file table #8601
Conversation
A misusage of the readFile function caused all parsing of shortcut files to fail. Fixes osquery#8599
| if (isPlatform(PlatformType::TYPE_WINDOWS)) { | ||
| auto link_path = boost::filesystem::path(expected_path); | ||
|
|
||
| if (row.at("path").rfind(".lnk") != std::string::npos) { |
There was a problem hiding this comment.
This was a bug in the test that would always be false so the shortcut functionality was never tested (because the loop iterates only through the kFilenameList and not the shortcut files)
|
There seem to be some slightly different paths on one of the Windows runners ( |
Smjert
left a comment
There was a problem hiding this comment.
Just a comment on performance, but overall looks good.
osquery/tables/utility/file.cpp
Outdated
| auto status = | ||
| readFile(link, header_size_field_bytes, kShellLinkHeaderSizeFieldSize); | ||
| std::string link_content; | ||
| auto status = readFile(link, link_content); |
There was a problem hiding this comment.
Did you test the performance of the table after this change, especially trying to list files are big in size?
I'm not sure how many file types end up using this readFile, but even with the max read size, this might cause the table to be slower because now it will attempt to read them fully, while we only wanted to read a small portion of them.
The original implementation of readFile (which I changed through another PR and apparently missed this use) exposed a number of bytes to read.
Furthermore its use was so that the non-blocking behavior was used (vs a simpler read), to avoid any hanging trying to read special files, but in hindsight I think we have still the issue with the COM interface above, which means that if performance is an issue, this could be substituted by a plain read of the header bytes.
There was a problem hiding this comment.
Ooh that makes sense that there used to be another overload for the readFile function. I didn't test the performance of this. I see the PR (#8410) where it was removed. Maybe we should just re-add the number of bytes overload in this PR?
Sorry I missed this; I think the test could try to use I would not use this directly in the table code since this walks the filesystem afaik. |
|
@Smjert I added back the optimization to read only the first 4 bytes. I also added an additional optimization that will skip all this logic if it's not a Still need to fix the test. |
|
Finally got the test working! |
|
@Smjert please take a look when you can |
tests/integration/tables/file.cpp
Outdated
| ASSERT_TRUE(link_index.has_value()); | ||
| const auto& row = data.at(link_index.value()); | ||
|
|
||
| auto short_path = directory.string(); |
There was a problem hiding this comment.
Here we need to first go from UTF8 -> UTF16 with stringToWString.
But we also need the name of the file here; they can be shortened too.
tests/integration/tables/file.cpp
Outdated
| // Transform the expected path to a "full path" using GetLongPathNameW | ||
| wchar_t long_path[MAX_PATH]; | ||
| auto result = GetLongPathNameW( | ||
| std::wstring(short_path.begin(), short_path.end()).c_str(), |
There was a problem hiding this comment.
and then here we have a short_path variable that's already a std::wstring
tests/integration/tables/file.cpp
Outdated
| long_path, | ||
| MAX_PATH); | ||
| EXPECT_EQ(row.at("shortcut_target_path"), | ||
| wstringToString(long_path) + "\\" + test_file_name); |
There was a problem hiding this comment.
After the changes above, we don't need to append test_file_name anymore
|
@Smjert I made your requested revisions. Test is still passing locally. Hopefully it also works on CI. |
|
It's now building and passing tests on CI. I'm not sure how it could have still been passing locally per my last comment -- I must not have actually built the newly modified files. But now it's working. |
A refactor of the
readFilefunction (in #8410) caused all parsing of shortcut files to fail. The test had a bug that caused the relevant checks to be skipped.Fixes #8599