Closed
Bug 831190
Opened 11 years ago
Closed 10 years ago
Passing empty string to nsIMsgFolder.createSubfolder() crashes Thunderbird
Categories
(MailNews Core :: Database, defect)
Tracking
(Not tracked)
RESOLVED
FIXED
Thunderbird 29.0
People
(Reporter: paulius.zaleckas, Assigned: aceman)
Details
(Keywords: crash)
Attachments
(1 file, 5 obsolete files)
11.91 KB,
patch
|
neil
:
review+
standard8
:
review+
|
Details | Diff | Splinter Review |
User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:17.0) Gecko/20100101 Firefox/17.0 Build ID: 20121130221340 Steps to reproduce: var acctMgr = Components.classes["@mozilla.org/messenger/account-manager;1"].getService(Components.interfaces.nsIMsgAccountManager); let localMsgFolder = acctMgr.localFoldersServer.rootMsgFolder; localMsgFolder.createSubfolder("", null); Actual results: Thunderbird crashed without any output to console. Expected results: I think nsIMsgFolder.createSubfolder() should return error if name is empty string. nsImapMailFolder::CreateSubfolder() and nsMsgNewsFolder::CreateSubfolder() checks if name is empty, but nsMsgLocalMailFolder::CreateSubfolder() does not.
Strangely I did not get the crash when running that code inside TB25 (from AccountManager.js) I got: Error: Component returned failure code: 0x80550013 [nsIMsgFolder.createSubfolder] But I see nsMsgLocalMailFolder::CreateSubfolder is missing the check. But maybe it is caugth deeper in nsMsgBrkMBoxStore::CreateFolder . Can you please test it on a newer TB version?
This test shows the crash does really happen in some circumstances.
Status: UNCONFIRMED → NEW
Component: Folder and Message Lists → Database
Ever confirmed: true
Product: Thunderbird → MailNews Core
Hardware: x86_64 → All
Version: 17 → Trunk
Comment 3•11 years ago
|
||
A very simple possible patch.
Attachment #777409 -
Flags: feedback?(acelists)
This is an optional more involved patch where I'd like the functions to return more meaningful values (as they already exist).
We will need comments from the module owners on which approach they like better.
Status: NEW → ASSIGNED
Flags: needinfo?(neil)
Flags: needinfo?(irving)
Flags: needinfo?(Pidgeot18)
Comment 6•11 years ago
|
||
I think all the public methods should bail out as soon as possible if an empty folder name is provided. I'm not too keen on the local folder throwing an alert message though, but that's probably because the imap folder ends up doing it async as a server alert anyway; the utility code shouldn't try to alert. The change in error code shouldn't be a problem.
Flags: needinfo?(neil)
+++ b/mailnews/local/src/nsLocalMailFolder.cpp @@ -554,17 +554,18 @@ nsMsgLocalMailFolder::CreateSubfolderInt nsCOMPtr<nsIMsgPluggableStore> msgStore; rv = GetMsgStore(getter_AddRefs(msgStore)); NS_ENSURE_SUCCESS(rv, rv); rv = msgStore->CreateFolder(this, folderName, aNewFolder); if (rv == NS_MSG_ERROR_INVALID_FOLDER_NAME) { ThrowAlertMsg("folderCreationFailed", msgWindow); // I'm returning this value so the dialog stays up return NS_MSG_FOLDER_EXISTS; David, do you remember why you had to return a changed error value and not real NS_MSG_ERROR_INVALID_FOLDER_NAME ?
Flags: needinfo?(mozilla)
(In reply to neil@parkwaycc.co.uk from comment #6) > I think all the public methods should bail out as soon as possible if an > empty folder name is provided. Which ones do we consider public here? But I assume the internal ones should check for it too, just without any dialog, just return value. > I'm not too keen on the local folder throwing > an alert message though, but that's probably because the imap folder ends up > doing it async as a server alert anyway; the utility code shouldn't try to > alert. Do you talk about the nsMsgDBFolder::CheckIfFolderExists dialog addition? It already shows a dialog in other error case. So I don't think I change any dialog behaviour anywhere. I only add dialogs where already other dialogs are used. Or please be more specific.
Flags: needinfo?(neil)
Comment 9•11 years ago
|
||
(In reply to aceman from comment #8) > (In reply to comment #6) > > I think all the public methods should bail out as soon as possible if an > > empty folder name is provided. > Which ones do we consider public here? Anything from a scriptable interface (i.e. most of the NS_IMETHODIMP ones). > > I'm not too keen on the local folder throwing > > an alert message though, but that's probably because the imap folder ends up > > doing it async as a server alert anyway; the utility code shouldn't try to > > alert. > Do you talk about the nsMsgDBFolder::CheckIfFolderExists dialog addition? It > already shows a dialog in other error case. Sorry, that wasn't obvious from the patch. (Given the other checks, I'm now unsure as to how that existing alert would get triggered.)
Flags: needinfo?(neil)
Comment 10•11 years ago
|
||
I'll agree with Neil here, the earlier things fail, the better.
Flags: needinfo?(Pidgeot18)
Assignee | ||
Comment 11•11 years ago
|
||
Which would mean in this case of empty name we show no dialog but in other cases we do. I find that inconsistent. But on the other hand we prevent to send and empty name in the UI. So if an empty name is sent it means it was from internal code and that probably only cares about the error value and no dialog.
Comment 12•11 years ago
|
||
Comment on attachment 777409 [details] [diff] [review] Patch Review of attachment 777409 [details] [diff] [review]: ----------------------------------------------------------------- ::: mailnews/local/src/nsLocalMailFolder.cpp @@ +531,5 @@ > NS_IMETHODIMP > nsMsgLocalMailFolder::CreateSubfolder(const nsAString& folderName, nsIMsgWindow *msgWindow) > { > + if (folderName.IsEmpty()) > + return NS_ERROR_FAILURE; As aceman uses in his proposed patch, we have a specific error code NS_MSG_ERROR_INVALID_FOLDER_NAME you should use in this case.
Attachment #777409 -
Flags: feedback?(acelists)
Comment 13•11 years ago
|
||
Comment on attachment 777423 [details] [diff] [review] patch (alternative) Review of attachment 777423 [details] [diff] [review]: ----------------------------------------------------------------- Subject to the comments below, I prefer this approach. My main concern is that we make sure the UI properly shows that the folder creation failed because the name was invalid. ::: mailnews/base/util/nsMsgDBFolder.cpp @@ +3799,5 @@ > + // Empty subfoldername can be considered a request for the parent, > + // which does exist. But return a different error code > + // if any caller is interested. > + ThrowAlertMsg("folderExists", msgWindow); > + return NS_MSG_ERROR_INVALID_FOLDER_NAME; I don't think we should check for validity in CheckIfFolderExists; we won't find an existing folder if the name is invalid and that's OK. It also leads to an issue I note below in CreateSubfolderInternal. ::: mailnews/local/src/nsLocalMailFolder.cpp @@ +554,4 @@ > nsCOMPtr<nsIMsgPluggableStore> msgStore; > rv = GetMsgStore(getter_AddRefs(msgStore)); > NS_ENSURE_SUCCESS(rv, rv); > rv = msgStore->CreateFolder(this, folderName, aNewFolder); Do the protocol-specific CreateFolder methods get called through any other paths? If not, you can do the validity check before this msgStore->CreateFolder() call rather than duplicating it in each class. @@ +559,5 @@ > { > ThrowAlertMsg("folderCreationFailed", msgWindow); > // I'm returning this value so the dialog stays up > + // ACE: I'd like to return NS_MSG_ERROR_INVALID_FOLDER_NAME here to match the other server types. > + return NS_MSG_ERROR_INVALID_FOLDER_NAME; Two things: if CheckIfFolderExists returns NS_MSG_ERROR_INVALID_FOLDER_NAME, we will have already exited this function early by "if NS_FAILED(rv) return rv". Aside from this, make sure changing the return value has the desired result as described in the existing comment - we need to tell the user that the folder name was not valid, and leave the dialog up. ::: mailnews/local/src/nsMsgBrkMBoxStore.cpp @@ +83,5 @@ > > // Now we have a valid directory or we have returned. > // Make sure the new folder name is valid > + if (aFolderName.IsEmpty()) > + return NS_MSG_ERROR_INVALID_FOLDER_NAME; Do this test before the CreateDirectoryForFolder(path) call above, to make sure we don't create the parent directory before we return failure. ::: mailnews/local/src/nsMsgMaildirStore.cpp @@ +181,5 @@ > NS_ENSURE_SUCCESS(rv, rv); > > // Make sure the new folder name is valid > + if (aFolderName.IsEmpty()) > + return NS_MSG_ERROR_INVALID_FOLDER_NAME; Same comment about CreateDirectoryForFolder()
Attachment #777423 -
Flags: feedback+
Updated•11 years ago
|
Flags: needinfo?(irving)
Updated•11 years ago
|
Attachment #777409 -
Attachment is obsolete: true
Updated•11 years ago
|
Assignee: nobody → acelists
Assignee | ||
Comment 14•11 years ago
|
||
I am not sure which dialog nsMsgLocalMailFolder::CreateSubfolderInternal is talking about (as e.g. in TB rightclick -> new folder does not call CreateSubFolder unless a nonempty string is input). It also seems to me that until now CreateFolder() never returned NS_MSG_ERROR_INVALID_FOLDER_NAME so that path was never taken. Does anybody have an idea?
Attachment #777381 -
Attachment is obsolete: true
Attachment #777423 -
Attachment is obsolete: true
Attachment #822484 -
Flags: feedback?(neil)
Attachment #822484 -
Flags: feedback?(irving)
Comment 15•11 years ago
|
||
(In reply to aceman from comment #14) > I am not sure which dialog nsMsgLocalMailFolder::CreateSubfolderInternal is > talking about (as e.g. in TB rightclick -> new folder does not call > CreateSubFolder unless a nonempty string is input). > It also seems to me that until now CreateFolder() never returned > NS_MSG_ERROR_INVALID_FOLDER_NAME so that path was never taken. Does anybody > have an idea? True, the new folder dialog validates its input so that you can't create a folder with an empty name. But the comment still applies to the case when the you create a folder with an invalid name. In this case createSubfolder throws an exception and I assume this prevents the dialog from closing.
Comment 16•11 years ago
|
||
Comment on attachment 822484 [details] [diff] [review] patch v2 The C++ changes look reasonable to me; I didn't look at the test. > if (rv == NS_MSG_ERROR_INVALID_FOLDER_NAME) > { > ThrowAlertMsg("folderCreationFailed", msgWindow); > // I'm returning this value so the dialog stays up >- return NS_MSG_FOLDER_EXISTS; >+ // ACE: I'd like to return NS_MSG_ERROR_INVALID_FOLDER_NAME here to match the other server types. >+ return NS_MSG_ERROR_INVALID_FOLDER_NAME; > } > if (rv == NS_MSG_FOLDER_EXISTS) > { > ThrowAlertMsg("folderExists", msgWindow); > return NS_MSG_FOLDER_EXISTS; > } Actually I'd prefer these to return rv; in a followup we should probably move the alert to the front end.
Attachment #822484 -
Flags: feedback?(neil) → feedback+
Assignee | ||
Comment 17•11 years ago
|
||
(In reply to neil@parkwaycc.co.uk from comment #15) > (In reply to aceman from comment #14) > > I am not sure which dialog nsMsgLocalMailFolder::CreateSubfolderInternal is > > talking about (as e.g. in TB rightclick -> new folder does not call > > CreateSubFolder unless a nonempty string is input). > > It also seems to me that until now CreateFolder() never returned > > NS_MSG_ERROR_INVALID_FOLDER_NAME so that path was never taken. Does anybody > > have an idea? > > True, the new folder dialog validates its input so that you can't create a > folder with an empty name. But the comment still applies to the case when > the you create a folder with an invalid name. In this case createSubfolder > throws an exception and I assume this prevents the dialog from closing. But why would we need to return a specific error value NS_MSG_FOLDER_EXISTS to make the dialog stick? Yes, the exception is there and is logged to the Error console. Wouldn't it be cleaner to catch it and handle properly in the dialog? (In reply to neil@parkwaycc.co.uk from comment #16) > Actually I'd prefer these to return rv; Yes, that is what I'd like too. > in a followup we should probably > move the alert to the front end. OK.
Assignee | ||
Comment 18•11 years ago
|
||
OK, I cleaned it up a bit.
Attachment #822484 -
Attachment is obsolete: true
Attachment #822484 -
Flags: feedback?(irving)
Attachment #829494 -
Flags: review?(neil)
Comment 19•11 years ago
|
||
Comment on attachment 829494 [details] [diff] [review] patch v3 >- if (rv == NS_MSG_ERROR_INVALID_FOLDER_NAME) >+ if (NS_FAILED(rv)) > { >- ThrowAlertMsg("folderCreationFailed", msgWindow); >- // I'm returning this value so the dialog stays up >- return NS_MSG_FOLDER_EXISTS; >+ if (rv == NS_MSG_ERROR_INVALID_FOLDER_NAME) >+ ThrowAlertMsg("folderCreationFailed", msgWindow); >+ >+ if (rv == NS_MSG_FOLDER_EXISTS) >+ ThrowAlertMsg("folderExists", msgWindow); >+ >+ return rv; > } >- if (rv == NS_MSG_FOLDER_EXISTS) >- { >- ThrowAlertMsg("folderExists", msgWindow); >- return NS_MSG_FOLDER_EXISTS; >- } > > nsCOMPtr<nsIMsgFolder> child = *aNewFolder; > >- if (NS_SUCCEEDED(rv)) >- { >- //we need to notify explicitly the flag change because it failed when we did AddSubfolder >- child->OnFlagChange(mFlags); >- child->SetPrettyName(folderName); //because empty trash will create a new trash folder >- NotifyItemAdded(child); >- if (aNewFolder) >- child.swap(*aNewFolder); >- } >- return rv; >+ // We need to notify explicitly the flag change because it failed >+ // when we did AddSubfolder. >+ child->OnFlagChange(mFlags); >+ child->SetPrettyName(folderName); //because empty trash will create a new trash folder >+ NotifyItemAdded(child); >+ if (aNewFolder) >+ child.forget(aNewFolder); >+ >+ return NS_OK; Sorry, I didn't really want you to rewrite the logic here. Can you not just fix up the returns, or in fact you can just remove them, as there's already a return rv; at the end?
Assignee | ||
Comment 20•11 years ago
|
||
OK.
Attachment #829494 -
Attachment is obsolete: true
Attachment #829494 -
Flags: review?(neil)
Attachment #829733 -
Flags: review?(neil)
Flags: needinfo?(mozilla)
Comment 21•11 years ago
|
||
Comment on attachment 829733 [details] [diff] [review] patch v4 >- nsCOMPtr<nsIMsgFolder> child = *aNewFolder; [Eek, how did I not notice this while reviewing bug 402392 :-( Earlier code should say nsCOMPtr<nsIMsgFolder> child; rv = msgStore->CreateFolder(this, folderName, getter_AddRefs(child)); Then in the success block the value is swapped into the outparam.]
Attachment #829733 -
Flags: review?(neil) → review+
Assignee | ||
Comment 22•11 years ago
|
||
Neil, am I supposed to change something due to the nit? :)
Flags: needinfo?(neil)
Assignee | ||
Comment 23•11 years ago
|
||
Comment on attachment 829733 [details] [diff] [review] patch v4 Review of attachment 829733 [details] [diff] [review]: ----------------------------------------------------------------- Standard8, please see the test change.
Attachment #829733 -
Flags: review?(mbanner)
Comment 24•11 years ago
|
||
(In reply to aceman from comment #22) > Neil, am I supposed to change something due to the nit? :) Not for this bug.
Flags: needinfo?(neil)
Updated•10 years ago
|
Attachment #829733 -
Flags: review?(mbanner) → review+
Comment 26•10 years ago
|
||
https://hg.mozilla.org/comm-central/rev/004bbca2f2bd
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Flags: in-testsuite+
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 29.0
You need to log in
before you can comment on or make changes to this bug.
Description
•