Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Subscription list metadata #13421

Merged
merged 4 commits into from
Jun 6, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions browser/brave_shields/ad_block_service_browsertest.cc
Original file line number Diff line number Diff line change
Expand Up @@ -802,6 +802,8 @@ IN_PROC_BROWSER_TEST_F(AdBlockServiceTest,
ASSERT_EQ(subscriptions[0].last_update_attempt, base::Time());
ASSERT_EQ(subscriptions[0].last_successful_update_attempt, base::Time());
ASSERT_EQ(subscriptions[0].enabled, true);
ASSERT_EQ(subscriptions[0].homepage, absl::nullopt);
ASSERT_EQ(subscriptions[0].title, absl::nullopt);
}

// Ensure that the subscription gets update attempts, and ultimately is
Expand Down Expand Up @@ -848,6 +850,8 @@ IN_PROC_BROWSER_TEST_F(AdBlockServiceTest,
ASSERT_EQ(subscriptions[0].last_successful_update_attempt,
subscriptions[0].last_update_attempt);
ASSERT_EQ(subscriptions[0].enabled, false);
ASSERT_EQ(subscriptions[0].homepage, "https://example.com/list.txt");
ASSERT_EQ(subscriptions[0].title, "Test list");
}

EXPECT_EQ(true,
Expand Down
6 changes: 6 additions & 0 deletions browser/ui/webui/brave_adblock_ui.cc
Original file line number Diff line number Diff line change
Expand Up @@ -308,6 +308,12 @@ void AdblockDOMHandler::RefreshSubscriptionsList() {
subscription.last_update_attempt.ToJsTime());
dict->SetDoubleKey("last_successful_update_attempt",
subscription.last_successful_update_attempt.ToJsTime());
if (subscription.homepage) {
dict->SetStringKey("homepage", *subscription.homepage);
}
if (subscription.title) {
dict->SetStringKey("title", *subscription.title);
}
list_value->Append(std::move(dict));
}
CallJavascriptFunction("brave_adblock.onGetListSubscriptions", *list_value);
Expand Down
4 changes: 2 additions & 2 deletions build/rust/Cargo.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 1 addition & 1 deletion components/adblock_rust_ffi/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@ authors = ["Brian R. Bondy <[email protected]>"]
edition = "2018"

[dependencies]
adblock = { version = "0.5.3", default-features = false, features = ["full-regex-handling", "object-pooling", "unsync-regex-caching"] }
adblock = { version = "0.5.5", default-features = false, features = ["full-regex-handling", "object-pooling", "unsync-regex-caching"] }
serde_json = "1.0"
libc = "0.2"

Expand Down
9 changes: 8 additions & 1 deletion components/adblock_rust_ffi/cbindgen.toml
Original file line number Diff line number Diff line change
@@ -1,6 +1,13 @@
# The language to output bindings in
language = "C"
include_guard = "ADBLOCK_RUST_FFI_H"
include_guard = "BRAVE_COMPONENTS_ADBLOCK_RUST_FFI_SRC_LIB_H_"

header = """
/* Copyright (c) 2019 The Brave Authors. All rights reserved.
* This Source Code Form is subject to the terms of the Mozilla Public
* License, v. 2.0. If a copy of the MPL was not distributed with this file,
* You can obtain one at http://mozilla.org/MPL/2.0/. */"""
line_length = 80

[parse]
# Whether to parse dependent crates and include their types in the generated
Expand Down
54 changes: 52 additions & 2 deletions components/adblock_rust_ffi/src/lib.h
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,12 @@
*/
typedef struct C_Engine C_Engine;

/**
* Includes information about any "special comments" as described by
* https://help.eyeo.com/adblockplus/how-to-write-filters#special-comments
*/
typedef struct C_FilterListMetadata C_FilterListMetadata;

/**
* An external callback that receives a hostname and two out-parameters for
* start and end position. The callback should fill the start and end positions
Expand All @@ -34,11 +40,36 @@ typedef void (*C_DomainResolverCallback)(const char*, uint32_t*, uint32_t*);
bool set_domain_resolver(C_DomainResolverCallback resolver);

/**
* Create a new `Engine`.
* Create a new `Engine`, interpreting `data` as a C string and then parsing as
* a filter list in ABP syntax.
*/
struct C_Engine* engine_create(const char* rules);
struct C_Engine* engine_create_from_buffer(const char* data, size_t data_size);

