Closed Bug 788741 Opened 7 years ago Closed 7 years ago
Crash in ns
Web Browser Persist::Calculate And Append File Ext() with too long file extension
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?
On Windows: bp-f95096c0-3352-41f2-bf8e-5a1ac2120905.
Severity: normal → critical
Crash Signature: [@ mozalloc_abort | NS_DebugBreak_P | nsACString_internal::SetLength ] → [@ mozalloc_abort | NS_DebugBreak_P | nsACString_internal::SetLength ] [@ mozalloc_abort(char const* const) | NS_DebugBreak_P | nsACString_internal::SetCapacity(unsigned int)]
I think it's safe to truncate the extension to if it's long enough...
Truncates the file name and/or extension if either is longer than the default file name length.
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 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?
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 on attachment 662209 [details] [diff] [review] patch This will still truncate to negative if fileExt is long but newFileName is not, won't it?
Attachment #662209 - Flags: review?(bzbarsky) → review-
(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 on attachment 662258 [details] [diff] [review] patch This, I buy.
Attachment #662258 - Flags: review?(bzbarsky) → review+
Status: NEW → ASSIGNED
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla18
You need to log in before you can comment on or make changes to this bug.