Bug #3070: coverity warnings in protocol detection - Suricata - Open Information Security Foundation

Actions

Copy link

Bug #3070

closed

coverity warnings in protocol detection

Added by Victor Julien almost 5 years ago. Updated almost 5 years ago.

Status:

Closed

Priority:

Normal

Assignee:

Philippe Antoine

Target version:

5.0rc1

Affected Versions:

Effort:

Difficulty:

Label:

Description

** CID 1449368:  Resource leaks  (RESOURCE_LEAK)
/src/app-layer-detect-proto.c: 1483 in AppLayerProtoDetectPMRegisterPattern()

________________________________________________________________________________________________________
*** CID 1449368:  Resource leaks  (RESOURCE_LEAK)
/src/app-layer-detect-proto.c: 1483 in AppLayerProtoDetectPMRegisterPattern()
1477                 PPFunc, pp_min_depth, pp_max_depth);
1478     
1479         goto end;
1480      error:
1481         ret = -1;
1482      end:
>>>     CID 1449368:  Resource leaks  (RESOURCE_LEAK)
>>>     Variable "cd" going out of scope leaks the storage it points to.
1483         SCReturnInt(ret);
1484     }
1485     
1486     /***** Protocol Retrieval *****/
1487     
1488     AppProto AppLayerProtoDetectGetProto(AppLayerProtoDetectThreadCtx *tctx,

** CID 1449367:  Error handling issues  (CHECKED_RETURN)
/src/app-layer-ssl.c: 2856 in RegisterSSLParsers()

________________________________________________________________________________________________________
*** CID 1449367:  Error handling issues  (CHECKED_RETURN)
/src/app-layer-ssl.c: 2856 in RegisterSSLParsers()
2850                                               "443",
2851                                               ALPROTO_TLS,
2852                                               0, 3,
2853                                               STREAM_TOSERVER,
2854                                               SSLProbingParser, NULL);
2855             } else {
>>>     CID 1449367:  Error handling issues  (CHECKED_RETURN)
>>>     Calling "AppLayerProtoDetectPPParseConfPorts" without checking return value (as is done elsewhere 12 out of 13 times).
2856                 AppLayerProtoDetectPPParseConfPorts("tcp", IPPROTO_TCP,
2857                                                     proto_name, ALPROTO_TLS,
2858                                                     0, 3,
2859                                                     SSLProbingParser, NULL);
2860             }
2861         } else {

Actions

Copy link

Updated by Philippe Antoine almost 5 years ago

The first one is indeed a leak.

The second is a false positive to me.
But we can check the return value and log a warning in case of an error

Actions

Copy link

Updated by Victor Julien almost 5 years ago

I think the warning is not a false positive as it is factually correct. It may not be very interesting, but that doesn't make it a FP.

If you check the smb or template parsers, you will see not just a warning but also some other registration happening on failure of AppLayerProtoDetectPPParseConfPorts. I think it makes sense to investigate if we need to do something similar here.

Actions

Copy link

Updated by Philippe Antoine almost 5 years ago

Sorry for the wording.
Indeed, the warning is factually correct as we call AppLayerProtoDetectPPParseConfPorts without checking its return value (and usually check for it)

Thanks to your SMB pointer, I will propose another PR

Actions

Copy link