/**
* Create a new `Engine`, interpreting `data` as a C string and then parsing as
* a filter list in ABP syntax. Also populates metadata from the filter list
* into `metadata`.
*/
struct C_Engine* engine_create_from_buffer_with_metadata(
const char* data,
size_t data_size,
struct C_FilterListMetadata** metadata);

/**
* Create a new `Engine`, interpreting `rules` as a null-terminated C string and
* then parsing as a filter list in ABP syntax.
*/
struct C_Engine* engine_create(const char* rules);

/**
* Create a new `Engine`, interpreting `rules` as a null-terminated C string and
* then parsing as a filter list in ABP syntax. Also populates metadata from the
* filter list into `metadata`.
*/
struct C_Engine* engine_create_with_metadata(
const char* rules,
struct C_FilterListMetadata** metadata);

/**
* Checks if a `url` matches for the specified `Engine` within the context.
*
Expand Down Expand Up @@ -109,6 +140,25 @@ bool engine_deserialize(struct C_Engine* engine,
*/
void engine_destroy(struct C_Engine* engine);

/**
* Puts a pointer to the homepage of the `FilterListMetadata` into `homepage`.
* Returns `true` if a homepage was returned.
*/
bool filter_list_metadata_homepage(const struct C_FilterListMetadata* metadata,
char** homepage);

/**
* Puts a pointer to the title of the `FilterListMetadata` into `title`. Returns
* `true` if a title was returned.
*/
bool filter_list_metadata_title(const struct C_FilterListMetadata* metadata,
char** title);

/**
* Destroy a `FilterListMetadata` once you are done with it.
*/
void filter_list_metadata_destroy(struct C_FilterListMetadata* metadata);

/**
* Destroy a `*c_char` once you are done with it.
*/
Expand Down
97 changes: 90 additions & 7 deletions components/adblock_rust_ffi/src/lib.rs
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
use adblock::blocker::Redirection;
use adblock::engine::Engine;
use adblock::lists::FilterListMetadata;
use adblock::resources::{MimeType, Resource, ResourceType};
use core::ptr;
use libc::size_t;
Expand Down Expand Up @@ -45,7 +46,8 @@ pub unsafe extern "C" fn set_domain_resolver(resolver: DomainResolverCallback) -
.is_ok()
}

/// Create a new `Engine`.
/// Create a new `Engine`, interpreting `data` as a C string and then parsing as a filter list in
/// ABP syntax.
#[no_mangle]
pub unsafe extern "C" fn engine_create_from_buffer(
data: *const c_char,
Expand All @@ -56,20 +58,59 @@ pub unsafe extern "C" fn engine_create_from_buffer(
eprintln!("Failed to parse filter list with invalid UTF-8 content");
""
});
engine_create_from_str(rules)
engine_create_from_str(rules).1
}

/// Create a new `Engine`, interpreting `data` as a C string and then parsing as a filter list in
/// ABP syntax. Also populates metadata from the filter list into `metadata`.
#[no_mangle]
pub unsafe extern "C" fn engine_create_from_buffer_with_metadata(
data: *const c_char,
data_size: size_t,
metadata: *mut *mut FilterListMetadata,
) -> *mut Engine {
let data: &[u8] = std::slice::from_raw_parts(data as *const u8, data_size);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I generally include an early return if data.is_null() or if data_size exceeds some reasonable maximum. There's not much to be done about invalid pointers, but I think it's good defensive programming.

That's obviously not directly related to this patch. Just a suggestion for the implementation in general.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's address this with the cxx port; it's already equally valid in the previous code and there's currently no handling for not returning an engine pointer from these functions.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fine with me, although you could return a ptr::null_mut and check for that on the other side.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yea, I'd also like to see if we can get rid of the out param, but waiting for cxx is fine

let rules = std::str::from_utf8(data).unwrap_or_else(|_| {
eprintln!("Failed to parse filter list with invalid UTF-8 content");
""
});
let (metadata_ptr, engine_ptr) = engine_create_from_str(rules);
*metadata = metadata_ptr;
engine_ptr
}

