ss filter problem

ss filter problem

[Phil Sutter]

Hi,

I am trying to fix a bug in ss filter code, but feel quite lost right
now. The issue is this:

| ss -nl -f inet '( sport = :22 )'

prints not only listening sockets (as requested by -l flag), but
established ones as well (reproduce by opening ssh connection to
127.0.0.1 before calling above).

In contrast, the following both don't show the established sockets:

| ss -nl '( sport = :22 )'
| ss -nl -f inet

My investigation led me to see that current_filter.states is altered
after ssfilter_parse() returns, and using gdb with a watchpoint I was
able to identify parse_hostcond() to be the bad guy: In line 1560, it
calls filter_af_set() after checking for fam != AF_UNSPEC (which is the
case, since fam = preferred_family and the latter is changed to AF_INET
when parsing '-f inet' parameter).

This whole jumping back and forth confuses me quite effectively. Since
you did some fixes in the past already, are you possibly able to point
out where/how this tiny mess has to be fixed?

I guess in an ideal world we would translate '-l' to 'state listen', '-f
inet' to 'src inet:*' and pass everything ANDed together to
ssfilter_parse(). Or maybe that would make things even worse. ;)

Cheers, Phil

Re: ss filter problem

[Vadim Kochan]

Hi Phil,

On Tue, Mar 29, 2016 at 09:32:42PM +0200, Phil Sutter wrote:
> Hi,
> 
> I am trying to fix a bug in ss filter code, but feel quite lost right
> now. The issue is this:
> 
> | ss -nl -f inet '( sport = :22 )'
> 
> prints not only listening sockets (as requested by -l flag), but
> established ones as well (reproduce by opening ssh connection to
> 127.0.0.1 before calling above).
> 
> In contrast, the following both don't show the established sockets:
> 
> | ss -nl '( sport = :22 )'
> | ss -nl -f inet
> 
> My investigation led me to see that current_filter.states is altered
> after ssfilter_parse() returns, and using gdb with a watchpoint I was
> able to identify parse_hostcond() to be the bad guy: In line 1560, it
> calls filter_af_set() after checking for fam != AF_UNSPEC (which is the
> case, since fam = preferred_family and the latter is changed to AF_INET
> when parsing '-f inet' parameter).

Yes, after removing of fam != AF_UNSPEC body - it works, because
it does not overwrite specified states (-l) from command line, but I
can't say what it may affect else, I will try to investigate it better.

> 
> This whole jumping back and forth confuses me quite effectively. Since
> you did some fixes in the past already, are you possibly able to point
> out where/how this tiny mess has to be fixed?
> 
> I guess in an ideal world we would translate '-l' to 'state listen', '-f
> inet' to 'src inet:*' and pass everything ANDed together to
> ssfilter_parse(). Or maybe that would make things even worse. ;)
> 
> Cheers, Phil

I thought I fixed & tested well ss filter, but seems it would be good to
have good automation testing.

Regards,
Vadim Kochan

[iproute PATCH 0/2] Minor ss filter fix and review

[Phil Sutter]

While looking for a solution to the problem described in patch 2/2, I
discovered the overly complicated assignment in filter_states_set() which
is simplified in patch 1/2.

Phil Sutter (2):
  ss: Drop silly assignment
  ss: Fix accidental state filter override

 misc/ss.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

-- 
2.8.0

[iproute PATCH 1/2] ss: Drop silly assignment

[Phil Sutter]

An expression of the form '(a | b) & b' will evaluate to the value of b
for any value of a or b.

Signed-off-by: Phil Sutter <phil@nwl.cc>
---
 misc/ss.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/misc/ss.c b/misc/ss.c
index 38cf3312a746e..d6090018c5dbb 100644
--- a/misc/ss.c
+++ b/misc/ss.c
@@ -267,7 +267,7 @@ static void filter_default_dbs(struct filter *f)
 static void filter_states_set(struct filter *f, int states)
 {
 	if (states)
-		f->states = (f->states | states) & states;
+		f->states = states;
 }
 
 static void filter_merge_defaults(struct filter *f)
-- 
2.8.0

[iproute PATCH 2/2] ss: Fix accidental state filter override

[Phil Sutter]

Passing a filter expression and selecting an address family using the
'-f' flag would overwrite the state filter by accident. Therefore
calling e.g. 'ss -nl -f inet '(sport = :22)' would not only print
listening sockets (as requested by '-l' flag) but connected ones, as
well.

Fix this by reusing the formerly ineffective call to filter_states_set()
to restore the state filter as it was before the call to
filter_af_set().

Signed-off-by: Phil Sutter <phil@nwl.cc>
---
 misc/ss.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/misc/ss.c b/misc/ss.c
index d6090018c5dbb..544def3f08ea8 100644
--- a/misc/ss.c
+++ b/misc/ss.c
@@ -1556,9 +1556,10 @@ void *parse_hostcond(char *addr, bool is_port)
 
 out:
 	if (fam != AF_UNSPEC) {
+		int states = f->states;
 		f->families = 0;
 		filter_af_set(f, fam);
-		filter_states_set(f, 0);
+		filter_states_set(f, states);
 	}
 
 	res = malloc(sizeof(*res));
-- 
2.8.0

Re: [iproute PATCH 0/2] Minor ss filter fix and review

[Stephen Hemminger]

On Wed, 13 Apr 2016 22:07:03 +0200
Phil Sutter <phil@nwl.cc> wrote:

> While looking for a solution to the problem described in patch 2/2, I
> discovered the overly complicated assignment in filter_states_set() which
> is simplified in patch 1/2.
> 
> Phil Sutter (2):
>   ss: Drop silly assignment
>   ss: Fix accidental state filter override
> 
>  misc/ss.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 

Applied