The code in nsDataObj::BuildPlatformHTML(), which is called when copying to the clipboard on Windows, has a slew of evil strcpy, strcat and wsprintf calls. It's also bug-prone, if the copied content contains "<!--EndFrag".

What ain't broke ...

Actually, I take that back. This should be fixed. It's just not high on my list of things to fix, so if there are any volunteers out there ... :-)

There's no overflow risk here. We allocate 400 bytes _plus_ the length of the HTML, which is more than enough to handle the start and end fragments. The concern about a pre-existing "<!-- EndFragment -->" is valid, and can certainly be handled better than we are currently.

Actually, i think we do this wrong anyway. StartFragment:<n> should point to the first character _after_ "<!-- StartFragment -->". If I copy text from Moz and paste it into an HTML message in Outlook Express, the source contains "<!-- StartFragment -->". Filed bug 158103 for that, to be fixed after this is fixed.

It would be so easy to rewrite the code to use simple, safe nsString routines, and eliminate all the strcat, and strstr calls. This code will show up as a hotspot in security code reviews (since the tools look for 'dangerous' routines like strcpy, strcat, and routines in the printf family). Do yourself a favour; rewrite the code, and get it off the radar.

sure, rewrite it. it's nasty code anyway and i'm not terribly proud of it ;)

But if we re-write it, how will we blame Pinkerton for this nasty code? :)

well, we can blame me for uglier code :)

Comment on attachment 91848 [details] [diff] [review] draft oh god no please

Is the crash in bug 158002 related? The stack pasted looks like garbage, but it does mention pasting. Others have had difficulty reproducing it, though.

ok, sfraser wants this to use mozilla string classes. blah

Untested patch more along the lines of what I was thinking.

+ const char* const numPlaceholder = "00000000"; + const char* const startHTMLPrefix = "Version:0.9\r\nStartHTML:"; + const char* const endHTMLPrefix = "\r\nEndHTML:"; + const char* const startFragPrefix = "\r\nStartFragment:"; + const char* const endFragPrefix = "\r\nEndFragment:"; + const char* const endFragTrailer = "\r\n"; Why two consts? Is there any chance of commenting the code that builds the final version. It's not ugly as the current version, but it's still a little confusing to try and read through.

const char* const testString = "foopy"; testString ++; // compile error *testString = 0; // compile error const char* testString2 = "foopy"; testString2 ++; // OK! *testString2 = 0; // compile error

char* testString2 = "foopy"; testString2 ++; // OK! *testString2 = 0; // OK!

char* testString2 = "foopy"; *testString2 = 0; // OK! yes, but this will probably crash your program. You might have got a compiler error, or at least a warning, on the first line.

|const char* testString = "foopy"| is what we typically use; since "foopy" may end up in read-only memory, it's safest to reflect that in your pointer by saying the pointer is of type |const char| and thus whatever it points to can't be changed. However, the pointer itself could still be changed in this case, e.g. to point to another literal. By marking it |const char* const testString = "foopy"| you're not only saying that the literal text can't be changed, you're also saying that you have no intention of changing the pointer. These |const|s will help the compiler catch any mistakes you or someone else makes (|*testString = 0;|) and it allows the compiler to optimize the pointer away where possible.

That's exactly what I was trying to demonstrate by example. Thank you, jag!

Comment on attachment 91866 [details] [diff] [review] Untested patch so does this work, and are we ok with it?

Do you want to address bug 158103 at the same time? All you'd need to do is change how you're calculating startFragOffset.

well since jag isn't working on this, and dean responded first :)

Comment on attachment 91866 [details] [diff] [review] Untested patch Couldn't we do something here with nsPrintfCString("%08u", len)?

At Dean Tessman's suggestion, I incorporated the bug fix for Bug 158103 as well as the cleanup for this bug (Bug 157994) into a patch. This is based on timeless' patch with changes to fix 158103 and to use the cleaner nsPrintfCString() that jag suggested.

Comment on attachment 158507 [details] [diff] [review] Proposed patch to cleanup and fix the win32 HTML Clipboard population >+/* >+ nsDependentCString headerString( >+ "Version:0.9\r\n" >+ "StartHTML:00000000\r\n" // offset of <html> >+ "EndHTML:00000000\r\n" // offset of end of </html> >+ "StartFragment:00000000\r\n" // offset of end of <!--StartFragment--> >+ "EndFragment:00000000\r\n" // offset of <!--EndFrag >+ ); >+*/ Remove this. >+ nsCAutoString numString; >+ numString.AppendInt(startHTMLOffset); This isn't used anywhere. >+ nsCString clipboardString; One too many spaces here. >+ *outPlatformHTML = ToNewCString(clipboardString);printf("outPlatformHTML='%s'\n", outPlatformHTML); Remove the printf. Other than that it looks much better than what we have. r=me