/// Create a new `Engine`, interpreting `rules` as a null-terminated C string and then parsing as a
/// filter list in ABP syntax.
#[no_mangle]
pub unsafe extern "C" fn engine_create(rules: *const c_char) -> *mut Engine {
let rules = CStr::from_ptr(rules).to_str().unwrap_or("");
engine_create_from_str(rules)
let rules = CStr::from_ptr(rules).to_str().unwrap_or_else(|_| {
eprintln!("Failed to parse filter list with invalid UTF-8 content");
""
});
engine_create_from_str(rules).1
}

/// Create a new `Engine`, interpreting `rules` as a null-terminated C string and then parsing as a
/// filter list in ABP syntax. Also populates metadata from the filter list into `metadata`.
#[no_mangle]
pub unsafe extern "C" fn engine_create_with_metadata(rules: *const c_char, metadata: *mut *mut FilterListMetadata) -> *mut Engine {
let rules = CStr::from_ptr(rules).to_str().unwrap_or_else(|_|{
eprintln!("Failed to parse filter list with invalid UTF-8 content");
""
});
let (metadata_ptr, engine_ptr) = engine_create_from_str(rules);
*metadata = metadata_ptr;
engine_ptr
}

fn engine_create_from_str(rules: &str) -> *mut Engine {
fn engine_create_from_str(rules: &str) -> (*mut FilterListMetadata, *mut Engine) {
let mut filter_set = adblock::lists::FilterSet::new(false);
filter_set.add_filter_list(&rules, Default::default());
let metadata = filter_set.add_filter_list(&rules, Default::default());
let engine = Engine::from_filter_set(filter_set, true);
Box::into_raw(Box::new(engine))
(
Box::into_raw(Box::new(metadata)),
Box::into_raw(Box::new(engine)),
)
}

/// Checks if a `url` matches for the specified `Engine` within the context.
Expand Down Expand Up @@ -234,6 +275,48 @@ pub unsafe extern "C" fn engine_destroy(engine: *mut Engine) {
}
}

/// Puts a pointer to the homepage of the `FilterListMetadata` into `homepage`. Returns `true` if a homepage was returned.
#[no_mangle]
pub unsafe extern "C" fn filter_list_metadata_homepage(metadata: *const FilterListMetadata, homepage: *mut *mut c_char) -> bool {
if let Some(this_homepage) = (*metadata).homepage.as_ref() {
let cstring = CString::new(this_homepage.as_str());
match cstring {
Ok(cstring) => {
*homepage = cstring.into_raw();
true
}
Err(_) => false,
}
} else {
false
}
}

/// Puts a pointer to the title of the `FilterListMetadata` into `title`. Returns `true` if a title was returned.
#[no_mangle]
pub unsafe extern "C" fn filter_list_metadata_title(metadata: *const FilterListMetadata, title: *mut *mut c_char) -> bool {
if let Some(this_title) = (*metadata).title.as_ref() {
let cstring = CString::new(this_title.as_str());
match cstring {
Ok(cstring) => {
*title = cstring.into_raw();
true
}
Err(_) => false,
}
} else {
false
}
}

/// Destroy a `FilterListMetadata` once you are done with it.
#[no_mangle]
pub unsafe extern "C" fn filter_list_metadata_destroy(metadata: *mut FilterListMetadata) {
if !metadata.is_null() {
drop(Box::from_raw(metadata));
}
}

