The previous fix from e757f7b7 only fixed a small part of the problem (before the parsing result was corrupted, while after the fix file details were not parsed).
https://winscp.net/tracker/1950
Source commit: 2853e4d6abe65217f72ab4f8f840b1a15b1184d6
* Cast between closure types with a functional cast, not reinterpret_cast
A Borland __closure is a built-in (code, data) pair, so reinterpret_cast
between two closure types is allowed. Under standard C++ a closure is a
class, and reinterpret_cast cannot convert one class object into another.
The functional cast is the old C-style cast this line had before
"Refactoring old-style casts" (e5efed895), so C++Builder compiles it as it
did then, and a compiler implementing closures as classes can give it the
same meaning.
* Read the knocked FTP port's IPv4 address as 32 bits
The address is read through an unsigned long pointer. That is 32 bits on
Windows (LLP64), so it reads exactly the four address bytes there; on LP64
targets unsigned long is 64 bits, and the read runs four bytes past the
address into whatever follows it in gethostbyname's buffer. The assignment
to the 32-bit s_addr then keeps the right bytes on little-endian machines,
which is why it goes unnoticed, but the over-read is undefined. ULONG is 32
bits on every Windows compiler, as Windows API layers for POSIX define it
too.
* Compile the synchronize checklist's Explorer file operation on Windows only
TSynchronizeChecklistFileOperation carries out a synchronization checklist
through Explorer's IFileOperation (a Shell COM object), so that local
deletions go to the recycle bin with Explorer's progress and undo. Only
WinSCP's Windows GUI uses it (CustomScpExplorer.cpp). Elsewhere neither the
object nor the interfaces exist, and declaring them only to compile would
pretend to a facility that is not there; a frontend on another platform
implements the operation its own way. The class and its implementation are
guarded with _WIN32, which C++Builder defines, so the Windows build is
unchanged.
* Declare the SIOCATMARK result as ULONG, the type p_ioctlsocket takes
network.c declares ioctlsocket as taking ULONG * (its DECL_WINDOWS_FUNCTION)
but passes a u_long. Both are 32 bits on Windows, so the two agree there. On
an LP64 platform u_long is 64 bits and ULONG 32, so the pointer types differ
and the callee would write half the variable. ULONG is what the declaration
says, and is the same type on Windows.
* Pick aes-ni.c's _mm_setr_epi8 for any GCC-compatible compiler
MM_SETR_EPI8 uses _mm_setr_epi8 only under _WIN64, falling back to a brace
initialiser that is correct for MSVC's __m128i (a byte union) but fills
clang's and GCC's (a two-lane vector) wrongly; the _WIN64 condition fixed
that for Win64 builds only. Any compiler defining __GNUC__ (clang does) now
takes the intrinsic.
* Compile WinSCP's Windows-only additions to neon on Windows only
WinSCP builds neon for Win32 alone, and three of its additions assume it:
ne_openssl.c includes <windows.h> (it uses nothing from it), timed_connect
switches the socket to non-blocking mode with Winsock's ioctlsocket instead
of upstream's fcntl, and ne_sock_sspi_init calls into SSPI unconditionally.
On POSIX neon uses BSD sockets directly, so the include is guarded with
_WIN32, the two ioctlsocket branches are taken only on _WIN32 (upstream's
fcntl code beside them is used otherwise), and ne_sock_sspi_init calls SSPI
under HAVE_SSPI, as ne_sock_init's own call does, reporting failure (-1)
without it -- which WebDAVFileSystem.cpp already handles as SSPI being
unavailable. C++Builder defines _WIN32 and neon's Win32 config.h defines
HAVE_SSPI, so the Windows build is unchanged.
* Use in_addr's s_addr in FileZilla's proxy layer
AsyncProxySocketLayer.cpp reaches the IPv4 address through Winsock's union,
sin_addr.S_un.S_addr. Winsock also defines s_addr as exactly that member
(#define s_addr S_un.S_addr), and s_addr is the name BSD sockets and POSIX
give the field; a POSIX in_addr has no S_un. Same field, so the Windows
build is unchanged.
* Pass FileZilla's two tool strings as UnicodeString, not std::wstring
CFileZillaTools::GetClientString and CustomReason return std::wstring; the
rest of the interface between core and FileZilla, and both sides' own code,
use UnicodeString or wchar_t pointers. Built with a 2-byte wchar_t on a
platform whose C++ library assumes 4 (clang's -fshort-wchar on Linux),
std::wstring is unusable: libstdc++ provides basic_string<wchar_t> as a
precompiled extern template for its own wchar_t, and char_traits<wchar_t>
calls the C library's 4-byte wcslen and wmemcpy: a Linux build of
FtpFileSystem.cpp imported wcslen through it. The values are only built from
UnicodeString, compared with empty() and read with c_str(), so UnicodeString
is a direct replacement (IsEmpty for empty). On Windows, where wchar_t is 2
bytes throughout, behaviour is unchanged.
* Build storage key paths with backslashes on every platform
THierarchicalStorage joins registry and INI key paths (Sessions\My site)
through its IncludeTrailingBackslash and ExcludeTrailingBackslash members,
which defer to the RTL functions of those names. The RTL's add and strip the
platform's path delimiter: a backslash on Windows, a slash on POSIX, where
these functions also build local file paths. Keys are not file paths: their
separator is a backslash everywhere, and the INI file's sections say so. The
members now add and strip a backslash themselves; on Windows that is what
the RTL did, so the behaviour there is unchanged. Without this, on POSIX the
current key became "Sessions/", matched no section, and no stored site
loaded.
* Hand neon a copy of the TLS-init closure, not its two pointers
SetNeonTlsInit stores the closure in the neon session by reinterpreting it
as a TMethod and saving its code and data pointers, and ne_init_ssl_session
rebuilds it with MakeMethod. That relies on a closure being exactly those
two pointers, which only Borland's __closure is; with any other closure type
the saved words are its internals, read from a by-value parameter that is
gone by the time neon calls back, and the rebuilt closure is garbage (a
clang build's first WebDAV-over-TLS connection crashed there). The session
now holds a heap copy of the closure, freed by neon's destroy-session hook
-- which also covers the sessions libs3 creates and destroys itself -- and
ne_init_ssl_session calls that copy. On C++Builder the behaviour is the
same, at the cost of one small allocation per session.
* Take CString's search results as const wchar_t *
CString::Find, ReverseFind and FindOneOf search the const string c_str()
returns and keep the result in a wchar_t *. ISO C++ declares wcschr,
wcsrchr, wcsstr and wcspbrk on a const string as returning const wchar_t *,
as MSVC's and newer mingw-w64 headers (llvm-mingw's) do, so the
initialisation does not compile there. The pointers are only compared and
subtracted: const is all they need.
* Index the string a string-list property returns, not the property
GetLocales takes a character of Exts->Strings[Index]. Under clang the
__declspec(property) standing for TStrings::Strings' __property reads a
second subscript as the property's own second index, so the expression is a
two-argument call to the getter and does not compile (parentheses do not
change that). Read into a UnicodeString first, the second subscript indexes
the string the property returns, as under C++Builder.
---------
Co-authored-by: Eduardo Correia <ecorreia@apliant.com.br>
ConvertPathFromOpenssh turns the forward slashes of an OpenSSH config's
paths into backslashes (GetNormalizedPath), as Windows' dialogs want them.
On POSIX that breaks the path: an imported site's IdentityFile
~/.ssh/id_ed25519 became \home\user\.ssh\id_ed25519, a file that does not
exist.
---------
Co-authored-by: Eduardo Correia <ecorreia@apliant.com.br>
TConfiguration::GetLocalEOLType always says CRLF: core has only run on
Windows, where local text files end their lines so. Text-mode transfers
convert to and from it (SFTP, SCP), so on POSIX a text-mode download gave
local files CRLF line endings, which POSIX programs do not expect, and the
internal editor would have written them back so.
---------
Co-authored-by: Eduardo Correia <ecorreia@apliant.com.br>
LoadFilesProperties asks DoLoadFileProperties for tags whenever a single
entry is selected, a folder too, although the Properties dialog shows tags
for files only. A folder that is only a prefix of its objects' keys -- as
folders made by other tools often are -- has no object of its own to read
tags from: GetObjectTagging answers NoSuchKey, and the folder's Properties
fail. Tags are now loaded for files only.
DoLoadFileProperties reads the object's ACL before its tags and fails on any
error, so on a server that implements no object ACLs -- versitygw answers
GetObjectAcl with 501 NotImplemented, as do other S3-compatible servers --
the Properties dialog does not open at all, the tags included. A
NotImplemented answer now reads as an ACL with no grants; any other error
still fails as before.
The session log's and the XML log's defaults are written as literals,
%TEMP%\!S.log and %TEMP%\!S.xml. On POSIX the backslash is not a path
delimiter, so the log was created beside the temporary directory, named
"tmp\!S.log", instead of in it. Built with IncludeTrailingBackslash, as the
random seed file's default already is, the delimiter is the platform's: on
Windows the same backslash as before, so the behaviour there is unchanged.
TTerminal::LogEvent is defined `inline` in Terminal.cpp, but the file
systems (FtpFileSystem.cpp and others) call it too. An inline function must
be defined in every translation unit that uses it; when it is not, the
program is ill-formed, and whether it links depends on the compiler keeping
an out-of-line copy. An optimizing clang build (-O2, -O3) inlines every call
within Terminal.cpp and emits none, so the other files' calls are undefined
at link time. Without `inline` the definition is the one every caller links
to; the calls within Terminal.cpp may still be inlined.
* Translate old-format RSA host keys in retrieve_host_key
retrieve_host_key comes from PuTTY's check_stored_host_key, where key is the
server's key and an old-format registry entry is translated and kept only if
it matches. In retrieve_host_key key is the output buffer, so the
translation is compared with whatever the caller's buffer holds (TSecureShell
passes an uninitialised one) and in practice never matches: an old-format
host key is reported as not cached.
The translation is now returned as the stored key, for the caller to compare
with the server's key, as it compares every other stored key. It is not
written back in the new format, as nothing here has verified it.
* Keep the original strbuf_to_str in retrieve_host_key's old-format path
Exclude only the comparison, the put_reg_sz and the else (#ifndef WINSCP),
so that it is clear the strbuf_to_str is what the original does when the
key matches.
---------
Co-authored-by: Eduardo Correia <ecorreia@apliant.com.br>
For rmdir, TScript::CreateFileList (fltDirectories) creates a bare
TRemoteFile with only its name and type: no directory list and no full
file name. TWebDAVFileSystem::DeleteFile takes the path from
File->FullFileName, and TRemoteFile::GetFullFileName then dereferences the
null Directory. With WinSCP 6.5.7 a script doing
mkdir /work/gone
rmdir /work/gone
over WebDAV fails with "Invalid access to memory" in
TRemoteFileList::GetFullDirectory (from TWebDAVFileSystem::FilePath,
TTerminal::DoDeleteFile, TScript::RmDirProc).
The entry now carries its absolute path, resolved as TTerminal::DoDeleteFile
resolves the same name. Protocols that use the file name (SFTP, SCP, FTP)
are unaffected; WebDAV now deletes the directory.
ec_cleanup frees every curve ecc-ssh.c sets up except Ed448: ec_ed448 lacks
the WINSCP_CURVE_CLEANUP its siblings have and ec_cleanup does not call it,
so its points and bignums leak at shutdown once an Ed448 key was used.
win_misc_cleanup frees get_system_dir's cached buffer but leaves the pointer
and its size, so a get_system_dir call after the cleanup returns freed
memory instead of fetching the directory again.
random_add_noise enters putty_section and then returns without leaving it
when the generator is not active. The section is recursive, so the thread
that returned keeps working, but any other thread entering it -- another
session's socket, RNG or bignum code -- then blocks for good.
aes-ni.c pushes its target attribute under #ifdef __clang__ but pops it
under #ifdef _WIN64. For clang targeting Win64 the two coincide. Otherwise
they do not: clang targeting 32-bit Windows (clang-cl in a WINSCP_VS build)
reports the push as unterminated and fails, and MSVC targeting x64 meets a
pop of a clang pragma it does not know (warning C4068). The pop now has the
push's condition.
memset takes the fill value before the size, so
memset(&ProcessEntry, sizeof(ProcessEntry), 0) zeroed nothing. It was
harmless, as Process32First fills the entry it reads, but the entry is now
cleared as intended.
FileGetSymLinkTargetSafe opens the link with CreateFile to read its reparse
point, but never closes the handle, so each call leaks one. DriveView calls
it for every symbolic link and junction it lists.
Seems to have no significant impact on release build (have some on debug build).
With 64-bit build, no impact at all. With 32-bit, there are some differences, but all seem to be in misidentified frames anyway (which are volatile, so the impact might not be due to change in sources)
Source commit: b8d538dd403c7f78778247278bed6e685a5589fb
