Coverity & clang scan

Previous Topic Next Topic
 
classic Classic list List threaded Threaded
5 messages Options
Reply | Threaded
Open this post in threaded view
|

Coverity & clang scan

Jaroslav Škarvada
Hi,

we run coverity & clang scans time to time. I filtered all the
false positives and the following are results which seemed
suspicious to me. It's mostly for unlikely code paths which
could be triggered by error state or invalid input. I am not
familiar with the code internals, so maybe it's unlikely or
impossible such conditions would ever trigger, but I am providing
the results for investigation. The scan was run over 3.5.8,
but I verified it's also valid for 3.5.9.


11. postfix-3.5.8/src/util/dict_inline.c:116: alloc_fn: Storage is returned from allocation function "extpar".
12. postfix-3.5.8/src/util/dict_inline.c:116: var_assign: Assigning: "free_me" = storage returned from "extpar(&nameval, "{}", 1)".
13. postfix-3.5.8/src/util/dict_inline.c:116: var_assign: Assigning: "err" = "free_me".
17. postfix-3.5.8/src/util/dict_inline.c:125: overwrite_var: Overwriting "err" in "err = free_me = dict_file_get_error(dict)" leaks the storage that "err" points to.
#   123|  
#   124|      if ((base64_buf = dict_file_to_b64(dict, value)) == 0) {
#   125|-> err = free_me = dict_file_get_error(dict);
#   126|   break;
#   127|      }

I think it could miss at least one free() for respective free_me.


14. postfix-3.5.8/src/util/dict_inline.c:124: uninit_use_in_call: Using uninitialized value "value" when calling "dict_file_to_b64".
#   122|      VSTRING *base64_buf;
#   123|  
#   124|->    if ((base64_buf = dict_file_to_b64(dict, value)) == 0) {
#   125|   err = free_me = dict_file_get_error(dict);
#   126|   break;

I think it could call dict_file_to_b64 with uninitialized value.


5. Defect type: NULL_RETURNS
6. postfix-3.5.8/src/global/haproxy_srvr.c:456: returned_null: "mystrtok" returns "NULL" (checked 62 out of 76 times).
7. postfix-3.5.8/src/global/haproxy_srvr.c:456: dereference: Dereferencing a pointer that might be "NULL" "mystrtok(&cp, " \r")" when calling "haproxy_srvr_parse_proto".
8. postfix-3.5.8/src/global/dict_ldap.c:1692: example_checked: Example 1: "mystrtok(&s, ", \t\r\n")" has its value checked in "(h = mystrtok(&s, ", \t\r\n")) != NULL".
9. postfix-3.5.8/src/global/mail_conf.c:158: example_checked: Example 2: "mystrtok(&value, ", \t\r\n")" has its value checked in "(cp = mystrtok(&value, ", \t\r\n")) != NULL".
10. postfix-3.5.8/src/global/mail_version.c:147: example_checked: Example 3: "mystrtok(&cp, "")" has its value checked in "(mp->snapshot = mystrtok(&cp, "")) == NULL".
11. postfix-3.5.8/src/global/map_search.c:211: example_checked: Example 4: "mystrtok(&attr_value, ", \t\r\n")" has its value checked in "(atom = mystrtok(&attr_value, ", \t\r\n")) != NULL".
12. postfix-3.5.8/src/global/match_service.c:107: example_checked: Example 5: "mystrtok(&bp, delim)" has its value checked in "(item = mystrtok(&bp, delim)) != NULL".
#   454|   else if (haproxy_srvr_parse_lit(NEXT_TOKEN, "PROXY", (char *) 0) < 0)
#   455|      err = "unexpected protocol header";
#   456|-> else if (haproxy_srvr_parse_proto(NEXT_TOKEN, &addr_family) < 0)
#   457|      err = "unsupported protocol type";
#   458|   else if (haproxy_srvr_parse_addr(NEXT_TOKEN, smtp_client_addr,

I think for malformed input it could fail by calling strncasecmp with NULL.


1. postfix-3.5.8/src/tlsproxy/tlsproxy.c:1881:49: warning[-Wmissing-braces]: missing braces around initializer
#      static const CONFIG_STR_TABLE str_table[] = {
#                                                  ^
#  1879|   0,
#  1880|       };
#  1881|->     static const CONFIG_STR_TABLE str_table[] = {
#  1882|   VAR_TLSP_TLS_CHAIN_FILES, DEF_TLSP_TLS_CHAIN_FILES, &var_tlsp_tls_chain_files, 0, 0,
#  1883|   VAR_TLSP_TLS_CERT_FILE, DEF_TLSP_TLS_CERT_FILE, &var_tlsp_tls_cert_file, 0, 0,

I think it should have braces around table lines. There are multiple tables with the same warning.


2. postfix-3.5.8/src/smtp/smtp_session.c:200: var_compare_op: Comparing "session->stream" to null implies that "session->stream" might be null.
5. postfix-3.5.8/src/smtp/smtp_session.c:208: var_deref_model: Passing null pointer "session->stream" to "tls_session_stop", which dereferences it.
#   206|      tls_proxy_context_free(session->tls_context);
#   207|   else
#   208|->    tls_client_stop(smtp_tls_ctx, session->stream,
#   209|    var_smtp_starttls_tmout, 0, session->tls_context);
#   210|       }

It's suspicious that there is the NULL check, but later it could fail on NULL dereference.


