Browse code

Remove NULL checks before calling free

We (and OpenSSL) already use calling free on null pointers in a number
of places and also C99 standards says free(NULL) does nothing.

The if (x) free(x) calls more often make code harder to read, instead
of easier, remove these NULL checks in favour of directly calling
free(x).

The OpenSSL *_free methods are also safe to call with NULL and
pkcs11h_certificate_freeCertificateIdList is also safe to be called with
NULL.

Signed-off-by: Arne Schwabe <arne@rfc2549.org>
Acked-by: Gert Doering <gert@greenie.muc.de>
Message-Id: <20201023113431.26691-5-arne@rfc2549.org>
URL: https://www.mail-archive.com/openvpn-devel@lists.sourceforge.net/msg21216.html
Signed-off-by: Gert Doering <gert@greenie.muc.de>

Arne Schwabe authored on 2020/10/23 20:34:31
Showing 20 changed files
... ...
@@ -173,10 +173,7 @@ openvpn_plugin_open_v3(const int v3structver,
173 173
     return OPENVPN_PLUGIN_FUNC_SUCCESS;
174 174
 
175 175
 error:
176
-    if (context)
177
-    {
178
-        free(context);
179
-    }
176
+    free(context);
180 177
     return OPENVPN_PLUGIN_FUNC_ERROR;
181 178
 }
182 179
 
... ...
@@ -184,10 +184,7 @@ buf_assign(struct buffer *dest, const struct buffer *src)
184 184
 void
185 185
 free_buf(struct buffer *buf)
186 186
 {
187
-    if (buf->data)
188
-    {
189
-        free(buf->data);
190
-    }
187
+    free(buf->data);
191 188
     CLEAR(*buf);
192 189
 }
193 190
 
... ...
@@ -488,11 +488,8 @@ close_syslog(void)
488 488
     {
489 489
         closelog();
490 490
         use_syslog = false;
491
-        if (pgmname_syslog)
492
-        {
493
-            free(pgmname_syslog);
494
-            pgmname_syslog = NULL;
495
-        }
491
+        free(pgmname_syslog);
492
+        pgmname_syslog = NULL;
496 493
     }
497 494
 #endif
498 495
 }
... ...
@@ -3597,14 +3597,9 @@ do_close_tls(struct context *c)
3597 3597
     }
3598 3598
 
3599 3599
     /* free options compatibility strings */
3600
-    if (c->c2.options_string_local)
3601
-    {
3602
-        free(c->c2.options_string_local);
3603
-    }
3604
-    if (c->c2.options_string_remote)
3605
-    {
3606
-        free(c->c2.options_string_remote);
3607
-    }
3600
+    free(c->c2.options_string_local);
3601
+    free(c->c2.options_string_remote);
3602
+
3608 3603
     c->c2.options_string_local = c->c2.options_string_remote = NULL;
3609 3604
 
3610 3605
     if (c->c2.pulled_options_state)
... ...
@@ -826,14 +826,8 @@ man_pkcs11_id_get(struct management *man, const int index)
826 826
         msg(M_CLIENT, ">PKCS11ID-ENTRY:'%d'", index);
827 827
     }
828 828
 
829
-    if (id != NULL)
830
-    {
831
-        free(id);
832
-    }
833
-    if (base64 != NULL)
834
-    {
835
-        free(base64);
836
-    }
829
+    free(id);
830
+    free(base64);
837 831
 }
838 832
 
839 833
 #endif /* ifdef ENABLE_PKCS11 */
