| Message ID | 20240516193700.212737-3-mkp@redhat.com |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | clang: Fix Clang's static analyzer detections. | expand |
| Context | Check | Description |
|---|---|---|
| ovsrobot/apply-robot | warning | apply and check: warning |
| ovsrobot/github-robot-_Build_and_Test | success | github build: passed |
| ovsrobot/intel-ovs-compilation | success | test: success |
Bleep bloop. Greetings Mike Pattrick, I am a robot and I have tried out your patch. Thanks for your contribution. I encountered some error that I wasn't expecting. See the details below. checkpatch: WARNING: The subject, '<area>: <summary>', is over 70 characters, i.e., 78. Subject: netdev-native-tnl: Fix Clang's static analyzer 'uninitialized value' warnings. Lines checked: 35, Warnings: 1, Errors: 0 Please check this out. If you feel there has been an error, please email aconole@redhat.com Thanks, 0-day Robot
On 5/16/24 21:36, Mike Pattrick wrote: > Clang's static analyzer will complain about an uninitialized value > because we weren't properly checking the error code from a function that > would have initialized the value. Please, be more specific. :) At least, mention the variable name. And the same comment for the subject of the patch. This one is actually a real potential issue. So, something like this would be an appropriate name: netdev-native-tnl: Fix use of uninitialized offset on SRv6 header pop. > > Instead, add a check for that return code. > > Signed-off-by: Mike Pattrick <mkp@redhat.com> > --- > lib/netdev-native-tnl.c | 4 +++- > 1 file changed, 3 insertions(+), 1 deletion(-) > > diff --git a/lib/netdev-native-tnl.c b/lib/netdev-native-tnl.c > index dee9ab344..6e6b15764 100644 > --- a/lib/netdev-native-tnl.c > +++ b/lib/netdev-native-tnl.c > @@ -1068,7 +1068,9 @@ netdev_srv6_pop_header(struct dp_packet *packet) > } > > pkt_metadata_init_tnl(md); > - netdev_tnl_ip_extract_tnl_md(packet, tnl, &hlen); > + if (netdev_tnl_ip_extract_tnl_md(packet, tnl, &hlen) == NULL) { if (!...) > + goto err; > + } Maybe an empty line here. > dp_packet_reset_packet(packet, hlen); > > return packet;
On 5/17/24 22:14, Ilya Maximets wrote: > On 5/16/24 21:36, Mike Pattrick wrote: >> Clang's static analyzer will complain about an uninitialized value >> because we weren't properly checking the error code from a function that >> would have initialized the value. > > Please, be more specific. :) At least, mention the variable name. > > And the same comment for the subject of the patch. This one is > actually a real potential issue. And it also needs a Fixes tag, since it's not a false-positive. > So, something like this would > be an appropriate name: > > netdev-native-tnl: Fix use of uninitialized offset on SRv6 header pop. > >> >> Instead, add a check for that return code. >> >> Signed-off-by: Mike Pattrick <mkp@redhat.com> >> --- >> lib/netdev-native-tnl.c | 4 +++- >> 1 file changed, 3 insertions(+), 1 deletion(-) >> >> diff --git a/lib/netdev-native-tnl.c b/lib/netdev-native-tnl.c >> index dee9ab344..6e6b15764 100644 >> --- a/lib/netdev-native-tnl.c >> +++ b/lib/netdev-native-tnl.c >> @@ -1068,7 +1068,9 @@ netdev_srv6_pop_header(struct dp_packet *packet) >> } >> >> pkt_metadata_init_tnl(md); >> - netdev_tnl_ip_extract_tnl_md(packet, tnl, &hlen); >> + if (netdev_tnl_ip_extract_tnl_md(packet, tnl, &hlen) == NULL) { > > if (!...) > >> + goto err; >> + } > > Maybe an empty line here. > >> dp_packet_reset_packet(packet, hlen); >> >> return packet; >
diff --git a/lib/netdev-native-tnl.c b/lib/netdev-native-tnl.c index dee9ab344..6e6b15764 100644 --- a/lib/netdev-native-tnl.c +++ b/lib/netdev-native-tnl.c @@ -1068,7 +1068,9 @@ netdev_srv6_pop_header(struct dp_packet *packet) } pkt_metadata_init_tnl(md); - netdev_tnl_ip_extract_tnl_md(packet, tnl, &hlen); + if (netdev_tnl_ip_extract_tnl_md(packet, tnl, &hlen) == NULL) { + goto err; + } dp_packet_reset_packet(packet, hlen); return packet;
Clang's static analyzer will complain about an uninitialized value because we weren't properly checking the error code from a function that would have initialized the value. Instead, add a check for that return code. Signed-off-by: Mike Pattrick <mkp@redhat.com> --- lib/netdev-native-tnl.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)