Bugfixes #1157

rrran · 2017-08-03T09:43:00Z

Lots of PHPDOC fixes
Some method visibility fixes
Method signature fixes
And more

PHPDOC fix

PHPDOC fixes

Fixed methods visibility

GrahamCampbell · 2017-08-03T21:06:19Z

phpseclib/Net/SFTP.php

@@ -3086,7 +3086,7 @@ public function getSupportedVersions()
     * @return bool
     * @access private
     */
-    private function disconnect_helper($reason)
+    protected function disconnect_helper($reason)


Please explain this?

SFTP class extends SSH2 and overrides method disconnect_helper($reason), also it calls parent's method:

protected function disconnect_helper($reason) { $this->pwd = false; parent::disconnect_helper($reason); }

Parent's method was declared as private, so could not be called outside the class.
I changed it's visibility to protected.
Also I have to change visibility of the successor's method.

GrahamCampbell · 2017-08-03T21:06:45Z

phpseclib/Net/SSH2.php

@@ -652,7 +652,7 @@ class SSH2
     * @see self::_get_channel_packet()
     * @access private
     */
-    private $curTimeout;
+    protected $curTimeout;


Please minimize these changes to those that are required, since the phpdoc changes are bug fixes. :)

terrafrost

It looks like the only reason this is needed is because of the ASN1::decodeLength. I did grep -r decodeLength . and it looks like it's not actually being used. ASN1::encodeLength is being used but not ASN1::decodeLength.

It looks like decodeLength ceased being used when I refactored the PKCS1/8 plugins in the master branch to facilitate re-use. At that point they used ASN1.php instead of having their own independent pseudo implementation of ASN1.

Overall, I'm thinking it'd probably just be better if decodeLength() was deleted

terrafrost · 2017-08-05T18:10:10Z

phpseclib/File/X509.php

