agentx and clang static analyzer

classic Classic list List threaded Threaded
3 messages Options
Reply | Threaded
Open this post in threaded view
|

agentx and clang static analyzer

Martijn van Duren-5
There are 3 things that actually look like valid complaints when running
clang's static analyzer.

1) A dead store in agentx_recv.
2) sizeof(ipaddress) intead of sizeof(*ipaddress). Since this is ipv4,
   this is only a problem if sizeof(pointer) is smaller then 4 bytes,
   which can't happen afaik.
3) dst does indeed need to be dereffed, but since dstlen and buflen are
   initialized to 0 and srclen is practically always larger then 0
   we're fine. I'm keeping the *dst here as an additional safeguard.

The rest seem like false positives to me.

OK?

martijn@

Index: agentx.c
===================================================================
RCS file: /cvs/src/usr.sbin/relayd/agentx.c,v
retrieving revision 1.16
diff -u -p -r1.16 agentx.c
--- agentx.c 14 Sep 2020 11:30:25 -0000 1.16
+++ agentx.c 15 Sep 2020 09:05:57 -0000
@@ -135,7 +135,6 @@ agentx_recv(struct agentx *ax)
  header.aph_packetid = agentx_pdutoh32(&header, u8);
  u8 += 4;
  header.aph_plength = agentx_pdutoh32(&header, u8);
