search

brave-experiments / ad-block

Ad block engine used in the Brave browser for ABP filter syntax based lists like EasyList.
https://www.brave.com
Mozilla Public License 2.0
240 stars 95 forks source link

AdBlockClient::deserialize may return true on bad inputs, instead of false #46

Open garvankeeley opened 7 years ago

garvankeeley commented 7 years ago

Due to a bug in iOS when trying SafeBrowsing 2, this was passed in to deserialize(), which then returned true, indicating the rules were parsed.

(It is a very minor problem as iOS should have been checking for HTTP error code first)

<?xml version=\"1.0\" encoding=\"UTF-8\"?>\n<Error><Code>AccessDenied</Code><Message>Access Denied</Message><RequestId>4E30A0E337655BBD</RequestId><HostId>x9aF0xlegZCmBKh9sLjUQDyR9C07MaUBhFytutVsHaZQHNRFNVzJS3w5kCTMkC1CZ4coEIaZ3G0=</HostId></Error>\r\n0\r\n\r\n
bbondy commented 7 years ago

you may want to force redownload or change the file paths to avoid corruption, deserialize assumes a valid data file format and it could crash if not on subsequent checks. Also if you aren't yet, it's best to download to another filename with a .tmp extension and then rename to the final filename. This ensures if the app is killed you never have a partial file that the app will use.

garvankeeley commented 7 years ago

Yup, iOS has those cases covered: changes file path locally on version changes, and uses atomic write API for the final data file output on disk

edit: the case in this bug is now handled also: https://github.com/brave/browser-ios/pull/657/commits/3b9634526c164a928902c54664f19ba5c46f65c5