373208 - "mailnews.reply_header_ondate" incorrect results

Status: NEW

(MailNews Core :: Composition, defect)

Description

[Neil]

User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.8.1.2) Gecko/20060601 Firefox/2.0.0.2 (Ubuntu-edgy)
Build Identifier: 1.5.0.10 (20070306)

When changing "mailnews.reply_header_ondate" to "On %a, %d  %b  %Y  %H:%M:%S  %z" (which is the strftime format for RFC-822-style dates), Thunderbird crashes when you try to reply to a mail.

Reproducible: Always

Steps to Reproduce:
1. Change "mailnews.reply_header_ondate" to "On %a, %d  %b  %Y  %H:%M:%S  %z".
2. Try to reply to an email.
3. Look at your desktop background.
Actual Results:  
Crashes.

Expected Results:  
Well, my comment here explains my ideal world: https://bugzilla.mozilla.org/show_bug.cgi?id=107884

But just as a bug fix, if a bad format is given, Thunderbird should use the default instead of crashing.

Comment 1

[Adam Guthrie]

Any sort of stack for the crash here would be nice. http://kb.mozillazine.org/Getting_a_stacktrace_with_gdb

Comment 2

[timeless]

stack would relate to nsTextFormatter::smprintf

smprintf assumes trusted input. the input here is very untrusted. My approach is to write a js component that implements smprintf because it can be much smarter and handle sucky things like this. Simon should remember that bug.

The crash would be stack or heap corruption probably, so it's not the kind of thing you would usefully signature. Anyway. In short: "Don't do that!" we never said you could, and if it hurts when you shoot yourself in the foot, don't.

I know of no place where we said the preference was strftime, as it happens, it's smprintf.

Comment 3

[Neil]

Yeah, well, I ranked it minor originally because the workaround is simply not to do it.  Clearly strftime isn't supposed here, so I just went back to defaults.

Stacktrace is coming...

Quick question, is there any disadvantage to running the debug version of TB instead of the normal one for everyday use?

Comment 4

[timeless]

it'll be slower, and on Windows you'll get lots of dialogs you can turn off w/ the registry. on Linux you'll get lots of beeps if you don't silence them in some way shape or form.

Otherwise, it should be fine. I used to do it w/ Mozilla long ago.

The characteristics of your mailbox are more relevant.

Comment 5

[Andrew Schultz]

> I know of no place where we said the preference was strftime, as it 
> happens, it's smprintf.

And beyond that, your string can contain only a single "%s" (and text).  
The smprintf implementation has no way to tell how many parameters are passed (it only gets one) and happily walks off the end.

The date is formatted according to your locale.

Comment 6

[Neil]

Yeah, I know no one said it was strftime.  I was just playing around with it, and that's what came to mind first (I didn't know it was smprintf).

Is there a particular reason not to change it to strftime?  It'd be a lot more flexible.

Comment 7

[timeless]

well, for one, it'd break anyone currently using the pref :). which means writing migration code.

beyond that, i don't own the code in question.

Comment 8

[Neil]

As you know, "mailnews.reply_header_type" is what determines which strings it uses when crafting the reply header.  Currently:
1 - "[Author] wrote:"
2 - "On [date] [author] wrote:"
3 - User-defined reply header.
With 3 being used with custom "mailnews.reply_header_authorwrote" and "mailnews.reply_header_ondate" prefs.

I visualized just adding an option 4 to reply_header_type, which would use a new pref string, maybe, reply_full_custom, which would have the strftime options plus new letters for [author] and [author-email].

That way it wouldn't break anyone's prefs since no one is using an option 4 right now.

Do you think that's feasible?

Comment 9

[timeless]

kinda messy. not technically impossible. not something i'd personally want to implement. parsing once is imo bad enough.

that said, i'm not a module owner and wouldn't reject such a thing. although explaining such a feature to users worries me.

Comment 10

[Neil]

Forgive me if this seems obvious to you; Thunderbird is well beyond the scope of any program I've ever written...

Why is it messy?  And why is it parsing twice in this case?

Comment 11

[timeless]

because strftime and snprintf both do parsing, and reinventing either or both of them is not something we really want to do. to do what you want, we'd either have to roll our own (which means handling all the wonderful edge cases of both) or send it through to both (which means making sure that neither of them have any other amusing unintended consequences).

it's just moderately messy, again, not impossible, just messy.

I'm not opposed, just not volunteering.

FWIW, you wrote "As you know," and as it happens, I didn't. I deal in crashes, especially amusing crashes (and this counts). So I thank you for informing me about how the code-path works, because otherwise I'd have had to figure it out by reading the code (which is not something I enjoy).

Here are some fun pieces of input:
* you reply to someone whose real name is: "yibber%s yabber%t snippe%tt" and whose email address is <[e-mail]>

So, perhaps I should restate the number of parsings as closer to 3 not 2. :)

Comment 12

[Dan Kreene]

(In reply to comment #8)
> I visualized just adding an option 4 to reply_header_type

seems like it would open up some fun add-ons

Comment 13

[Wayne Mery (:wsmwk)]

I can't reproduce crash with Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9.1.5pre) Gecko/20091013 Shredder/3.0pre

Comment 14

[Neil]

No longer crashes, but still displays some undesirable behavior.

Using the string I used previously[1] resulted in (what I suspect is) the %d showing 4 bytes of the date in numerical form, and then the later %s showing gibberish from memory.

But it did not crash, and replies were apparently still possible (I didn't actually try sending it with all that).

While I've sort of given up on having a 4th reply type option; I might still like to see some input validation. On the other hand, this /is/ about:config, there's already a disclaimer on the front that bad stuff can happen if you mess around in there, and that's probably sufficient. Protecting users from the stupid things they (I) do should only have to go so far.

Either way, I think we can close this bug, but I'll leave the final decision to people with more experience than me.

1. ie. changing "mailnews.reply_header_ondate" to "On %a, %d %b %Y %H:%M:%S %z".

Comment 15

[Neil]

Oh, and last comment was using Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1.8) Gecko/20100227 Thunderbird/3.0.3