7. postfix-3.5.8/src/smtpd/smtpd.c:1712: var_compare_op: Comparing "state->milters" to null implies that "state->milters" might be null.
9. postfix-3.5.8/src/smtpd/smtpd.c:1727: var_deref_model: Passing "state" to "mail_reset", which dereferences null "state->milters".
#  1725|   helo_reset(state);
#  1726|       chat_reset(state, var_smtpd_hist_thrsh);
#  1727|->     mail_reset(state);
#  1728|       rcpt_reset(state);
#  1729|       state->helo_name = mystrdup(printable(argv[1].strval, '?'));

It's suspicious that there is the NULL check, but later it could fail on NULL dereference.



The following are few suspicious clang warnings with the codepaths which could
trigger them, I haven't done deeper analysis of them:


1. postfix-3.5.8/src/util/dict_inline.c:131:2: warning[core.CallAndMessage]: 2nd function call argument is an uninitialized value
#         dict->update(dict, vname, value);
#         ^                  ~~~~~
4. postfix-3.5.8/src/util/dict_inline.c:59:23: note: 'vname' declared without an initial value
#     char   *nameval, *vname, *value;
#                       ^~~~~
7. postfix-3.5.8/src/util/dict_inline.c:79:9: note: Assuming 'open_flags' is equal to O_RDONLY
#     if (open_flags != O_RDONLY)
#         ^~~~~~~~~~~~~~~~~~~~~~
10. postfix-3.5.8/src/util/dict_inline.c:79:5: note: Taking false branch
#     if (open_flags != O_RDONLY)
#     ^
13. postfix-3.5.8/src/util/dict_inline.c:88:9: note: Assuming 'util_utf8_enable' is 0
#     if (DICT_NEED_UTF8_ACTIVATION(util_utf8_enable, dict_flags)
#         ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
16. postfix-3.5.8/src/util/dict.h:176:3: note: expanded from macro 'DICT_NEED_UTF8_ACTIVATION'
#         ((enable) && ((flags) & DICT_FLAG_UTF8_MASK))
#          ^~~~~~~~
19. postfix-3.5.8/src/util/dict_inline.c:88:9: note: Left side of '&&' is false
20. postfix-3.5.8/src/util/dict.h:176:12: note: expanded from macro 'DICT_NEED_UTF8_ACTIVATION'
#         ((enable) && ((flags) & DICT_FLAG_UTF8_MASK))
#                   ^
23. postfix-3.5.8/src/util/dict_inline.c:101:9: note: Assuming the condition is false
#     if ((len = balpar(name, CHARS_BRACE)) == 0 || name[len] != 0
#         ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
26. postfix-3.5.8/src/util/dict_inline.c:101:9: note: Left side of '||' is false
27. postfix-3.5.8/src/util/dict_inline.c:101:51: note: Assuming the condition is false
#     if ((len = balpar(name, CHARS_BRACE)) == 0 || name[len] != 0
#                                                   ^~~~~~~~~~~~~~
30. postfix-3.5.8/src/util/dict_inline.c:101:9: note: Left side of '||' is false
#     if ((len = balpar(name, CHARS_BRACE)) == 0 || name[len] != 0
#         ^
33. postfix-3.5.8/src/util/dict_inline.c:102:5: note: Assuming the condition is false
#         || *(cp = saved_name = mystrndup(name + 1, len - 2)) == 0)
#            ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
36. postfix-3.5.8/src/util/dict_inline.c:101:5: note: Taking false branch
#     if ((len = balpar(name, CHARS_BRACE)) == 0 || name[len] != 0
#     ^
39. postfix-3.5.8/src/util/dict_inline.c:115:12: note: Assuming the condition is true
#     while ((nameval = mystrtokq(&cp, CHARS_COMMA_SP, CHARS_BRACE)) != 0) {
#            ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
42. postfix-3.5.8/src/util/dict_inline.c:115:5: note: Loop condition is true. Entering loop body
#     while ((nameval = mystrtokq(&cp, CHARS_COMMA_SP, CHARS_BRACE)) != 0) {
#     ^
45. postfix-3.5.8/src/util/dict_inline.c:116:7: note: Assuming the condition is false
#         if ((nameval[0] != CHARS_BRACE[0]
#              ^~~~~~~~~~~~~~~~~~~~~~~~~~~~
48. postfix-3.5.8/src/util/dict_inline.c:116:7: note: Left side of '||' is false
49. postfix-3.5.8/src/util/dict_inline.c:117:10: note: Assuming the condition is false
#              || (err = free_me = extpar(&nameval, CHARS_BRACE, EXTPAR_FLAG_STRIP)) == 0)
#                 ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
52. postfix-3.5.8/src/util/dict_inline.c:118:6: note: Left side of '&&' is false
#             && (err = split_qnameval(nameval, &vname, &value)) != 0)
#             ^
55. postfix-3.5.8/src/util/dict_inline.c:121:6: note: Assuming the condition is false
#         if ((dict->flags & DICT_FLAG_SRC_RHS_IS_FILE) != 0) {
#             ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
58. postfix-3.5.8/src/util/dict_inline.c:121:2: note: Taking false branch
#         if ((dict->flags & DICT_FLAG_SRC_RHS_IS_FILE) != 0) {
#         ^
61. postfix-3.5.8/src/util/dict_inline.c:131:2: note: 2nd function call argument is an uninitialized value
#         dict->update(dict, vname, value);
#         ^                  ~~~~~
#   129|   }
#   130|   /* No duplicate checks. See comments in dict_thash.c. */
#   131|-> dict->update(dict, vname, value);
#   132|   count += 1;
#   133|       }



1. postfix-3.5.8/src/tls/tls_proxy_client_scan.c:335:9: warning[core.NullDereference]: Access to field 'next' results in a dereference of a null pointer (loaded from variable 'tp')
#     if (tp->next)
#         ^
4. postfix-3.5.8/src/tls/tls_proxy_client_scan.c:509:5: note: 'head' initialized to a null pointer value
#     TLS_TLSA *head = 0;
#     ^~~~~~~~~~~~~~
7. postfix-3.5.8/src/tls/tls_proxy_client_scan.c:516:9: note: Assuming 'msg_verbose' is 0
#     if (msg_verbose)
#         ^~~~~~~~~~~
10. postfix-3.5.8/src/tls/tls_proxy_client_scan.c:516:5: note: Taking false branch
#     if (msg_verbose)
#     ^
13. postfix-3.5.8/src/tls/tls_proxy_client_scan.c:519:30: note: Assuming 'ret' is not equal to 1
#     for (tpp = &head, n = 0; ret == 1 && n < count; n++, tpp = &tp->next) {
#                              ^~~~~~~~
16. postfix-3.5.8/src/tls/tls_proxy_client_scan.c:519:39: note: Left side of '&&' is false
#     for (tpp = &head, n = 0; ret == 1 && n < count; n++, tpp = &tp->next) {
#                                       ^
19. postfix-3.5.8/src/tls/tls_proxy_client_scan.c:540:9: note: 'ret' is not equal to 1
#     if (ret != 1) {
#         ^~~
22. postfix-3.5.8/src/tls/tls_proxy_client_scan.c:540:5: note: Taking true branch
#     if (ret != 1) {
#     ^
25. postfix-3.5.8/src/tls/tls_proxy_client_scan.c:541:29: note: Passing null pointer value via 1st parameter 'tp'
#         tls_proxy_client_tlsa_free(head);
#                                    ^~~~
28. postfix-3.5.8/src/tls/tls_proxy_client_scan.c:541:2: note: Calling 'tls_proxy_client_tlsa_free'
#         tls_proxy_client_tlsa_free(head);
#         ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
31. postfix-3.5.8/src/tls/tls_proxy_client_scan.c:335:9: note: Access to field 'next' results in a dereference of a null pointer (loaded from variable 'tp')
#     if (tp->next)
#         ^~
#   333|   static void tls_proxy_client_tlsa_free(TLS_TLSA *tp)
#   334|   {
#   335|->     if (tp->next)
#   336|   tls_proxy_client_tlsa_free(tp->next);
#   337|       myfree(tp->mdalg);



1. postfix-3.5.8/src/tls/tls_proxy_client_scan.c:324:9: warning[core.NullDereference]: Access to field 'next' results in a dereference of a null pointer (loaded from variable 'tp')
#     if (tp->next)
#         ^
4. postfix-3.5.8/src/tls/tls_proxy_client_scan.c:451:5: note: 'head' initialized to a null pointer value
#     TLS_PKEYS *head = 0;
#     ^~~~~~~~~~~~~~~
7. postfix-3.5.8/src/tls/tls_proxy_client_scan.c:458:9: note: Assuming 'msg_verbose' is 0
#     if (msg_verbose)
#         ^~~~~~~~~~~
10. postfix-3.5.8/src/tls/tls_proxy_client_scan.c:458:5: note: Taking false branch
#     if (msg_verbose)
#     ^
13. postfix-3.5.8/src/tls/tls_proxy_client_scan.c:461:30: note: Assuming 'ret' is not equal to 1
#     for (tpp = &head, n = 0; ret == 1 && n < count; n++, tpp = &tp->next) {
#                              ^~~~~~~~
16. postfix-3.5.8/src/tls/tls_proxy_client_scan.c:461:39: note: Left side of '&&' is false
#     for (tpp = &head, n = 0; ret == 1 && n < count; n++, tpp = &tp->next) {
#                                       ^
19. postfix-3.5.8/src/tls/tls_proxy_client_scan.c:489:9: note: Assuming 'buf' is null
#     if (buf)
#         ^~~
22. postfix-3.5.8/src/tls/tls_proxy_client_scan.c:489:5: note: Taking false branch
#     if (buf)
#     ^
25. postfix-3.5.8/src/tls/tls_proxy_client_scan.c:491:9: note: 'ret' is not equal to 1
#     if (ret != 1) {
#         ^~~
28. postfix-3.5.8/src/tls/tls_proxy_client_scan.c:491:5: note: Taking true branch
#     if (ret != 1) {
#     ^
31. postfix-3.5.8/src/tls/tls_proxy_client_scan.c:492:30: note: Passing null pointer value via 1st parameter 'tp'
#         tls_proxy_client_pkeys_free(head);
#                                     ^~~~
34. postfix-3.5.8/src/tls/tls_proxy_client_scan.c:492:2: note: Calling 'tls_proxy_client_pkeys_free'
#         tls_proxy_client_pkeys_free(head);
#         ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
37. postfix-3.5.8/src/tls/tls_proxy_client_scan.c:324:9: note: Access to field 'next' results in a dereference of a null pointer (loaded from variable 'tp')
#     if (tp->next)
#         ^~
#   322|   static void tls_proxy_client_pkeys_free(TLS_PKEYS *tp)
#   323|   {
#   324|->     if (tp->next)
#   325|   tls_proxy_client_pkeys_free(tp->next);
#   326|       if (tp->pkey)



