- Dillo-dev - lists.mailman3.com

Valgrind: hashing StyleAttr objects accesses uninitialized data

by onepoint＠starurchin.org

On Thu, Jun 12, 2008 at 09:56:49PM +0200, Johannes Hofmann wrote: > On Wed, Jun 11, 2008 at 11:36:29PM +0100, Jeremy Henty wrote: > > BTW, StyleAttrs::initValues() fails to set some other data members > > such as font and color , but fixing this has no effect on the > > valgrind warnings. > > As far as I can see this is intentional. font and color have to be > set explicitely by the caller of Style::create(). And if valgrind > does not complain, that seems to be the case. OK, then it's not a problem. Thanks for checking it out. Regards, Jeremy henty

June 13, 2008

Valgrind: hashing StyleAttr objects accesses uninitialized data

by jcid＠dillo.org

On Thu, Jun 12, 2008 at 09:56:49PM +0200, Johannes Hofmann wrote: > yOn Wed, Jun 11, 2008 at 11:36:29PM +0100, Jeremy Henty wrote: > > On Wed, Jun 11, 2008 at 09:51:17PM +0200, Johannes Hofmann wrote: > > > > > I think even hashValue() should not access uninitialized data. Not > > > only because of the annoying valgrind messages. > > > > Got it! It's StyleAttrs::textAlignChar ! If I add "textAlignChar = > > '.';" to StyleAttrs::initValues() (which StyleAttrs::resetValues() > > already does!) then the valgrind warnings disappear. Only > > a_Html_tag_set_align_attr() sets textAlignChar , but it only does so > > if it sees "align='char'". > > Great. So it's just this inititialization missing. Patchlet > attached. Committed. -- Cheers Jorge.-

June 12, 2008

Valgrind: hashing StyleAttr objects accesses uninitialized data

by Johannes.Hofmann＠gmx.de

yOn Wed, Jun 11, 2008 at 11:36:29PM +0100, Jeremy Henty wrote: > On Wed, Jun 11, 2008 at 09:51:17PM +0200, Johannes Hofmann wrote: > > > I think even hashValue() should not access uninitialized data. Not > > only because of the annoying valgrind messages. > > Got it! It's StyleAttrs::textAlignChar ! If I add "textAlignChar = > '.';" to StyleAttrs::initValues() (which StyleAttrs::resetValues() > already does!) then the valgrind warnings disappear. Only > a_Html_tag_set_align_attr() sets textAlignChar , but it only does so > if it sees "align='char'". Great. So it's just this inititialization missing. Patchlet attached. > > BTW, StyleAttrs::initValues() fails to set some other data members > such as font and color , but fixing this has no effect on the valgrind > warnings. As far as I can see this is intentional. font and color have to be set explicitely by the caller of Style::create(). And if valgrind does not complain, that seems to be the case. Cheers, Johannes

June 12, 2008

Valgrind: hashing StyleAttr objects accesses uninitialized data

by onepoint＠starurchin.org

On Wed, Jun 11, 2008 at 09:51:17PM +0200, Johannes Hofmann wrote: > I will try to fix that. Can you please post some of the errors > reported by valgrind? I've attached four log snippets showing valgrind warnings from: * hashTable::get() * hashTable::put() * hashTable::remove() * StyleAttrs::equals() Regards, Jeremy Henty

June 12, 2008

Valgrind: hashing StyleAttr objects accesses uninitialized data

by onepoint＠starurchin.org

On Wed, Jun 11, 2008 at 09:51:17PM +0200, Johannes Hofmann wrote: > I think even hashValue() should not access uninitialized data. Not > only because of the annoying valgrind messages. Got it! It's StyleAttrs::textAlignChar ! If I add "textAlignChar = '.';" to StyleAttrs::initValues() (which StyleAttrs::resetValues() already does!) then the valgrind warnings disappear. Only a_Html_tag_set_align_attr() sets textAlignChar , but it only does so if it sees "align='char'". BTW, StyleAttrs::initValues() fails to set some other data members such as font and color , but fixing this has no effect on the valgrind warnings. Hope this helps, Jeremy Henty

June 12, 2008

Valgrind: hashing StyleAttr objects accesses uninitialized data

by Johannes.Hofmann＠gmx.de

