May 2008 - Dillo-dev - lists.mailman3.com

Fuzzing dillo

by 4winter＠informatik.uni-hamburg.de

-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1 Jorge Arellano Cid wrote: > On Sun, May 25, 2008 at 01:12:38PM +0200, Justus Winter wrote: >> -----BEGIN PGP SIGNED MESSAGE----- >> Hash: SHA1 >> >> Hi :) >> >> I was playing around with Michal Zalewskis html fuzzer 'mangle' trying >> to crash dillo and succeeded :). > > Good. > > It's no surprise because dillo2 needs a careful review of > behaviour when facing strange/malicious values. Obviously this > phase was procrastinated until basic functionality was completed. > > For instance, most probably you'll get to crash dillo2 by > passing it some negative values in attributes. The problem of > being robust when parsing garbage or malicious code needs a > general strategy. Are you saying that it is too early to locate problems this way? The process of finding problems using a fuzzer and generating minimal testcases that trigger the problem is mostly automatic and I could script the last bits that do require manual intervention with ease. In case these bug reports are useful at this point, here is another one: In file html.cc, function Html_tag_close_select: int size = input->select->options->size (); fails since input->select is NULL. The html fragment that triggers this fault is attached. Justus - -- gpg key fingerprint: C82D 382A AB38 1A54 5290 19D6 A0F9 B035 686C 6996 -----BEGIN PGP SIGNATURE----- Version: GnuPG v1.4.6 (GNU/Linux) Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org iD8DBQFIOayFoPmwNWhsaZYRArZgAJ4hdzV2Gs2fVNRgblDASKmEmaF0gACfaKBT XpPVTVS8l4RITppTqprbAlM= =/sCO -----END PGP SIGNATURE-----

May 25, 2008

remaining preferences

by Johannes.Hofmann＠gmx.de

On Sun, May 25, 2008 at 10:51:47AM -0400, Jorge Arellano Cid wrote: <snip> > > > # Enabling this will restrain OpenUrl and FindText, but may be required > > # for the ION window manager. > > transient_dialogs > > > > from fltk::Window::child_of(): > > Tell the system that this window will not have an icon, it will > > dissappear and reappear when \a parent is iconized or shown, and it > > is forced to always be above \a parent. On X this is called a > > "Transient window", and Windows calls this a "overlapping child". \a > > parent is different than the parent(), which must be zero. > > (and down in fltk::CreatedWindow::create, there's a > > if (window->child_of() && window->child_of()->shown()) > > XSetTransientForHint(xdisplay, x->xid, window->child_of()->i->xid);) > > > > If Justus gets rid of the dialogs, obviously the pref can be removed. > > Enabling this means making our own dialogs for message(), choice(), input(). > > Somebody using ION may check this. Just searched my email. I had requested that feature some time ago - well, it was Nov 2001 ;-) I no longer use ion, but I will check how it works with current dillo. The file dialogs are the only that will stay, right? Cheers, Johannes

May 25, 2008

[patch] dw2 memory reduction

by Johannes.Hofmann＠gmx.de