... ...
@@ -2613,10 +2607,7 @@ man_connection_close(struct management *man)
2613 2613
 {
2614 2614
     struct man_connection *mc = &man->connection;
2615 2615
 
2616
-    if (mc->es)
2617
-    {
2618
-        event_free(mc->es);
2619
-    }
2616
+    event_free(mc->es);
2620 2617
 #ifdef _WIN32
2621 2618
     net_event_win32_close(&mc->ne32);
2622 2619
 #endif
... ...
@@ -2629,14 +2620,10 @@ man_connection_close(struct management *man)
2629 2629
     {
2630 2630
         man_close_socket(man, mc->sd_cli);
2631 2631
     }
2632
-    if (mc->in)
2633
-    {
2634
-        command_line_free(mc->in);
2635
-    }
2636
-    if (mc->out)
2637
-    {
2638
-        buffer_list_free(mc->out);
2639
-    }
2632
+
2633
+    command_line_free(mc->in);
2634
+    buffer_list_free(mc->out);
2635
+
2640 2636
     in_extra_reset(&man->connection, IER_RESET);
2641 2637
     buffer_list_free(mc->ext_key_input);
2642 2638
     man_connection_clear(mc);
... ...
@@ -3896,6 +3883,10 @@ command_line_reset(struct command_line *cl)
3896 3896
 void
3897 3897
 command_line_free(struct command_line *cl)
3898 3898
 {
3899
+    if (!cl)
3900
+    {
3901
+        return;
3902
+    }
3899 3903
     command_line_reset(cl);
3900 3904
     free_buf(&cl->buf);
3901 3905
     free_buf(&cl->residual);
... ...
@@ -4015,10 +4006,8 @@ log_entry_print(const struct log_entry *e, unsigned int flags, struct gc_arena *
4015 4015
 static void
4016 4016
 log_entry_free_contents(struct log_entry *e)
4017 4017
 {
4018
-    if (e->string)
4019
-    {
4020
-        free((char *)e->string);
4021
-    }
4018
+    /* Cast away constness of const char* */
4019
+    free((char *)e->string);
4022 4020
     CLEAR(*e);
4023 4021
 }
4024 4022
 
... ...
@@ -229,10 +229,7 @@ multi_tcp_free(struct multi_tcp *mtcp)
229 229
     if (mtcp)
230 230
     {
231 231
         event_free(mtcp->es);
232
-        if (mtcp->esr)
233
-        {
234
-            free(mtcp->esr);
235
-        }
232
+        free(mtcp->esr);
236 233
         free(mtcp);
237 234
     }
238 235
 }
... ...
@@ -73,10 +73,7 @@ id(struct multi_instance *mi)
73 73
 static void
74 74
 set_cc_config(struct multi_instance *mi, struct buffer_list *cc_config)
75 75
 {
76
-    if (mi->cc_config)
77
-    {
78
-        buffer_list_free(mi->cc_config);
79
-    }
76
+    buffer_list_free(mi->cc_config);
80 77
     mi->cc_config = cc_config;
81 78
 }
82 79
 #endif
... ...
@@ -4016,10 +4013,7 @@ management_client_pf(void *arg,
4016 4016
         ret = pf_load_from_buffer_list(&mi->context, pf_config);
4017 4017
     }
4018 4018
 
4019
-    if (pf_config)
4020
-    {
4021
-        buffer_list_free(pf_config);
4022
-    }
4019
+    buffer_list_free(pf_config);
4023 4020
     return ret;
4024 4021
 }
4025 4022
 #endif /* ifdef MANAGEMENT_PF */
... ...
@@ -103,10 +103,7 @@ packet_id_free(struct packet_id *p)
103 103
     if (p)
104 104
     {
105 105
         dmsg(D_PID_DEBUG, "PID packet_id_free");
106
-        if (p->rec.seq_list)
107
-        {
108
-            free(p->rec.seq_list);
109
-        }
106
+        free(p->rec.seq_list);
110 107
         CLEAR(*p);
111 108
     }
112 109
 }
... ...
@@ -461,11 +461,8 @@ pkcs11_management_id_count(void)
461 461
 
462 462
 cleanup:
463 463
 
464
-    if (id_list != NULL)
465
-    {
466
-        pkcs11h_certificate_freeCertificateIdList(id_list);
467
-        id_list = NULL;
468
-    }
464
+    pkcs11h_certificate_freeCertificateIdList(id_list);
465
+    id_list = NULL;
469 466
 
