diff mbox

[nf-next] netfilter: ftp: Remove the useless dlen==0 condition check in find_pattern

Message ID 1472696057-15819-1-git-send-email-fgao@ikuai8.com
State Changes Requested
Delegated to: Pablo Neira
Headers show

Commit Message

高峰 Sept. 1, 2016, 2:14 a.m. UTC 
From: Gao Feng <fgao@ikuai8.com>

The caller function "help" has already make sure the datalen could not be zero
before invoke find_pattern as a parameter by the following codes

	if (dataoff >= skb->len) {
		pr_debug("ftp: dataoff(%u) >= skblen(%u)\n", dataoff,
			 skb->len);
		return NF_ACCEPT;
	}
	datalen = skb->len - dataoff;

And the latter codes "ends_in_nl = (fb_ptr[datalen - 1] == '\n');" use datalen
directly without checking if it is zero.

So it is unneccessary to check it in find_pattern too.

Signed-off-by: Gao Feng <fgao@ikuai8.com>
---
 net/netfilter/nf_conntrack_ftp.c | 12 ------------
 1 file changed, 12 deletions(-)

Comments

Feng Gao Sept. 1, 2016, 2:26 a.m. UTC | #1 
On Thu, Sep 1, 2016 at 10:14 AM,  <fgao@ikuai8.com> wrote:
> From: Gao Feng <fgao@ikuai8.com>
>
> The caller function "help" has already make sure the datalen could not be zero
> before invoke find_pattern as a parameter by the following codes
>
>         if (dataoff >= skb->len) {
>                 pr_debug("ftp: dataoff(%u) >= skblen(%u)\n", dataoff,
>                          skb->len);
>                 return NF_ACCEPT;
>         }
>         datalen = skb->len - dataoff;
>
> And the latter codes "ends_in_nl = (fb_ptr[datalen - 1] == '\n');" use datalen
> directly without checking if it is zero.
>
> So it is unneccessary to check it in find_pattern too.
>
> Signed-off-by: Gao Feng <fgao@ikuai8.com>
> ---
>  net/netfilter/nf_conntrack_ftp.c | 12 ------------
>  1 file changed, 12 deletions(-)
>
> diff --git a/net/netfilter/nf_conntrack_ftp.c b/net/netfilter/nf_conntrack_ftp.c
> index b6934b5..77f05ac 100644
> --- a/net/netfilter/nf_conntrack_ftp.c
> +++ b/net/netfilter/nf_conntrack_ftp.c
> @@ -301,8 +301,6 @@ static int find_pattern(const char *data, size_t dlen,
>         size_t i = plen;
>
>         pr_debug("find_pattern `%s': dlen = %Zu\n", pattern, dlen);
> -       if (dlen == 0)
> -               return 0;
>
>         if (dlen <= plen) {
>                 /* Short packet: try for partial? */
> @@ -312,16 +310,6 @@ static int find_pattern(const char *data, size_t dlen,
>         }
>
>         if (strncasecmp(data, pattern, plen) != 0) {
> -#if 0
> -               size_t i;
> -
> -               pr_debug("ftp: string mismatch\n");
> -               for (i = 0; i < plen; i++) {
> -                       pr_debug("ftp:char %u `%c'(%u) vs `%c'(%u)\n",
> -                                i, data[i], data[i],
> -                                pattern[i], pattern[i]);
> -               }
> -#endif
>                 return 0;
>         }
>
> --
> 1.9.1
>
>

Sorry, I forget one point. I also remove some codes which are
commented out by #if0.

Regards
Feng
--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Pablo Neira Ayuso Sept. 1, 2016, 10:20 a.m. UTC | #2 
On Thu, Sep 01, 2016 at 10:26:08AM +0800, Feng Gao wrote:
> On Thu, Sep 1, 2016 at 10:14 AM,  <fgao@ikuai8.com> wrote:
> > From: Gao Feng <fgao@ikuai8.com>
> >
> > The caller function "help" has already make sure the datalen could not be zero
> > before invoke find_pattern as a parameter by the following codes
> >
> >         if (dataoff >= skb->len) {
> >                 pr_debug("ftp: dataoff(%u) >= skblen(%u)\n", dataoff,
> >                          skb->len);
> >                 return NF_ACCEPT;
> >         }
> >         datalen = skb->len - dataoff;
> >
> > And the latter codes "ends_in_nl = (fb_ptr[datalen - 1] == '\n');" use datalen
> > directly without checking if it is zero.
> >
> > So it is unneccessary to check it in find_pattern too.
> >
> > Signed-off-by: Gao Feng <fgao@ikuai8.com>
> > ---
> >  net/netfilter/nf_conntrack_ftp.c | 12 ------------
> >  1 file changed, 12 deletions(-)
> >
> > diff --git a/net/netfilter/nf_conntrack_ftp.c b/net/netfilter/nf_conntrack_ftp.c
> > index b6934b5..77f05ac 100644
> > --- a/net/netfilter/nf_conntrack_ftp.c
> > +++ b/net/netfilter/nf_conntrack_ftp.c
> > @@ -301,8 +301,6 @@ static int find_pattern(const char *data, size_t dlen,
> >         size_t i = plen;
> >
> >         pr_debug("find_pattern `%s': dlen = %Zu\n", pattern, dlen);
> > -       if (dlen == 0)
> > -               return 0;
> >
> >         if (dlen <= plen) {
> >                 /* Short packet: try for partial? */
> > @@ -312,16 +310,6 @@ static int find_pattern(const char *data, size_t dlen,
> >         }
> >
> >         if (strncasecmp(data, pattern, plen) != 0) {
> > -#if 0
> > -               size_t i;
> > -
> > -               pr_debug("ftp: string mismatch\n");
> > -               for (i = 0; i < plen; i++) {
> > -                       pr_debug("ftp:char %u `%c'(%u) vs `%c'(%u)\n",
> > -                                i, data[i], data[i],
> > -                                pattern[i], pattern[i]);
> > -               }
> > -#endif
> >                 return 0;
> >         }
> >
> > --
> > 1.9.1
> >
> >
> 
> Sorry, I forget one point. I also remove some codes which are
> commented out by #if0.

Please, do this in a separated patch. Thanks.
--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Feng Gao Sept. 1, 2016, 11:01 a.m. UTC | #3 
On Thu, Sep 1, 2016 at 6:20 PM, Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> On Thu, Sep 01, 2016 at 10:26:08AM +0800, Feng Gao wrote:
>> On Thu, Sep 1, 2016 at 10:14 AM,  <fgao@ikuai8.com> wrote:
>> > From: Gao Feng <fgao@ikuai8.com>
>> >
>> > The caller function "help" has already make sure the datalen could not be zero
>> > before invoke find_pattern as a parameter by the following codes
>> >
>> >         if (dataoff >= skb->len) {
>> >                 pr_debug("ftp: dataoff(%u) >= skblen(%u)\n", dataoff,
>> >                          skb->len);
>> >                 return NF_ACCEPT;
>> >         }
>> >         datalen = skb->len - dataoff;
>> >
>> > And the latter codes "ends_in_nl = (fb_ptr[datalen - 1] == '\n');" use datalen
>> > directly without checking if it is zero.
>> >
>> > So it is unneccessary to check it in find_pattern too.
>> >
>> > Signed-off-by: Gao Feng <fgao@ikuai8.com>
>> > ---
>> >  net/netfilter/nf_conntrack_ftp.c | 12 ------------
>> >  1 file changed, 12 deletions(-)
>> >
>> > diff --git a/net/netfilter/nf_conntrack_ftp.c b/net/netfilter/nf_conntrack_ftp.c
>> > index b6934b5..77f05ac 100644
>> > --- a/net/netfilter/nf_conntrack_ftp.c
>> > +++ b/net/netfilter/nf_conntrack_ftp.c
>> > @@ -301,8 +301,6 @@ static int find_pattern(const char *data, size_t dlen,
>> >         size_t i = plen;
>> >
>> >         pr_debug("find_pattern `%s': dlen = %Zu\n", pattern, dlen);
>> > -       if (dlen == 0)
>> > -               return 0;
>> >
>> >         if (dlen <= plen) {
>> >                 /* Short packet: try for partial? */
>> > @@ -312,16 +310,6 @@ static int find_pattern(const char *data, size_t dlen,
>> >         }
>> >
>> >         if (strncasecmp(data, pattern, plen) != 0) {
>> > -#if 0
>> > -               size_t i;
>> > -
>> > -               pr_debug("ftp: string mismatch\n");
>> > -               for (i = 0; i < plen; i++) {
>> > -                       pr_debug("ftp:char %u `%c'(%u) vs `%c'(%u)\n",
>> > -                                i, data[i], data[i],
>> > -                                pattern[i], pattern[i]);
>> > -               }
>> > -#endif
>> >                 return 0;
>> >         }
>> >
>> > --
>> > 1.9.1
>> >
>> >
>>
>> Sorry, I forget one point. I also remove some codes which are
>> commented out by #if0.
>
> Please, do this in a separated patch. Thanks.

Done.
I thought the "#if 0" change is very tiny, and it did not deserve one commit.

Now I get it. It needs one separate commit, even though how tiny it is.

Best Regards
Feng
--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Pablo Neira Ayuso Sept. 1, 2016, 11:08 a.m. UTC | #4 
On Thu, Sep 01, 2016 at 07:01:51PM +0800, Feng Gao wrote:
> On Thu, Sep 1, 2016 at 6:20 PM, Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> > On Thu, Sep 01, 2016 at 10:26:08AM +0800, Feng Gao wrote:
> >> On Thu, Sep 1, 2016 at 10:14 AM,  <fgao@ikuai8.com> wrote:
[...]
> >> Sorry, I forget one point. I also remove some codes which are
> >> commented out by #if0.
> >
> > Please, do this in a separated patch. Thanks.
> 
> Done.
> I thought the "#if 0" change is very tiny, and it did not deserve one commit.
> 
> Now I get it. It needs one separate commit, even though how tiny it is.

Better split patches into logical changes.

In general, if you end up describing two changes that don't relate
each other in the patch description, then this usually means you need
two patches.

Thanks for addressing my feedback!
--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

diff --git a/net/netfilter/nf_conntrack_ftp.c b/net/netfilter/nf_conntrack_ftp.c
index b6934b5..77f05ac 100644
--- a/net/netfilter/nf_conntrack_ftp.c
+++ b/net/netfilter/nf_conntrack_ftp.c
@@ -301,8 +301,6 @@  static int find_pattern(const char *data, size_t dlen,
 	size_t i = plen;
 
 	pr_debug("find_pattern `%s': dlen = %Zu\n", pattern, dlen);
-	if (dlen == 0)
-		return 0;
 
 	if (dlen <= plen) {
 		/* Short packet: try for partial? */
@@ -312,16 +310,6 @@  static int find_pattern(const char *data, size_t dlen,
 	}
 
 	if (strncasecmp(data, pattern, plen) != 0) {
-#if 0
-		size_t i;
-
-		pr_debug("ftp: string mismatch\n");
-		for (i = 0; i < plen; i++) {
-			pr_debug("ftp:char %u `%c'(%u) vs `%c'(%u)\n",
-				 i, data[i], data[i],
-				 pattern[i], pattern[i]);
-		}
-#endif
 		return 0;
 	}