Bug 110171 - StringHasher functions require alignment that call sites do not all guarantee
Summary: StringHasher functions require alignment that call sites do not all guarantee
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Template Framework (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Darin Adler
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2013-02-18 18:10 PST by Darin Adler
Modified: 2013-03-11 15:23 PDT (History)
11 users (show)

See Also:


Attachments
Patch (10.23 KB, patch)
2013-02-18 19:02 PST, Darin Adler
no flags Details | Formatted Diff | Diff
Patch (10.23 KB, patch)
2013-02-18 19:13 PST, Darin Adler
no flags Details | Formatted Diff | Diff
Patch (42.88 KB, patch)
2013-03-02 17:32 PST, Darin Adler
benjamin: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Darin Adler 2013-02-18 18:10:19 PST
StringHasher functions require alignment that call sites do not all guarantee
Comment 1 Darin Adler 2013-02-18 19:02:36 PST
Created attachment 188973 [details]
Patch
Comment 2 Darin Adler 2013-02-18 19:09:42 PST
Comment on attachment 188973 [details]
Patch

Oops, looks like I uploaded something that doesn’t build.
Comment 3 Early Warning System Bot 2013-02-18 19:12:43 PST
Comment on attachment 188973 [details]
Patch

Attachment 188973 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/16613441
Comment 4 Darin Adler 2013-02-18 19:13:35 PST
Created attachment 188974 [details]
Patch
Comment 5 Benjamin Poulain 2013-02-19 00:09:25 PST
Comment on attachment 188974 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=188974&action=review

The new methods addCharactersAssumingAligned() are public. Wouldn't it be better to have them private until they are needed to be public by client code? Exposing those low level functions increases the risk of misuses like the one you fix with this patch.

If you keep the new methods public, it would be nice to have new test coverage for them.

> Source/WTF/wtf/StringHasher.h:77
> -            addCharactersToHash(m_pendingCharacter, Converter(*data++));
> -            m_hasPendingCharacter = false;
> -            --length;
> +            addCharactersInternal(m_pendingCharacter, a);
> +            m_pendingCharacter = b;
> +            return;

It would be nice to get rid of addCharactersInternal to simplify the class.
Is there really any extra cost to?:
    m_hasPendingCharacter = false;
    addCharactersInternal(m_pendingCharacter, a);
    m_pendingCharacter = b;
    m_hasPendingCharacter = true;
Comment 6 Darin Adler 2013-02-26 09:39:36 PST
(In reply to comment #5)
> The new methods addCharactersAssumingAligned() are public. Wouldn't it be better to have them private until they are needed to be public by client code?

I suppose they could be private; my intention is that they are part of the public interface.

> Exposing those low level functions increases the risk of misuses like the one you fix with this patch.

Sure, but given their names it seems unlikely they’d be misused. The issue before was the hidden precondition that was not part of the name. I don’t think Geoff would have gotten it wrong if it was part of the name.

> If you keep the new methods public, it would be nice to have new test coverage for them.

That’s a good point. Whether public or not it would be good to have tests for the functions in this class. I don’t know exactly how to do unit tests; I’m sure it’s easy but I don’t know where to put them.

> It would be nice to get rid of addCharactersInternal to simplify the class.

Will do.
Comment 7 Darin Adler 2013-03-02 17:32:46 PST
Created attachment 191122 [details]
Patch
Comment 8 Darin Adler 2013-03-02 17:33:34 PST
The patch was actually already reviewed by Ben Poulain. This new version makes most of the changes he suggested, except for the “make more private” one, which we could still do. And I added lots of tests.
Comment 9 Benjamin Poulain 2013-03-02 17:50:18 PST
Comment on attachment 191122 [details]
Patch

Thanks for doing all the tests!

My only comment is it would be useful to have a comments for each group of test.
E.g.:
    // addCharactersAssumingAligned with zero characters should produce an emptyStringHash.
    // addCharactersAssumingAligned with \0.
    // addCharactersAssumingAligned with 5 characters at once.
etc.

Otherwise it takes some reading to find what is tested in each paragraph.
Comment 10 Darin Adler 2013-03-02 21:49:23 PST
Committed r144552: <http://trac.webkit.org/changeset/144552>
Comment 11 Thiago Marcos P. Santos 2013-03-07 15:09:22 PST
(In reply to comment #10)
> Committed r144552: <http://trac.webkit.org/changeset/144552>

This test is failing on the EFL Debug bots from the moment it was introduced (although it is passing on the Release bots):

/home/buildslave-1/webkit-buildslave/efl-linux-64-debug-wk2/build/Tools/TestWebKitAPI/Tests/WTF/StringHasher.cpp:173: Failure
Value of: hasher.hash()
  Actual: 2754808611
Expected: testBHash5
Which is: 2032145993
[  FAILED  ] WTF.StringHasher_addCharacters (0 ms)
[ RUN      ] WTF.StringHasher_addCharactersAssumingAligned
/home/buildslave-1/webkit-buildslave/efl-linux-64-debug-wk2/build/Tools/TestWebKitAPI/Tests/WTF/StringHasher.cpp:328: Failure
Value of: hasher.hash()
  Actual: 2754808611
Expected: testBHash5
Which is: 2032145993
[  FAILED  ] WTF.StringHasher_addCharactersAssumingAligned (0 ms


http://build.webkit.org/builders/EFL%20Linux%2064-bit%20Debug%20WK2/builds/10028/steps/API%20tests/logs/stdio
Comment 12 Darin Adler 2013-03-08 17:24:09 PST
I see the problem. I forgot to null-terminate testBUChars!
Comment 13 Darin Adler 2013-03-11 15:23:41 PDT
Geoff Garen fixed the failing tests problem in r145412.