1. postfix-3.5.8/src/tls/tls_proxy_client_scan.c:313:9: warning[core.NullDereference]: Access to field 'next' results in a dereference of a null pointer (loaded from variable 'tp')
#     if (tp->next)
#         ^
4. postfix-3.5.8/src/tls/tls_proxy_client_scan.c:392:5: note: 'head' initialized to a null pointer value
#     TLS_CERTS *head = 0;
#     ^~~~~~~~~~~~~~~
7. postfix-3.5.8/src/tls/tls_proxy_client_scan.c:399:9: note: Assuming 'msg_verbose' is 0
#     if (msg_verbose)
#         ^~~~~~~~~~~
10. postfix-3.5.8/src/tls/tls_proxy_client_scan.c:399:5: note: Taking false branch
#     if (msg_verbose)
#     ^
13. postfix-3.5.8/src/tls/tls_proxy_client_scan.c:402:30: note: Assuming 'ret' is not equal to 1
#     for (tpp = &head, n = 0; ret == 1 && n < count; n++, tpp = &tp->next) {
#                              ^~~~~~~~
16. postfix-3.5.8/src/tls/tls_proxy_client_scan.c:402:39: note: Left side of '&&' is false
#     for (tpp = &head, n = 0; ret == 1 && n < count; n++, tpp = &tp->next) {
#                                       ^
19. postfix-3.5.8/src/tls/tls_proxy_client_scan.c:430:9: note: 'buf' is null
#     if (buf)
#         ^~~
22. postfix-3.5.8/src/tls/tls_proxy_client_scan.c:430:5: note: Taking false branch
#     if (buf)
#     ^
25. postfix-3.5.8/src/tls/tls_proxy_client_scan.c:432:9: note: 'ret' is not equal to 1
#     if (ret != 1) {
#         ^~~
28. postfix-3.5.8/src/tls/tls_proxy_client_scan.c:432:5: note: Taking true branch
#     if (ret != 1) {
#     ^
31. postfix-3.5.8/src/tls/tls_proxy_client_scan.c:433:30: note: Passing null pointer value via 1st parameter 'tp'
#         tls_proxy_client_certs_free(head);
#                                     ^~~~
34. postfix-3.5.8/src/tls/tls_proxy_client_scan.c:433:2: note: Calling 'tls_proxy_client_certs_free'
#         tls_proxy_client_certs_free(head);
#         ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
37. postfix-3.5.8/src/tls/tls_proxy_client_scan.c:313:9: note: Access to field 'next' results in a dereference of a null pointer (loaded from variable 'tp')
#     if (tp->next)
#         ^~
#   311|   static void tls_proxy_client_certs_free(TLS_CERTS *tp)
#   312|   {
#   313|->     if (tp->next)
#   314|   tls_proxy_client_certs_free(tp->next);
#   315|       if (tp->cert)



1. postfix-3.5.8/src/global/maillog_client.c:256:10: warning[core.NonNullParamChecker]: Null pointer passed to 2nd parameter expecting 'nonnull'
#             if (setenv(POSTLOG_SERVICE_ENV, service_path, 1) < 0)
#                 ^                           ~~~~~~~~~~~~
4. postfix-3.5.8/src/global/maillog_client.c:170:10: note: Value assigned to 'import_service_path'
#     if ((import_service_path = safe_getenv(POSTLOG_SERVICE_ENV)) != 0
#          ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
7. postfix-3.5.8/src/global/maillog_client.c:170:9: note: Assuming pointer value is null
#     if ((import_service_path = safe_getenv(POSTLOG_SERVICE_ENV)) != 0
#         ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
10. postfix-3.5.8/src/global/maillog_client.c:171:2: note: Left side of '&&' is false
#         && *import_service_path == 0)
#         ^
13. postfix-3.5.8/src/global/maillog_client.c:173:9: note: Assuming the condition is false
#     if ((import_hostname = safe_getenv(POSTLOG_HOSTNAME_ENV)) != 0
#         ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
16. postfix-3.5.8/src/global/maillog_client.c:174:2: note: Left side of '&&' is false
#         && *import_hostname == 0)
#         ^
19. postfix-3.5.8/src/global/maillog_client.c:193:9: note: Assuming 'var_maillog_file' is non-null
#     if (var_maillog_file ? *var_maillog_file == 0 : import_service_path == 0) {
#         ^~~~~~~~~~~~~~~~
22. postfix-3.5.8/src/global/maillog_client.c:193:9: note: '?' condition is true
23. postfix-3.5.8/src/global/maillog_client.c:193:28: note: Assuming the condition is false
#     if (var_maillog_file ? *var_maillog_file == 0 : import_service_path == 0) {
#                            ^~~~~~~~~~~~~~~~~~~~~~
26. postfix-3.5.8/src/global/maillog_client.c:193:5: note: Taking false branch
#     if (var_maillog_file ? *var_maillog_file == 0 : import_service_path == 0) {
#     ^
29. postfix-3.5.8/src/global/maillog_client.c:207:9: note: 'logger_mode' is equal to MAILLOG_CLIENT_MODE_POSTLOG
#     if (logger_mode == MAILLOG_CLIENT_MODE_POSTLOG) {
#         ^~~~~~~~~~~
32. postfix-3.5.8/src/global/maillog_client.c:207:5: note: Taking true branch
#     if (logger_mode == MAILLOG_CLIENT_MODE_POSTLOG) {
#     ^
35. postfix-3.5.8/src/global/maillog_client.c:211:6: note: 'var_maillog_file' is non-null
#         if (var_maillog_file && *var_maillog_file) {
#             ^~~~~~~~~~~~~~~~
38. postfix-3.5.8/src/global/maillog_client.c:211:6: note: Left side of '&&' is true
39. postfix-3.5.8/src/global/maillog_client.c:211:2: note: Taking true branch
#         if (var_maillog_file && *var_maillog_file) {
#         ^
42. postfix-3.5.8/src/global/maillog_client.c:216:6: note: Loop condition is true. Entering loop body
#             for (cpp = good_prefixes->argv; /* see below */ ; cpp++) {
#             ^
45. postfix-3.5.8/src/global/maillog_client.c:217:7: note: Assuming the condition is false
#                 if (*cpp == 0)
#                     ^~~~~~~~~
48. postfix-3.5.8/src/global/maillog_client.c:217:3: note: Taking false branch
#                 if (*cpp == 0)
#                 ^
51. postfix-3.5.8/src/global/maillog_client.c:221:3: note: Taking true branch
#                 if (strncmp(var_maillog_file, *cpp, strlen(*cpp)) == 0)
#                 ^
54. postfix-3.5.8/src/global/maillog_client.c:222:7: note: Execution continues on line 224
#                     break;
#                     ^
57. postfix-3.5.8/src/global/maillog_client.c:226:6: note: Assuming 'var_myhostname' is non-null
#         if (var_myhostname && *var_myhostname) {
#             ^~~~~~~~~~~~~~
60. postfix-3.5.8/src/global/maillog_client.c:226:6: note: Left side of '&&' is true
61. postfix-3.5.8/src/global/maillog_client.c:226:24: note: Assuming the condition is true
#         if (var_myhostname && *var_myhostname) {
#                               ^~~~~~~~~~~~~~~
64. postfix-3.5.8/src/global/maillog_client.c:226:2: note: Taking true branch
#         if (var_myhostname && *var_myhostname) {
#         ^
67. postfix-3.5.8/src/global/maillog_client.c:234:6: note: Assuming 'var_postlog_service' is null
#         if (var_postlog_service) {
#             ^~~~~~~~~~~~~~~~~~~
70. postfix-3.5.8/src/global/maillog_client.c:234:2: note: Taking false branch
#         if (var_postlog_service) {
#         ^
73. postfix-3.5.8/src/global/maillog_client.c:238:6: note: Null pointer value stored to 'service_path'
#             service_path = import_service_path;
#             ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
76. postfix-3.5.8/src/global/maillog_client.c:242:5: note: Assuming the condition is false
#                         (flags & MAILLOG_CLIENT_FLAG_LOGWRITER_FALLBACK) ?
#                          ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
79. postfix-3.5.8/src/global/maillog_client.c:242:4: note: '?' condition is false
#                         (flags & MAILLOG_CLIENT_FLAG_LOGWRITER_FALLBACK) ?
#                         ^
82. postfix-3.5.8/src/global/maillog_client.c:251:6: note: 'import_service_path' is equal to null
#         if (import_service_path == 0
#             ^~~~~~~~~~~~~~~~~~~
85. postfix-3.5.8/src/global/maillog_client.c:252:6: note: Left side of '||' is true
#             || strcmp(service_path, import_service_path) != 0) {
#             ^
88. postfix-3.5.8/src/global/maillog_client.c:256:10: note: Null pointer passed to 2nd parameter expecting 'nonnull'
#             if (setenv(POSTLOG_SERVICE_ENV, service_path, 1) < 0)
#                 ^                           ~~~~~~~~~~~~
#   254|      msg_info("export %s=%s", POSTLOG_SERVICE_ENV, service_path);
#   255|   #endif
#   256|->    if (setenv(POSTLOG_SERVICE_ENV, service_path, 1) < 0)
#   257|   msg_fatal("setenv: %m");
#   258|   }



1. postfix-3.5.8/src/global/mail_task.c:74:13: warning[core.NullDereference]: Dereference of null pointer
#     return (vstring_str(canon_name));
#             ^~~~~~~~~~~~~~~~~~~~~~~
4. postfix-3.5.8/include/vstring.h:70:36: note: expanded from macro 'vstring_str'
# #define vstring_str(vp)         ((char *) (vp)->vbuf.data)
#                                           ^~~~~~~~~~~~~~~
7. postfix-3.5.8/src/global/mail_task.c:58:5: note: 'canon_name' initialized to a null pointer value
#     static VSTRING *canon_name;
#     ^~~~~~~~~~~~~~~~~~~~~~~~~~
10. postfix-3.5.8/src/global/mail_task.c:62:9: note: Assuming 'argv0' is null
#     if (argv0) {
#         ^~~~~
13. postfix-3.5.8/src/global/mail_task.c:62:5: note: Taking false branch
#     if (argv0) {
#     ^
16. postfix-3.5.8/src/global/mail_task.c:74:13: note: Dereference of null pointer
#     return (vstring_str(canon_name));
#             ^~~~~~~~~~~~~~~~~~~~~~~
19. postfix-3.5.8/include/vstring.h:70:36: note: expanded from macro 'vstring_str'
# #define vstring_str(vp)         ((char *) (vp)->vbuf.data)
#                                           ^~~~~~~~~~~~~~~
#    72|   vstring_sprintf(canon_name, "%s/%s", tag, argv0);
#    73|       }
#    74|->     return (vstring_str(canon_name));
#    75|   }


