Valgrind: hashing StyleAttr objects accesses uninitialized data
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
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
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
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
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.-
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
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
participants (3)
-
jcid＠dillo.org -
Johannes.Hofmann＠gmx.de -
onepoint＠starurchin.org