470 467
     dmsg(
471 468
         D_PKCS11_DEBUG,
... ...
@@ -630,29 +627,17 @@ pkcs11_management_id_get(
630 630
 
631 631
 cleanup:
632 632
 
633
-    if (id_list != NULL)
634
-    {
635
-        pkcs11h_certificate_freeCertificateIdList(id_list);
636
-        id_list = NULL;
637
-    }
633
+    pkcs11h_certificate_freeCertificateIdList(id_list);
634
+    id_list = NULL;
638 635
 
639
-    if (internal_id != NULL)
640
-    {
641
-        free(internal_id);
642
-        internal_id = NULL;
643
-    }
636
+    free(internal_id);
637
+    internal_id = NULL;
644 638
 
645
-    if (internal_base64 != NULL)
646
-    {
647
-        free(internal_base64);
648
-        internal_base64 = NULL;
649
-    }
639
+    free(internal_base64);
640
+    internal_base64 = NULL;
650 641
 
651
-    if (certificate_blob != NULL)
652
-    {
653
-        free(certificate_blob);
654
-        certificate_blob = NULL;
655
-    }
642
+    free(certificate_blob);
643
+    certificate_blob = NULL;
656 644
 
657 645
     dmsg(
658 646
         D_PKCS11_DEBUG,
... ...
@@ -1005,19 +990,13 @@ cleanup1:
1005 1005
             certificate = NULL;
1006 1006
         }
1007 1007
 
1008
-        if (ser != NULL)
1009
-        {
1010
-            free(ser);
1011
-            ser = NULL;
1012
-        }
1008
+        free(ser);
1009
+        ser = NULL;
1013 1010
     }
1014 1011
 
1015 1012
 cleanup:
1016
-    if (user_certificates != NULL)
1017
-    {
1018
-        pkcs11h_certificate_freeCertificateIdList(user_certificates);
1019
-        user_certificates = NULL;
1020
-    }
1013
+    pkcs11h_certificate_freeCertificateIdList(user_certificates);
1014
+    user_certificates = NULL;
1021 1015
 
1022 1016
     pkcs11h_terminate();
1023 1017
     gc_free(&gc);
... ...
@@ -102,17 +102,11 @@ cleanup:
102 102
      * openssl objects have reference
103 103
      * count, so release them
104 104
      */
105
-    if (x509 != NULL)
106
-    {
107
-        X509_free(x509);
108
-        x509 = NULL;
109
-    }
105
+    X509_free(x509);
106
+    x509 = NULL;
110 107
 
111
-    if (evp != NULL)
112
-    {
113
-        EVP_PKEY_free(evp);
114
-        evp = NULL;
115
-    }
108
+    EVP_PKEY_free(evp);
109
+    evp = NULL;
116 110
 
117 111
     if (openssl_session != NULL)
118 112
     {
... ...
@@ -138,11 +132,8 @@ pkcs11_certificate_dn(pkcs11h_certificate_t certificate, struct gc_arena *gc)
138 138
     dn = x509_get_subject(x509, gc);
139 139
 
140 140
 cleanup:
141
-    if (x509 != NULL)
142
-    {
143
-        X509_free(x509);
144
-        x509 = NULL;
145
-    }
141
+    X509_free(x509);
142
+    x509 = NULL;
146 143
 
147 144
     return dn;
148 145
 }
... ...
@@ -183,12 +174,9 @@ pkcs11_certificate_serial(pkcs11h_certificate_t certificate, char *serial,
183 183
     ret = 0;
184 184
 
185 185
 cleanup:
186
+    X509_free(x509);
187
+    x509 = NULL;
186 188
 
187
-    if (x509 != NULL)
188
-    {
189
-        X509_free(x509);
190
-        x509 = NULL;
191
-    }
192 189
     return ret;
193 190
 }
194 191
 #endif /* defined(ENABLE_PKCS11) && defined(ENABLE_OPENSSL) */
... ...
@@ -366,10 +366,7 @@ get_proxy_authenticate(socket_descriptor_t sd,
366 366
 static void
367 367
 store_proxy_authenticate(struct http_proxy_info *p, char *data)
368 368
 {
369
-    if (p->proxy_authenticate)
370
-    {
371
-        free(p->proxy_authenticate);
372
-    }
369
+    free(p->proxy_authenticate);
373 370
     p->proxy_authenticate = data;
374 371
 }
375 372
 
... ...
@@ -1105,10 +1105,7 @@ tls_session_free(struct tls_session *session, bool clear)
1105 1105
         key_state_free(&session->key[i], false);
1106 1106
     }
1107 1107
 
1108
-    if (session->common_name)
1109
-    {
1110
-        free(session->common_name);
1111
-    }
1108
+    free(session->common_name);
1112 1109
 
1113 1110
     cert_hash_free(session->cert_hash_set);
1114 1111
 
... ...
@@ -1300,16 +1297,8 @@ tls_multi_free(struct tls_multi *multi, bool clear)
1300 1300
     auth_set_client_reason(multi, NULL);
1301 1301
 
1302 1302
     free(multi->peer_info);
1303
-
1304
-    if (multi->locked_cn)
1305
-    {
1306
-        free(multi->locked_cn);
1307
-    }
1308
-
1309
-    if (multi->locked_username)
1310
-    {
1311
-        free(multi->locked_username);
1312
-    }
1303
+    free(multi->locked_cn);
1304
+    free(multi->locked_username);
1313 1305
 
1314 1306
     cert_hash_free(multi->locked_cert_hash_set);
1315 1307
 
... ...
@@ -138,53 +138,31 @@ tls_ctx_free(struct tls_root_ctx *ctx)
138 138
     if (ctx)
139 139
     {
140 140
         mbedtls_pk_free(ctx->priv_key);
141
-        if (ctx->priv_key)
142
-        {
143
-            free(ctx->priv_key);
144
-        }
141
+        free(ctx->priv_key);
145 142
 
146 143
         mbedtls_x509_crt_free(ctx->ca_chain);
147
-        if (ctx->ca_chain)
148
-        {
149
-            free(ctx->ca_chain);
150
-        }
144
+        free(ctx->ca_chain);
151 145
 
152 146
         mbedtls_x509_crt_free(ctx->crt_chain);
153
-        if (ctx->crt_chain)
154
-        {
155
-            free(ctx->crt_chain);
156
-        }
147
+        free(ctx->crt_chain);
157 148
 
158 149
         mbedtls_dhm_free(ctx->dhm_ctx);
159
-        if (ctx->dhm_ctx)
160
-        {
161
-            free(ctx->dhm_ctx);
162
-        }
150
+        free(ctx->dhm_ctx);
163 151
 
164 152
         mbedtls_x509_crl_free(ctx->crl);
165
-        if (ctx->crl)
166
-        {
167
-            free(ctx->crl);
168
-        }
153
+        free(ctx->crl);
169 154
 
170 155
 #if defined(ENABLE_PKCS11)
171 156
         pkcs11h_certificate_freeCertificate(ctx->pkcs11_cert);
172 157
 #endif
173 158
 
174
-        if (ctx->allowed_ciphers)
175
-        {
176
-            free(ctx->allowed_ciphers);
177
-        }
159
+        free(ctx->allowed_ciphers);
178 160
 
179
-        if (ctx->groups)
180
-        {
181
-            free(ctx->groups);
182
-        }
161
+        free(ctx->groups);
183 162
 
184 163
         CLEAR(*ctx);
185 164
 
186 165
         ctx->initialised = false;
187
-
188 166
     }
189 167
 }
