haraka / Haraka

A fast, highly extensible, and event driven SMTP server
https://haraka.github.io
MIT License
4.91k stars 653 forks source link

Fix DNS error crash #3376

Closed analogic closed 1 month ago

analogic commented 1 month ago

First idea how to fix #3375

Checklist:

msimerson commented 1 month ago

Looks fine. Other than changing to async/await, it should do exactly the same thing as the current code, right? Does this change actually make any difference?

analogic commented 1 month ago

The ".catch(this.get_mx_err)" line is the problem because the callback doesn't have "this" context and therefore fails on the first line with this.lognotice(). Am I right? Even if I am not, this fixes the problem!

msimerson commented 1 month ago

Am I right?

Nope. Yes, and maybe. The .catch line was indeed the problem, and this does have context there, but as you say, the called function doesn't have this. The immediate problem was calling a non-existing function:

index b9c02d80..cf7d3b17 100644
--- a/outbound/hmail.js
+++ b/outbound/hmail.js
@@ -231,7 +231,7 @@ class HMailItem extends events.EventEmitter {
                 this.bounce(`Nowhere to deliver mail to for domain: ${this.todo.domain}`);
             }
         })
-        .catch(this.get_mx_err)
+        .catch(this.get_mx_error)
     }

     get_mx_error (err) {

Which, when fixed, yields this error:

[CRIT] [-] [core] TypeError: Cannot read properties of undefined (reading 'lognotice')
[CRIT] [-] [core]     at get_mx_error (/Users/matt/git/haraka/Haraka/outbound/hmail.js:238:14)

In order for get_mx_err to also have this context, it must be called this way:

--- a/outbound/hmail.js
+++ b/outbound/hmail.js
@@ -231,7 +231,9 @@ class HMailItem extends events.EventEmitter {
                 this.bounce(`Nowhere to deliver mail to for domain: ${this.todo.domain}`);
             }
         })
-        .catch(this.get_mx_err)
+        .catch((err) => {
+            this.get_mx_error(err)
+        })
     }