Static analysis scan results

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

Static analysis scan results

Jaroslav Škarvada
Hi,

I got to Coverity static analysis scan report for postfix. Most of the
errors were false positives, but two minor errors seems suspicious, that
it could cause leaks, so sharing them for review. Unfortunately, it's
result for postfix-3.2.5, but the same code is in the latest postfix -
just the lines from the report have slight offset, sorry about that, but
hopefully it will be also useful.

Error: RESOURCE_LEAK (CWE-772): [#def16]
postfix-3.2.5/src/posttls-finger/posttls-finger.c:1412: alloc_fn: Storage is returned from allocation function "connect_unix".
postfix-3.2.5/src/posttls-finger/posttls-finger.c:941:5: alloc_fn: Storage is returned from allocation function "connect_sock".
postfix-3.2.5/src/posttls-finger/posttls-finger.c:870:5: alloc_fn: Storage is returned from allocation function "vstream_fdopen".
postfix-3.2.5/src/util/vstream.c:1214:5: alloc_fn: Storage is returned from allocation function "mymalloc".
postfix-3.2.5/src/util/mymalloc.c:160:5: alloc_fn: Storage is returned from allocation function "malloc".
postfix-3.2.5/src/util/mymalloc.c:160:5: var_assign: Assigning: "real_ptr" = "malloc(16UL + len)".
postfix-3.2.5/src/util/mymalloc.c:163:5: var_assign: Assigning: "ptr" = "real_ptr".
postfix-3.2.5/src/util/mymalloc.c:164:5: noescape: Resource "ptr" is not freed or pointed-to in function "memset". [Note: The source code implementation of the function has been overridden by a builtin model.]
postfix-3.2.5/src/util/mymalloc.c:165:5: return_alloc: Returning allocated memory "ptr".
postfix-3.2.5/src/util/vstream.c:1214:5: var_assign: Assigning: "stream" = "mymalloc(320L)".
postfix-3.2.5/src/util/vstream.c:1229:5: return_alloc: Returning allocated memory "stream".
postfix-3.2.5/src/posttls-finger/posttls-finger.c:870:5: var_assign: Assigning: "stream" = "vstream_fdopen(sock, 2)".
postfix-3.2.5/src/posttls-finger/posttls-finger.c:890:2: noescape: Resource "stream" is not freed or pointed-to in function "vstream_tweak_tcp".
postfix-3.2.5/src/util/vstream_tweak.c:86:36: noescape: "vstream_tweak_tcp(VSTREAM *)" does not free or save its parameter "fp".
postfix-3.2.5/src/posttls-finger/posttls-finger.c:892:5: return_alloc: Returning allocated memory "stream".
postfix-3.2.5/src/posttls-finger/posttls-finger.c:941:5: return_alloc_fn: Directly returning storage allocated by "connect_sock".
postfix-3.2.5/src/posttls-finger/posttls-finger.c:1412: leaked_storage: Ignoring storage allocated by "connect_unix(state, dest + 5)" leaks it.
# 1410|       if (state->smtp == 0) {
# 1411|   if (strncmp(dest, "unix:", 5) == 0) {
# 1412|->    connect_unix(state, dest + 5);
# 1413|      if (!state->stream)
# 1414|   msg_info("Failed to establish session to %s: %s",

Error: RESOURCE_LEAK (CWE-772): [#def18]
postfix-3.2.5/src/util/dict_random.c:108: alloc_fn: Storage is returned from allocation function "argv_splitq".
postfix-3.2.5/src/util/argv_splitq.c:69:19: alloc_fn: Storage is returned from allocation function "argv_alloc".
postfix-3.2.5/src/util/argv.c:157:5: alloc_fn: Storage is returned from allocation function "mymalloc".
postfix-3.2.5/src/util/mymalloc.c:160:5: alloc_fn: Storage is returned from allocation function "malloc".
postfix-3.2.5/src/util/mymalloc.c:160:5: var_assign: Assigning: "real_ptr" = "malloc(16UL + len)".
postfix-3.2.5/src/util/mymalloc.c:163:5: var_assign: Assigning: "ptr" = "real_ptr".
postfix-3.2.5/src/util/mymalloc.c:164:5: noescape: Resource "ptr" is not freed or pointed-to in function "memset". [Note: The source code implementation of the function has been overridden by a builtin model.]
postfix-3.2.5/src/util/mymalloc.c:165:5: return_alloc: Returning allocated memory "ptr".
postfix-3.2.5/src/util/argv.c:157:5: var_assign: Assigning: "argvp" = "mymalloc(24L)".
postfix-3.2.5/src/util/argv.c:164:5: return_alloc: Returning allocated memory "argvp".
postfix-3.2.5/src/util/argv_splitq.c:69:19: var_assign: Assigning: "argvp" = "argv_alloc(1L)".
postfix-3.2.5/src/util/argv_splitq.c:76:5: noescape: Resource "argvp" is not freed or pointed-to in function "argv_terminate".
postfix-3.2.5/src/util/argv.c:242:30: noescape: "argv_terminate(ARGV *)" does not free or save its parameter "argvp".
postfix-3.2.5/src/util/argv_splitq.c:78:5: return_alloc: Returning allocated memory "argvp".
postfix-3.2.5/src/util/dict_random.c:108: var_assign: Assigning: "argv" = storage returned from "argv_splitq(saved_name, ", \t\r\n", "{}")".
postfix-3.2.5/src/util/dict_random.c:112: leaked_storage: Variable "argv" going out of scope leaks the storage it points to.
#  110|   || ((argv = argv_splitq(saved_name, CHARS_COMMA_SP, CHARS_BRACE)),
#  111|      (argv->argc == 0)))
#  112|-> DICT_RANDOM_RETURN(dict_surrogate(DICT_TYPE_RANDOM, name,
#  113|    open_flags, dict_flags,
#  114|    "bad syntax: \"%s:%s\"; "

thanks & regards

Jaroslav
Reply | Threaded
Open this post in threaded view
|

Re: Static analysis scan results

Viktor Dukhovni
On Sun, Dec 02, 2018 at 02:48:47PM -0500, Jaroslav Skarvada wrote:

> src/posttls-finger/posttls-finger.c:1412:
> # 1410|       if (state->smtp == 0) {
> # 1411|   if (strncmp(dest, "unix:", 5) == 0) {
> # 1412|->    connect_unix(state, dest + 5);
> # 1413|      if (!state->stream)
> # 1414|   msg_info("Failed to establish session to %s: %s",

That's not so much a memory leak as a bug in the little used support
for unix-domain sockets.  Connections to unix-domain sockets will always
appear to fail.  The correct code is:

--- src/posttls-finger/posttls-finger.c
+++ src/posttls-finger/posttls-finger.c
@@ -1547,7 +1547,7 @@ static int connect_dest(STATE *state)
      */
     if (state->smtp == 0) {
  if (strncmp(dest, "unix:", 5) == 0) {
-    connect_unix(state, dest + 5);
+    state->stream = connect_unix(state, dest + 5);
     if (!state->stream)
  msg_info("Failed to establish session to %s: %s",
  dest, vstring_str(state->why->reason));

> src/util/dict_random.c:112: leaked_storage: Variable "argv" ...
> #  110|   || ((argv = argv_splitq(saved_name, CHARS_COMMA_SP, CHARS_BRACE)),
> #  111|      (argv->argc == 0)))
> #  112|-> DICT_RANDOM_RETURN(dict_surrogate(DICT_TYPE_RANDOM, name,
> #  113|    open_flags, dict_flags,
> #  114|    "bad syntax: \"%s:%s\"; "

This code was refactored in postfix-3.4-20181105, and the "leak"
is gone.  In earlier versions this "leak" only happens during process
initialization (i.e. just once per process, so not important) and
only for a malformed "random" table definition with no values.

I don't think this warrants any changes for the earlier releases.
 
--
        Viktor.
Reply | Threaded
Open this post in threaded view
|

Re: Static analysis scan results

Wietse Venema
In reply to this post by Jaroslav Škarvada
Jaroslav Skarvada:
> postfix-3.2.5/src/util/dict_random.c:112: leaked_storage: Variable "argv" going out of scope leaks the storage it points to.
> #  110|   || ((argv = argv_splitq(saved_name, CHARS_COMMA_SP, CHARS_BRACE)),
> #  111|      (argv->argc == 0)))
> #  112|-> DICT_RANDOM_RETURN(dict_surrogate(DICT_TYPE_RANDOM, name,
> #  113|    open_flags, dict_flags,
> #  114|    "bad syntax: \"%s:%s\"; "

Error during error recovery during process initialization. This is
not important enough to change code for. We do memory leak checks
for many code paths, but leaks during program initialization errors
are not covered well.

This was fixed in Postfix 3.4 as I rewrote this module to support
(key, filename) support.

Thanks for not posting the false positives.

        Wietse
Reply | Threaded
Open this post in threaded view
|

Re: Static analysis scan results

Viktor Dukhovni
On Sun, Dec 02, 2018 at 05:23:31PM -0500, Wietse Venema wrote:

> Thanks for not posting the false positives.

Yes, very much appreciated, I should have said the same.  It is a
rare pleasure to see a report where the required effort was put in
to eliminate noisy distractions.  Thanks again.

--
        Viktor.