Ticket #3581 (closed defect: fixed)

Opened 4 years ago

Last modified 4 years ago

[PATCH] sftp: fix connection memleaks

Reported by: and Owned by: andrew_b
Priority: major Milestone: 4.8.16
Component: mc-vfs Version: master
Keywords: Cc:
Blocked By: Blocking:
Branch state: merged Votes for changeset: committed-master

Description

1) open_connection abort not handled correct yet
2) missing libssh2_session_free() on close_connection
3) internal list from libssh2_userauth_list() must not freed by mc
4) cosmetic: re-order open_connection and close_connection steps
5) cosmetic: sftpfs_super_data created at sftpfs_cb_open_connection() was freed at sftpfs_close_connection()

it should be sftpfs_cb_close_connection() for logical right location

6) add FIXME for deprected libssh2_session_startup() since libssh2 1.2.8

found by Clang/AddressSanitizer?

Change History

Changed 4 years ago by and

comment:1 Changed 4 years ago by andrew_b

Is it possible to split this patch into several ones with independent issues?

comment:2 Changed 4 years ago by and

sure, I will do so.

Changed 4 years ago by and

Changed 4 years ago by and

Changed 4 years ago by and

Changed 4 years ago by and

Changed 4 years ago by and

comment:3 Changed 4 years ago by andrew_b

  • Status changed from new to accepted
  • Owner set to andrew_b
  • Component changed from mc-core to mc-vfs
  • Branch state changed from no branch to on review

Thanks!

Branch: 3581_sftp_fixes
Initial changeset:fa4056b405a47935aa9fd21370d0d0fe92b41d7a

comment:4 follow-up: ↓ 5 Changed 4 years ago by zaytsev

Hi Andreas,

Maybe you will find it interesting to have a look at this Debian bug, which also has to do with SFTP:

https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=774135

So, in essence, the user is running Debian x32 (!) and there SFTP is completely broken. Probably this happens due to some type size weirdness on x32, because we can't reproduce it on i386 and amd64.

--Yury

comment:5 in reply to: ↑ 4 Changed 4 years ago by andrew_b

Replying to zaytsev:

https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=774135

So, in essence, the user is running Debian x32 (!) and there SFTP is completely broken. Probably this happens due to some type size weirdness on x32, because we can't reproduce it on i386 and amd64.

I've added fix of config file reading. I don't think that it fixes that bug, but who knows.

comment:6 Changed 4 years ago by zaytsev

Some new info from the Debian bug, with 4.8.15 it looks like this:

$ touch .ssh/config
$ mc
$ cd /sftp://localhost

╔═════════════════════════════════════════ Error ═════════════════════════════════════════╗
║                                                                                         ║
║ 25: sftp: an error occurred while reading ~/.ssh/config: Inappropriate ioctl for device ║
║                                                                                         ║
╚═════════════════════════════════════════════════════════════════════════════════════════╝

╔═══════════════ Error ════════════════╗
║                                      ║
║ Cannot chdir to "/sftp://localhost/" ║
║           File exists (17)           ║
║                                      ║
╚══════════════════════════════════════╝

Asked to test with the latest 3581_sftp_fixes branch...

comment:7 Changed 4 years ago by and

Can Thorsten test following patch?

when reading ssh_config with matching host entry mc do bad pointer arithmetic

comment:8 Changed 4 years ago by andrew_b

Ticket #3590 has been marked as a duplicate of this ticket.

comment:9 Changed 4 years ago by andrew_b

  • Votes for changeset set to andrew_b
  • Branch state changed from on review to approved

comment:10 Changed 4 years ago by andrew_b

  • Status changed from accepted to testing
  • Votes for changeset changed from andrew_b to committed-master
  • Resolution set to fixed
  • Branch state changed from approved to merged

Merged to master: [ab0c2afba5a142c6091bd40d913c75f4669f1280].

git log --pretty=oneline 9dcf033..ab0c2af

comment:11 Changed 4 years ago by andrew_b

  • Status changed from testing to closed
Note: See TracTickets for help on using tickets.