Bug 197399 – Protect against null crash in fetchDiskCacheEntries

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Alex Christensen]

Protect against null crash in fetchDiskCacheEntries

[Alex Christensen]

Created attachment 368530 [details]
Patch

[Alex Christensen]

<rdar://problem/47759337>

[EWS Watchlist]

Attachment 368530 [details] did not pass style-queue:


ERROR: Source/WebKit/NetworkProcess/NetworkProcess.cpp:1275:  Missing space before ( in switch(  [whitespace/parens] [5]
Total errors found: 1 in 4 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Alex Christensen]

Created attachment 368537 [details]
Patch

[Antti Koivisto]

Comment on attachment 368537 [details]
Patch

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

> Source/WebKit/NetworkProcess/NetworkProcess.cpp:1277
> +            case NetworkCache::Cache::TraversalResult::DecodeError:
> +                ASSERT_NOT_REACHED();

This shouldn't assert. The design is to handle serialization format changes (and corruption for any reason) silently.

> Source/WebKit/NetworkProcess/NetworkProcess.cpp:1461
> +            ASSERT_NOT_REACHED();

Same here.

[Antti Koivisto]

Comment on attachment 368537 [details]
Patch

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

> Source/WebKit/NetworkProcess/cache/NetworkCache.h:124
> -    void traverse(Function<void(const TraversalEntry*)>&&);
> +    enum class TraversalResult : bool { FinishedTraversing, DecodeError };
> +    void traverse(Function<void(Expected<std::reference_wrapper<const TraversalEntry>, TraversalResult>&&)>&&);

How about simply having a separate 'finished' bit ? All this seems bit too much.

[Alex Christensen]

Comment on attachment 368537 [details]
Patch

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

>> Source/WebKit/NetworkProcess/NetworkProcess.cpp:1277
>> +                ASSERT_NOT_REACHED();
> 
> This shouldn't assert. The design is to handle serialization format changes (and corruption for any reason) silently.

Ok.

>> Source/WebKit/NetworkProcess/cache/NetworkCache.h:124
>> +    void traverse(Function<void(Expected<std::reference_wrapper<const TraversalEntry>, TraversalResult>&&)>&&);
> 
> How about simply having a separate 'finished' bit ? All this seems bit too much.

That would make it so the function signature expresses that there is a state where the TraversalEntry could be non null and finished could be true, which is not the case.  This way expresses that we either have a valid TraversalEntry or one of two other states.  I think this is better unless you feel strongly.

[Antti Koivisto]

Comment on attachment 368537 [details]
Patch

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

> Source/WebKit/NetworkProcess/cache/NetworkCache.cpp:484
>          if (!entry) {
> -            traverseHandler(nullptr);
> +            traverseHandler(makeUnexpected(TraversalResult::DecodeError));
>              return;
>          }

Actually, can't we simply remove the traverseHandler() call here? There is nothing useful a client can do there.

[Alex Christensen]

Comment on attachment 368537 [details]
Patch

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

>> Source/WebKit/NetworkProcess/cache/NetworkCache.cpp:484
>>          }
> 
> Actually, can't we simply remove the traverseHandler() call here? There is nothing useful a client can do there.

Wow, that's so elegant.  That makes this whole patch a 1 line change.

[Alex Christensen]

Created attachment 368707 [details]
Patch

[Alex Christensen]

http://trac.webkit.org/r244850