/// Destroy a `*c_char` once you are done with it.
#[no_mangle]
pub unsafe extern "C" fn c_char_buffer_destroy(s: *mut c_char) {
Expand Down
44 changes: 44 additions & 0 deletions components/adblock_rust_ffi/src/wrapper.cc
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,50 @@ FilterList::FilterList(const FilterList& other) = default;

FilterList::~FilterList() {}

FilterListMetadata::FilterListMetadata() {}

FilterListMetadata::FilterListMetadata(C_FilterListMetadata* metadata) {
char* str_buffer;

if (filter_list_metadata_homepage(metadata, &str_buffer)) {
homepage = absl::make_optional(std::string(str_buffer));
c_char_buffer_destroy(str_buffer);
}

if (filter_list_metadata_title(metadata, &str_buffer)) {
title = absl::make_optional(std::string(str_buffer));
c_char_buffer_destroy(str_buffer);
}
}

FilterListMetadata::~FilterListMetadata() {}

FilterListMetadata::FilterListMetadata(FilterListMetadata&&) = default;

std::pair<FilterListMetadata, std::unique_ptr<Engine>> engineWithMetadata(
const std::string& rules) {
C_FilterListMetadata* c_metadata;
std::unique_ptr<Engine> engine = std::make_unique<Engine>(
engine_create_with_metadata(rules.c_str(), &c_metadata));
FilterListMetadata metadata = FilterListMetadata(c_metadata);
filter_list_metadata_destroy(c_metadata);

return std::make_pair(std::move(metadata), std::move(engine));
}

std::pair<FilterListMetadata, std::unique_ptr<Engine>>
engineFromBufferWithMetadata(const char* data, size_t data_size) {
C_FilterListMetadata* c_metadata;
std::unique_ptr<Engine> engine = std::make_unique<Engine>(
engine_create_from_buffer_with_metadata(data, data_size, &c_metadata));
FilterListMetadata metadata = FilterListMetadata(c_metadata);
filter_list_metadata_destroy(c_metadata);

return std::make_pair(std::move(metadata), std::move(engine));
}

Engine::Engine(C_Engine* c_engine) : raw(c_engine) {}

Engine::Engine() : raw(engine_create("")) {}

Engine::Engine(const std::string& rules) : raw(engine_create(rules.c_str())) {}
Expand Down
24 changes: 24 additions & 0 deletions components/adblock_rust_ffi/src/wrapper.h
Original file line number Diff line number Diff line change
Expand Up @@ -5,11 +5,14 @@

#ifndef BRAVE_COMPONENTS_ADBLOCK_RUST_FFI_SRC_WRAPPER_H_
#define BRAVE_COMPONENTS_ADBLOCK_RUST_FFI_SRC_WRAPPER_H_

#include <memory>
#include <string>
#include <utility>
#include <vector>

#include "base/memory/raw_ptr.h"
#include "third_party/abseil-cpp/absl/types/optional.h"

extern "C" {
#include "lib.h" // NOLINT
Expand Down Expand Up @@ -66,9 +69,23 @@ class ADBLOCK_EXPORT FilterList {
static std::vector<FilterList> regional_list;
};

typedef ADBLOCK_EXPORT struct FilterListMetadata {
FilterListMetadata();
explicit FilterListMetadata(C_FilterListMetadata* metadata);
~FilterListMetadata();

absl::optional<std::string> homepage;
absl::optional<std::string> title;

FilterListMetadata(FilterListMetadata&&);

FilterListMetadata(const FilterListMetadata&) = delete;
} FilterListMetadata;

class ADBLOCK_EXPORT Engine {
public:
Engine();
explicit Engine(C_Engine* c_engine);
explicit Engine(const std::string& rules);
Engine(const char* data, size_t data_size);
void matches(const std::string& url,
Expand Down Expand Up @@ -100,12 +117,19 @@ class ADBLOCK_EXPORT Engine {
const std::vector<std::string>& exceptions);
~Engine();

Engine(Engine&&) = default;

private:
Engine(const Engine&) = delete;
void operator=(const Engine&) = delete;
raw_ptr<C_Engine> raw = nullptr;
};

std::pair<FilterListMetadata, std::unique_ptr<Engine>> engineWithMetadata(
const std::string& rules);
std::pair<FilterListMetadata, std::unique_ptr<Engine>>
engineFromBufferWithMetadata(const char* data, size_t data_size);

} // namespace adblock

#endif // BRAVE_COMPONENTS_ADBLOCK_RUST_FFI_SRC_WRAPPER_H_
Loading