Changeset e2ba939 in mod_gnutls for src


Ignore:
Timestamp:
Jun 15, 2016, 1:32:14 PM (3 years ago)
Author:
Thomas Klute <thomas2.klute@…>
Branches:
debian/master, debian/stretch-backports, master, upstream
Children:
81433f1
Parents:
317b569
git-author:
Thomas Klute <thomas2.klute@…> (06/15/16 01:52:34)
git-committer:
Thomas Klute <thomas2.klute@…> (06/15/16 13:32:14)
Message:

Prevent memory leaks in post_conf hook

Valgrind indicated memory leaks from GnuTLS structure initializations
in mgs_load_files(). This method is called from the post_config hook,
so there was no continuous leak, but graceful restart might have been
affected. There were two main reasons for the leaks:

  • GnuTLS does not use the APR memory pool infrastructure, so its structures are not automatically deinitialized when the pool lifetime ends. Registering a cleanup hook with the pool solves this problem.
  • Memory for the X.509 certificate chain (sc->certs_x509_crt_chain) is allocated from the config pool at the beginning of mgs_load_files(), but the certificates were loaded using gnutls_x509_crt_list_import2(), which allocates memory on its own, the pointer to which was lost when the server config pool was destroyed. Switch to gnutls_x509_crt_list_import() to use the pool memory.

Additionally, all pointers to the structures released by the pool
cleanup hook must be properly initialized to NULL (and reset on
deinit) to make sure they are only deinitialized if they have been
actually been initialized. Not doing that results in interesting stack
traces, but not a working server.

Note that this commit fixes only the leaks visible in my Valgrind
analysis of a server with an X.509 identity and an anonymous
client. Other structures will be covered by follow-up commits after
additional review.

File:
1 edited

Legend:

