Skip to content
This repository has been archived by the owner on Jun 14, 2018. It is now read-only.

Fix crash with open file. #45

Merged
merged 3 commits into from
Feb 3, 2012
Merged

Fix crash with open file. #45

merged 3 commits into from
Feb 3, 2012

Conversation

gruehle
Copy link
Member

@gruehle gruehle commented Feb 2, 2012

@peterflynn

Fixes issue #39.

@redmunds
Copy link
Contributor

redmunds commented Feb 3, 2012

reviewing

@@ -227,7 +227,7 @@ class BracketsExtensionHandler : public CefV8Handler
bool canChooseFiles = !canChooseDirectories;
std::string title = arguments[2]->GetStringValue();
std::wstring wtitle = StringToWString(title);
std::string initialPath = arguments[3]->GetStringValue();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(I can't figure out how to insert a comment in the full file view, but a few lines up...) the code does array bounds checking by verifying the length of the array, but it does not verify that the array elements, which are assumed to be ptrs, are non-null. IOW, the code should check (arguments[i] != null) for each i.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

CEF always passes non-null values for arguments, even if null or undefined are passed to the JavaScript layer. I don't think we need an additional null check here- if null is passed, something is seriously wrong and the app will crash, regardless of what this function does.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since this dialog was called from JS, and we want to be extensible, it seemed like a good thing to check. But I verified that passing null doesn't crash, so it seems to be OK as it is.

redmunds added a commit that referenced this pull request Feb 3, 2012
@redmunds redmunds merged commit b56eaf9 into master Feb 3, 2012
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants