Closed Bug 646993 Opened 13 years ago Closed 13 years ago

Adding a bookmark with a long title causes a hang

Categories

(Firefox :: Bookmarks & History, defect)

Product:
Firefox
Component:
Bookmarks & History
Type:
defect
Priority:
Not set
Severity:
critical

Tracking

()

Status:
RESOLVED FIXED
Milestone:
Firefox 10

People

(Reporter: sascha.neufeldt, Assigned: MattN)

References

(
URL
)

Details

(Keywords: hang, Whiteboard: [sg:moderate])

Attachments

(5 files, 3 obsolete files)

Test file for adding an bookmark without accepting it
467 bytes, text/html
Details
Stack Trace 1 nsNavBookmarks::InsertBookmarkInDB -> mozilla::storage
72.92 KB, text/plain
Details
Stack Trace 1 - JS portion
1.19 KB, text/plain
Details
Storage logging
85.40 KB, text/plain
Details
v.3 Cap bookmark title length at TITLE_LENGTH_MAX
15.63 KB, patch
mak
: review+
Details | Diff | Splinter Review 
User-Agent:       Mozilla/5.0 (Windows NT 6.1; WOW64; rv:2.0) Gecko/20100101 Firefox/4.0
Build Identifier: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:2.0) Gecko/20100101 Firefox/4.0

It is possible to open the "Add to Bookmark" dialog with JavaScript:
window.sidebar.addPanel(title, url, "");
but this function isn't safe at the moment. 

If the inserted "title" is really long (look at my example, where a long title is generated), the browser will crash and adds the url to your bookmarks without showing the dialog and accepting it. This can be used to add a link which leads to an "modified" online banking Site / phishing site or any other bad site. 

This can be probably used to insert malicious code.

Reproducible: Always

Steps to Reproduce:
1.Click on a Link (which has a javascript function on it for bookmarking a site --> window.sidebar.addPanel(title, url, "");)
2.Title is a really long String 
3.Click on the link with the javascript 

Actual Results:  
The Browser crashes, because he cant handle the long input and the site defined in "url" will be added to the bookmarks (without accepting it)

Expected Results:  
Check that the inserted title isnt that long and dont add the bookmark without accepting it.

Here is the Code for testing this:

<html>
<head>
<title>
Testpage
</title>
</head>
<body>
<script type="text/javascript">
function bookmark(){

var title = "BadDomain";
var url = "http://google.com";

var count = 23;
for(var i = 0; i < count; i++){
    title = title + title;
}

//if Firefox
if (window.sidebar) 
    window.sidebar.addPanel(title, url, "");
}
</script>


<a href="javascript:bookmark()">Bookmark the Site described without accepting it!</a>

</body>
</html>
>the browser will crash 