Unmodified
Added
Removed
  • src/gnutls_config.c

    r317b569 re2ba939  
    9595        "-----END DH PARAMETERS-----\n";
    9696
     97/*
     98 * Clean up the various GnuTLS data structures allocated from
     99 * mgs_load_files()
     100 */
     101static apr_status_t mgs_pool_free_credentials(void *arg)
     102{
     103    mgs_srvconf_rec *sc = (mgs_srvconf_rec *) arg;
     104
     105    if (sc->certs)
     106    {
     107        gnutls_certificate_free_credentials(sc->certs);
     108        sc->certs = NULL;
     109    }
     110
     111    if (sc->anon_creds)
     112    {
     113        gnutls_anon_free_server_credentials(sc->anon_creds);
     114        sc->anon_creds = NULL;
     115    }
     116
     117#ifdef ENABLE_SRP
     118    if (sc->srp_creds)
     119    {
     120        gnutls_srp_free_server_credentials(sc->srp_creds);
     121        sc->srp_creds = NULL;
     122    }
     123#endif
     124
     125    if (sc->dh_params)
     126    {
     127        gnutls_dh_params_deinit(sc->dh_params);
     128        sc->dh_params = NULL;
     129    }
     130
     131    for (unsigned int i = 0; i < sc->certs_x509_chain_num; i++)
     132    {
     133        gnutls_pcert_deinit(&sc->certs_x509_chain[i]);
     134        gnutls_x509_crt_deinit(sc->certs_x509_crt_chain[i]);
     135    }
     136
     137    if (sc->privkey_x509)
     138    {
     139        gnutls_privkey_deinit(sc->privkey_x509);
     140        sc->privkey_x509 = NULL;
     141    }
     142
     143    if (sc->priorities)
     144    {
     145        gnutls_priority_deinit(sc->priorities);
     146        sc->priorities = NULL;
     147    }
     148
     149    return APR_SUCCESS;
     150}
     151
    97152int mgs_load_files(apr_pool_t * p, server_rec * s)
    98153{
     
    110165    sc->cert_crt_pgp = apr_pcalloc(p, sizeof(sc->cert_crt_pgp[0]));
    111166    sc->certs_x509_chain =
    112         apr_pcalloc(p, MAX_CHAIN_SIZE * sizeof(sc->certs_x509_chain[0]));
     167        apr_pcalloc(p, MAX_CHAIN_SIZE * sizeof(sc->certs_x509_chain[0]));
    113168    sc->certs_x509_crt_chain =
    114         apr_pcalloc(p,
    115                     MAX_CHAIN_SIZE * sizeof(sc->certs_x509_crt_chain[0]));
    116 
    117     ret = gnutls_certificate_allocate_credentials(&sc->certs);
    118     if (ret < 0) {
    119         ap_log_error(APLOG_MARK, APLOG_STARTUP, 0, s,
    120                      "GnuTLS: Failed to initialize" ": (%d) %s", ret,
    121                      gnutls_strerror(ret));
    122         ret = -1;
    123         goto cleanup;
    124     }
    125 
    126     ret = gnutls_anon_allocate_server_credentials(&sc->anon_creds);
    127     if (ret < 0) {
    128         ap_log_error(APLOG_MARK, APLOG_STARTUP, 0, s,
    129                      "GnuTLS: Failed to initialize" ": (%d) %s", ret,
    130                      gnutls_strerror(ret));
    131         ret = -1;
    132         goto cleanup;
     169        apr_pcalloc(p, MAX_CHAIN_SIZE * sizeof(sc->certs_x509_crt_chain[0]));
     170
     171    /* Cleanup function for the GnuTLS structures allocated below */
     172    apr_pool_cleanup_register(p, sc, mgs_pool_free_credentials,
     173                              apr_pool_cleanup_null);
     174
     175    if (sc->certs == NULL)
     176    {
     177        ret = gnutls_certificate_allocate_credentials(&sc->certs);
     178        if (ret < 0) {
     179            ap_log_error(APLOG_MARK, APLOG_STARTUP, 0, s,
     180                         "GnuTLS: Failed to initialize" ": (%d) %s", ret,
     181                         gnutls_strerror(ret));
     182            ret = -1;
     183            goto cleanup;
     184        }
     185    }
     186
     187    if (sc->anon_creds == NULL)
     188    {
     189        ret = gnutls_anon_allocate_server_credentials(&sc->anon_creds);
     190        if (ret < 0) {
     191            ap_log_error(APLOG_MARK, APLOG_STARTUP, 0, s,
     192                         "GnuTLS: Failed to initialize" ": (%d) %s", ret,
     193                         gnutls_strerror(ret));
     194            ret = -1;
     195            goto cleanup;
     196        }
    133197    }
    134198
    135199    /* Load SRP parameters */
    136200#ifdef ENABLE_SRP
    137     ret = gnutls_srp_allocate_server_credentials(&sc->srp_creds);
    138     if (ret < 0) {
    139         ap_log_error(APLOG_MARK, APLOG_STARTUP, 0, s,
    140                      "GnuTLS: Failed to initialize" ": (%d) %s", ret,
    141                      gnutls_strerror(ret));
    142         ret = -1;
    143         goto cleanup;
     201    if (sc->srp_creds == NULL)
     202    {
     203        ret = gnutls_srp_allocate_server_credentials(&sc->srp_creds);
     204        if (ret < 0) {
     205            ap_log_error(APLOG_MARK, APLOG_STARTUP, 0, s,
     206                         "GnuTLS: Failed to initialize" ": (%d) %s", ret,
     207                         gnutls_strerror(ret));
     208            ret = -1;
     209            goto cleanup;
     210        }
    144211    }
    145212
     
    161228#endif
    162229
    163     ret = gnutls_dh_params_init(&sc->dh_params);
    164     if (ret < 0) {
    165             ap_log_error(APLOG_MARK, APLOG_STARTUP, 0, s,
    166                          "GnuTLS: Failed to initialize"
    167                          ": (%d) %s", ret, gnutls_strerror(ret));
    168             ret = -1;
    169             goto cleanup;
    170     }
     230    if (sc->dh_params == NULL)
     231    {
     232        ret = gnutls_dh_params_init(&sc->dh_params);
     233        if (ret < 0) {
     234            ap_log_error(APLOG_MARK, APLOG_STARTUP, 0, s,
     235                         "GnuTLS: Failed to initialize"
     236                         ": (%d) %s", ret, gnutls_strerror(ret));
     237            ret = -1;
     238            goto cleanup;
     239        }
    171240
    172241    /* Load DH parameters */
     
    205274        }
    206275    }
    207 
    208     if (sc->x509_cert_file != NULL) {
    209         unsigned int chain_num, i;
    210         unsigned format = GNUTLS_X509_FMT_PEM;
     276    }
     277
     278    if (sc->x509_cert_file != NULL && sc->certs_x509_crt_chain[0] == NULL)
     279    {
     280        unsigned int chain_num = MAX_CHAIN_SIZE;
     281        unsigned format = GNUTLS_X509_FMT_PEM;
    211282
    212283        /* Load X.509 certificate */
     
    259330        }
    260331
    261         ret =
    262             gnutls_x509_crt_list_import2(&sc->certs_x509_crt_chain,
    263                                         &chain_num, &data, format,
    264                                         GNUTLS_X509_CRT_LIST_FAIL_IF_UNSORTED);
     332        ret = gnutls_x509_crt_list_import(sc->certs_x509_crt_chain,
     333                                      &chain_num, &data, format,
     334                                      GNUTLS_X509_CRT_LIST_FAIL_IF_UNSORTED);
    265335        gnutls_free(data.data);
    266336        sc->certs_x509_chain_num = chain_num;
     
    274344        }
    275345
    276         for (i = 0; i < chain_num; i++) {
     346    for (unsigned int i = 0; i < chain_num; i++)
     347    {
    277348            ret =
    278349                gnutls_pcert_import_x509(&sc->certs_x509_chain[i],
     
    289360    }
    290361
    291     if (sc->x509_key_file) {
     362    if (sc->x509_key_file && sc->privkey_x509 == NULL)
     363    {
    292364        ret = gnutls_privkey_init(&sc->privkey_x509);
    293365        if (ret < 0) {
     
    513585    }
    514586
    515     if (sc->priorities_str) {
     587    if (sc->priorities_str && sc->priorities == NULL)
     588    {
    516589        const char *err;
    517590        ret = gnutls_priority_init(&sc->priorities, sc->priorities_str, &err);
     
    9571030    sc->privkey_x509 = NULL;
    9581031    sc->privkey_pgp = NULL;
     1032    sc->anon_creds = NULL;
     1033#ifdef ENABLE_SRP
     1034    sc->srp_creds = NULL;
     1035#endif
     1036    sc->certs = NULL;
    9591037    sc->certs_x509_chain_num = 0;
    9601038    sc->p11_modules = NULL;
Note: See TracChangeset for help on using the changeset viewer.