On Sun, May 25, 2008 at 09:12:11AM -0400, Jorge Arellano Cid wrote: > On Sun, May 25, 2008 at 10:08:12AM +0200, Johannes Hofmann wrote: > > Hi, > > > > On Sat, May 24, 2008 at 04:39:42PM -0400, Jorge Arellano Cid wrote: > > > On Sat, May 24, 2008 at 02:58:08PM +0200, Johannes Hofmann wrote: > > > > Hi, > > > > > > > > attached is patch to reduce the memory usage of dw2: > > > > > > Excellent! > > > (AFAIU this is near 30% memory usage reduction in dw2) > > > > > > Committed. > > > > > > > > > > > * Styles are now reused if they are equal. This brings down the > > > > number of Style objects on the mysql page from around 180000 to > > > > 23000 and reduces memory usage by about 25M at the cost of an > > > > additional HashTable. > > > > Maybe the hash table should be part of fltkplatform > > > > as the hash tables for Color and Font? > > > > There would be even more sharing of styles possible, if > > > > x_link would not be part of style. On the other hand about 3M for > > > > styles on the huge mysql page is pretty ok. > > > > > > > > * Size of struct Word is reduced by 4 integers by using shorts. I > > > > know that this is questionable, but I think in the case of struct > > > > Word that can be allocated a lot it is worth the hassle. > > > > Please check whether my assumption that space lengths and word > > > > lengths can be stored in shorts are valid in general. > > > > > > It looks OK at first sight, but to be sure it'd be good to know > > > what happens when the limit is surpassed. > > > > Just tried it on a page with 40000 char long word. With short for > > hlStart/hlEnd, selection is - obviously - no longer possible. > > That's clearly a regression. > > Please revert with attached patch. > > A max of 32K for a single word, looks quite reasonable to me. > I meant, if the browser doesn't crash, and fails gracefully, why > not. > > BTW, it can be extended to 64K with unsigned, by using 65535 as > sentinel value (instead of -1), but again, is it worth? > > It'd be good to know how much "struct Word" is reduced in size > (after compiler alignment), and how much it impacts overall > memory usage. I mean if struct word is reduced 20% and that > yields a 4% overall memory reduction, it may be not worth > complicating the data type. > > OTOH, ... On the very long mysql page, addWord is called 1192256 times. But due to the allocation in 2^n steps in SimpleVector memory for some more of them could be allocated structs on 32bit systems are 8 byte aligned. struct Word with highlight stuff as int is 48 byte. With short it's 40 byte. On the mysql page I observe a total memory reduction of about 9M by using short's for the highlight stuff. Apart from the highlight stuff dillo can handle those long words just fine. So we would add this restriction just to save 9M of memory on this rather extreme page. I would keep int's for now and try to get hlStart and hlEnd out of struct Word and in Textblock which would bring down struct Word to 32 bytes - hopefully without any restrictions. Cheers, Johannes

May 25, 2008

Fuzzing dillo

by onepoint＠starurchin.org

On Sun, May 25, 2008 at 10:31:23AM -0400, Jorge Arellano Cid wrote: > On Sun, May 25, 2008 at 01:12:38PM +0200, Justus Winter wrote: > > Furthermore I'd suggest to break down html.cc into several files. > > The hard part is a good choice on what to separate. ;) I suggest splitting out everything that handles forms and form elements. That's a fair amount of code that represents a reasonably self-contained chunk. The forms code can also be be cleaned up in a few ways. There's a lot of cut-and-paste code like "foo->get (foo->size () - 1)". Adding getLast() methods to some lout classes and using them wherever possible would remove a lot of clutter. html.cc looks like a C file that was subsequently converted to C++ because it makes little use of C++. If DilloHtmlForm and DilloHtmlInput were given constructors and destructors and managed with new and delete then lots of allocation/deallocation code would be neatly factored out instead of sitting inline with all the other logic. The type element of DilloHtmlInput leads to much horrible case code. If DilloHtmlInput were an abstract class and the different Input types were subclasses then all the type-dependent code could go into virtual methods. This would break up the huge case statements that make much of this code hard to follow. I'll propose patches if people are positive about these suggestions. Jeremy Henty

May 25, 2008

Q: why does Nav_stack_clean() free the data and Nav_stack_truncate() not?

by onepoint＠starurchin.org

On Sun, May 25, 2008 at 09:31:30AM -0400, Jorge Arellano Cid wrote: > Hi Jeremy, > > Nav_stack_truncate() and Nav_stack_insert() are ad-hoc > bindings. They're called just in one place of the code. They're > not clean well-defined functions of an ADT. > > This is "your" mistake. :-) I'll cheerfully abandon my expectations of consistency. :-) But what about memory[1]? Doesn't it leak when Nav_stack_truncate() shrinks bw->nav_stack ? It seems to me that Nav_stack_truncate() should free all those pointers (and therefore Nav_stack_insert() shouldn't free anything). I'll supply a patch if you agree. > The nav.c module is asking for a clean ADT, and if you can work on > it, it's certainly welcomed. Then I hope people will review the patch I sent to the discussion with corvid about the Redirect/Reload crash bug, which is all about the handling of the navigation stack. > BTW, if it's to be rewritten it'd be good to have the TABs > interface in mind. Oh noes, you have spotted my hidden agenda! :-) I was indeed thinking that if I got my head around this it would prepare me for looking at tabs. > This is, if we end with a clean ADT or object, we could have an > array of them, one for each tab, and switch freely among navigation > stacks. As a first step, maybe we could modify uicmd.cc to indirect though a new BrowserWindowSet object. The callbacks would be changed to look up the current BrowserWindow of the BrowserWindowSet . Initially the BrowserWindowSet could be a stub that managed a single BrowserWindow , later it could wrap an array of BrowserWindows . Regards, Jeremy Henty [1] Dillo's memory, that is, not mine. I'm afraid fixing Dillo's memory leaks is easier than fixing my own!

May 25, 2008

[BUG]: Redirect to previous page + Reload crashes

by onepoint＠starurchin.org

On Sun, May 25, 2008 at 12:31:51AM +0000, corvid wrote: > Since it looks like Nav_stack_truncate() already got rid of > everything at stack_idx and beyond, maybe Nav_stack_insert() could > be replaced by a Nav_stack_append() function. Good point! And there is no need for Nav_stack_insert() to update bw->nav_stack_ptr . In fact it *mustn't* do that because the code that calls Nav_stack_insert() assumes that bw->nav_stack_ptr *won't* change and increments it itself. Here's a patch that replaces Nav_stack_insert() by Nav_stack_append() as you suggest. Regards, Jeremy Henty

May 25, 2008

remaining preferences

by jcid＠dillo.org

On Sat, May 24, 2008 at 07:28:47PM +0000, corvid wrote: > # Dicache is where the Decompressed Images are cached (not the original ones). > # If you have a lot of memory and a slow CPU, use YES, otherwise use NO > use_dicache > > The code always behaves like use_dicache=NO > > /* > * Unrefs the counter of a dicache entry, and _if_ no DwImage is acessing > * this buffer, then we call Dicache_free to do the dirty job. > */ > void a_Dicache_unref(const DilloUrl *Url, int version) > { > DICacheEntry *entry; > > if ((entry = Dicache_get_entry_version(Url, version))) { > /*if (--entry->RefCount == 0 && (entry->next || !prefs.use_dicache)) {*/ > if (--entry->RefCount == 0) { > Dicache_remove(Url, version); > } > } > } > > Perhaps this was done for a reason. Yes, I wrote cache/dicache code before imgbuf was introduced. The dicache showed impressive performance when clicking Back/Forward on pages with images because they were decoded once and the RGB data kept in the dicache. Time showed it wasn't as useful as it seemed because memory usage was to high and usually machines with big memory came with a big CPU too. Probably the dicache is obsolete and can be completely removed now. We left that task pending because we had yet a lot to do after images were enabled in dillo2. > # If you prefer oblique over italic fonts, uncoment next line > use_oblique > > FLTK doesn't know about oblique. I don't know whether anybody cares enough > to change that. I don't. Cleanup patch welcomed. ;) > # Show tooltip popup for images? > # Note: We use the "title" attribute and not "alt". > # More info at: http://bugzilla.mozilla.org/show_bug.cgi?id=25537 > show_tooltip > > I did the easy part of tooltips long ago, but I haven't been able to > care enough to want to do the fltk part. > > > # Set this to YES, if you want to limit the word wrap width to the vieport > # width (may be useful for iPAQ) > limit_text_width > > It doesn't do anything. I don't think it's ever done anything. > It sure would be nice if it did something. But I don't have the expertise > to change that. For instance: Some times HTML pages for mailing lists render one looong line for a message. The MUA packs the message as a single line and the mail2html filter wraps it with PRE. AFAIR the SPEC said original formatting was to be honoured in PRE, but browsers, in practice, tend to wrap it. Sometimes you need one behaviour and then another. > # Here you can choose to hide some widgets of the dillo panel... > show_menubar > > Not currently used. Meaningless with the current panel-cycling with > right click on the search button (which I imagine is not going to be > the permanent way to do that). Not the permanent way, but quite handy! ;) > # Enabling this will restrain OpenUrl and FindText, but may be required > # for the ION window manager. > transient_dialogs > > from fltk::Window::child_of(): > Tell the system that this window will not have an icon, it will > dissappear and reappear when \a parent is iconized or shown, and it > is forced to always be above \a parent. On X this is called a > "Transient window", and Windows calls this a "overlapping child". \a > parent is different than the parent(), which must be zero. > (and down in fltk::CreatedWindow::create, there's a > if (window->child_of() && window->child_of()->shown()) > XSetTransientForHint(xdisplay, x->xid, window->child_of()->i->xid);) > > If Justus gets rid of the dialogs, obviously the pref can be removed. > Enabling this means making our own dialogs for message(), choice(), input(). Somebody using ION may check this. > # When filling forms, our default behaviour is to submit on enterpress, > # but only when there's a single text entry (to avoid incomplete submits). > # OTOH, if you have to fill the same form lots of times, you may find > # useful to keep away from the mouse by forcing enter to submit. > enterpress_forces_submit > > The code currently always submits on enter. I like it that way and > didn't realize that it wasn't supposed to. It depends on the FORM, and user. I'd like to hav it working. -- Cheers Jorge.-

May 25, 2008

patch: Re: [Dillo-dev] Fuzzing dillo

by corvid＠lavabit.com

Justus wrote: > I was playing around with Michal Zalewskis html fuzzer 'mangle' trying > to crash dillo and succeeded :). Furthermore I was able to reduce the > ~100k page that was triggering a NULL pointer dereferencing to three > tags (see the attached page). > > Some digging around (html.cc is a monster ^^) revealed that > > style_attrs.setBorderColor ( > Color::createShaded(HT2LT(html), > style_attrs.backgroundColor->getColor())); > > in Html_tag_open_table was failing due to the fact that > style_attrs.backgroundColor was NULL. > > Html_tag_open_select sets the backgroundColor to NULL and this is > inherited by the table tag since it's a child of the select tag. I doubt I realized you can try to put things like tables inside selects back when I wrote that. Maybe Jorge will know something easy and general that can be done based on parse modes to keep dillo from wasting its time calling things like Html_tag_open_table that aren't going to be displayed anyway, but the attached patch at least fixes the crash. > Greping through the code shows that there are at least two other > functions that set backgroundColor (and color) to NULL > (Html_tag_open_input and Html_tag_open_isindex) and twelve locations > calling getColor() on either color or backgroundColor. I think those are all right because they aren't allowed to contain anything, although, as I hinted above, my knowledge of the parsing code is not really detailed. > Furthermore I'd suggest to break down html.cc into several files. It is > about 6000 lines long (-> hard to dig into) and it takes gcc roughly > five seconds to compile on my box. I'd like this as well.

