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

Memory leak with consecutive ftp_nb_fget #10562

Closed
nielsdos opened this issue Feb 10, 2023 · 0 comments
Closed

Memory leak with consecutive ftp_nb_fget #10562

nielsdos opened this issue Feb 10, 2023 · 0 comments

Comments

@nielsdos
Copy link
Member

nielsdos commented Feb 10, 2023

Description

The following code:

<?php

include "ext/ftp/tests/server.inc";

$conn_id = ftp_connect("127.0.0.1", $port) or die("no connection to ftp");

ftp_login($conn_id, 'anonymous', 'IEUser@');

$fout = fopen("test-output.tmp", "w");
$ret = ftp_nb_fget($conn_id, $fout, "fget", FTP_BINARY, 10);
$ret = ftp_nb_fget($conn_id, $fout, "fget", FTP_BINARY, 10);
fclose($fout);

Resulted in this output:

Warning: ftp_nb_fget(): Closing data Connection. in /home/niels/php-src/memleak.php on line 11
[Fri Feb 10 21:53:49 2023]  Script:  '/home/niels/php-src/memleak.php'
/home/niels/php-src/ext/ftp/ftp.c(1631) :  Freeing 0x00007f3176889000 (4108 bytes), script=/home/niels/php-src/memleak.php
=== Total 1 memory leaks detected ===

But I expected this output instead:

Warning: ftp_nb_fget(): Closing data Connection. in /home/niels/php-src/memleak.php on line 11

Seems like the ftp->data pointer gets overwritten before the previous action is done(? not sure, I haven't looked into detail). In any case the pointer is overwritten and the old one isn't freed.
Discovered while developing PR #10525.

There's actually a couple of places where ftp->data could get overwritten I think. Maybe we shouldn't allow doing two non-blocking actions at the same time?

PHP Version

PHP >=8.1

Operating System

Linux

@nielsdos nielsdos self-assigned this Jul 6, 2023
nielsdos added a commit to nielsdos/php-src that referenced this issue Jul 6, 2023
…b_fget

When the user does not fully consume the data stream, but instead opens
a new one, a memory leak occurs. Moreover, the state is invalid: when
more commands arrive they'll be handled out-of-sync because the state of
the client does not match what the server is doing.
This leads to all sorts of weirdness, for example:
  Warning: ftp_nb_fget(): OK.

Fix it by gracefully closing the old data stream when a new data stream
is started.
nielsdos added a commit that referenced this issue Jul 7, 2023
* PHP-8.1:
  Fix GH-10562: Memory leak and invalid state with consecutive ftp_nb_fget
nielsdos added a commit that referenced this issue Jul 7, 2023
* PHP-8.2:
  Fix GH-10562: Memory leak and invalid state with consecutive ftp_nb_fget
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant