Skip to content

[Mailer] Handle null error_get_last() to prevent array offset error on SMTP auth failure #59351

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

Closed
skmedix opened this issue Jan 2, 2025 · 3 comments

Comments

@skmedix
Copy link
Contributor

skmedix commented Jan 2, 2025

Symfony version(s) affected

6.4.13

Description

Since #53157, when we attempt to send an email and SMTP fails to authenticate due to an unexpected response code (e.g. "Too many failed login attempts"), it attempts to retrieve an error message from the error_get_last() function, even if the returned value is null. This results in an Trying to access array offset on value of type null error, rather than providing an explanation as to why an email was not sent.

How to reproduce

I'll try to make a reproducer in the next few days if it's still needed

Possible Solution

From my internal testing, only throwing $this->getReadConnectionDescription() when error_get_last() equals null correctly fallbacks to a correct error message.

Additional Context

No response

@xabbuh
Copy link
Member

xabbuh commented Jan 3, 2025

This reminds me of #58650. We can reopen it, but I would still like to understand what we are missing here (see the discussion in the linked PR).

@skmedix
Copy link
Contributor Author

skmedix commented Jan 3, 2025

@xabbuh After looking into it, it seems the issue is very specific and only happens on Windows. I couldn't recreate it with Mailpit, so I set an environment variable to use Mailtrap with bad credentials.

Minimal reproducer: https://github.com/skmedix/symfony-issue-59351/blob/master/.github/workflows/smtp-runner.yaml

Failed run: https://github.com/skmedix/symfony-issue-59351/actions/runs/12597218959/job/35109776421

@skmedix
Copy link
Contributor Author

skmedix commented Jan 7, 2025

Puzzle solved. Debugged this even deeper, as it kept bugging me! So I persisted to find out. It took me a couple of hours, but I discovered that the error_get_last() is NULL on both systems, but this warning does not appear on Linux because the line 92 is never reached there, whereas it is on Windows. The problem comes with the stream_get_meta_data function (here) and it's return value, while on Linux the last command returns [eof => true], but on Windows it's [eof => false]. That's why on Linux the exception is thrown earlier on $metas['eof'], while on Windows the code goes further and throws a warning because error_get_last() is null.

Last stream_get_meta_data var_dump diff:

-   ["eof"]=>
-   bool(true)
+   ["eof"]=>
+   bool(false)

I tried changing $metas['eof'] to use \feof($this->out) and this solution works both on windows and linux, I guess now I can make a PR :)

Edit: Here's a run with PR #59404 applied as a patch. Both steps (test and retest) show the results before and after applying the fix:
https://github.com/skmedix/symfony-issue-59351/actions/runs/12659627195/job/35279113129#annotation:6:19
https://github.com/skmedix/symfony-issue-59351/actions/runs/12659627195/job/35279113129#annotation:9:22

nicolas-grekas added a commit that referenced this issue Jan 9, 2025
…feof() (skmedix)

This PR was merged into the 6.4 branch.

Discussion
----------

[Mailer] Fix SMTP stream EOF handling on Windows by using feof()

| Q             | A
| ------------- | ---
| Branch?       | 6.4
| Bug fix?      | yes
| New feature?  | no <!-- please update src/**/CHANGELOG.md files -->
| Deprecations? | no <!-- please update UPGRADE-*.md and src/**/CHANGELOG.md files -->
| Issues        | Fix #59351
| License       | MIT

<!--
Replace this notice by a description of your feature/bugfix.
This will help reviewers and should be a good start for the documentation.

Additionally (see https://symfony.com/releases):
 - Always add tests and ensure they pass.
 - Bug fixes must be submitted against the lowest maintained branch where they apply
   (lowest branches are regularly merged to upper ones so they get the fixes too).
 - Features and deprecations must be submitted against the latest branch.
 - For new features, provide some code snippets to help understand usage.
 - Changelog entry should follow https://symfony.com/doc/current/contributing/code/conventions.html#writing-a-changelog-entry
 - Never break backward compatibility (see https://symfony.com/bc).
-->

Full explanation of this change here: #59351 (comment)
Help with tests would be appreciated.

Commits
-------

ed18c7c [Mailer] Fix SMTP stream EOF handling on Windows by using feof()
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants