-
Notifications
You must be signed in to change notification settings - Fork 3.6k
[camera_windows] Use temp directory fallback when Pictures folder is unavailable #10723
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -86,17 +86,34 @@ std::string GetCurrentTimeString() { | |
| } | ||
|
|
||
| // Builds file path for picture capture. | ||
| /// If Pictures folder exist use it, otherwise use temp directory | ||
| /// This fallback prevents crashes or unexpected failures on systems where the Pictures folder is unavailable or restricted | ||
| std::optional<std::string> GetFilePathForPicture() { | ||
| ComHeapPtr<wchar_t> known_folder_path; | ||
| HRESULT hr = SHGetKnownFolderPath(FOLDERID_Pictures, KF_FLAG_CREATE, nullptr, | ||
| &known_folder_path); | ||
| if (FAILED(hr)) { | ||
| return std::nullopt; | ||
|
|
||
| std::wstring wpath; | ||
|
|
||
| if (SUCCEEDED(hr)) { | ||
| wpath = std::wstring(known_folder_path); | ||
| } else { | ||
| // Fallback to temp folder | ||
| wchar_t tempPath[MAX_PATH]; | ||
| DWORD len = GetTempPathW(MAX_PATH, tempPath); | ||
| if (len == 0 || len > MAX_PATH) { | ||
| return std::nullopt; | ||
| } | ||
| wpath = std::wstring(tempPath); | ||
| } | ||
|
Comment on lines
+98
to
108
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This change introduces new logic, including a fallback path, but doesn't include corresponding tests. The repository style guide states that 'Code should be tested'. To ensure the correctness and robustness of this new functionality, please add unit tests that cover both the successful retrieval of the Pictures folder and the fallback to the temporary directory. This might require some refactoring to allow for mocking of the Windows API calls. References
|
||
|
|
||
| std::string path = Utf8FromUtf16(std::wstring(known_folder_path)); | ||
| if (!wpath.empty() && wpath.back() != L'\\' && wpath.back() != L'/') { | ||
| wpath.push_back(L'\\'); | ||
| } | ||
|
|
||
| std::string path = Utf8FromUtf16(wpath); | ||
|
|
||
| return path + "\\" + "PhotoCapture_" + GetCurrentTimeString() + "." + | ||
| return path + "PhotoCapture_" + GetCurrentTimeString() + "." + | ||
| kPictureCaptureExtension; | ||
| } | ||
|
|
||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The condition
len > MAX_PATHis incorrect and could lead to a buffer over-read. For WinAPI functions likeGetTempPathW, if the returned length is equal to the buffer size (MAX_PATHin this case), the written string is not guaranteed to be null-terminated. Constructing astd::wstringfrom a non-null-terminated C-style string results in undefined behavior as it will read past the end of the buffer. The check should belen >= MAX_PATHto correctly handle this edge case.