thanks & regards

Jaroslav

Reply | Threaded
Open this post in threaded view
|

Re: Coverity & clang scan

Viktor Dukhovni
On Fri, Mar 19, 2021 at 11:18:27AM -0400, Jaroslav Skarvada wrote:

> 14. postfix-3.5.8/src/util/dict_inline.c:124: uninit_use_in_call: Using uninitialized value "value" when calling "dict_file_to_b64".
> 17. postfix-3.5.8/src/util/dict_inline.c:125: overwrite_var: Overwriting "err" in "err = free_me = dict_file_get_error(dict)" leaks the storage that "err" points to.
> #   123|  
> #   124|->    if ((base64_buf = dict_file_to_b64(dict, value)) == 0) {
> #   125|-> err = free_me = dict_file_get_error(dict);
> #   126|   break;
> #   127|      }
>
> I think it could call dict_file_to_b64 with uninitialized value.

Yes, when inline tables in the main.cf file are malformed in a
particular way, this may not be handled correctly.  Patch below.

> 61. postfix-3.5.8/src/util/dict_inline.c:131:2: note: 2nd function call argument is an uninitialized value
> #         dict->update(dict, vname, value);
> #         ^                  ~~~~~
> #   129|   }
> #   130|   /* No duplicate checks. See comments in dict_thash.c. */
> #   131|-> dict->update(dict, vname, value);
> #   132|   count += 1;
> #   133|       }

Same dict_inline issue as above...

> 7. postfix-3.5.8/src/global/haproxy_srvr.c:456: dereference: Dereferencing a pointer that might be "NULL" "mystrtok(&cp, " \r")" when calling "haproxy_srvr_parse_proto".
>
> #   454|   else if (haproxy_srvr_parse_lit(NEXT_TOKEN, "PROXY", (char *) 0) < 0)
> #   455|      err = "unexpected protocol header";
> #   456|-> else if (haproxy_srvr_parse_proto(NEXT_TOKEN, &addr_family) < 0)
> #   457|      err = "unsupported protocol type";
> #   458|   else if (haproxy_srvr_parse_addr(NEXT_TOKEN, smtp_client_addr,
>
> I think for malformed input it could fail by calling strncasecmp with NULL.

Yes, parse_proto is missing a NULL check.  Patch below.

> 1. postfix-3.5.8/src/tlsproxy/tlsproxy.c:1881:49: warning[-Wmissing-braces]: missing braces around initializer
> #      static const CONFIG_STR_TABLE str_table[] = {
> #                                                  ^
> #  1879|   0,
> #  1880|       };
> #  1881|->     static const CONFIG_STR_TABLE str_table[] = {
> #  1882|   VAR_TLSP_TLS_CHAIN_FILES, DEF_TLSP_TLS_CHAIN_FILES, &var_tlsp_tls_chain_files, 0, 0,
> #  1883|   VAR_TLSP_TLS_CERT_FILE, DEF_TLSP_TLS_CERT_FILE, &var_tlsp_tls_cert_file, 0, 0,
>
> I think it should have braces around table lines. There are multiple tables with the same warning.

The table content is correct as written, and I don't expect we'll be
changing it.  But if Wietse is inclined to rototill all the tables...

> 2. postfix-3.5.8/src/smtp/smtp_session.c:200: var_compare_op: Comparing "session->stream" to null implies that "session->stream" might be null.
> 5. postfix-3.5.8/src/smtp/smtp_session.c:208: var_deref_model: Passing null pointer "session->stream" to "tls_session_stop", which dereferences it.
> #   206|      tls_proxy_context_free(session->tls_context);
> #   207|   else
> #   208|->    tls_client_stop(smtp_tls_ctx, session->stream,
> #   209|    var_smtp_starttls_tmout, 0, session->tls_context);
> #   210|       }
>
> It's suspicious that there is the NULL check, but later it could fail on NULL dereference.

In that particular branch the stream is sure to be non-null.

> 7. postfix-3.5.8/src/smtpd/smtpd.c:1712: var_compare_op: Comparing "state->milters" to null implies that "state->milters" might be null.
> 9. postfix-3.5.8/src/smtpd/smtpd.c:1727: var_deref_model: Passing "state" to "mail_reset", which dereferences null "state->milters".
> #  1725|   helo_reset(state);
> #  1726|       chat_reset(state, var_smtpd_hist_thrsh);
> #  1727|->     mail_reset(state);
> #  1728|       rcpt_reset(state);
> #  1729|       state->helo_name = mystrdup(printable(argv[1].strval, '?'));
>
> It's suspicious that there is the NULL check, but later it could fail on NULL dereference.

The flag that guards the milter dereference is only set when the milter
pointer is not null.

> 31. postfix-3.5.8/src/tls/tls_proxy_client_scan.c:335:9: note: Access to field 'next' results in a dereference of a null pointer (loaded from variable 'tp')
> #     if (tp->next)
> #         ^~
> #   333|   static void tls_proxy_client_tlsa_free(TLS_TLSA *tp)
> #   334|   {
> #   335|->     if (tp->next)
> #   336|   tls_proxy_client_tlsa_free(tp->next);
> #   337|       myfree(tp->mdalg);

