Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix DNS error crash #3376

Merged
merged 3 commits into from
Jun 4, 2024
Merged

Fix DNS error crash #3376

merged 3 commits into from
Jun 4, 2024

Conversation

analogic
Copy link
Collaborator

@analogic analogic commented Jun 4, 2024

First idea how to fix #3375

  • slight rewrite of code around get_mx and making get_mx_respond async

Checklist:

  • docs updated
  • tests updated
  • Changes updated

@analogic analogic requested a review from msimerson June 4, 2024 18:42
@msimerson
Copy link
Member

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
Copy link
Collaborator Author

analogic commented Jun 4, 2024

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
Copy link
Member

msimerson commented Jun 4, 2024

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)
+        })
     }

@msimerson msimerson merged commit 45d8b3d into haraka:master Jun 4, 2024
13 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

queryMx critical error
2 participants