190 168
 
... ...
@@ -144,10 +144,7 @@ void
144 144
 tls_ctx_free(struct tls_root_ctx *ctx)
145 145
 {
146 146
     ASSERT(NULL != ctx);
147
-    if (NULL != ctx->ctx)
148
-    {
149
-        SSL_CTX_free(ctx->ctx);
150
-    }
147
+    SSL_CTX_free(ctx->ctx);
151 148
     ctx->ctx = NULL;
152 149
 }
153 150
 
... ...
@@ -978,14 +975,8 @@ end:
978 978
         crypto_print_openssl_errors(M_DEBUG);
979 979
     }
980 980
 
981
-    if (in != NULL)
982
-    {
983
-        BIO_free(in);
984
-    }
985
-    if (x)
986
-    {
987
-        X509_free(x);
988
-    }
981
+    BIO_free(in);
982
+    X509_free(x);
989 983
 }
990 984
 
991 985
 int
... ...
@@ -1044,14 +1035,8 @@ tls_ctx_load_priv_file(struct tls_root_ctx *ctx, const char *priv_key_file,
1044 1044
     ret = 0;
1045 1045
 
1046 1046
 end:
1047
-    if (pkey)
1048
-    {
1049
-        EVP_PKEY_free(pkey);
1050
-    }
1051
-    if (in)
1052
-    {
1053
-        BIO_free(in);
1054
-    }
1047
+    EVP_PKEY_free(pkey);
1048
+    BIO_free(in);
1055 1049
     return ret;
1056 1050
 }
1057 1051
 
... ...
@@ -1312,12 +1297,9 @@ err:
1312 1312
     {
1313 1313
         RSA_free(rsa);
1314 1314
     }
1315
-    else
1315
+    else if (rsa_meth)
1316 1316
     {
1317
-        if (rsa_meth)
1318
-        {
1319
-            RSA_meth_free(rsa_meth);
1320
-        }
1317
+        RSA_meth_free(rsa_meth);
1321 1318
     }
1322 1319
     return 0;
1323 1320
 }
... ...
@@ -1441,14 +1423,8 @@ tls_ctx_use_external_ec_key(struct tls_root_ctx *ctx, EVP_PKEY *pkey)
1441 1441
 
1442 1442
 err:
1443 1443
     /* Reach here only when ec and privkey can be independenly freed */