> 37. postfix-3.5.8/src/tls/tls_proxy_client_scan.c:324:9: note: Access to field 'next' results in a dereference of a null pointer (loaded from variable 'tp')
> #     if (tp->next)
> #         ^~
> #   322|   static void tls_proxy_client_pkeys_free(TLS_PKEYS *tp)
> #   323|   {
> #   324|->     if (tp->next)
> #   325|   tls_proxy_client_pkeys_free(tp->next);
> #   326|       if (tp->pkey)

> 37. postfix-3.5.8/src/tls/tls_proxy_client_scan.c:313:9: note: Access to field 'next' results in a dereference of a null pointer (loaded from variable 'tp')
> #     if (tp->next)
> #         ^~
> #   311|   static void tls_proxy_client_certs_free(TLS_CERTS *tp)
> #   312|   {
> #   313|->     if (tp->next)
> #   314|   tls_proxy_client_certs_free(tp->next);
> #   315|       if (tp->cert)

This code has been replaced in Postfix 3.6, patch for earlier versions
below my signature.  The NPE can only arise when internal communication
protocols are interrupted midstream.

> 88. postfix-3.5.8/src/global/maillog_client.c:256:10: note: Null pointer passed to 2nd parameter expecting 'nonnull'
> #             if (setenv(POSTLOG_SERVICE_ENV, service_path, 1) < 0)
> #                 ^                           ~~~~~~~~~~~~
> #   254|      msg_info("export %s=%s", POSTLOG_SERVICE_ENV, service_path);
> #   255|   #endif
> #   256|->    if (setenv(POSTLOG_SERVICE_ENV, service_path, 1) < 0)
> #   257|   msg_fatal("setenv: %m");
> #   258|   }

This looks plausible, but I'm not sure what the right fix is.

> 19. postfix-3.5.8/include/vstring.h:70:36: note: expanded from macro 'vstring_str'
> # #define vstring_str(vp)         ((char *) (vp)->vbuf.data)
> #                                           ^~~~~~~~~~~~~~~
> #    72|   vstring_sprintf(canon_name, "%s/%s", tag, argv0);
> #    73|       }
> #    74|->     return (vstring_str(canon_name));
> #    75|   }

This is a non-problem, the first call always sets a non-null value, but
I'm adding a harmless fallback, just in case argv[0] is NULL.

----------
Patch for Postfix 3.5 and 3.6 (perhaps also earlier versions?)
---------

--- a/src/global/haproxy_srvr.c
+++ b/src/global/haproxy_srvr.c
@@ -201,6 +201,8 @@ static int haproxy_srvr_parse_proto(const char *str, int *addr_family)
     if (msg_verbose)
  msg_info("haproxy_srvr_parse: proto=%s", STR_OR_NULL(str));
 
+    if (str == 0)
+ return (-1);
 #ifdef AF_INET6
     if (strcasecmp(str, "TCP6") == 0) {
  if (strchr((char *) proto_info->sa_family_list, AF_INET6) != 0) {
--- a/src/global/mail_task.c
+++ b/src/global/mail_task.c
@@ -71,5 +71,7 @@ const char *mail_task(const char *argv0)
  mail_conf_eval(DEF_SYSLOG_NAME);
  vstring_sprintf(canon_name, "%s/%s", tag, argv0);
     }
-    return (vstring_str(canon_name));
+    if (canon_name)
+ return (vstring_str(canon_name));
+    return ("unknown");
 }
