788741 - Crash in nsWebBrowserPersist::CalculateAndAppendFileExt() with too long file extension

Status: RESOLVED FIXED

(Core Graveyard :: Embedding: APIs, defect)

Description

[Tim Taubert [:ttaubert] (inactive)]

I can easily crash Firefox by opening http://hbr.org/2012/09/too-many-pivots-too-little-passion/ar/1 and then trying to save it locally as "Web Page, complete". It fails with the following error:

###!!! ASSERTION: Truncate cannot make string longer: 'newLength <= mLength', file ../../dist/include/nsTSubstring.h, line 494
###!!! ABORT: OOM: file /home/tim/workspace/fx-team/xpcom/string/src/nsTSubstring.cpp, line 533

--------------

The problem here is that the URL

http://hbr.org.woopra-ns.com/visit/ra=0JC9SNP0FDQXBK2ST4VOZNAQMDC9FBF6&alias=hbr.org&cookie=&meta=NDgyMjgzNDMmNyY3JjAmMTM0Njg2MDA1MTY0NyYxMzQ2ODcwNDk4NTQwJiYxMDAmJjImJiYm&screen=1366x768&language=en-US&referer=&idle=0&vs=r&ce_type=pageview&ce_title=Too%20Many%20Pivots%2C%20Too%20Little%20Passion%0A%09%09%09%09%20-%20Harvard%20Business%20Review&ce_url=%2F2012%2F09%2Ftoo-many-pivots-too-little-passion%2Far%2F1&ce_name=pv

produces a fileName "ra0JC9SNP0FDQXBK2ST4VOZNAQMDC9FBF6aliashbr" and an extension containing the rest of the URL. The code here tries to truncate the name the file will be saved under to fit into the limit of kDefaultMaxFilenameLength. It doesn't take a too long extension into account.

I wasn't sure what approach should be chosen to fix this. Should we truncate the extension? Should we move as many characters as possible from the extension to the file name? Should we not allow extensions this long at all?

Comment 1

[Scoobidiver (away)]

On Windows: bp-f95096c0-3352-41f2-bf8e-5a1ac2120905.

Comment 2

[Boris Zbarsky [:bzbarsky]]

I think it's safe to truncate the extension to if it's long enough...

Comment 4

[Andrew Quartey [:drexler]]

Attachment: patch

Truncates the file name and/or extension if either is longer than the default file name length.

Comment 5

[Tim Taubert [:ttaubert] (inactive)]

I think we still need to check if |(newFileName.Length() + fileExt.Length() + 1) > kDefaultMaxFilenameLength|. Both parts can be smaller but concatenated they're larger than kDefaultMaxFilenameLength.

Also, Truncate() does probably not do the right thing if you use |newLength| to truncate the whole filename.

Comment 6

[Andrew Quartey [:drexler]]

Comment on attachment 661442 [details] [diff] [review]
patch


Vacating review request as patch is erroneous.

> I think we still need to check if |(newFileName.Length() + fileExt.Length()
> + 1) > kDefaultMaxFilenameLength|. Both parts can be smaller but
> concatenated they're larger than kDefaultMaxFilenameLength.

True. I didn't think about that case. 

> Also, Truncate() does probably not do the right thing if you use |newLength|
> to truncate the whole filename.

In the case where the filename and extension are both longer than the |kDefaultMaxFilenameLength|, what is given priority in fitting the desired length - most of the filename or extension?

Comment 7

[Boris Zbarsky [:bzbarsky]]

What I would recommend is that if the whole name is too long then if extension is > kDefaultMaxFilenameLength/2 then you truncate the extension to kDefaultMaxFilenameLength/2.  After that, truncate the filename as needed.

Comment 8

[Andrew Quartey [:drexler]]

Attachment: patch

Comment 9

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 662209 [details] [diff] [review]
patch

This will still truncate to negative if fileExt is long but newFileName is not, won't it?

Comment 10

[Andrew Quartey [:drexler]]

Attachment: patch

(In reply to Boris Zbarsky (:bz) from comment #9)

> This will still truncate to negative if fileExt is long but newFileName is
> not, won't it?

Oops. It would.

Comment 11

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 662258 [details] [diff] [review]
patch

This, I buy.

Comment 12

[Andrew Quartey [:drexler]]

Sold! https://hg.mozilla.org/integration/mozilla-inbound/rev/e1850b0b8d4a

Comment 13

[Graeme McCutcheon [:graememcc]]

https://hg.mozilla.org/mozilla-central/rev/e1850b0b8d4a