sendEmail invokes defer res.Body.Close() in a naked for-loop but those only get invoked when the function is returning; many emails could cause resource exhaustion #13

odeke-em · 2024-03-20T17:03:51Z

Noticed in a quick code audit that we've got this code

Lines 95 to 102 in b893964

    
           resp, err := client.Client.Do(req) 
        
           if err != nil { 
        
           	log.Error().Msgf("Error sending email: %s", err) 
        
           	return err 
        
           } 
        
           defer resp.Body.Close()

and Go's defer per the specification https://go.dev/ref/spec#Defer_statements runs when the surrounding function is returning or panicking.

The surrounding function in this case is sendEmail

This means that those response handles won't be closed until the for loop is done (if there are many emails to be sent, this could cause resource exhaustion problems)

Fix

We can extract that code into a per-email helper function and even better we can rename the calling function from "sendEmail" to "sendEmails" to signify the purpose, and the extracted code would look like this in a diff

diff --git a/notify_client.go b/notify_client.go
index a30a413..00cf956 100644
--- a/notify_client.go
+++ b/notify_client.go
@@ -92,28 +92,35 @@ func sendEmail(client *NotifyClient, email *NotifyEmail) error {
 		req.Header.Set("Content-Type", contentType)
 		req.Header.Set("Authorization", fmt.Sprintf("ApiKey-v1 %s", client.ApiKey))
 
-		resp, err := client.Client.Do(req)
-
-		if err != nil {
-			log.Error().Msgf("Error sending email: %s", err)
+		if err := doSendEmail(client.Client, req); err != nil {
 			return err
 		}
+	}
+
+	return nil
+}
 
-		defer resp.Body.Close()
+func doSendEmail(client *http.Client, req *http.Request) error {
+	resp, err := client.Do(req)
+	if err != nil {
+		log.Error().Msgf("Error sending email: %s", err)
+		return err
+	}
+	defer resp.Body.Close()
 
-		if resp.StatusCode != 201 {
-			log.Error().Msgf("Unexpected status code: %d", resp.StatusCode)
-			respbody, err := io.ReadAll(resp.Body)
+	if resp.StatusCode == 201 {
+		return nil
+	}
 
-			if err != nil {
-				log.Error().Msgf("Error reading response body: %s", err)
-				return err
-			}
-			log.Error().Msgf("Response: %s", respbody)
+	// A non-201 status code so craft the error.
+	log.Error().Msgf("Unexpected status code: %d", resp.StatusCode)
+	respbody, err := io.ReadAll(resp.Body)
 
-			return fmt.Errorf("unexpected status code: %d", resp.StatusCode)
-		}
+	if err != nil {
+		log.Error().Msgf("Error reading response body: %s", err)
+		return err
 	}
+	log.Error().Msgf("Response: %s", respbody)
 
-	return nil
+	return fmt.Errorf("unexpected status code: %d", resp.StatusCode)
 }

This change extracts per-email sending logic into a helper function for which proper resource closing can be performed. Previously it invoke defer res.Body.Close in a for-loop but Go's specifications and operations dictate that the defer will only be invoked when the surrounding function is returning or panicking: this means then that if very many emails are being sent, that risks potential resource exhaustion because so many connections won't be closed yet nor can they be garbage collected. Fixes cds-snc#13

odeke-em linked a pull request Mar 20, 2024 that will close this issue

fix: properly close HTTP responses after sending each email #14

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

sendEmail invokes defer res.Body.Close() in a naked for-loop but those only get invoked when the function is returning; many emails could cause resource exhaustion #13

sendEmail invokes defer res.Body.Close() in a naked for-loop but those only get invoked when the function is returning; many emails could cause resource exhaustion #13

odeke-em commented Mar 20, 2024

sendEmail invokes defer res.Body.Close() in a naked for-loop but those only get invoked when the function is returning; many emails could cause resource exhaustion #13

sendEmail invokes defer res.Body.Close() in a naked for-loop but those only get invoked when the function is returning; many emails could cause resource exhaustion #13

Comments

odeke-em commented Mar 20, 2024

Fix