From 4ef68b195fbb294a5e4b03e5485c2092430cd53f Mon Sep 17 00:00:00 2001 From: Jan Henning Date: Sun, 3 Nov 2024 00:37:04 +0100 Subject: [PATCH 1/4] Fix off-by-one error in SearchContext.restartAt(FileInfo) code The problem is that in order to do the comparison between the file name from the iterator and the passed-in FileInfo, we always need to consume the respective Path from the DirectoryStream iterator, so even when the comparison declares success, we have already consumed that Path. This means that the next FIND_NEXT2 call will skip that entry and start one Path too late when it accesses this search context again. For correctness, we therefore have to rewind the iterator by one after having found the correct Path, so that the next nextFileInfo() call restarts at the right point. The current solution isn't the best for large directories performance-wise due to having to iterate twice over all files up to the resume point, but at least it is correct and better than omitting the first file from every FIND_NEXT2 response (or equivalent split-up Find request reply for SMB2). --- .../java/org/filesys/smb/server/disk/JavaNIOSearchContext.java | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/main/java/org/filesys/smb/server/disk/JavaNIOSearchContext.java b/src/main/java/org/filesys/smb/server/disk/JavaNIOSearchContext.java index 7b0bcd0..166f6ab 100644 --- a/src/main/java/org/filesys/smb/server/disk/JavaNIOSearchContext.java +++ b/src/main/java/org/filesys/smb/server/disk/JavaNIOSearchContext.java @@ -438,7 +438,8 @@ public boolean restartAt(FileInfo info) { while ( curPath != null && restartOK == false) { if ( curPath.getFileName().toString().equalsIgnoreCase(info.getFileName())) { - restartOK = true; + // Rewind by one, so the next call to nextFileInfo will return the correct item + restartOK = restartAt(m_idx - 1); } else { curPath = m_pathIter.next(); From 43a548f2768f17955299252912ba9536d2dcf368 Mon Sep 17 00:00:00 2001 From: Jan Henning Date: Mon, 4 Nov 2024 19:26:35 +0100 Subject: [PATCH 2/4] Have FileInfo.resetInfo() and copyFrom() handle the short name, too Since I'm adding another caller for FileInfo.copyFrom(), I've had a look at that method and it seems that we ought to handle m_shortname there, too. And while we're at it, same thing for resetInfo(), too. --- src/main/java/org/filesys/server/filesys/FileInfo.java | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/main/java/org/filesys/server/filesys/FileInfo.java b/src/main/java/org/filesys/server/filesys/FileInfo.java index 44a6742..86f592d 100644 --- a/src/main/java/org/filesys/server/filesys/FileInfo.java +++ b/src/main/java/org/filesys/server/filesys/FileInfo.java @@ -639,6 +639,7 @@ public final FileType isFileType() { */ public final void resetInfo() { m_name = ""; + m_shortName = null; m_path = null; m_size = 0L; @@ -666,6 +667,7 @@ public final void resetInfo() { */ public final void copyFrom(FileInfo finfo) { m_name = finfo.getFileName(); + m_shortName = finfo.getShortName(); m_path = finfo.getPath(); m_size = finfo.getSize(); From 517f074ea41fd43b296594c8729e87f6023cbddd Mon Sep 17 00:00:00 2001 From: Jan Henning Date: Sun, 3 Nov 2024 19:27:30 +0100 Subject: [PATCH 3/4] Cache most recently retrieved FileInfo object to speed up directory listing The problem with the way our search code is implemented is that when we need to split up a large search response (with lots of files) into multiple packets, at the boundary between each packet we need to retrieve the FileInfo object for the same file twice - once to find out that it doesn't fit into the previous packet anymore, and a second time to actually transmit it in the followup packet. The protocol handler therefore calls restartAt() on the SearchContext in order to effectively rewind it by one entry, so that the next call to nextFileInfo() during the subsequent response packet returns the correct FileInfo entry (compare also the previous commit). With the NIO files API, this turns into a problem, because the Streams-based iterator cannot be rewound, so every time we need to backtrack by one entry, we need to reiterate through all the directory's contents up to the desired restart point. (And even worse, the restartAt(FileInfo)-based method needs actually needs to iterate twice for every call.) For large directories with thousands of files (or more), this turns into a very noticeable overhead when listing the directory contents. To work around this issue, we now cache the last returned FileInfo object, and check in restartAt(FileInfo) whether the call corresponds to the common case of going back by merely one entry. If so, instead of expensively rewinding the iterator, we simply set the next call to nextFileInfo() to return the previously cached FileInfo object and only subsequently to resume iterating normally through the directory. --- .../smb/server/disk/JavaNIOSearchContext.java | 37 ++++++++++++++++--- 1 file changed, 32 insertions(+), 5 deletions(-) diff --git a/src/main/java/org/filesys/smb/server/disk/JavaNIOSearchContext.java b/src/main/java/org/filesys/smb/server/disk/JavaNIOSearchContext.java index 166f6ab..c4dd0f6 100644 --- a/src/main/java/org/filesys/smb/server/disk/JavaNIOSearchContext.java +++ b/src/main/java/org/filesys/smb/server/disk/JavaNIOSearchContext.java @@ -46,6 +46,10 @@ public class JavaNIOSearchContext extends SearchContext { private Iterator m_pathIter; private int m_idx; + // Help speed up a common restartAt(FileInfo info) call pattern + private FileInfo m_cachedFileInfo; + private boolean m_restartFromCache; + // File attributes private int m_attr; @@ -74,6 +78,11 @@ public int getResumeId() { return m_idx; } + private void resetIndex() { + m_idx = 0; + m_cachedFileInfo = null; + } + /** * Determine if there are more files to return for this search * @@ -87,7 +96,7 @@ public boolean hasMoreFiles() { else if (m_stream == null || m_pathIter == null) return false; else if ( m_pathIter != null) - return m_pathIter.hasNext(); + return m_pathIter.hasNext() || m_restartFromCache && m_cachedFileInfo != null; return true; } @@ -104,7 +113,7 @@ public final void initSinglePathSearch( Path singlePath) { // Indicate this is a single file/folder search setSingleFileSearch( true); - m_idx = 0; + resetIndex(); m_stream = null; m_pathIter = null; @@ -131,7 +140,7 @@ public final void initWildcardSearch( Path folderPath, int attr, WildCard wildCa // Indicate a multi-file search setSingleFileSearch( false); - m_idx = 0; + resetIndex(); // Start the folder search m_stream = Files.newDirectoryStream(m_root); @@ -223,6 +232,11 @@ public boolean nextFileInfo(FileInfo info) { infoValid = true; } } + else if (m_restartFromCache == true && m_cachedFileInfo != null) { + m_restartFromCache = false; + info.copyFrom(m_cachedFileInfo); + infoValid = true; + } else if (m_pathIter != null && m_pathIter.hasNext()) { // Find the next file/folder that matches the search attributes @@ -329,11 +343,14 @@ else if (fname.equalsIgnoreCase("Desktop.ini") || // Indicate that the file information is valid infoValid = true; + + m_cachedFileInfo = info; } } } catch ( IOException ex) { infoValid = false; + m_cachedFileInfo = null; } // Return the file information valid state @@ -355,6 +372,10 @@ public String nextFileName() { else return null; } + else if (m_restartFromCache == true && m_cachedFileInfo != null) { + m_restartFromCache = false; + return m_cachedFileInfo.getFileName(); + } else if ( m_pathIter != null && m_pathIter.hasNext()) { // Find the next matching file name @@ -397,7 +418,7 @@ public boolean restartAt(int resumeId) { return false; } - m_idx = 0; + resetIndex(); while ( m_pathIter.hasNext() && m_idx != resumeId) { m_pathIter.next(); @@ -420,9 +441,15 @@ public boolean restartAt(FileInfo info) { boolean restartOK = false; if (m_stream != null) { + if (m_cachedFileInfo != null && m_cachedFileInfo == info) { + // Avoid expensively reiterating the whole directory if we're asked to merely go + // back to the immediately previously retrieved entry. + m_restartFromCache = true; + return true; + } // Restart the stream iterator and find the required restart path - m_idx = 0; + resetIndex(); try { m_stream = Files.newDirectoryStream(m_root); From 5882813457a0d5980d499c67a0ca65042c19eb35 Mon Sep 17 00:00:00 2001 From: Jan Henning Date: Mon, 4 Nov 2024 17:59:06 +0100 Subject: [PATCH 4/4] Avoid leaking unclosed DirectoryStreams from searches --- .../smb/server/disk/JavaNIOSearchContext.java | 17 +++++++++++++++++ 1 file changed, 17 insertions(+) diff --git a/src/main/java/org/filesys/smb/server/disk/JavaNIOSearchContext.java b/src/main/java/org/filesys/smb/server/disk/JavaNIOSearchContext.java index c4dd0f6..8c44c61 100644 --- a/src/main/java/org/filesys/smb/server/disk/JavaNIOSearchContext.java +++ b/src/main/java/org/filesys/smb/server/disk/JavaNIOSearchContext.java @@ -411,6 +411,7 @@ public boolean restartAt(int resumeId) { // Restart the iteration and skip to the required position try { + m_stream.close(); m_stream = Files.newDirectoryStream(m_root); m_pathIter = m_stream.iterator(); } @@ -452,6 +453,7 @@ public boolean restartAt(FileInfo info) { resetIndex(); try { + m_stream.close(); m_stream = Files.newDirectoryStream(m_root); m_pathIter = m_stream.iterator(); } @@ -513,4 +515,19 @@ public final void setRelativePath(String relPath) { if (m_relPath != null && m_relPath.endsWith(FileName.DOS_SEPERATOR_STR) == false) m_relPath = m_relPath + FileName.DOS_SEPERATOR_STR; } + + /** + * Close the search. + */ + public void closeSearch() { + if (m_stream != null) { + try { + m_stream.close(); + } + catch (IOException unused) { + } + } + super.closeSearch(); + } + }