May 25, 2008

Fuzzing dillo

by jcid＠dillo.org

On Sun, May 25, 2008 at 01:12:38PM +0200, Justus Winter wrote: > -----BEGIN PGP SIGNED MESSAGE----- > Hash: SHA1 > > Hi :) > > I was playing around with Michal Zalewskis html fuzzer 'mangle' trying > to crash dillo and succeeded :). Good. It's no surprise because dillo2 needs a careful review of behaviour when facing strange/malicious values. Obviously this phase was procrastinated until basic functionality was completed. For instance, most probably you'll get to crash dillo2 by passing it some negative values in attributes. The problem of being robust when parsing garbage or malicious code needs a general strategy. I was delighted to read DJB's paper: "Some thoughts on security after ten years of qmail 1.0" (Strongly suggested to everyone!). There're lots of good ideas there, tried and true. It's clear that implementing them all is a huge task, but just a couple of them can make wonders, and reading the paper makes you think in a different way when designing code. For instance, in dlib, I took the simple approach of checking each function's input before processing (not 100% complete, but you get the idea), and it has proven rock solid. The extra time spent on checks doesn't impact performance noticeably, but the simplicity and joy of relying on a strong ADT set is priceless. Another strategy that allowed me to simplify dillo a lot, was to avoid multiple execution paths. With multiple paths, code gets much more complex, and maintenance is even harder, minor changes have side effects to track and testing depends on the path taken. A single path is tested by common use all the time, has only one place to make changes, is simpler to understand/debug/modify, and the simplicity leverage clearly overcomes the possible performace tradeoff. Beautifuly expressed: "The key to performance is elegance, not battalions of special cases." Jon Bentley and Doug McIlroy e.g. The CCC used to have one-branch-per-protocol. When it was switched to a single "universal" branch, CCC code size was reduced importantly, a lot of the complexity vanished, fixing a bug benefited all protocols, there were no multiple paths to track, etc. e.g.2 Memory handling in dillo. The general idea of trying to make each function deallocate what it allocated, made a hell of memory traceback debugging vanish. And the performance impact was unnoticed! > Furthermore I was able to reduce the > ~100k page that was triggering a NULL pointer dereferencing to three > tags (see the attached page). > > Some digging around (html.cc is a monster ^^) revealed that > > style_attrs.setBorderColor ( > Color::createShaded(HT2LT(html), > style_attrs.backgroundColor->getColor())); > > in Html_tag_open_table was failing due to the fact that > style_attrs.backgroundColor was NULL. > > Html_tag_open_select sets the backgroundColor to NULL and this is > inherited by the table tag since it's a child of the select tag. > > Greping through the code shows that there are at least two other > functions that set backgroundColor (and color) to NULL > (Html_tag_open_input and Html_tag_open_isindex) and twelve locations > calling getColor() on either color or backgroundColor. > > I have no idea how to _properly_ fix this issue. Not me either. I'd check what NULL means. A quick look hints towards a non inherited value: /** * \brief Reset those style attributes to their standard values, which are * not inherited, according to CSS. */ void StyleAttrs::resetValues () A default backgroundColor should be a good place to start. HTH. > > Furthermore I'd suggest to break down html.cc into several files. It is > about 6000 lines long (-> hard to dig into) and it takes gcc roughly > five seconds to compile on my box. I agree. The hard part is a good choice on what to separate. ;) -- Cheers Jorge.-

