Bug 43203 - WebBackForwardList::back/ForwardListWithLimit() crashes if passed a limit larger than max int
Summary: WebBackForwardList::back/ForwardListWithLimit() crashes if passed a limit lar...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit2 (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: John Sullivan
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2010-07-29 12:36 PDT by John Sullivan
Modified: 2010-07-29 14:36 PDT (History)
1 user (show)

See Also:


Attachments
Patch to cast to unsigned rather than int, to avoid wrapping (1.77 KB, patch)
2010-07-29 13:59 PDT, John Sullivan
sam: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description John Sullivan 2010-07-29 12:36:24 PDT
WebBackForwardList::backListWithLimit() and forwardListWithLimit() crash if the limit parameter, an unsigned value, is larger than numeric_limits<int>::max().

The crash occurs here, with i == 0 but an empty m_entries:

        WebBackForwardListItem* item = m_entries[i].get();


The crash occurs due to this incorrect logic:

    unsigned size = std::min(backListCount(), static_cast<unsigned>(limit));
    if (!size)
        return ImmutableArray::create();

This is attempting to return early for the empty case, but casting the unsigned limit to an int can make it negative, and thus size is negative, and thus the test for !size fails.

I've got a fix that I'll send out after lunch, if nobody beats me to it.
Comment 1 John Sullivan 2010-07-29 13:59:53 PDT
Created attachment 62989 [details]
Patch to cast to unsigned rather than int, to avoid wrapping
Comment 2 Adam Roben (:aroben) 2010-07-29 14:06:05 PDT
Comment on attachment 62989 [details]
Patch to cast to unsigned rather than int, to avoid wrapping

> -    unsigned size = std::min(backListCount(), static_cast<int>(limit));
> +    unsigned size = std::min(static_cast<unsigned>(backListCount()), limit);

Why does backListCount return an int? Seems like it should return unsigned.
Comment 3 John Sullivan 2010-07-29 14:24:41 PDT
I agree that backForwardCount() should not return an int. Probably all of these functions should deal with size_t's. But I didn't want to get into that territory for this fix.

Checked in as http://trac.webkit.org/changeset/64306
Comment 4 John Sullivan 2010-07-29 14:36:06 PDT
I filed a bug about the inconsistent use of types in this area: <https://bugs.webkit.org/show_bug.cgi?id=43214>