Skip to content

[Mailer] Stream timeout not detected due to checking only string result of function fgets #48074

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.

Sign up for GitHub

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

Jump to bottom
Closed
Sezil opened this issue Nov 2, 2022 · 0 comments
Closed

[Mailer] Stream timeout not detected due to checking only string result of function fgets #48074

Sezil opened this issue Nov 2, 2022 · 0 comments
Labels
Bug Mailer Status: Needs Review

Comments

@Sezil
Copy link
Contributor

Sezil commented Nov 2, 2022 •
edited
Loading

Symfony version(s) affected

5.4

Description

If stream timeouts on some network that is blocking the connection, PHP function fgets(resource $stream, ?int $length = null): string|false returns false instead of empty string '' so the 'timed_out' metadata check will not be executed and you will end up with the error Expected response code "220" but got empty code..
AbstractStream.php#L77

$line = fgets($this->out);
if ('' === $line) {
    $metas = stream_get_meta_data($this->out);
    if ($metas['timed_out']) {
        throw new TransportException(sprintf('Connection to "%s" timed out.', $this->getReadConnectionDescription()));
    }
    if ($metas['eof']) {
        throw new TransportException(sprintf('Connection to "%s" has been closed unexpectedly.', $this->getReadConnectionDescription()));
    }
}

How to reproduce

In my case, there is a blocked port 587 so when I try to connect to any XOauth2 SMTP servers it will wait for the amount of time set by the timeout setting and then throw Expected response code "220" but got empty code. instead of Connection to "SMTP" timed out..

Since the PHP documentation clearly states that fgets can return false I don't see the point in trying to reproduce it, stream metadata should be checked even if $line = false.

Possible Solution

stream metadata should be checked even if $line = false.

$line = fgets($this->out);
if ('' === $line || false === $line) {
    $metas = stream_get_meta_data($this->out);
    if ($metas['timed_out']) {
        throw new TransportException(sprintf('Connection to "%s" timed out.', $this->getReadConnectionDescription()));
    }
    if ($metas['eof']) {
        throw new TransportException(sprintf('Connection to "%s" has been closed unexpectedly.', $this->getReadConnectionDescription()));
    }
}

Additional Context

EXCEPTION
Class: Symfony\Component\Mailer\Exception\TransportException
Message: Expected response code "220" but got empty code.
Code: 0
Stack trace:

  • 0 ...\vendor\symfony\mailer\Transport\Smtp\SmtpTransport.php(252): Symfony\Component\Mailer\Transport\Smtp\SmtpTransport->assertResponseCode('', Array)
  • 1 ...\vendor\symfony\mailer\Transport\Smtp\SmtpTransport.php(194): Symfony\Component\Mailer\Transport\Smtp\SmtpTransport->start()
  • 2 ...\vendor\symfony\mailer\Transport\AbstractTransport.php(71): Symfony\Component\Mailer\Transport\Smtp\SmtpTransport->doSend(Object(Symfony\Component\Mailer\SentMessage))
@Sezil Sezil added the Bug label Nov 2, 2022
@Sezil Sezil mentioned this issue Nov 2, 2022
Merged
fabpot added a commit that referenced this issue Nov 4, 2022
@fabpot
bug #48075 [Mailer] Stream timeout not detected fgets returns false (…
538491b 
…Sezil)

This PR was submitted for the 5.4 branch but it was merged into the 4.4 branch instead.

Discussion
----------

[Mailer] Stream timeout not detected fgets returns false

#48074 [Mailer] Stream timeout not detected due to checking only string result of function fgets

| Q             | A
| ------------- | ---
| Branch?       | 6.2 for features / 4.4, 5.4, 6.0 or 6.1 for bug fixes <!-- see below -->
| Bug fix?      | yes/no
| New feature?  | yes/no <!-- please update src/**/CHANGELOG.md files -->
| Deprecations? | yes/no <!-- please update UPGRADE-*.md and src/**/CHANGELOG.md files -->
| Tickets       | Fix #... <!-- prefix each issue number with "Fix #", no need to create an issue if none exist, explain below instead -->
| License       | MIT
| Doc PR        | symfony/symfony-docs#... <!-- required for new features -->
<!--
Replace this notice by a short README for 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).
-->

Commits
-------

486f2c5 [Mailer] Stream timeout not detected due to checking only string result of function fgets
@fabpot fabpot closed this as completed Nov 4, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
Bug Mailer Status: Needs Review
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
3 participants
@fabpot @carsonbot @Sezil