Closed Bug 831190 Opened 11 years ago Closed 10 years ago

Passing empty string to nsIMsgFolder.createSubfolder() crashes Thunderbird

Categories

(MailNews Core :: Database, defect)

All
Linux
defect
Not set
critical

Tracking

(Not tracked)

RESOLVED FIXED
Thunderbird 29.0

People

(Reporter: paulius.zaleckas, Assigned: aceman)

Details

(Keywords: crash)

Attachments

(1 file, 5 obsolete files)

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.
Severity: normal → critical
Keywords: crash
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?
Attached patch test (obsolete) — Splinter Review
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
Attached patch Patch (obsolete) — Splinter Review
A very simple possible patch.
Attachment #777409 - Flags: feedback?(acelists)
Attached patch patch (alternative) (obsolete) — Splinter Review
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)
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)
(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)
I'll agree with Neil here, the earlier things fail, the better.
Flags: needinfo?(Pidgeot18)
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 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 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+
Flags: needinfo?(irving)
Attachment #777409 - Attachment is obsolete: true
Assignee: nobody → acelists
Attached patch patch v2 (obsolete) — Splinter Review
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)
(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 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+
(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.
Attached patch patch v3 (obsolete) — Splinter Review
OK, I cleaned it up a bit.
Attachment #822484 - Attachment is obsolete: true
Attachment #822484 - Flags: feedback?(irving)
Attachment #829494 - Flags: review?(neil)
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?
Attached patch patch v4Splinter Review
OK.
Attachment #829494 - Attachment is obsolete: true
Attachment #829494 - Flags: review?(neil)
Attachment #829733 - Flags: review?(neil)
Flags: needinfo?(mozilla)
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+
Neil, am I supposed to change something due to the nit? :)
Flags: needinfo?(neil)
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)
(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)
Attachment #829733 - Flags: review?(mbanner) → review+
Thanks.
Keywords: checkin-needed
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.