Skip to content

Commit

Permalink
Pull request: 4916 Editing filter
Browse files Browse the repository at this point in the history 
Merge in DNS/adguard-home from 4916-fix-filter-edit to master

Closes AdguardTeam#4916.

Squashed commit of the following:

commit c31be58
Merge: c9f3e33 67d8966
Author: Eugene Burkov <E.Burkov@AdGuard.COM>
Date:   Fri Oct 21 19:58:16 2022 +0300

    Merge branch 'master' into 4916-fix-filter-edit

commit c9f3e33
Author: Eugene Burkov <E.Burkov@AdGuard.COM>
Date:   Fri Oct 21 14:49:53 2022 +0300

    filtering: imp docs

commit ef8228f
Author: Eugene Burkov <E.Burkov@AdGuard.COM>
Date:   Fri Oct 21 12:40:00 2022 +0300

    filtering: imp code

commit 57fdbfc
Author: Eugene Burkov <E.Burkov@AdGuard.COM>
Date:   Thu Oct 20 11:54:39 2022 +0300

    filtering: imp docs

commit 670ac9a
Author: Eugene Burkov <E.Burkov@AdGuard.COM>
Date:   Wed Oct 19 21:03:26 2022 +0300

    home: unexport close of clients container

commit f5b2916
Merge: 2e57624 2de4228
Author: Eugene Burkov <E.Burkov@AdGuard.COM>
Date:   Wed Oct 19 21:02:33 2022 +0300

    Merge branch 'master' into 4916-fix-filter-edit

commit 2e57624
Author: Eugene Burkov <E.Burkov@AdGuard.COM>
Date:   Wed Oct 19 21:01:19 2022 +0300

    filtering: imp code, tests

commit be56df7
Author: Eugene Burkov <E.Burkov@AdGuard.COM>
Date:   Tue Oct 18 15:31:30 2022 +0300

    filtering: fix url edit
  • Loading branch information
@EugeneOne1
EugeneOne1 committed Oct 21, 2022
1 parent 67d8966 commit a149d81
Show file tree
Hide file tree
Showing 8 changed files with 267 additions and 119 deletions.
3 changes: 3 additions & 0 deletions CHANGELOG.md
View file
Expand Up @@ -30,6 +30,8 @@ and this project adheres to

### Fixed