1444
-    if (privkey)
1445
-    {
1446
-        EVP_PKEY_free(privkey);
1447
-    }
1448
-    if (ec)
1449
-    {
1450
-        EC_KEY_free(ec);
1451
-    }
1444
+    EVP_PKEY_free(privkey);
1445
+    EC_KEY_free(ec);
1452 1446
     return 0;
1453 1447
 }
1454 1448
 #endif /* OPENSSL_VERSION_NUMBER > 1.1.0 dev && !defined(OPENSSL_NO_EC) */
... ...
@@ -1645,10 +1621,7 @@ tls_ctx_load_ca(struct tls_root_ctx *ctx, const char *ca_file,
1645 1645
             }
1646 1646
         }
1647 1647
 
1648
-        if (in)
1649
-        {
1650
-            BIO_free(in);
1651
-        }
1648
+        BIO_free(in);
1652 1649
     }
1653 1650
 
1654 1651
     /* Set a store for certs (CA & CRL) with a lookup on the "capath" hash directory */
... ...
@@ -841,11 +841,9 @@ cleanup:
841 841
 void
842 842
 auth_set_client_reason(struct tls_multi *multi, const char *client_reason)
843 843
 {
844
-    if (multi->client_reason)
845
-    {
846
-        free(multi->client_reason);
847
-        multi->client_reason = NULL;
848
-    }
844
+    free(multi->client_reason);
845
+    multi->client_reason = NULL;
846
+
849 847
     if (client_reason && strlen(client_reason))
850 848
     {
851 849
         multi->client_reason = string_alloc(client_reason, NULL);
... ...
@@ -372,11 +372,7 @@ x509_get_subject(X509 *cert, struct gc_arena *gc)
372 372
     subject[subject_mem->length] = '\0';
373 373
 
374 374
 err:
375
-    if (subject_bio)
376
-    {
377
-        BIO_free(subject_bio);
378
-    }
379
-
375
+    BIO_free(subject_bio);
380 376
     return subject;
381 377
 }
382 378
 
... ...
@@ -203,10 +203,8 @@ status_close(struct status_output *so)
203 203
                 ret = false;
204 204
             }
205 205
         }
206
-        if (so->filename)
207
-        {
208
-            free(so->filename);
209
-        }
206
+        free(so->filename);
207
+
210 208
         if (buf_defined(&so->read_buf))
211 209
         {
212 210
             free_buf(&so->read_buf);
... ...
@@ -1835,10 +1835,8 @@ close_tun_generic(struct tuntap *tt)
1835 1835
     {
1836 1836
         close(tt->fd);
1837 1837
     }
1838
-    if (tt->actual_name)
1839
-    {
1840
-        free(tt->actual_name);
1841
-    }
1838
+
1839
+    free(tt->actual_name);
1842 1840
     clear_tuntap(tt);
1843 1841
 }
1844 1842
 #endif /* !_WIN32 */
... ...
@@ -2522,10 +2520,7 @@ close_tun(struct tuntap *tt, openvpn_net_ctx_t *ctx)
2522 2522
 
2523 2523
     solaris_close_tun(tt);
2524 2524
 
2525
-    if (tt->actual_name)
2526
-    {
2527
-        free(tt->actual_name);
2528
-    }
2525
+    free(tt->actual_name);
2529 2526
 
2530 2527
     clear_tuntap(tt);
2531 2528
     free(tt);
... ...
@@ -6901,10 +6896,7 @@ close_tun(struct tuntap *tt, openvpn_net_ctx_t *ctx)
6901 6901
         }
6902 6902
     }
6903 6903
 
6904
-    if (tt->actual_name)
6905
-    {
6906
-        free(tt->actual_name);
6907
-    }
6904
+    free(tt->actual_name);
6908 6905
 
6909 6906
     if (tt->windows_driver == WINDOWS_DRIVER_WINTUN)
6910 6907
     {
... ...
@@ -506,10 +506,7 @@ openvpn_plugin_open_v3(const int v3structver,
506 506
     }
507 507
 
508 508
 error:
509
-    if (context)
510
-    {
511
-        free(context);
512
-    }
509
+    free(context);
513 510
     return OPENVPN_PLUGIN_FUNC_ERROR;
514 511
 }
515 512
 
... ...
@@ -238,10 +238,7 @@ free_context(struct down_root_context *context)
238 238
 {
239 239
     if (context)
240 240
     {
241
-        if (context->command)
242
-        {
243
-            free(context->command);
244
-        }
241
+        free(context->command);
245 242
         free(context);
246 243
     }
247 244
 }