- u8 += 4;
 
  if (header.aph_version != 1) {
  errno = EPROTO;
Index: subagentx.c
===================================================================
RCS file: /cvs/src/usr.sbin/relayd/subagentx.c,v
retrieving revision 1.1
diff -u -p -r1.1 subagentx.c
--- subagentx.c 14 Sep 2020 11:30:25 -0000 1.1
+++ subagentx.c 15 Sep 2020 09:05:58 -0000
@@ -2929,7 +2929,7 @@ getnext:
  index->sav_idatacomplete = 1;
  break;
  case AGENTX_DATA_TYPE_IPADDRESS:
- ipaddress = calloc(1, sizeof(ipaddress));
+ ipaddress = calloc(1, sizeof(*ipaddress));
  if (ipaddress == NULL) {
  subagentx_log_sag_warn(sag,
     "Failed to bind ipaddress index");
@@ -3833,7 +3833,7 @@ subagentx_strcat(char **dst, const char
  }
 
  srclen = strlen(src);
- if (dst == NULL || dstlen + srclen > buflen) {
+ if (*dst == NULL || dstlen + srclen > buflen) {
  nbuflen = (((dstlen + srclen) / 512) + 1) * 512;
  tmp = recallocarray(*dst, buflen, nbuflen, sizeof(*tmp));
  if (tmp == NULL)

Reply | Threaded
Open this post in threaded view
|

Re: agentx and clang static analyzer

Bob Beck-2
On Tue, Sep 15, 2020 at 11:08:04AM +0200, Martijn van Duren wrote:

> There are 3 things that actually look like valid complaints when running
> clang's static analyzer.
>
> 1) A dead store in agentx_recv.
> 2) sizeof(ipaddress) intead of sizeof(*ipaddress). Since this is ipv4,
>    this is only a problem if sizeof(pointer) is smaller then 4 bytes,
>    which can't happen afaik.
> 3) dst does indeed need to be dereffed, but since dstlen and buflen are
>    initialized to 0 and srclen is practically always larger then 0
>    we're fine. I'm keeping the *dst here as an additional safeguard.
>
> The rest seem like false positives to me.
>
> OK?
>
> martijn@
>
> Index: agentx.c
> ===================================================================
> RCS file: /cvs/src/usr.sbin/relayd/agentx.c,v
> retrieving revision 1.16
> diff -u -p -r1.16 agentx.c
> --- agentx.c 14 Sep 2020 11:30:25 -0000 1.16
> +++ agentx.c 15 Sep 2020 09:05:57 -0000
> @@ -135,7 +135,6 @@ agentx_recv(struct agentx *ax)
>   header.aph_packetid = agentx_pdutoh32(&header, u8);
>   u8 += 4;
>   header.aph_plength = agentx_pdutoh32(&header, u8);
> - u8 += 4;
>  
>   if (header.aph_version != 1) {
>   errno = EPROTO;

ok for this piece above


> Index: subagentx.c
> ===================================================================
> RCS file: /cvs/src/usr.sbin/relayd/subagentx.c,v
> retrieving revision 1.1
> diff -u -p -r1.1 subagentx.c
> --- subagentx.c 14 Sep 2020 11:30:25 -0000 1.1
> +++ subagentx.c 15 Sep 2020 09:05:58 -0000
> @@ -2929,7 +2929,7 @@ getnext:
>   index->sav_idatacomplete = 1;
>   break;
>   case AGENTX_DATA_TYPE_IPADDRESS:
> - ipaddress = calloc(1, sizeof(ipaddress));
> + ipaddress = calloc(1, sizeof(*ipaddress));
>   if (ipaddress == NULL) {
>   subagentx_log_sag_warn(sag,
>      "Failed to bind ipaddress index");

Not ok for this, it's probably correct, but there are other instances of this
in this code and so you need to engage brain, not static analyzer, go fix or
don't fix them all in a separate commit.


> @@ -3833,7 +3833,7 @@ subagentx_strcat(char **dst, const char
>   }
>  
>   srclen = strlen(src);
> - if (dst == NULL || dstlen + srclen > buflen) {
> + if (*dst == NULL || dstlen + srclen > buflen) {
>   nbuflen = (((dstlen + srclen) / 512) + 1) * 512;
>   tmp = recallocarray(*dst, buflen, nbuflen, sizeof(*tmp));
>   if (tmp == NULL)
>
ok for this piece above

Reply | Threaded
Open this post in threaded view
|

Re: agentx and clang static analyzer

Martijn van Duren-5
> > Index: subagentx.c
> > ===================================================================
> > RCS file: /cvs/src/usr.sbin/relayd/subagentx.c,v
> > retrieving revision 1.1
> > diff -u -p -r1.1 subagentx.c
> > --- subagentx.c 14 Sep 2020 11:30:25 -0000 1.1
> > +++ subagentx.c 15 Sep 2020 09:05:58 -0000
> > @@ -2929,7 +2929,7 @@ getnext:
> >   index->sav_idatacomplete = 1;
> >   break;
> >   case AGENTX_DATA_TYPE_IPADDRESS:
> > - ipaddress = calloc(1, sizeof(ipaddress));
> > + ipaddress = calloc(1, sizeof(*ipaddress));
> >   if (ipaddress == NULL) {
> >   subagentx_log_sag_warn(sag,
> >      "Failed to bind ipaddress index");
>
> Not ok for this, it's probably correct, but there are other instances of this
> in this code and so you need to engage brain, not static analyzer, go fix or
> don't fix them all in a separate commit.
>
Could only find one more:

Index: subagentx.c
===================================================================
RCS file: /cvs/src/usr.sbin/relayd/subagentx.c,v
retrieving revision 1.1
diff -u -p -r1.1 subagentx.c
--- subagentx.c 14 Sep 2020 11:30:25 -0000 1.1
+++ subagentx.c 15 Sep 2020 09:27:46 -0000
@@ -2929,7 +2929,7 @@ getnext:
  index->sav_idatacomplete = 1;
  break;
  case AGENTX_DATA_TYPE_IPADDRESS:
- ipaddress = calloc(1, sizeof(ipaddress));
+ ipaddress = calloc(1, sizeof(*ipaddress));
  if (ipaddress == NULL) {
  subagentx_log_sag_warn(sag,
     "Failed to bind ipaddress index");
@@ -2951,7 +2951,7 @@ getnext:
  }
  if (j <= sav->sav_vb.avb_oid.aoi_idlen)
  index->sav_idatacomplete = 1;
- data->avb_ostring.aos_slen = sizeof(ipaddress);
+ data->avb_ostring.aos_slen = sizeof(*ipaddress);
  data->avb_ostring.aos_string =
     (unsigned char *)ipaddress;
  break;