- Editing an enabled rule list's URL now also includes validation of the filter
contents preventing from saving a bad one ([#4916]).
- The default value of `dns.cache_size` accidentally set to 0 has now been
reverted to 4 MiB ([#5010]).
- Responses for which the DNSSEC validation had explicitly been omitted aren't
Expand All @@ -38,6 +40,7 @@ and this project adheres to

[#2926]: https://github.com/AdguardTeam/AdGuardHome/issues/2926
[#3418]: https://github.com/AdguardTeam/AdGuardHome/issues/3418
[#4916]: https://github.com/AdguardTeam/AdGuardHome/issues/4916
[#4925]: https://github.com/AdguardTeam/AdGuardHome/issues/4925
[#4942]: https://github.com/AdguardTeam/AdGuardHome/issues/4942
[#4986]: https://github.com/AdguardTeam/AdGuardHome/issues/4986
Expand Down
140 changes: 75 additions & 65 deletions internal/filtering/filter.go
View file
Expand Up @@ -54,97 +54,120 @@ func (filter *FilterYAML) Path(dataDir string) string {
}

const (
statusFound = 1 << iota
statusEnabledChanged
statusURLChanged
statusURLExists
statusUpdateRequired
// errFilterNotExist is returned from [filterSetProperties] when there are
// no lists with the desired URL to update.
//
// TODO(e.burkov): Use wherever the same error is needed.
errFilterNotExist errors.Error = "url doesn't exist"

// errFilterExists is returned from [filterSetProperties] when there is
// another filter having the same URL as the one updated.
//
// TODO(e.burkov): Use wherever the same error is needed.
errFilterExists errors.Error = "url already exists"
)

// Update properties for a filter specified by its URL
// Return status* flags.
func (d *DNSFilter) filterSetProperties(url string, newf FilterYAML, whitelist bool) int {
r := 0
// filterSetProperties searches for the particular filter list by url and sets
// the values of newList to it, updating afterwards if needed. It returns true
// if the update was performed and the filtering engine restart is required.
func (d *DNSFilter) filterSetProperties(
listURL string,
newList FilterYAML,
isAllowlist bool,
) (shouldRestart bool, err error) {
d.filtersMu.Lock()
defer d.filtersMu.Unlock()

filters := d.Filters
if whitelist {
if isAllowlist {
filters = d.WhitelistFilters
}

i := slices.IndexFunc(filters, func(filt FilterYAML) bool {
return filt.URL == url
})
i := slices.IndexFunc(filters, func(filt FilterYAML) bool { return filt.URL == listURL })
if i == -1 {
return 0
return false, errFilterNotExist
}

filt := &filters[i]
log.Debug(
"filtering: set name to %q, url to %s, enabled to %t for filter %s",
newList.Name,
newList.URL,
newList.Enabled,
filt.URL,
)

defer func(oldURL, oldName string, oldEnabled bool, oldUpdated time.Time) {
if err != nil {
filt.URL = oldURL
filt.Name = oldName
filt.Enabled = oldEnabled
filt.LastUpdated = oldUpdated
}
}(filt.URL, filt.Name, filt.Enabled, filt.LastUpdated)

log.Debug("filter: set properties: %s: {%s %s %v}", filt.URL, newf.Name, newf.URL, newf.Enabled)
filt.Name = newf.Name
filt.Name = newList.Name

if filt.URL != newf.URL {
r |= statusURLChanged | statusUpdateRequired
if d.filterExistsNoLock(newf.URL) {
return statusURLExists
if filt.URL != newList.URL {
if d.filterExistsLocked(newList.URL) {
return false, errFilterExists
}

filt.URL = newf.URL
filt.unload()
shouldRestart = true

filt.URL = newList.URL
filt.LastUpdated = time.Time{}
filt.checksum = 0
filt.RulesCount = 0
}

if filt.Enabled != newf.Enabled {
r |= statusEnabledChanged
filt.Enabled = newf.Enabled
if filt.Enabled {
if (r & statusURLChanged) == 0 {
err := d.load(filt)
if err != nil {
// TODO(e.burkov): It seems the error is only returned when
// the file exists and couldn't be open. Investigate and
// improve.
log.Error("loading filter %d: %s", filt.ID, err)

filt.LastUpdated = time.Time{}
filt.checksum = 0
filt.RulesCount = 0
r |= statusUpdateRequired
}
}
} else {
filt.unload()
filt.unload()
}

if filt.Enabled != newList.Enabled {
filt.Enabled = newList.Enabled
shouldRestart = true
}

if filt.Enabled {
if shouldRestart {
// Download the filter contents.
shouldRestart, err = d.update(filt)
}
} else {
// TODO(e.burkov): The validation of the contents of the new URL is
// currently skipped if the rule list is disabled. This makes it
// possible to set a bad rules source, but the validation should still
// kick in when the filter is enabled. Consider making changing this
// behavior to be stricter.
filt.unload()
}

return r | statusFound
return shouldRestart, err
}

// Return TRUE if a filter with this URL exists
func (d *DNSFilter) filterExists(url string) bool {
// filterExists returns true if a filter with the same url exists in d. It's
// safe for concurrent use.
func (d *DNSFilter) filterExists(url string) (ok bool) {
d.filtersMu.RLock()
defer d.filtersMu.RUnlock()

r := d.filterExistsNoLock(url)
r := d.filterExistsLocked(url)

return r
}

func (d *DNSFilter) filterExistsNoLock(url string) bool {
// filterExistsLocked returns true if d contains the filter with the same url.
// d.filtersMu is expected to be locked.
func (d *DNSFilter) filterExistsLocked(url string) (ok bool) {
for _, f := range d.Filters {
if f.URL == url {
return true
}
}

for _, f := range d.WhitelistFilters {
if f.URL == url {
return true
}
}

return false
}

Expand All @@ -155,7 +178,7 @@ func (d *DNSFilter) filterAdd(flt FilterYAML) bool {
defer d.filtersMu.Unlock()

// Check for duplicates
if d.filterExistsNoLock(flt.URL) {
if d.filterExistsLocked(flt.URL) {
return false
}

Expand Down Expand Up @@ -258,18 +281,6 @@ func (d *DNSFilter) tryRefreshFilters(block, allow, force bool) (updated int, is
return updated, isNetworkErr, ok
}

// refreshFilters updates the lists and returns the number of updated ones.
// It's safe for concurrent use, but blocks at least until the previous
// refreshing is finished.
func (d *DNSFilter) refreshFilters(block, allow, force bool) (updated int) {
d.refreshLock.Lock()
defer d.refreshLock.Unlock()

updated, _ = d.refreshFiltersIntl(block, allow, force)

return updated
}

// listsToUpdate returns the slice of filter lists that could be updated.
func (d *DNSFilter) listsToUpdate(filters *[]FilterYAML, force bool) (toUpd []FilterYAML) {
now := time.Now()
Expand All @@ -279,7 +290,6 @@ func (d *DNSFilter) listsToUpdate(filters *[]FilterYAML, force bool) (toUpd []Fi

for i := range *filters {
flt := &(*filters)[i] // otherwise we will be operating on a copy
log.Debug("checking list at index %d: %v", i, flt)

if !flt.Enabled {
continue
Expand Down
50 changes: 28 additions & 22 deletions internal/filtering/filter_test.go
View file
Expand Up @@ -4,40 +4,43 @@ import (
"io/fs"
"net"
"net/http"
"net/netip"
"net/url"
"os"
"path"
"path/filepath"
"testing"
"time"

"github.com/AdguardTeam/golibs/netutil"
"github.com/AdguardTeam/AdGuardHome/internal/aghnet"
"github.com/AdguardTeam/golibs/testutil"
"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/require"
)

const testFltsFileName = "1.txt"

func testStartFilterListener(t *testing.T, fltContent *[]byte) (l net.Listener) {
// serveFiltersLocally is a helper that concurrently listens on a free port to
// respond with fltContent. It also gracefully closes the listener when the
// test under t finishes.
func serveFiltersLocally(t *testing.T, fltContent []byte) (ipp netip.AddrPort) {
t.Helper()

h := http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) {
n, werr := w.Write(*fltContent)
require.NoError(t, werr)
require.Equal(t, len(*fltContent), n)
pt := testutil.PanicT{}

n, werr := w.Write(fltContent)
require.NoError(pt, werr)
require.Equal(pt, len(fltContent), n)
})

var err error
l, err = net.Listen("tcp", ":0")
l, err := net.Listen("tcp", ":0")
require.NoError(t, err)

go func() {
_ = http.Serve(l, h)
}()
go func() { _ = http.Serve(l, h) }()
testutil.CleanupAndRequireSuccess(t, l.Close)

return l
addr := l.Addr()
require.IsType(t, new(net.TCPAddr), addr)

return netip.AddrPortFrom(aghnet.IPv4Localhost(), uint16(addr.(*net.TCPAddr).Port))
}

func TestFilters(t *testing.T) {
Expand All @@ -49,7 +52,7 @@ func TestFilters(t *testing.T) {

fltContent := []byte(content)

l := testStartFilterListener(t, &fltContent)
addr := serveFiltersLocally(t, fltContent)

tempDir := t.TempDir()

Expand All @@ -64,11 +67,7 @@ func TestFilters(t *testing.T) {
f := &FilterYAML{
URL: (&url.URL{
Scheme: "http",
Host: (&netutil.IPPort{
IP: net.IP{127, 0, 0, 1},
Port: l.Addr().(*net.TCPAddr).Port,
}).String(),
Path: path.Join(filterDir, testFltsFileName),
Host: addr.String(),
}).String(),
}

Expand Down Expand Up @@ -101,8 +100,15 @@ func TestFilters(t *testing.T) {
})

t.Run("refresh_actually", func(t *testing.T) {
fltContent = []byte(`||example.com^`)
t.Cleanup(func() { fltContent = []byte(content) })
anotherContent := []byte(`||example.com^`)
oldURL := f.URL

ipp := serveFiltersLocally(t, anotherContent)
f.URL = (&url.URL{
Scheme: "http",
Host: ipp.String(),
}).String()
t.Cleanup(func() { f.URL = oldURL })

updateAndAssert(t, require.True, 1)
})
Expand Down
16 changes: 9 additions & 7 deletions internal/filtering/filtering.go
View file
Expand Up @@ -345,27 +345,29 @@ func (d *DNSFilter) SetFilters(blockFilters, allowFilters []Filter, async bool)
blockFilters: blockFilters,
}

d.filtersInitializerLock.Lock() // prevent multiple writers from adding more than 1 task
d.filtersInitializerLock.Lock()
defer d.filtersInitializerLock.Unlock()

// remove all pending tasks
stop := false
for !stop {
// Remove all pending tasks.
removeLoop:
for {
select {
case <-d.filtersInitializerChan:
//
// Continue removing.
default:
stop = true
break removeLoop
}
}

d.filtersInitializerChan <- params

return nil
}

err := d.initFiltering(allowFilters, blockFilters)
if err != nil {
log.Error("Can't initialize filtering subsystem: %s", err)
log.Error("filtering: can't initialize filtering subsystem: %s", err)

return err
}

Expand Down

0 comments on commit a149d81

Please sign in to comment.