Skip to content

Commit 6edd449

Browse files
funkyshuCopilot
andauthored
Fixed typed nil pointer bug in sftp backend. Fixes #274. (#275)
* Fixed typed nil pointer bug in sftp backend. Fixes #274. * fix version in changelog * Update backend/sftp/fileSystem.go Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> --------- Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
1 parent 97e7933 commit 6edd449

File tree

3 files changed

+217
-1
lines changed

3 files changed

+217
-1
lines changed

CHANGELOG.md

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,10 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
66

77
## [Unreleased]
88

9+
## [v7.8.1] - 2025-08-07
10+
### Fixed
11+
- Fixed typed nil pointer bug in sftp backend. Fixes #274.
12+
913
## [v7.8.0] - 2025-08-06
1014
### Added
1115
- updated github actions workflows to support multi-modules.

backend/sftp/concurrency_test.go

Lines changed: 211 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,211 @@
1+
package sftp
2+
3+
import (
4+
"fmt"
5+
"io"
6+
"sync"
7+
"testing"
8+
"time"
9+
10+
"github.com/stretchr/testify/suite"
11+
"golang.org/x/crypto/ssh"
12+
13+
"github.com/c2fo/vfs/v7/utils/authority"
14+
)
15+
16+
// SFTPConcurrencyTestSuite tests typed nil handling and concurrency robustness
17+
// in the SFTP backend under failure scenarios
18+
type SFTPConcurrencyTestSuite struct {
19+
suite.Suite
20+
originalClientGetter func(authority.Authority, Options) (Client, io.Closer, error)
21+
}
22+
23+
func TestSFTPConcurrencyTestSuite(t *testing.T) {
24+
suite.Run(t, new(SFTPConcurrencyTestSuite))
25+
}
26+
27+
func (s *SFTPConcurrencyTestSuite) SetupTest() {
28+
// Save the original client getter (which might be mocked by other tests)
29+
s.originalClientGetter = defaultClientGetter
30+
31+
// Reset to the real client getter to force actual SFTP connection attempts
32+
defaultClientGetter = func(auth authority.Authority, opts Options) (Client, io.Closer, error) {
33+
return GetClient(auth, opts)
34+
}
35+
}
36+
37+
func (s *SFTPConcurrencyTestSuite) TearDownTest() {
38+
// Restore the original client getter
39+
defaultClientGetter = s.originalClientGetter
40+
}
41+
42+
// TestClientTypedNilHandling tests that the Client() method properly handles
43+
// typed nil pointers that can occur when client creation fails
44+
func (s *SFTPConcurrencyTestSuite) TestClientTypedNilHandling() {
45+
// Create filesystem with invalid configuration to trigger connection failures
46+
fs := NewFileSystem(WithOptions(Options{
47+
Username: "nonexistentuser",
48+
Password: "wrongpassword",
49+
KnownHostsCallback: ssh.InsecureIgnoreHostKey(), //nolint:gosec // Test code - insecure host key acceptable
50+
}))
51+
52+
// Use a non-existent host to ensure connection failures
53+
auth, err := authority.NewAuthority("sftp://nonexistentuser@nonexistent-host:22")
54+
s.Require().NoError(err)
55+
56+
// Multiple attempts should fail gracefully without panics
57+
for i := 0; i < 3; i++ {
58+
client, err := fs.Client(auth)
59+
s.T().Logf("Attempt %d: client=%v, err=%v", i+1, client, err)
60+
if err == nil {
61+
s.T().Logf("Client type: %T, is nil: %v", client, client == nil)
62+
}
63+
s.Assert().Error(err, "Should get connection error due to invalid configuration")
64+
}
65+
}
66+
67+
// TestConcurrentFailedConnections tests that multiple goroutines attempting
68+
// to create failed connections don't cause panics or race conditions
69+
func (s *SFTPConcurrencyTestSuite) TestConcurrentFailedConnections() {
70+
// Create filesystem with invalid configuration
71+
fs := NewFileSystem(WithOptions(Options{
72+
Username: "testuser",
73+
Password: "wrongpassword",
74+
KnownHostsCallback: ssh.InsecureIgnoreHostKey(), //nolint:gosec // Test code - insecure host key acceptable
75+
}))
76+
77+
auth, err := authority.NewAuthority("sftp://testuser@nonexistent-host:22")
78+
s.Require().NoError(err)
79+
80+
// Start multiple goroutines that will all fail to connect
81+
const numGoroutines = 10
82+
var wg sync.WaitGroup
83+
panicChan := make(chan interface{}, numGoroutines)
84+
errorChan := make(chan error, numGoroutines)
85+
86+
for i := 0; i < numGoroutines; i++ {
87+
wg.Add(1)
88+
go func() {
89+
defer wg.Done()
90+
defer func() {
91+
if r := recover(); r != nil {
92+
panicChan <- r
93+
}
94+
}()
95+
96+
// This should fail but not panic
97+
_, err := fs.Client(auth)
98+
errorChan <- err
99+
}()
100+
}
101+
102+
wg.Wait()
103+
close(panicChan)
104+
close(errorChan)
105+
106+
// Collect any panics that occurred
107+
var panics []interface{}
108+
for panic := range panicChan {
109+
panics = append(panics, panic)
110+
}
111+
112+
// Collect errors
113+
var errors []error
114+
for err := range errorChan {
115+
errors = append(errors, err)
116+
}
117+
118+
// Assert that no panics occurred
119+
s.Assert().Empty(panics, "No goroutines should panic")
120+
121+
// Assert that all operations failed with connection errors
122+
s.Assert().NotEmpty(errors, "All operations should fail with connection errors")
123+
for _, err := range errors {
124+
s.Assert().Error(err, "Each operation should return an error")
125+
}
126+
}
127+
128+
// TestClientFailureRobustness simulates multiple failed connection attempts
129+
// to ensure the filesystem remains stable and doesn't panic
130+
func (s *SFTPConcurrencyTestSuite) TestClientFailureRobustness() {
131+
s.Run("Multiple failed connection attempts should not panic", func() {
132+
for i := 0; i < 5; i++ {
133+
s.Run(fmt.Sprintf("Attempt %d", i+1), func() {
134+
// Create a new filesystem for each attempt to avoid state pollution
135+
fs := NewFileSystem(WithOptions(Options{
136+
Username: "testuser",
137+
Password: "wrongpassword", // Wrong password to trigger connection failure
138+
KnownHostsCallback: ssh.InsecureIgnoreHostKey(), //nolint:gosec // Test code - insecure host key acceptable
139+
}))
140+
141+
auth, err := authority.NewAuthority("sftp://testuser@nonexistent-host:22")
142+
s.Require().NoError(err)
143+
144+
// Test multiple client creation attempts that will fail
145+
// This is where the typed nil issue would occur
146+
for j := 0; j < 3; j++ {
147+
_, _ = fs.Client(auth) // This should fail but not panic
148+
}
149+
150+
// Verify that the filesystem is still usable after failures
151+
_, err = fs.Client(auth)
152+
// We expect an error due to wrong credentials, but no panic
153+
s.Assert().Error(err, "Should get connection error due to wrong credentials")
154+
})
155+
}
156+
})
157+
}
158+
159+
// TestTimerCleanupRobustness tests the interaction between the auto-disconnect
160+
// timer and active client operations to ensure no panics occur
161+
func (s *SFTPConcurrencyTestSuite) TestTimerCleanupRobustness() {
162+
// Create filesystem with very short auto-disconnect to trigger timer quickly
163+
fs := NewFileSystem(WithOptions(Options{
164+
Username: "testuser",
165+
Password: "wrongpassword",
166+
KnownHostsCallback: ssh.InsecureIgnoreHostKey(), //nolint:gosec // Test code - insecure host key acceptable
167+
AutoDisconnect: 1, // Very short timeout to trigger timer quickly
168+
}))
169+
170+
auth, err := authority.NewAuthority("sftp://testuser@nonexistent-host:22")
171+
s.Require().NoError(err)
172+
173+
// Start multiple operations that will fail but might trigger the timer
174+
const numOperations = 10
175+
var wg sync.WaitGroup
176+
panicChan := make(chan interface{}, numOperations)
177+
178+
for i := 0; i < numOperations; i++ {
179+
wg.Add(1)
180+
go func() {
181+
defer wg.Done()
182+
defer func() {
183+
if r := recover(); r != nil {
184+
panicChan <- r
185+
}
186+
}()
187+
188+
// Only test client creation to avoid mock conflicts
189+
// This is where the typed nil issue would occur
190+
_, _ = fs.Client(auth)
191+
192+
// Add a small delay to increase chance of timer firing
193+
time.Sleep(100 * time.Millisecond)
194+
195+
// Try another client operation to see if timer cleanup caused issues
196+
_, _ = fs.Client(auth)
197+
}()
198+
}
199+
200+
wg.Wait()
201+
close(panicChan)
202+
203+
// Collect any panics that occurred
204+
var panics []interface{}
205+
for panic := range panicChan {
206+
panics = append(panics, panic)
207+
}
208+
209+
// Assert that no panics occurred
210+
s.Assert().Empty(panics, "Timer cleanup should not cause panics")
211+
}

backend/sftp/fileSystem.go

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,7 @@ import (
55
"io"
66
"os"
77
"path"
8+
"reflect"
89
"sync"
910
"time"
1011

@@ -118,7 +119,7 @@ func (fs *FileSystem) Scheme() string {
118119
func (fs *FileSystem) Client(a authority.Authority) (Client, error) {
119120
// first stop connection timer, if any
120121
fs.connTimerStop()
121-
if fs.sftpclient == nil {
122+
if fs.sftpclient == nil || (reflect.ValueOf(fs.sftpclient).IsValid() && reflect.ValueOf(fs.sftpclient).IsNil()) {
122123
var err error
123124
fs.sftpclient, fs.sshConn, err = defaultClientGetter(a, fs.options)
124125
if err != nil {

0 commit comments

Comments
 (0)