@@ -2796,7 +2797,7 @@ public function setEndDate($date)
        */
        if (strtolower($date) == 'lifetime') {
            $temp = '99991231235959Z';
-            $temp = chr(ASN1::TYPE_GENERALIZED_TIME) . Functions::encodeLength(strlen($temp)) . $temp;
+            $temp = chr(ASN1::TYPE_GENERALIZED_TIME) . ASN1::encodeLength(strlen($temp)) . $temp;


Nice catch! I'll have to add a unit test for this

terrafrost · 2017-08-05T18:30:29Z

phpseclib/File/X509.php

    {
-        return $this->getExtensionHelper($id, $cert);
+        return $this->getExtensionHelper($id, $cert, $path);


Looks like a unit test will need to be written for this too. This is only called with a third parameter by getRevokedCertificateExtension.

getExtensions (plural vs singular) probably ought to be updated similarly, as well, and then getRevokedCertificateExtensions updated to call getExtensions instead of getExtensionsHelper.

Also, spaces ought to be added before and after the = in the $path=null bit. PSR-1/2 do appear to discuss this but in areas where those standards do not provide clarity I'd say, as a general rule of thumb, the PEAR standards apply. Quoting https://pear.php.net/manual/en/standards.funcalls.php ,

As displayed above, there should be one space on either side of an equals sign used to assign the return value of a function to a variable.

idk how well the phpseclib codebase actually conforms to these standards but, anyway, I can take care of this when I merge.

terrafrost

What's the motivation behind this change?

edit: it doesn't seem like this comment is linked to the specific commit so I made a new comment from where the specific change I'm referring to is more clear.

terrafrost · 2017-08-05T19:04:42Z

phpseclib/Math/BigInteger/Engines/PHP/Base.php

@@ -80,7 +80,7 @@ public static function isValidEngine()
     * @param string $class
     * @return \phpseclib\Math\BigInteger\Engines\PHP
     */
-    protected static function powModHelper(PHP $x, PHP $e, PHP $n, $class)
+    public static function powModHelper(PHP $x, PHP $e, PHP $n, $class)


What's the motivation behind this change?

File PHP.php
method:

protected function powModInner(PHP $e, PHP $n)
{
try {
$class = static::$modexpEngine;
return $class::powModHelper($this, $e, $n, static::class);
} catch (\Exception $err) {
return PHP\DefaultEngine::powModHelper($this, $e, $n, static::class);
}
}

As you can see here's a static call which can't be performed if the powModHelper method is not public

Engines/PHP/Base extends Engines/PHP so visibility shouldn't be an issue. Indeed, if visibility were an issue then one would think that changing the visibility from protected to private wouldn't break any unit tests but it does:

https://travis-ci.org/terrafrost/phpseclib/jobs/261813738

terrafrost · 2017-08-05T19:09:10Z

phpseclib/Math/BigInteger/Engines/BCMath/Base.php

@@ -60,7 +60,7 @@ public static function isValidEngine()
     * @param string $class
     * @return \phpseclib\Math\BigInteger\Engines\BCMath
     */
-    protected static function powModHelper(BCMath $x, BCMath $e, BCMath $n, $class)
+    public static function powModHelper(BCMath $x, BCMath $e, BCMath $n, $class)


You made a similar change in f314f97#r131527744 . I don't understand the motivation for that change nor do I understand it for this change.

File PHP.php
method:

protected function powModInner(PHP $e, PHP $n)
{
try {
$class = static::$modexpEngine;
return $class::powModHelper($this, $e, $n, static::class);
} catch (\Exception $err) {
return PHP\DefaultEngine::powModHelper($this, $e, $n, static::class);
}
}

As you can see here's a static call which can't be performed if the powModHelper method is not public

terrafrost · 2017-08-05T19:18:30Z

The changes look good to me!

I guess I have a few things I need to do:

Delete ASN1::decodeLength per Bugfixes #1157 (review)
Write a unit test for X509::setEndDate per Bugfixes #1157 (comment)
Write a unit test for X509::getRevokedCertificateExtension and make some changes in X509::getExtensions and getRevokedCertificateExtensions per Bugfixes #1157 (comment)
There's a call to $this->disconnectHepler in SSH2.php - that should be changed to $this->disconnect_helper.

A few questions I have that it'd be cool if you could answer or do:

Why did you change the visibility of powModHelper from protected to public in both the PHP and BCMath engines per f314f97#r131527744 and 6f36c49#r131527812?

Thanks!

terrafrost · 2017-08-06T17:42:09Z

phpseclib/File/X509.php

     * @access public
     * @return mixed
     */
-    public function getExtension($id, $cert = null)
+    public function getExtension($id, $cert = null, $path)


I think that should be $path = null. As is not all the calls to getExtension have a third parameter:

$issuerAltName = $this->getExtension('id-ce-subjectAltName', $issuer->currentCert);

Also, that's how it's defined with getExtensionHelper:

private function getExtensionHelper($id, $cert = null, $path = null)

Never mind this - it looks like you fixed this specific issue with eb21fb2

terrafrost · 2017-08-06T22:37:47Z

The changes I mention that I'd do have been done here:

https://github.com/terrafrost/phpseclib/tree/pre-1157-changes

This branch has failing unit tests that you're changes should fix. Just LMK about the powModHelper thing and then I'll merge.

Thanks!

Sokolovskyy Roman added 15 commits August 3, 2017 09:11

Set of PHPDOC fixes

167718a

PHPDOC fix

5fe61b3

added missing use statement

0c6d860

PHPDOC fix

Wrong class name fix

f48995a

PHPDOC fixes

Set of PHPDOC fixes

a3b2521

Set of PHPDOC fixes

5583703

Set of PHPDOC fixes

6181b2b

Set of PHPDOC fixes

ccfb6c9

Set of PHPDOC fixes

0fd58ae

Fixed methods visibility

Fixed methods visibility

f314f97

PHPDOC fixes

d13f429

Fixed methods visibility

6f36c49

Set of PHPDOC fixes

7bffa6b

Fixed wrong use of new self(1);

de2e466

Default value for a parameter missed

eb21fb2

GrahamCampbell requested changes Aug 3, 2017

View reviewed changes

terrafrost reviewed Aug 5, 2017

View reviewed changes

terrafrost reviewed Aug 6, 2017

View reviewed changes

This was referenced Aug 7, 2017

Use the actual PHP test suite for validating mcrypt_compat behavior phpseclib/mcrypt_compat#9

Open

Unit tests fixes #1164

Closed

terrafrost merged commit eb21fb2 into phpseclib:master Aug 8, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Bugfixes #1157

Bugfixes #1157

rrran commented Aug 3, 2017

GrahamCampbell Aug 3, 2017

rrran Aug 4, 2017 •

edited by terrafrost

Loading

GrahamCampbell Aug 3, 2017

terrafrost left a comment

terrafrost Aug 5, 2017

terrafrost Aug 5, 2017

terrafrost Aug 5, 2017

terrafrost left a comment •

edited

Loading

terrafrost Aug 5, 2017

rrran Aug 7, 2017

terrafrost Aug 7, 2017

terrafrost Aug 5, 2017

rrran Aug 7, 2017

terrafrost commented Aug 5, 2017 •

edited

Loading

terrafrost Aug 6, 2017

terrafrost Aug 6, 2017

terrafrost commented Aug 6, 2017

Bugfixes #1157

Bugfixes #1157

Conversation

rrran commented Aug 3, 2017

Choose a reason for hiding this comment

rrran Aug 4, 2017 • edited by terrafrost Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

terrafrost left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

terrafrost left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

terrafrost commented Aug 5, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

terrafrost commented Aug 6, 2017

rrran Aug 4, 2017 •

edited by terrafrost

Loading

terrafrost left a comment •

edited

Loading

terrafrost commented Aug 5, 2017 •

edited

Loading