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 undefined index notice in stream touch() #1615

Closed
wants to merge 3 commits into from
Closed

fix undefined index notice in stream touch() #1615

wants to merge 3 commits into from

Conversation

stingray-11
Copy link
Contributor

I was getting a PHP notice in Stream.php. It would happen when using touch( ) with the stream wrapper and no atime/mtime given.

vendor/phpseclib/phpseclib/phpseclib/Net/SFTP/Stream.php(450): Undefined offset: 0

@terrafrost
Copy link
Member

terrafrost commented Feb 22, 2021

It's not clear to me under what circumstances the two optional parameters wouldn't be set. I tried calling touch() with one parameter and stream_metadata was called with three parameters. Sample code:

<?php

class VariableStream {
    public function __call($name, $arguments)
    {
        echo $name . "\n";
        var_dump($arguments);
        return false;
    }
}

stream_wrapper_register("var", "VariableStream");
    
touch("var://myvar");

https://3v4l.org/UeVTQ

If these parameters aren't being passed to stream_metadata under as yet unknown conditions then we should probably make the second parameter default to time() since that's what https://www.php.net/touch by default does. The third parameter would also need to be set to whatever the second parameter was set to if it wasn't explicitly set.

But what I just don't understand is why it's not getting all three parameters for you whereas in the 3v4l.org thing it's getting all three parameters in every single version of PHP that they have...

@stingray-11
Copy link
Contributor Author

stingray-11 commented Feb 22, 2021

I use both PHP 7.4 and 8.0 for development, not sure which I was using at the time. I'll do more digging.

I am using Ubuntu 20.10 with PHP builds from http://ppa.launchpad.net/ondrej/php/ubuntu/ and Nginx FPM.

This change, while seeming like it should be unnecessary, is at least a safe one that shouldn't cause any side effects. The actual touch() implementation in SFTP.php looks like it takes care of the params being null. so we're good there.

    public function touch($filename, $time = null, $atime = null)
    {
        .......
        if (!isset($time)) {
            $time = time();
        }
        if (!isset($atime)) {
            $atime = $time;
        }

@stingray-11
Copy link
Contributor Author

stingray-11 commented Feb 22, 2021

Also it looks like in the link you sent, it's displaying the behavior I described.

It says the arguments it's sending are...


array(3) {
  [0]=> string(11) "var://myvar"
  [1]=> int(1)
  [2]=> array(0) {  }
}

The third argument (the array with zero length) is what becomes $var in stream_metadata() so it makes sense that you would get a notice about indices 0 and 1 being undefined.

@terrafrost
Copy link
Member

Oh right - good call on both points.

Also it looks like in the link you sent, it's displaying the behavior I described.

I think I just saw that it was two parameters and assumed that the two parameters were atime and mtime after a super quick glance. Sorry about that!

Anyway, I'm going to squash this into one commit, cherry-pick that one commit to the 1.0 branch and then merge up to master.

@terrafrost
Copy link
Member

See 488db53

Thanks!

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.

None yet

2 participants