Improper Cleanup on Thrown Exception

Description

Improper Cleanup on Thrown Exception is a vulnerability where the product does not clean up its state or incorrectly cleans up its state when an exception is thrown, leading to unexpected state or control flow. When code becomes complex and resource cleanup is needed at multiple points, exceptions can disrupt normal control flow and prevent necessary cleanup operations from occurring. This leaves the application in an inconsistent state with resources not properly released, locks not freed, or flags not reset.

Risk

Improper exception cleanup leads to resource exhaustion, security vulnerabilities, and unpredictable application behavior. Unreleased locks can cause deadlocks in concurrent applications. Unreleased resources accumulate causing denial of service. Security-critical state like authentication flags may remain set after failed operations, potentially allowing unauthorized access. Transaction state left incomplete can cause data corruption. The risk is compounded in long-running server applications where leaked resources accumulate over time.

Solution

Ensure cleanup happens when breaking out of loops or exiting functions via exceptions. Use try-finally blocks (or equivalent language constructs like try-with-resources in Java, using statements in C#, context managers in Python) to guarantee cleanup code executes regardless of exceptions. Use RAII (Resource Acquisition Is Initialization) patterns in C++ to tie resource lifetime to object scope. Design cleanup code to be idempotent so it can safely run multiple times. Consider using exceptions conservatively rather than relying on them for normal flow control.

Common Consequences

Impact	Details
Other	Scope: Other Unexpected State - The code could be left in a bad state with resources unreleased, locks held, or flags incorrectly set.
Availability	Scope: Availability DoS: Resource Consumption - Resources not released on exception accumulate, eventually causing resource exhaustion.
Access Control	Scope: Access Control Gain Privileges - Security-critical state not properly reset on exception may allow unauthorized access.

Example Code

Vulnerable Code

// Vulnerable: Lock not released on exception
public class VulnerableLockHandler {

    private boolean threadLock = false;

    public void processData(Data data) throws ProcessingException {
        // Acquire lock
        threadLock = true;

        try {
            // Processing that may throw
            validateData(data);
            transformData(data);
            storeData(data);

            // Vulnerable: Only releases lock on success
            threadLock = false;

        } catch (ValidationException e) {
            // Vulnerable: threadLock remains true!
            throw new ProcessingException("Validation failed", e);
        } catch (TransformException e) {
            // Vulnerable: threadLock remains true!
            throw new ProcessingException("Transform failed", e);
        }
        // If storeData() throws, lock is never released
    }
}

# Vulnerable: Database transaction not rolled back on exception
class VulnerableTransactionHandler:
    def update_records(self, records):
        self.db.begin_transaction()

        try:
            for record in records:
                self.validate_record(record)  # May throw
                self.db.update(record)        # May throw

            self.db.commit()

        except ValidationError as e:
            # Vulnerable: Transaction not rolled back
            # Database left in inconsistent state
            raise

        except DatabaseError as e:
            # Vulnerable: Partial updates committed
            # or transaction left open
            raise

    def authenticate_user(self, username, password):
        self.auth_in_progress = True
        self.current_user = username

        try:
            user = self.lookup_user(username)  # May throw
            valid = self.check_password(user, password)  # May throw

            if valid:
                self.authenticated = True
            else:
                self.authenticated = False

        except UserNotFoundError:
            # Vulnerable: auth_in_progress and current_user not reset
            raise

        except Exception:
            # Vulnerable: State variables left set
            raise

        finally:
            # This finally block is missing!
            pass

        self.auth_in_progress = False
        # Never reached if exception thrown

// Vulnerable: File handle and memory leaked on error
#include <stdio.h>
#include <stdlib.h>
#include <setjmp.h>

jmp_buf exception_env;

typedef struct {
    char* data;
    size_t size;
} FileContent;

FileContent* vulnerable_read_file(const char* path) {
    FILE* file = fopen(path, "r");
    if (!file) {
        longjmp(exception_env, 1);  // "Throw" exception
    }

    FileContent* content = malloc(sizeof(FileContent));
    if (!content) {
        // Vulnerable: file handle leaked
        longjmp(exception_env, 2);
    }

    fseek(file, 0, SEEK_END);
    content->size = ftell(file);
    fseek(file, 0, SEEK_SET);

    content->data = malloc(content->size);
    if (!content->data) {
        // Vulnerable: content struct leaked, file handle leaked
        longjmp(exception_env, 3);
    }

    if (fread(content->data, 1, content->size, file) != content->size) {
        // Vulnerable: all resources leaked
        longjmp(exception_env, 4);
    }

    fclose(file);
    return content;
}

// Vulnerable: Connection and transaction not cleaned up
public class VulnerableOrderProcessor {

    private SqlConnection connection;
    private SqlTransaction transaction;
    private bool processingActive = false;

    public void ProcessOrder(Order order) {
        processingActive = true;

        connection = new SqlConnection(connectionString);
        connection.Open();
        transaction = connection.BeginTransaction();

        try {
            ValidateOrder(order);      // May throw
            ReserveInventory(order);   // May throw
            ChargePayment(order);      // May throw
            CompleteOrder(order);      // May throw

            transaction.Commit();
            processingActive = false;
            connection.Close();

        } catch (ValidationException ex) {
            // Vulnerable: Transaction not rolled back
            // Connection not closed
            // processingActive still true
            throw;
        } catch (PaymentException ex) {
            // Vulnerable: Inventory reserved but not released
            // Transaction partially complete
            throw;
        }
    }
}

Fixed Code

// Fixed: Lock properly released using try-finally
public class SecureLockHandler {

    private boolean threadLock = false;

    public void processData(Data data) throws ProcessingException {
        // Acquire lock
        threadLock = true;

        try {
            validateData(data);
            transformData(data);
            storeData(data);

        } catch (ValidationException e) {
            throw new ProcessingException("Validation failed", e);

        } catch (TransformException e) {
            throw new ProcessingException("Transform failed", e);

        } finally {
            // Fixed: Lock always released
            threadLock = false;
        }
    }

    // Fixed: Using ReentrantLock for better control
    private final ReentrantLock lock = new ReentrantLock();

    public void processDataWithLock(Data data) throws ProcessingException {
        lock.lock();
        try {
            validateData(data);
            transformData(data);
            storeData(data);
        } finally {
            lock.unlock();  // Always released
        }
    }
}

# Fixed: Proper cleanup on exception
class SecureTransactionHandler:
    def update_records(self, records):
        self.db.begin_transaction()

        try:
            for record in records:
                self.validate_record(record)
                self.db.update(record)

            self.db.commit()

        except Exception:
            # Fixed: Always rollback on exception
            self.db.rollback()
            raise

    # Fixed: Using context manager pattern
    @contextmanager
    def transaction(self):
        self.db.begin_transaction()
        try:
            yield
            self.db.commit()
        except Exception:
            self.db.rollback()
            raise

    def update_records_with_context(self, records):
        with self.transaction():
            for record in records:
                self.validate_record(record)
                self.db.update(record)

    def authenticate_user(self, username, password):
        self.auth_in_progress = True
        self.current_user = username

        try:
            user = self.lookup_user(username)
            valid = self.check_password(user, password)

            if valid:
                self.authenticated = True
            else:
                self.authenticated = False
                self.current_user = None

        except Exception:
            # Fixed: State reset on any exception
            self.authenticated = False
            self.current_user = None
            raise

        finally:
            # Fixed: Always reset in-progress flag
            self.auth_in_progress = False

// Fixed: Proper cleanup using goto pattern
#include <stdio.h>
#include <stdlib.h>

typedef struct {
    char* data;
    size_t size;
    int error;
    char error_msg[256];
} FileResult;

FileResult secure_read_file(const char* path) {
    FileResult result = {NULL, 0, 0, ""};
    FILE* file = NULL;
    char* data = NULL;

    file = fopen(path, "r");
    if (!file) {
        result.error = 1;
        snprintf(result.error_msg, sizeof(result.error_msg),
                 "Cannot open file");
        goto cleanup;
    }

    fseek(file, 0, SEEK_END);
    result.size = ftell(file);
    fseek(file, 0, SEEK_SET);

    data = malloc(result.size);
    if (!data) {
        result.error = 2;
        snprintf(result.error_msg, sizeof(result.error_msg),
                 "Memory allocation failed");
        goto cleanup;
    }

    if (fread(data, 1, result.size, file) != result.size) {
        result.error = 3;
        snprintf(result.error_msg, sizeof(result.error_msg),
                 "Read failed");
        goto cleanup;
    }

    result.data = data;
    data = NULL;  // Transfer ownership

cleanup:
    // Fixed: Always cleanup resources
    if (data) {
        free(data);
    }
    if (file) {
        fclose(file);
    }

    return result;
}

// Fixed: C++ RAII approach
#ifdef __cplusplus
class FileHandle {
    FILE* file;
public:
    FileHandle(const char* path, const char* mode)
        : file(fopen(path, mode)) {}

    ~FileHandle() {
        if (file) fclose(file);
    }

    FILE* get() { return file; }
    operator bool() { return file != nullptr; }
};

std::vector<char> secure_read_file_cpp(const char* path) {
    FileHandle file(path, "r");  // RAII: automatically closed
    if (!file) {
        throw std::runtime_error("Cannot open file");
    }

    // Even if exception thrown, file is closed by destructor
    fseek(file.get(), 0, SEEK_END);
    size_t size = ftell(file.get());
    fseek(file.get(), 0, SEEK_SET);

    std::vector<char> data(size);
    if (fread(data.data(), 1, size, file.get()) != size) {
        throw std::runtime_error("Read failed");
    }

    return data;
}
#endif

// Fixed: Proper cleanup using try-finally and using
public class SecureOrderProcessor {

    public void ProcessOrder(Order order) {
        bool processingActive = false;
        SqlConnection connection = null;
        SqlTransaction transaction = null;
        bool inventoryReserved = false;

        try {
            processingActive = true;

            connection = new SqlConnection(connectionString);
            connection.Open();
            transaction = connection.BeginTransaction();

            ValidateOrder(order, transaction);

            ReserveInventory(order, transaction);
            inventoryReserved = true;

            ChargePayment(order, transaction);
            CompleteOrder(order, transaction);

            transaction.Commit();

        } catch (Exception) {
            // Fixed: Compensating actions for partial completion
            if (inventoryReserved && transaction != null) {
                try {
                    ReleaseInventory(order, transaction);
                } catch { /* Log but don't mask original exception */ }
            }

            // Fixed: Rollback transaction
            if (transaction != null) {
                try {
                    transaction.Rollback();
                } catch { /* Log but don't mask original exception */ }
            }

            throw;

        } finally {
            // Fixed: Always cleanup
            processingActive = false;

            transaction?.Dispose();
            connection?.Close();
            connection?.Dispose();
        }
    }

    // Fixed: Using pattern for cleaner code
    public void ProcessOrderWithUsing(Order order) {
        using (var connection = new SqlConnection(connectionString))
        using (var transaction = connection.BeginTransaction()) {
            try {
                connection.Open();

                ValidateOrder(order, transaction);
                ReserveInventory(order, transaction);
                ChargePayment(order, transaction);
                CompleteOrder(order, transaction);

                transaction.Commit();

            } catch {
                transaction.Rollback();
                throw;
            }
        }
        // Connection and transaction automatically disposed
    }
}

---

CVE Examples

No specific CVEs are listed in the MITRE database for this CWE. However, the pattern is common in:

• Database transaction handling vulnerabilities
• Lock management errors in concurrent applications
• Resource cleanup failures in exception paths

---

References

1. MITRE Corporation. "CWE-460: Improper Cleanup on Thrown Exception." https://cwe.mitre.org/data/definitions/460.html
2. CERT Oracle Secure Coding Standard for Java. "ERR05-J. Do not let checked exceptions escape from a finally block."