| Message ID | 20200815165030.5849-1-ztong0001@gmail.com |
|---|---|
| State | Awaiting Upstream |
| Delegated to: | David Miller |
| Headers | show |
| Series | netfilter: nf_conntrack_sip: fix parsing error | expand |
Tong Zhang <ztong0001@gmail.com> wrote: > ct_sip_parse_numerical_param can only return 0 or 1, but the caller is > checking parsing error using < 0 Reviewed-by: Florian Westphal <fw@strlen.de>
On Sat, Aug 15, 2020 at 12:50:30PM -0400, Tong Zhang wrote: > ct_sip_parse_numerical_param can only return 0 or 1, but the caller is > checking parsing error using < 0 Is this are real issue in your setup or probably some static analysis tool is reporting? You are right that ct_sip_parse_numerical_param() never returns < 0, however, looking at: https://tools.ietf.org/html/rfc3261 see Page 161 expires is optional, my understanding is that your patch is making this option mandatory.
Hi Pablo, I'm not an expert in this networking stuff. But from my point of view there's no point in checking if this condition is always true. There's also no need of returning anything from the ct_sip_parse_numerical_param() if they are all being ignored like this. On Fri, Aug 28, 2020 at 2:07 PM Pablo Neira Ayuso <pablo@netfilter.org> wrote: > Is this are real issue in your setup or probably some static analysis > tool is reporting? > > You are right that ct_sip_parse_numerical_param() never returns < 0, > however, looking at: > > https://tools.ietf.org/html/rfc3261 see Page 161 > > expires is optional, my understanding is that your patch is making > this option mandatory.
On Fri, Aug 28, 2020 at 02:14:48PM -0400, Tong Zhang wrote: > Hi Pablo, > I'm not an expert in this networking stuff. > But from my point of view there's no point in checking if this > condition is always true. Understood. > There's also no need of returning anything from the > ct_sip_parse_numerical_param() > if they are all being ignored like this. Then probably update this code to ignore the return value?
I think the original code complaining parsing error is there for a reason, A better way is to modify ct_sip_parse_numerical_param() and let it return a real parsing error code instead of returning FOUND(1) and NOT FOUND(0) if deemed necessary Once again I'm not an expert and I'm may suggest something stupid, please pardon my ignorance -- - Tong On Fri, Aug 28, 2020 at 2:19 PM Pablo Neira Ayuso <pablo@netfilter.org> wrote: > > Then probably update this code to ignore the return value?
diff --git a/net/netfilter/nf_conntrack_sip.c b/net/netfilter/nf_conntrack_sip.c index b83dc9bf0a5d..08873694120a 100644 --- a/net/netfilter/nf_conntrack_sip.c +++ b/net/netfilter/nf_conntrack_sip.c @@ -1269,7 +1269,7 @@ static int process_register_request(struct sk_buff *skb, unsigned int protoff, if (ct_sip_parse_numerical_param(ct, *dptr, matchoff + matchlen, *datalen, - "expires=", NULL, NULL, &expires) < 0) { + "expires=", NULL, NULL, &expires) == 0) { nf_ct_helper_log(skb, ct, "cannot parse expires"); return NF_DROP; } @@ -1375,7 +1375,7 @@ static int process_register_response(struct sk_buff *skb, unsigned int protoff, matchoff + matchlen, *datalen, "expires=", NULL, NULL, &c_expires); - if (ret < 0) { + if (ret == 0) { nf_ct_helper_log(skb, ct, "cannot parse expires"); return NF_DROP; }
ct_sip_parse_numerical_param can only return 0 or 1, but the caller is checking parsing error using < 0 Signed-off-by: Tong Zhang <ztong0001@gmail.com> --- net/netfilter/nf_conntrack_sip.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)