Use of @ to suppress ssh function calls - why?

[ProfessorGram]

Description of the bug

In /core/includes/filetransfer/ssh.inc I am noticing the use of "@" to suppress function call error messages? Wondering why?
I don't know if error suppression is considered a "bug" or just a cultural choice. Normally, error suppression is discouraged.

A short description of what the problem is.

Look at the source file and you will see two instances of @ being used to suppress calls to ssh2 library member functions

https://github.com/backdrop/backdrop/blob/1.x/core/includes/filetransfer/ssh.inc

$this->connection = @ssh2_connect($this->hostname, $this->port);

if (!@ssh2_auth_password($this->connection, $this->username, $this->password)) {

Steps To Reproduce

To reproduce the behavior:

1. Go to '...'
2. Click on '....'
3. Scroll down to '....'

Actual behavior

Describe what was the result of following the steps above. Add screenshots if
you think they would help.

Nothing displayed due to error suppression

Expected behavior

Describe what you expected to happen instead.
Add mockup screenshots if possible.

I think error messages shouldn't normally be suppressed.
If they need to be, an explanatory comment would suffice, to help explain why.

Additional information

Add any other information that could help, such as:

• Backdrop CMS version:
• Web server and its version:
• PHP version:
• Database sever (MySQL or MariaDB?) and its version:
• Operating System and its version:
• Browser(s) and their versions:

Vanilla Backdrop CMS installation

[avpaderno]

The calls to ssh2_connect() and ssh2_auth_password() are in the core/includes/filetransfer/ssh.inc file.

In that file, the error suppression operator is used because an exception is instead thrown.

    $this->connection = @ssh2_connect($this->hostname, $this->port);
    if (!$this->connection) {
      throw new FileTransferException('SSH Connection failed to @host:@port', NULL, array('@host' => $this->hostname, '@port' => $this->port));
    }
    if (!@ssh2_auth_password($this->connection, $this->username, $this->password)) {
      throw new FileTransferException('The supplied username/password combination was not accepted.');
    }

    if (!@ssh2_scp_send($this->connection, $source, $destination)) {
      throw new FileTransferException('Cannot copy @source_file to @destination_file.', NULL, array('@source' => $source, '@destination' => $destination));
    }

```php
    if (@!ssh2_exec($this->connection, 'cp -Rp ' . escapeshellarg($source) . ' ' . escapeshellarg($destination))) {
      throw new FileTransferException('Cannot copy directory @directory.', NULL, array('@directory' => $source));
    }

```php
    if ($output = @ssh2_exec($this->connection, $cmd)) {
      if ($output == 'yes') {
        return TRUE;
      }
      return FALSE;
    } else {
      throw new FileTransferException('Cannot check @path.', NULL, array('@path' => $path));
    }

In these cases, the error is replaced from an exception, which is a better way to handle a runtime error.

[avpaderno]

Looking at the PHP documentation for the SSH2 functions, I do not see any emitted warning documented for those functions, which is instead documented, for example, in file().

It could also mean that, in PHP 5.6, those functions can emit warnings, and that is no longer documented because PHP 5.6 is not supported.

[ProfessorGram]

Thank you @avpaderno - as always a very high quality response.

What about the error message, which seems to not be replacing the labels? Any insight there?

[ProfessorGram]

I took a look at the same documentation. Error just returns NULL.

Return Values:
Returns a resource on success, or false on error.

But what did catch my eye was a sample connection:

$connection = ssh2_connect('shell.example.com', 22, $methods, $callbacks);

In the Backdrop CMS call, I can confirm the values: $this->hostname is "localhost" and $this->port is "22".

I did this by writing them into the string, and also inspecting the $this object directly (which btw has a cleartext password)

I just wanted to note the fact that the sample call featured 4 parameters and their purpose, not 2.

Finally, I do not see any notation as to whether $methods or $callback may be omitted. Do you?

[avpaderno]

The warning for FileTransferSSH::$connection is caused by the fact FileTransfer::__get() expects the used property to be $connectionHandle.

  if ($name == 'connection') {
    $this->connect();
    return $this->connectionHandle;
  }

  if ($name == 'chroot') {
    $this->setChroot();
    return $this->chrootPath;
  }

This is different from suppressing warnings that apparently are not raised by SSH2 functions. It should be reported in a different issue, but still be resolved (if I did not misunderstand the used code).

[avpaderno]

Actually, that warning should not be thrown. The magic __set() method uses the following code.

  public function __set($name, $value) {
    if ($name == 'connection') {
      $this->connectionHandle = $value;
    }
    elseif ($name == 'chroot') {
      $this->chrootPath = $value;
    }
  }

When a class sets $this->connection, __set() is called and sets $this->connectionHandle, which is an existing property.

[avpaderno]

I was able to reproduce the warning with two classes containing similar code used by the FileTransfer and FileTransferSSH classes, at least with PHP 8.3 installed on my computer.

On https://3v4l.org/, the same warning/notice is output also from earlier PHP version, including PHP 5.0.0.

[ProfessorGram]

So are we saying that the code is basically non-functional for a long time now? In your opinion, did it ever work?

[avpaderno]

A warning about an undefined property does not mean the code does not work.

[ProfessorGram]

I note here that the function as implemented in Drupal 9 does not conform physically to that in the Drupal 7 implementation, and some parameters appear to have been changed (replacing NULL with 0, for example, and updated array notation):

public function connect() {
    $this->connection = @ssh2_connect($this->hostname, $this->port);
    if (!$this->connection) {
        throw new FileTransferException('SSH Connection failed to @host:@port', 0, [
            '@host' => $this->hostname,
            '@port' => $this->port,
        ]);
    }
    if (!@ssh2_auth_password($this->connection, $this->username, $this->password)) {
        throw new FileTransferException('The supplied username/password combination was not accepted.');
    }
}

Source:

https://api.drupal.org/api/drupal/core%21lib%21Drupal%21Core%21FileTransfer%21SSH.php/function/SSH%3A%3Aconnect/9

I am still wondering why the placeholders @host:@port end up being displayed to screen and not replaced with the supplied values, which cosmetically look OK, and when transposed into the string directly function properly.

[ProfessorGram]

[ProfessorGram]

Additional color:

Undefined properties previously triggered an E_NOTICE, however this was upgraded to E_WARNING as part of the PHP 8.0 Engine Warnings RFC: https://wiki.php.net/rfc/engine_warnings - By the time this RFC would come into effect, if passed, undefined properties would have emitted a warning for 5+ years.

[ProfessorGram]

I can confirm that the FileTransferSSH Object contains the needed information:

FileTransferSSH Object
(
    [username:protected] => ********** (obfuscated, but correct)
    [password:protected] => ********** (obfuscated, but correct)
    [hostname:protected] => localhost
    [port:protected] => 22
    [jail:protected] => /var/www/vhosts/contrib2.com/web/site
    [chrootPath:FileTransfer:private] => 
    [connectionHandle:FileTransfer:private] => 
)

I do not know if these properties are supposed to have values, pre-connection, or not:

[chrootPath:FileTransfer:private] => 
[connectionHandle:FileTransfer:private] => 

[ProfessorGram]

Error is:

Warning: Undefined property: FileTransferSSH::$connection in FileTransferSSH->connect()
(line 27 of /var/www/vhosts/contrib2.com/web/site/core/includes/filetransfer/ssh.inc)

Here's what I think is happening:

The connect() method is failing because it is not getting the connection parameters it needs.

The very nature of the connection (SSH) is mentioned in the actual Object name (FileTransferSSH) but that doesn't mean it really knows that it is an SSH session builder. What it does know is that it cannot access its own connection property (by undefined, does that mean exists, is_blank or isnull?)

The connect() attempt then fails because the object cannot somehow access its own properties, which contain the (blank) connectionHandle and (populated) host. port, username and password properties but no property named connection far as I can see.

Perhaps this is a scope/visibility issue. Drupal 9 mentions the fact that nested __get calls now fail w.r.t this route:

Problem/Motivation
#3308744: Fix magic connection property access from \Drupal\Core\FileTransfer\FileTransfer::__get() Introduced magic methods to access Drupal\Core\FileTransfer\FTPExtension->connection however now FTPExtension->connect() always throws an exception (which is caught) due to nested calls to __get() .

Perhaps this workflow (applicable to FTP) can shed some light on the situation:

Cannot connect to FTP Server, check settings

Why:

1. Call FTPExtension->connect(); in code.
2. FTPExtension->connect() sets the $connection property
3. $connection is set via parent class FileTransfer's private property $connectionHandle via __set()
4. FTPExtension->connect() then checks if $connection property was set to non-Falsey value.
5. This triggers FileTransfer's __get() which calls FTPExtension->connect()
6. Repeat step 2 ( FTPExtension->connect() sets the $connection property)
7. Repeat step 3 ( $connection is set via parent class FileTransfer's private property $connectionHandle via __set() )
8. Repeat step 4 ( FTPExtension->connect() then checks if $connection property was set to non-Falsey value.)
9. FileTransfer's __get() is not triggered as __get() has already been called in the call stack with the same parameters and so PHP treats it as regular property which is undefined (since the real property is stored in private property $connectionHandle ), so the else condition is triggered which is throwing an exception. (If it didn't it, would be an infinite loop which is equally bad)

[ProfessorGram]

Another weird thing is the property being accessed is FileTransferSSH::$connection

$connection?

Is it normal that the $ is exposed?

I don't know if a variable variable is allowed in an object property.

I do know that I cannot change it from $connectionHandle to $$connection in code. It fails.

[ProfessorGram]

The following code sample produces a success message

    // TEST CODE
    $connection = ssh2_connect($this->hostname, $this->port);

    if (ssh2_auth_password($connection, $this->username, $this->password)) {
      echo "connection is authenticated";
    }
    else {
      echo "failed!";
    }

[ProfessorGram]

I have a suspicion that what is breaking is not ssh2_connect() but rather the absence of ssh2_auth_password();

<?php
  $connection = ssh2_connect('shell.example.com', 22);
  ssh2_auth_password($connection, 'username', 'password');
  $stream = ssh2_exec($connection, '/usr/local/bin/php -i');
?>
```

[avpaderno]

Another weird thing is the property being accessed is FileTransferSSH::$connection

PHP warnings or error messages always use that format for a property, as if the property were a static property. That is not different from showing a method like FileTransferSSH::connect().

I would focus on using the error-suppressing operator, which should eventually be removed, if those functions do not throw any warning.

[ProfessorGram]

I will look into this more. If you choose to do so as well, that would be grand.