I incorporated the changes suggested by Dean. (the printf wasn't supposed to be in the final version of the previous patch!)

Comment on attachment 158549 [details] [diff] [review] Update of proposed patch to cleanup and fix the win32 HTML Clipboard population Carrying forward flags.

+ nsDependentCString htmlHeaderString("<html><body>\r\n"); NS_NAMED_LITERAL_CSTRING(htmlHeaderString, "<html>...."); is slightly faster (calculates length at compile time)

Jag, are you OK with the patch if the change in comment 27 is made?

Comment on attachment 158549 [details] [diff] [review] Update of proposed patch to cleanup and fix the win32 HTML Clipboard population Do we want this on the aviary branch as well?

Comment on attachment 158549 [details] [diff] [review] Update of proposed patch to cleanup and fix the win32 HTML Clipboard population I think that this is important to also get into the Aviary-1.0 branch and SeaMonkey, particularly because it fixes Bug 158103.

we may still approve the patch, but not going to block on this.

jst says this isn't critical for the branch. dean, what's the risk here?

Comment on attachment 158549 [details] [diff] [review] Update of proposed patch to cleanup and fix the win32 HTML Clipboard population (In reply to comment #32) > jst says this isn't critical for the branch. dean, what's the risk here? I agree that it's not critical, but I thought I'd ask. Clearing branch request flags.

I'm not familiar with the criteria for determining what is considered critical and what isn't, but I would venture to say that in my opinion, this fix should go into the aviary branch because the current win32 HTML copying functionality is somewhat broken. Bug 158103 describes the issues that it suffers from in detail, but basically, if what is placed into the HTML copy buffer is pasted directly, there is a garbage "<!--StartFragment-->" at the beginning of the pasted segment. Thankfully most applications just ignore it because it is a HTML comment, but it is still pasted. Don't we want to have fully functional win32 copy/paste functionality in FireFox 1.0?

Comment on attachment 158549 [details] [diff] [review] Update of proposed patch to cleanup and fix the win32 HTML Clipboard population > Don't we want to have fully functional win32 copy/paste functionality in > FireFox 1.0? I forgot about that part of the fix. I was concentrating only on the potential overflow. The copied HTML doesn't mess up the HTML output at all, since <!--StartFragment--> is a self-contained comment. That being said, I don't think we want to get lambasted for propogating that comment into web pages, emails, etc. Putting back approval requests to let someone with decision-making power flex said power.

Comment on attachment 158549 [details] [diff] [review] Update of proposed patch to cleanup and fix the win32 HTML Clipboard population a=mkaply for the branches. Don't forget to dupe the other bug to this. Has this patch gone in on the trunk?

(In reply to comment #36) > Has this patch gone in on the trunk? Not that I've seen.

Comment on attachment 158549 [details] [diff] [review] Update of proposed patch to cleanup and fix the win32 HTML Clipboard population mozilla/widget/src/windows/nsDataObj.cpp 1.64 mozilla/widget/src/windows/nsDataObj.cpp 1.59.16.1

Bump... (reminder that this still needs to be committed/merged to the AVIARY branch). (FF 1.0 release date is fast approaching and i'm looking forward to having this in there)

> mozilla/widget/src/windows/nsDataObj.cpp 1.59.16.1 since this fixed this on the 1.7 branch, adding keyword. should this bug be marked fixed now that the patch is checked in, or is there more to do?

Comment on attachment 158549 [details] [diff] [review] Update of proposed patch to cleanup and fix the win32 HTML Clipboard population This hasn't gone in on the aviary branch. We had approval once but didn't get it checked in in time. Re-requesting approval for 1.0.

Comment on attachment 158549 [details] [diff] [review] Update of proposed patch to cleanup and fix the win32 HTML Clipboard population too late for 1.0

(In reply to comment #41) > (From update of attachment 158549 [details] [diff] [review]) > This hasn't gone in on the aviary branch. We had approval once but didn't get > it checked in in time. Re-requesting approval for 1.0. > Can this fixe be merged to the aviary branch now so that this bug and Bug 158103 can be closed? (or do we want to just close them because it has been committed to HEAD?)

The fix has been committed to everywhere that it needs to be at this time, as far as I can tell.