On Wed, Jun 11, 2008 at 07:41:02PM +0100, Jeremy Henty wrote: > > Ever since StyleAttr objects were cached in a hash to promote sharing, > valgrind has reported nearly 1000 branches dependent on uninitialised > data when dillo renders the splash page. These all come from > StyleAttrs::hashValue() and StyleAttrs::equals() : if I hack > hashValue() to return ((intptr_t) this) and equals() to return (this > == otherAttrs) then the warnings disappear. > > Is this a problem? If these methods are accessing uninitialised data > members then what happens when that data is initialised? The return > values of hashValue() and equals() could change, which sounds like a > potential source of bugs. Even if it's not a bug, if the value of > hashValue() is unpredictable then we might not be sharing StyleAttr > objects as much as we could and so we could be wasting memory. > > If it is correct to have code that raises these warnings, perhaps > there could be a "#define VALGRIND" switch so developers could easily > compile a dillo that doesn't raise them? (It's possible to silence > them with a valgrind suppression file but that is tedious and risks > hiding other warnings. It would be much more straightforward to have > a valgrind-clean version of the code.) > > Comments are welcome, I think even hashValue() should not access uninitialized data. Not only because of the annoying valgrind messages. I will try to fix that. Can you please post some of the errors reported by valgrind? Johannes

June 11, 2008

Valgrind: hashing StyleAttr objects accesses uninitialized data

by onepoint＠starurchin.org

Ever since StyleAttr objects were cached in a hash to promote sharing, valgrind has reported nearly 1000 branches dependent on uninitialised data when dillo renders the splash page. These all come from StyleAttrs::hashValue() and StyleAttrs::equals() : if I hack hashValue() to return ((intptr_t) this) and equals() to return (this == otherAttrs) then the warnings disappear. Is this a problem? If these methods are accessing uninitialised data members then what happens when that data is initialised? The return values of hashValue() and equals() could change, which sounds like a potential source of bugs. Even if it's not a bug, if the value of hashValue() is unpredictable then we might not be sharing StyleAttr objects as much as we could and so we could be wasting memory. If it is correct to have code that raises these warnings, perhaps there could be a "#define VALGRIND" switch so developers could easily compile a dillo that doesn't raise them? (It's possible to silence them with a valgrind suppression file but that is tedious and risks hiding other warnings. It would be much more straightforward to have a valgrind-clean version of the code.) Comments are welcome, Jeremy Henty

June 11, 2008

Splitting html.cc: what next?

by onepoint＠starurchin.org

On Wed, Jun 11, 2008 at 12:13:13PM -0400, Jorge Arellano Cid wrote: > I'd say go with table... OK, here's the patch. It's a much smaller change than the form.cc patch (table.cc is only 10k) because table parsing is much simpler and form.cc includes form submission code as well as parsing. It doesn't make nearly as much of an impact on the size of html.cc . It also moves more declarations into html_common.hh (stuff to parse colours and lengths and set styles). As before developers will have to " make maintainer-clean ; ./autogen.sh " so that table.cc gets built. > ...and that's it. :-( I was looking forward to more slicing and dicing! ;-) What do others think? Regards, Jeremy Henty

June 11, 2008

recent style/layout segfaults

by jcid＠dillo.org

On Tue, Jun 10, 2008 at 07:21:35PM +0200, Johannes Hofmann wrote: > On Sat, Jun 07, 2008 at 10:16:14PM +0000, corvid wrote: > > Johannes wrote: > > > Comments welcome! I feel a bit uncomfortable proposing a > > > pretty drastic change like this. > > > > Although obviously I don't know the code well, it seems like an improvement. > > After thinking some more about it, I believe this is the right way to > go. My initial patch had some issues though: It was complete > nonsense of course to make the destructor of the derived class > (FltkFont and FltkColor) virtual. It only worked because Font and > Color are derived from lout::Object which already has a virtual > destructor. So in fact the change does not add any additional > virtual methods. > I also decided to keep the interface to dillo2 as it is (the now > unused layout parameter in Style::create()). > > So attached is my official proposal to fix the style related > segfaults that where introduced by my style caching patch. Trying to be practical I'd say: if we don't have a comment in a week let's commit. -- Cheers Jorge.-

June 11, 2008

Splitting html.cc: what next?

by jcid＠dillo.org

On Sun, Jun 08, 2008 at 09:06:07PM +0100, Jeremy Henty wrote: > > Off the top of my head, some suggestions for more files that could > usefully be split out from html.cc: > > - table.cc , handles <table> , <tr> , <td> , <th> > - list.cc , handles <ul> , <ol> , <li> , <dl> , <dt> , <dd> > - text.cc , handles , , , , <strike> > - paragraph.cc , handles , <blockquote> , <center> , <address> > - page.cc , handles <html> , <head> , <title> , <meta> , <body> > > Thoughts? Considering: [...] 16 -rw-r--r-- 1 jcid jcid 14378 2008-06-01 21:59 nav.c 16 -rw-r--r-- 1 jcid jcid 15023 2008-02-29 14:40 png.c 16 -rw-r--r-- 1 jcid jcid 15318 2008-05-25 18:24 prefs.c 20 -rw-r--r-- 1 jcid jcid 16619 2008-06-01 21:59 uicmd.cc 20 -rw-r--r-- 1 jcid jcid 17543 2008-03-29 12:04 url.c 20 -rw-r--r-- 1 jcid jcid 17962 2008-06-01 21:59 capi.c 28 -rw-r--r-- 1 jcid jcid 26697 2008-05-21 18:56 ui.cc 32 -rw-r--r-- 1 jcid jcid 29415 2008-03-15 14:24 gif.c 36 -rw-r--r-- 1 jcid jcid 35558 2008-06-01 22:00 cache.c 64 -rw-r--r-- 1 jcid jcid 59540 2008-06-11 12:01 form.cc 132 -rw-r--r-- 1 jcid jcid 130045 2008-06-10 10:47 html.cc I'd say go with table and that's it. -- Cheers Jorge.-

June 11, 2008