------------------------------------------------------------ revno: 11701 revision-id: squid3@treenet.co.nz-20121126082604-wshgczw45e2qrsc3 parent: squid3@treenet.co.nz-20121110043936-xukvbox38z87gh4j fixes bug(s): http://bugs.squid-cache.org/show_bug.cgi?id=3405 author: Christos Tsantilas committer: Amos Jeffries branch nick: 3.2 timestamp: Mon 2012-11-26 01:26:04 -0700 message: Bug 3405: ssl_crtd crashes failing to remove certificate - Try to update the index file in all cases the database modified rows. Currently we are using the new operator. - The find operator in database should not modify the database. Currently if an entry is expired, ssl_crtd removes the cert file but does not update the index file. - Fix a small memory leak when remove entries from database: A row object removed from TXT_DB indexes but never released. This patch: * Use OPENSSL_malloc and OPENSSL_free to allocate/release memory for TXT_DB rows. OpenSSL SDK assumes that always allocated using these functions. * Add code in Ssl::CertificateDb::Row destructor to correctly release a TXT_DB row. * Add the sq_TXT_DB_delete and sq_TXT_DB_delete_row functions which removes a row from TXT_DB indexes. This is a Measurement Factory project ------------------------------------------------------------ # Bazaar merge directive format 2 (Bazaar 0.90) # revision_id: squid3@treenet.co.nz-20121126082604-wshgczw45e2qrsc3 # target_branch: http://bzr.squid-cache.org/bzr/squid3/branches\ # /SQUID_3_2 # testament_sha1: 67c784f09be87195e5a7834c63ba22575444f1f5 # timestamp: 2012-11-26 08:36:41 +0000 # source_branch: http://bzr.squid-cache.org/bzr/squid3/branches\ # /SQUID_3_2 # base_revision_id: squid3@treenet.co.nz-20121110043936-\ # xukvbox38z87gh4j # # Begin patch === modified file 'src/ssl/certificate_db.cc' --- src/ssl/certificate_db.cc 2012-07-28 05:38:50 +0000 +++ src/ssl/certificate_db.cc 2012-11-26 08:26:04 +0000 @@ -108,19 +108,38 @@ Ssl::CertificateDb::Row::Row() : width(cnlNumber) { - row = new char *[width + 1]; + row = (char **)OPENSSL_malloc(sizeof(char *) * (width + 1)); for (size_t i = 0; i < width + 1; ++i) row[i] = NULL; } +Ssl::CertificateDb::Row::Row(char **aRow, size_t aWidth): width(aWidth) +{ + row = aRow; +} + Ssl::CertificateDb::Row::~Row() { - if (row) { - for (size_t i = 0; i < width + 1; ++i) { - delete[](row[i]); - } - delete[](row); + if (!row) + return; + + void *max; + if ((max = (void *)row[width]) != NULL) { + // It is an openSSL allocated row. The TXT_DB_read function stores the + // index and row items one one memory segment. The row[width] points + // to the end of buffer. We have to check for items in the array which + // are not stored in this segment. These items should released. + for (size_t i = 0; i < width + 1; ++i) { + if (((row[i] < (char *)row) || (row[i] > max)) && (row[i] != NULL)) + OPENSSL_free(row[i]); + } + } else { + for (size_t i = 0; i < width + 1; ++i) { + if (row[i]) + OPENSSL_free(row[i]); + } } + OPENSSL_free(row); } void Ssl::CertificateDb::Row::reset() @@ -135,7 +154,7 @@ free(row[cell]); } if (value) { - row[cell] = static_cast(malloc(sizeof(char) * (strlen(value) + 1))); + row[cell] = static_cast(OPENSSL_malloc(sizeof(char) * (strlen(value) + 1))); memcpy(row[cell], value, sizeof(char) * (strlen(value) + 1)); } else row[cell] = NULL; @@ -146,6 +165,55 @@ return row; } +void Ssl::CertificateDb::sq_TXT_DB_delete(TXT_DB *db, const char **row) +{ + if (!db) + return; + +#if OPENSSL_VERSION_NUMBER >= 0x1000004fL + for (int i = 0; i < sk_OPENSSL_PSTRING_num(db->data); ++i) { + const char ** current_row = ((const char **)sk_OPENSSL_PSTRING_value(db->data, i)); +#else + for (int i = 0; i < sk_num(db->data); ++i) { + const char ** current_row = ((const char **)sk_value(db->data, i)); +#endif + if (current_row == row) { + sq_TXT_DB_delete_row(db, i); + return; + } + } +} + +#define countof(arr) (sizeof(arr)/sizeof(*arr)) +void Ssl::CertificateDb::sq_TXT_DB_delete_row(TXT_DB *db, int idx) +{ + char **rrow; +#if OPENSSL_VERSION_NUMBER >= 0x1000004fL + rrow = (char **)sk_OPENSSL_PSTRING_delete(db->data, idx); +#else + rrow = (char **)sk_delete(db->data, idx); +#endif + + if (!rrow) + return; + + Row row(rrow, cnlNumber); // row wrapper used to free the rrow + + const Columns db_indexes[]={cnlSerial, cnlName}; + for (unsigned int i = 0; i < countof(db_indexes); ++i) { + void *data = NULL; +#if OPENSSL_VERSION_NUMBER >= 0x1000004fL + if (LHASH_OF(OPENSSL_STRING) *fieldIndex = db->index[db_indexes[i]]) + data = lh_OPENSSL_STRING_delete(fieldIndex, rrow); +#else + if (LHASH *fieldIndex = db->index[db_indexes[i]]) + data = lh_delete(fieldIndex, rrow); +#endif + if (data) + assert(data == rrow); + } +} + unsigned long Ssl::CertificateDb::index_serial_hash(const char **a) { const char *n = a[Ssl::CertificateDb::cnlSerial]; @@ -219,13 +287,24 @@ } row.setValue(cnlSerial, serial_string.c_str()); char ** rrow = TXT_DB_get_by_index(db.get(), cnlSerial, row.getRow()); - if (rrow != NULL) - return false; + if (rrow != NULL) { + // TODO: check if the stored row is valid. + return true; + } { TidyPointer subject(X509_NAME_oneline(X509_get_subject_name(cert.get()), NULL, 0)); - if (pure_find(subject.get(), cert, pkey)) + Ssl::X509_Pointer findCert; + Ssl::EVP_PKEY_Pointer findPkey; + if (pure_find(subject.get(), findCert, findPkey)) { + // Replace with database certificate + cert.reset(findCert.release()); + pkey.reset(findPkey.release()); return true; + } + // pure_find may fail because the entry is expired, or because the + // certs file is corrupted. Remove any entry with given hostname + deleteByHostname(subject.get()); } // check db size while trying to minimize calls to size() @@ -235,8 +314,10 @@ // there are no more invalid ones, but there must be valid certificates do { - if (!deleteOldestCertificate()) + if (!deleteOldestCertificate()) { + save(); // Some entries may have been removed. Update the index file. return false; // errors prevented us from freeing enough space + } } while (size() > max_db_size); break; } @@ -250,13 +331,22 @@ row.setValue(cnlName, subject.get()); } - if (!TXT_DB_insert(db.get(), row.getRow())) + if (!TXT_DB_insert(db.get(), row.getRow())) { + // failed to add index (???) but we may have already modified + // the database so save before exit + save(); return false; - + } + rrow = row.getRow(); row.reset(); + std::string filename(cert_full + "/" + serial_string + ".pem"); - if (!writeCertAndPrivateKeyToFile(cert, pkey, filename.c_str())) + if (!writeCertAndPrivateKeyToFile(cert, pkey, filename.c_str())) { + //remove row from txt_db and save + sq_TXT_DB_delete(db.get(), (const char **)rrow); + save(); return false; + } addSize(filename); save(); @@ -382,10 +472,8 @@ if (rrow == NULL) return false; - if (!sslDateIsInTheFuture(rrow[cnlExp_date])) { - deleteByHostname(rrow[cnlName]); + if (!sslDateIsInTheFuture(rrow[cnlExp_date])) return false; - } // read cert and pkey from file. std::string filename(cert_full + "/" + rrow[cnlSerial] + ".pem"); @@ -485,26 +573,10 @@ } // Normally defined in defines.h file -#define countof(arr) (sizeof(arr)/sizeof(*arr)) void Ssl::CertificateDb::deleteRow(const char **row, int rowIndex) { const std::string filename(cert_full + "/" + row[cnlSerial] + ".pem"); -#if OPENSSL_VERSION_NUMBER >= 0x1000004fL - sk_OPENSSL_PSTRING_delete(db.get()->data, rowIndex); -#else - sk_delete(db.get()->data, rowIndex); -#endif - - const Columns db_indexes[]={cnlSerial, cnlName}; - for (unsigned int i = 0; i < countof(db_indexes); ++i) { -#if OPENSSL_VERSION_NUMBER >= 0x1000004fL - if (LHASH_OF(OPENSSL_STRING) *fieldIndex = db.get()->index[db_indexes[i]]) - lh_OPENSSL_STRING_delete(fieldIndex, (char **)row); -#else - if (LHASH *fieldIndex = db.get()->index[db_indexes[i]]) - lh_delete(fieldIndex, row); -#endif - } + sq_TXT_DB_delete_row(db.get(), rowIndex); subSize(filename); int ret = remove(filename.c_str()); === modified file 'src/ssl/certificate_db.h' --- src/ssl/certificate_db.h 2012-03-16 00:52:11 +0000 +++ src/ssl/certificate_db.h 2012-11-26 08:26:04 +0000 @@ -81,6 +81,8 @@ public: /// Create row wrapper. Row(); + ///Create row wrapper for row with width items + Row(char **row, size_t width); /// Delete all row. ~Row(); void setValue(size_t number, char const * value); ///< Set cell's value in row @@ -123,6 +125,11 @@ bool deleteOldestCertificate(); ///< Delete oldest certificate. bool deleteByHostname(std::string const & host); ///< Delete using host name. + /// Removes the first matching row from TXT_DB. Ignores failures. + static void sq_TXT_DB_delete(TXT_DB *db, const char **row); + /// Remove the row on position idx from TXT_DB. Ignores failures. + static void sq_TXT_DB_delete_row(TXT_DB *db, int idx); + /// Callback hash function for serials. Used to create TXT_DB index of serials. static unsigned long index_serial_hash(const char **a); /// Callback compare function for serials. Used to create TXT_DB index of serials.