--- a/src/util/dict_inline.c
+++ b/src/util/dict_inline.c
@@ -113,9 +113,11 @@ DICT   *dict_inline_open(const char *name, int open_flags, int dict_flags)
     dict = dict_open3(DICT_TYPE_HT, name, open_flags, dict_flags);
     dict_type_override(dict, DICT_TYPE_INLINE);
     while ((nameval = mystrtokq(&cp, CHARS_COMMA_SP, CHARS_BRACE)) != 0) {
- if ((nameval[0] != CHARS_BRACE[0]
-     || (err = free_me = extpar(&nameval, CHARS_BRACE, EXTPAR_FLAG_STRIP)) == 0)
-    && (err = split_qnameval(nameval, &vname, &value)) != 0)
+ if (nameval[0] != CHARS_BRACE[0])
+    err = free_me = extpar(&nameval, CHARS_BRACE, EXTPAR_FLAG_STRIP);
+ if (!err)
+    err = split_qnameval(nameval, &vname, &value);
+ if (err)
     break;
 
  if ((dict->flags & DICT_FLAG_SRC_RHS_IS_FILE) != 0) {

--
    Viktor.

----------
Below patch for Postfix 3.5 only:
----------

--- a/postfix/src/tls/tls_proxy_client_scan.c
+++ b/postfix/src/tls/tls_proxy_client_scan.c
@@ -310,6 +310,8 @@ int     tls_proxy_client_init_scan(ATTR_SCAN_MASTER_FN scan_fn, VSTREAM *fp,
 
 static void tls_proxy_client_certs_free(TLS_CERTS *tp)
 {
+    if (tp == 0)
+ return;
     if (tp->next)
  tls_proxy_client_certs_free(tp->next);
     if (tp->cert)
@@ -321,6 +323,8 @@ static void tls_proxy_client_certs_free(TLS_CERTS *tp)
 
 static void tls_proxy_client_pkeys_free(TLS_PKEYS *tp)
 {
+    if (tp == 0)
+ return;
     if (tp->next)
  tls_proxy_client_pkeys_free(tp->next);
     if (tp->pkey)
@@ -332,6 +336,8 @@ static void tls_proxy_client_pkeys_free(TLS_PKEYS *tp)
 
 static void tls_proxy_client_tlsa_free(TLS_TLSA *tp)
 {
+    if (tp == 0)
+ return;
     if (tp->next)
  tls_proxy_client_tlsa_free(tp->next);
     myfree(tp->mdalg);
Reply | Threaded
Open this post in threaded view
|

Re: Coverity & clang scan

Wietse Venema
Viktor Dukhovni:

> On Fri, Mar 19, 2021 at 11:18:27AM -0400, Jaroslav Skarvada wrote:
>
> > 14. postfix-3.5.8/src/util/dict_inline.c:124: uninit_use_in_call: Using uninitialized value "value" when calling "dict_file_to_b64".
> > 17. postfix-3.5.8/src/util/dict_inline.c:125: overwrite_var: Overwriting "err" in "err = free_me = dict_file_get_error(dict)" leaks the storage that "err" points to.
> > #   123|  
> > #   124|->    if ((base64_buf = dict_file_to_b64(dict, value)) == 0) {
> > #   125|-> err = free_me = dict_file_get_error(dict);
> > #   126|   break;
> > #   127|      }
> >
> > I think it could call dict_file_to_b64 with uninitialized value.
>
> Yes, when inline tables in the main.cf file are malformed in a
> particular way, this may not be handled correctly.  Patch below.

Can someone provide an input that demonstrates there is a problem?

The 'value' variable is initialized only when 'err' is zero. Otherwise,
the loop will be exited before the 'value' variable would be used.

Also, the claim that 'err' loses a reference to allocated memory is
irrelevant, because such memory is tracked with the 'free_me' variable,
and once it is set, 'free_me' is not overwritten.

I'll simplify the code a bit to make it easier to analyze.

> --- a/src/util/dict_inline.c
> +++ b/src/util/dict_inline.c
> @@ -113,9 +113,11 @@ DICT   *dict_inline_open(const char *name, int open_flags, int dict_flags)
>      dict = dict_open3(DICT_TYPE_HT, name, open_flags, dict_flags);
>      dict_type_override(dict, DICT_TYPE_INLINE);
>      while ((nameval = mystrtokq(&cp, CHARS_COMMA_SP, CHARS_BRACE)) != 0) {
> - if ((nameval[0] != CHARS_BRACE[0]
> -     || (err = free_me = extpar(&nameval, CHARS_BRACE, EXTPAR_FLAG_STRIP)) == 0)
> -    && (err = split_qnameval(nameval, &vname, &value)) != 0)
> + if (nameval[0] != CHARS_BRACE[0])
> +    err = free_me = extpar(&nameval, CHARS_BRACE, EXTPAR_FLAG_STRIP);
> + if (!err)
> +    err = split_qnameval(nameval, &vname, &value);
> + if (err)
>      break;
>  
>   if ((dict->flags & DICT_FLAG_SRC_RHS_IS_FILE) != 0) {

I think that the patch should use 'nameval[0]  == CHARS_BRACE[0]'

        Wietse
Reply | Threaded
Open this post in threaded view
|

Re: Coverity & clang scan

Wietse Venema
Wietse Venema:

> Viktor Dukhovni:
> > On Fri, Mar 19, 2021 at 11:18:27AM -0400, Jaroslav Skarvada wrote:
> >
> > > 14. postfix-3.5.8/src/util/dict_inline.c:124: uninit_use_in_call: Using uninitialized value "value" when calling "dict_file_to_b64".
> > > 17. postfix-3.5.8/src/util/dict_inline.c:125: overwrite_var: Overwriting "err" in "err = free_me = dict_file_get_error(dict)" leaks the storage that "err" points to.
> > > #   123|  
> > > #   124|->    if ((base64_buf = dict_file_to_b64(dict, value)) == 0) {
> > > #   125|-> err = free_me = dict_file_get_error(dict);
> > > #   126|   break;
> > > #   127|      }
> > >
> > > I think it could call dict_file_to_b64 with uninitialized value.
> >
> > Yes, when inline tables in the main.cf file are malformed in a
> > particular way, this may not be handled correctly.  Patch below.
>
> Can someone provide an input that demonstrates there is a problem?
>
> The 'value' variable is initialized only when 'err' is zero. Otherwise,
> the loop will be exited before the 'value' variable would be used.

Allright, I found one.

        Wietse
Reply | Threaded
Open this post in threaded view
|

Re: Coverity & clang scan

Wietse Venema
In reply to this post by Viktor Dukhovni
Viktor Dukhovni:

> > 88. postfix-3.5.8/src/global/maillog_client.c:256:10: note: Null pointer passed to 2nd parameter expecting 'nonnull'
> > #             if (setenv(POSTLOG_SERVICE_ENV, service_path, 1) < 0)
> > #                 ^                           ~~~~~~~~~~~~
> > #   254|      msg_info("export %s=%s", POSTLOG_SERVICE_ENV, service_path);
> > #   255|   #endif
> > #   256|->    if (setenv(POSTLOG_SERVICE_ENV, service_path, 1) < 0)
> > #   257|   msg_fatal("setenv: %m");
> > #   258|   }
>
> This looks plausible, but I'm not sure what the right fix is.

Not a problem. I have added comments that explain why service_path
cannot be null.

        Wietse