May 25, 2008

Q: why does Nav_stack_clean() free the data and Nav_stack_truncate() not?

by jcid＠dillo.org

Hi Jeremy, On Sat, May 24, 2008 at 08:15:47PM +0100, Jeremy Henty wrote: > > While reviewing nav.c to figure out the reload crash bug I noticed > something that seems inconsistent: both Nav_stack_truncate() and > Nav_stack_clean() remove items from the navigation stack, yet > Nav_stack_truncate() does not free the data it removes and > Nav_stack_clean() does. Is this a bug? If not, what haven't I > understood? Nav_stack_truncate() and Nav_stack_insert() are ad-hoc bindings. They're called just in one place of the code. They're not clean well-defined functions of an ADT. This is "your" mistake. :-) I stumbled upon Nav_stack_clean() some time ago, and wondered whether it still serves a clear purpose or if it could be replaced. The nav.c module is asking for a clean ADT, and if you can work on it, it's certainly welcomed. Beware that it's more complex than it looks. Especially the repush code. Nav.c takes care of redirections, reloads, repushes, expects, jumps, pushes, etc. BTW, if it's to be rewritten it'd be good to have the TABs interface in mind. This is, if we end with a clean ADT or object, we could have an array of them, one for each tab, and switch freely among navigation stacks. -- Cheers Jorge.-

May 25, 2008