does it really crash can you provide a crashID (see <https://support.mozilla.com/en-US/kb/Firefox%20crashes>)?
(In reply to comment #2)
> >the browser will crash 
> 
> does it really crash can you provide a crashID (see
> <https://support.mozilla.com/en-US/kb/Firefox%20crashes>)?

Hello Ludovic,
it doesnt really "crash", but its freezing / not responding.
(In reply to comment #3)

> Hello Ludovic,
> it doesnt really "crash", but its freezing / not responding.

can you provide  a stack trace as described at https://developer.mozilla.org/en/how_to_get_a_stacktrace_with_windbg ?
Keywords: hang
Attached file Stacktrace (obsolete) — 
Stacktrace like described by comment #3
(In reply to comment #5)
> Stacktrace like described by comment #3
Sadly, symbols weren't found when you generated that, so it's not terribly useful.
Shawn: forget the hang (for now), we really shouldn't be adding bookmarks the user hasn't OK'd. After you kill the process and restart, there it is.
Assignee: nobody → sdwilsh
Status: UNCONFIRMED → NEW
Ever confirmed: true
Whiteboard: [sg:moderate]
Here is a stack trace on:
Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:10.0a1) Gecko/20111004 Firefox/10.0a1

I also get two unresponsive script dialogs:
chrome://browser/content/places/browserPlacesViews.js:495
.../bindings/text/...

I clicked to stop the two scripts and eventually the Add Bookmark dialog was displayed.  I then Force Quit Nightly and on restart I saw that the bookmarks was created.
Attachment #523616 - Attachment is obsolete: true
(I just remembered that dveditz said to forget about the hang for now in Comment 7)

Mak/Shawn, is it intentional that we insert the bookmark in the DB before the user confirms it?

I will attach the JS call stack associated with attachment 564997 [details].

Running this again, I saw an unresponsive script dialog for chrome://browser/content/browser.js:1493

which maps to PSB_onItemChanged:
https://mxr.mozilla.org/mozilla-central/source/browser/base/content/browser-places.js#1109

Note to myself of related code:
nsNavBookmarks::InsertBookmarkInDB:
https://mxr.mozilla.org/mozilla-central/source/toolkit/components/places/nsNavBookmarks.cpp#867

mDBInsertBookmark
https://mxr.mozilla.org/mozilla-central/source/toolkit/components/places/nsNavBookmarks.cpp#441

BTW, there is an unused variable minimalUI from what I can tell:
https://mxr.mozilla.org/mozilla-central/source/browser/components/places/src/PlacesUIUtils.jsm#590
OS: Windows 7 → All
Hardware: x86_64 → All

    
    

    
  
(In reply to Matthew N. [:MattN] from comment #9)
> (I just remembered that dveditz said to forget about the hang for now in
> Comment 7)
> 
> Mak/Shawn, is it intentional that we insert the bookmark in the DB before
> the user confirms it?

Yes, all bookmarks dialog are instant apply and revert on Cancel. Changing this now may be really time consuming.
The issue here is that we should not expose a method to add a bookmark to content, addPanel was implemented for parity-opera but is mostly abused, and we already plan to drop sidebar panels support (bug 560305)

I don't see why this should freeze though, we crop long uris and long titles to 256 chars iirc.
Sorry but I was not aware of this bug so I couldn't investigate it.

    
    

    
  

      
      
      
      

        Attached file
          
        Storage logging
        — 
      

    


  
(In reply to Marco Bonardo [:mak] from comment #11)
> I don't see why this should freeze though, we crop long uris and long titles
> to 256 chars iirc.

I don't find any code that does this.  Only 
#define URI_LENGTH_MAX 65536
#define TITLE_LENGTH_MAX 4096
in nsNavHistory.cpp which doesn't seem to be used for inserts into moz_bookmarks.  The whole string was inserted into the table and caused my places.sqlite file to increase by over 100MB.

I actually think one of the causes of a hang is nsLineBreaker with the long string (75497472 characters) that the test case creates:

#0  0x00000001014a8519 in nsTArray<unsigned char, nsTArrayDefaultAllocator>::Elements (this=0x7fff5fbee830) at nsTArray.h:498
#1  0x00000001011e6a12 in nsTArray<unsigned char, nsTArrayDefaultAllocator>::ElementAt (this=0x7fff5fbee830, i=75402033) at nsTArray.h:514
#2  0x00000001011e6a39 in nsTArray<unsigned char, nsTArrayDefaultAllocator>::operator[] (this=0x7fff5fbee830, i=75402033) at nsTArray.h:546
#3  0x0000000101723274 in nsLineBreaker::AppendText (this=0x7fff5fbf2388, aLangGroup=0x10e005910, aText=0x18e802000 "BadDomainBadDomainBadDomainBadDomainBadDomainBadDomainBadDomainBadDomainBadDomainBadDomainBadDomainBadDomainBadDomainBadDomainBadDomainBadDomainBadDomainBadDomainBadDomainBadDomainBadDomainBadDomainBa"..., aLength=75497472, aFlags=7, aSink=0x111da9340) at /Users/matthew/mozilla-central/content/base/src/nsLineBreaker.cpp:407

I'll attach a patch to trim bookmarks to the lengths in the constants above.
Assignee: sdwilsh → mnoorenberghe+bmo
Status: NEW → ASSIGNED

    
    

    
  


  
This reduces this specific hang to less than a second for me and is something we thought we were doing anyways.

I put the length cap in the functions that modify the DB so that all callers don't need to make this change but the downside is that the observer notifications are sent with the input title rather than the actual capped value inserted.  Does it make sense to cap the title in both places ie. InsertBookmarkInDB and InsertBookmark?

Should I do something different to have TITLE_LENGTH_MAX defined once instead of making a new const in the test?

        Attachment #569202 -
        Flags: review?(mak77)

    
    

    
  
(In reply to Matthew N. [:MattN] from comment #13)
> Created attachment 569202 [details] [diff] [review] [diff] [details] [review]
> Cap bookmark title length at TITLE_LENGTH_MAX
> 
> This reduces this specific hang to less than a second for me and is
> something we thought we were doing anyways.

seems to make sense, will look into the patch soon,I would also like to check if other code paths are correctly limiting the length. Will better answer your questions then.

> I put the length cap in the functions that modify the DB so that all callers
> don't need to make this change but the downside is that the observer
> notifications are sent with the input title rather than the actual capped
> value inserted.

This may be a perf problem yet, just think about Sync for example. I'll take a look if there's a better way to get correct notifications too.

> Does it make sense to cap the title in both places ie.
> InsertBookmarkInDB and InsertBookmark?

The former is a common entry point for all kind of bookmarks (thus also folders, for example), but the only "automatic" bookmarks we add as a web panel are the ones entering through InsertBookmark.
Still, I'd be fine with limiting titles for all kind of entries provided we add a @note in the idl javadocs for the affected APIs.

> Should I do something different to have TITLE_LENGTH_MAX defined once
> instead of making a new const in the test?

You can put the const in head_common.js, that is shared across all Places xpcshell tests automatically.

    
    

    
  
Comment on attachment 569202 [details] [diff] [review]
Cap bookmark title length at TITLE_LENGTH_MAX

Review of attachment 569202 [details] [diff] [review]:
-----------------------------------------------------------------

I checked history entries and they seem to correctly cap the title, so we are fine there.

Clearing review for now, since there is still some work to do.

::: toolkit/components/places/nsNavBookmarks.cpp
@@ +908,5 @@
>    // Support NULL titles.
>    if (aTitle.IsVoid())
>      rv = stmt->BindNullByName(NS_LITERAL_CSTRING("item_title"));
>    else
> +    rv = stmt->BindUTF8StringByName(NS_LITERAL_CSTRING("item_title"), StringHead(aTitle, TITLE_LENGTH_MAX));

After evaluating, I prefer if you make a new capped title substring in the callers (insertBookmark and createContainerWithID) and pass it to insertBookmarkInDB, so that we notify the correct final title.

::: toolkit/components/places/tests/bookmarks/test_bookmarks.js
@@ +648,5 @@
>    hs.addVisit(uri1, Date.now() * 1000, null, hs.TRANSITION_TYPED, false, 0);
>  
> +  // bug 646993 - test bookmark titles longer than the maximum allowed length
> +  const TITLE_LENGTH_MAX = 4096;
> +  let title15 = Array(TITLE_LENGTH_MAX /* TITLE_LENGTH_MAX */ + 5).join("X");

as said, please move the constant to head_common.js

why there is that apparently-useless comment in there?

@@ +652,5 @@
> +  let title15 = Array(TITLE_LENGTH_MAX /* TITLE_LENGTH_MAX */ + 5).join("X");
> +  let newId15 = bs.insertBookmark(testRoot, uri("http://evil.com/"),
> +                                  bs.DEFAULT_INDEX, title15);
> +
> +  do_check_eq(bs.getItemTitle(newId15).length, title15.substring(0, TITLE_LENGTH_MAX).length);

nit: second arg to newline to crop at 80 chars

@@ +655,5 @@
> +
> +  do_check_eq(bs.getItemTitle(newId15).length, title15.substring(0, TITLE_LENGTH_MAX).length);
> +  // test title length after updates
> +  bs.setItemTitle(newId15, title15 + " updated");
> +  do_check_eq(bs.getItemTitle(newId15).length, title15.substring(0, TITLE_LENGTH_MAX).length);

nit: ditto

please also test .createFolder() for capping folder titles

        Attachment #569202 -
        Flags: review?(mak77)

    
    

    
  


  
Thanks for the quick review.  I have tried to address all of your review comments.  One thing I considered was using my new helper "TruncateTitle" in the existing history title truncation code but I wanted to get review on this approach first.

        Attachment #569202 -
        Attachment is obsolete: true

        Attachment #569570 -
        Flags: review?(mak77)

    
    

    
  
Comment on attachment 569570 [details] [diff] [review]
v.2 Cap bookmark title length at TITLE_LENGTH_MAX

Review of attachment 569570 [details] [diff] [review]:
-----------------------------------------------------------------

I likely bitrotted you on mozilla-inbound, since I touched 90% of Places cpp/h files, thus I think the patch may need a refresh (just matter of reinserting in the right position)

Some minor things yet:

::: toolkit/components/places/Helpers.cpp
@@ +352,5 @@
>    return true;
>  }
>  
>  void
> +TruncateTitle(const nsACString& aTitle, nsACString& aTrimmed) {

function brace on new line please, according to the module (And global) style

@@ +353,5 @@
>  }
>  
>  void
> +TruncateTitle(const nsACString& aTitle, nsACString& aTrimmed) {
> +  if (aTitle.IsVoid()) {

Rather than doing this, in the more common case of strings not needing a cap, we can likely avoid a string copy by just assigning. So, rather than doing this, you may check if string length is < TITLE_LENGTH_MAX and in such a case just assign.

::: toolkit/components/places/Helpers.h
@@ +222,5 @@
>   */
>  bool IsValidGUID(const nsCString& aGUID);
>  
>  /**
> + * Truncates the title if it's too long.

add a reference to TITLE_LENGTH_MAX in this javadoc

::: toolkit/components/places/nsINavBookmarksService.idl
@@ +477,5 @@
>     *         The id of the item whose title should be updated 
>     *  @param aTitle
>     *         The new title for the bookmark.
> +   *
> +   *  @note  aTitle will be truncated to TITLE_LENGTH_MAX

nit: closing period

::: toolkit/components/places/nsNavBookmarks.h
@@ +209,5 @@
> +  /**
> +   * @note If aFolder is -1, uses the autoincrement id for folder index.
> +   * @return index of the new folder in aIndex, whether it was passed in or
> +   *         generated by autoincrement.
> +   * @note aTitle will be truncated to TITLE_LENGTH_MAX

nit: put the @return before both the @note(s)

::: toolkit/components/places/tests/bookmarks/test_bookmarks.js
@@ +652,5 @@
> +  let newId15 = bs.insertBookmark(testRoot, uri("http://evil.com/"),
> +                                  bs.DEFAULT_INDEX, title15);
> +
> +  do_check_eq(bs.getItemTitle(newId15).length,
> +              title15.substring(0, TITLE_LENGTH_MAX).length);

we should also check what the observer gets.
Now, I know this test sucks (someday someone will rewrite it), but you may use the global bookmarksObserver, onItemAdded you may save this._itemTitle = title; and then here you may check bookmarksObserver._itemTitle

@@ +656,5 @@
> +              title15.substring(0, TITLE_LENGTH_MAX).length);
> +  // test title length after updates
> +  bs.setItemTitle(newId15, title15 + " updated");
> +  do_check_eq(bs.getItemTitle(newId15).length,
> +              title15.substring(0, TITLE_LENGTH_MAX).length);

ditto here onItemChanged with property == "title" should give you the title in the value param.

@@ +700,5 @@
>    let folder = bs.createFolder(parent, "test folder", bs.DEFAULT_INDEX);
>    bs.setItemTitle(folder, "test folder");
>  
> +  let longName = Array(TITLE_LENGTH_MAX + 5).join("A");
> +  let folderLongName = bs.createFolder(parent, longName, bs.DEFAULT_INDEX);

ditto: check the observer here.

@@ +736,5 @@
> +
> +  // update with another long title
> +  bs.setItemTitle(folderLongName, longName + " updated");
> +  result = hs.executeQuery(query, options);
> +  rootNode = result.root;

fwiw, here you may just have rootNode = hs.executeQuery(query, options).root;
but likely you don't need all of this code, see below.

@@ +739,5 @@
> +  result = hs.executeQuery(query, options);
> +  rootNode = result.root;
> +  rootNode.containerOpen = true;
> +  node = rootNode.getChild(3);
> +  do_check_eq(node.title, longName.substring(0, TITLE_LENGTH_MAX));

I think you should not need to close and reopen the container, we have live updates.

and, if you use liveupdate, you likely don't need to also check the observer, since it drives the liveupdate stuff. But bonus points if you also want to do that.

        Attachment #569570 -
        Flags: review?(mak77)

    
    

    
  


  
Here is an updated patch addressing the review comments and rebased on top of your changes.

        Attachment #569570 -
        Attachment is obsolete: true

        Attachment #570207 -
        Flags: review?(mak77)

    
    

    
  
Comment on attachment 570207 [details] [diff] [review]
v.3 Cap bookmark title length at TITLE_LENGTH_MAX

Review of attachment 570207 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good, thank you for fixing this!

::: toolkit/components/places/Helpers.cpp
@@ +355,5 @@
>  void
> +TruncateTitle(const nsACString& aTitle, nsACString& aTrimmed)
> +{
> +  aTrimmed = aTitle;
> +  if (!aTitle.IsVoid() && aTitle.Length() > TITLE_LENGTH_MAX) {

doesn't a void string have length 0 (Thus the first test is useless)?

::: toolkit/components/places/tests/bookmarks/test_bookmarks.js
@@ +661,5 @@
> +  do_check_eq(bs.getItemTitle(newId15).length,
> +              title15.substring(0, TITLE_LENGTH_MAX).length);
> +  do_check_eq(bookmarksObserver._itemChangedId, newId15);
> +  do_check_eq(bookmarksObserver._itemChangedProperty, "title");
> +  do_check_eq(bookmarksObserver._itemChangedValue, title15.substring(0, TITLE_LENGTH_MAX));

nit: put title15.substring(0, TITLE_LENGTH_MAX) in a temp var and use that, rather than substring everywhere

        Attachment #570207 -
        Flags: review?(mak77) → review+

    
    

    
  
https://hg.mozilla.org/integration/mozilla-inbound/rev/1a52298b6718
Flags: in-testsuite+
Summary: Adding a Bookmark on link click without accepting it → Adding a bookmark with a long title causes a hang
Target Milestone: --- → Firefox 10

    
    

    
  
thank you for taking care of this

https://hg.mozilla.org/mozilla-central/rev/1a52298b6718
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED

    
  
Group: core-security → core-security-release
Group: core-security-release

          You need to log in
          before you can comment on or make changes to this bug.
        






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: