Remy Lebeau

And that Community Edition of 10.4 doesn't exist yet. I think Embarcadero needs to give him a free license, for all the work he does to fix their issues. And then get him into the betas and make sure he finds broken things so they can fix them BEFORE they release!

DO NOT free the TJSONValue that is returned by TJSONPair.JsonValue or TJSONObject.Values[]. You do not own that TJSONValue, so you are not responsible for freeing it. The parent TJSONObject owns it, and will free it for you when itself is freed. The only JSON object you should be freeing manually in this code is the TJSONValue that is returned by TJSONObject.ParseJSONValue(). You can also remove manual freeing of the Indy SSL and Log objects by utilizing TComponent ownership semantics. Thus, the only objects you really need to free manually in this code are the TIdHTTP and TStringList objects, eg: function TBetFairApi.GetToken: Boolean; var LJSONObject: TJSONObject; LJSONValue, LJSONToken: TJSONValue; {$IFDEF VER230} LJSONPair: TJSONPair; {$ENDIF} IdHTTP: TIdHTTP; IdSSL: TIdSSLIOHandlerSocketOpenSSL; IdLogFile: TIdLogFile; sList: TStringList; begin Result := False; FToken := ''; IdHTTP := TIdHTTP.Create(nil); try IdHTTP.HTTPOptions := IdHTTP.HTTPOptions + [hoForceEncodeParams]; IdHTTP.HandleRedirects := True; IdSSL := TIdSSLIOHandlerSocketOpenSSL.Create(IdHTTP); IdSSL.SSLOptions.SSLVersions := [sslvTLSv1, sslvTLSv1_1, sslvTLSv1_2]; IdHTTP.IOHandler := IdSSL; IdLogFile := TIdLogFile.Create(IdHTTP); IdLogFile.Filename := 'c:\' + ChangeFileExt(ExtractFileName(ParamStr(0)), '.log'); IdLogFile.Active := True; IdHTTP.Intercept := IdLogFile; IdHTTP.Request.Accept := 'application/json'; IdHTTP.Request.CustomHeaders.Values['X-Application'] := cBetfair_AppKey; IdHTTP.Request.ContentType := 'application/x-www-form-urlencoded'; sList := TStringList.Create; try sList.Add('username=' + FUserID); sList.Add('password=' + FPassword); LJSONValue := TJSONObject.ParseJSONValue(IdHTTP.Post(URL_LOGIN, sList)); try if LJSONValue is TJSONObject then begin LJSONObject := TJSONObject(LJSONValue); {$IFDEF VER230} LJSONPair := LJSONObject.Get('token'); if LJSONPair <> nil then LJSONToken := LJSONPair.JsonValue else LJSONToken := nil; {$ELSE} LJSONToken := LJSONObject.Values['token']; {$ENDIF} if LJSONToken <> nil then FToken := LJSONToken.Value; end; finally LJSONValue.Free; end; finally sList.Free; end; finally IdHTTP.Free; end; Result := (FToken <> ''); end;

Managed records have Initialize() and Finalize() operators that users can override. But that also means they have to be called automatically when the record enters and leaves scope. So, just like other managed types, there is an hidden try..finally to ensure Finalize() is called, yes. That is not the case with unmanaged records. Inline variables begin and end their lifetime in the same scope that they are declared in. So that is where their initialization and cleanup is performed.

Not on all platforms. On iOS 64bit and Linux 64bit, Longint is 8 bytes where Integer is 4 bytes.

Unless the result of FPosition + Count is higher than High(Int64), in which case the result will wrap to a negative value. Granted, that is unlikely to happen, but it is still a possibility with signed integer arithmetic. Perhaps, but Seek() is a very low overhead operation in a memory stream, so this is not really an issue. Then you should take this up with Embarcadero directly, since they are the only ones who can actually change it.

Not equivalent as-is, no. However, StrPas() converts a null-terminated C-style string to a Delphi string. A StrLPas()-like function would convert a C-style string to a Delphi string up to a given length or the null terminator, whichever is reached first. You can easily write your own StrLPas() function using SetString(), eg: function StrLPas(const Str: PChar; const MaxLength: Integer): String; var L: Integer; P: PChar; begin P := Str; L := 0; while (P^ <> #0) and (L < MaxLength) do begin Inc(P); Inc(L); end; SetString(Result, Str, L); end;

SetString copies EXACTLY the length you specify, no more, no less. It is roughly equivalent to: var S: String; //SetString(S, Source, Length); SetLength(S, Length); Move(Source^, Pointer(S)^, Length * SizeOf(Char));

Note that when using StrLCopy(), it copies up to the max length you specify and then adds +1 for a null terminator. So, to account for the null terminator, you need to either +1 the memory allocation, or -1 the max length to copy. This is documented behavior. Actually, they did - it is called SetString().

Can you be more specific? What is the actual exception that is being raised? Connect() does call Connected(), but it should not raise an exception if there is no connection. And if that exception is due to your earlier issues with fSSLSocket being nil, your recent patches should have addressed that. Honestly, you are the only person who seems to be having so many problems with TIdSSLIOHandlerSocketOpenSSL. Other people use it all the time just fine. So either you are just not using it correctly (and you posted WAY too much code for me to go through it all - I asked you to test a simple example that doesn't use your wrappers at all), or maybe your Indy installation is faulty/corrupted - have you tried upgrading to the latest Indy release yet? It should not be doing that. Rather than continuing to apply band-aid patches, have you tried actually debugging the IOHandler's startup to figure out why it is in such a bad state to begin with? Because what you have described so far is NOT how it is supposed to behave under normal conditions. Which makes me wonder if maybe there are other problems in your code that may be corrupting memory and the IOHandler is just the victim of that.

That stack trace is useless without CONTEXT. WHERE in your code is that AccessViolation occurring exactly? At best, all I can gleam from this report is that a nil pointer is still being accessed. Probably when the IOHandler's ReadFromSource() calls TIdSSLIOHandlerSocketOpenSSL.CheckForError(), which then calls fSSLSocket.GetSSLError() when fPassThrough is False. But as I told you earlier, fSSLSocket cannot be nil when fPassThrough is False. So where is the nil exactly? You need to debug your code, I can't do it remotely for you. Usually no. But I can't say for certain as you did not show ALL of your relevant code for how the client gets created and passed around and used. It is really hard to follow your code. Can you please do a separate test where you just create a simple thread that connects a simple TIdTCPClient and TIdSSLIOHandlerSocketOpenSSL to your server, without all the extra noise? Do you still run into problems when doing that test?

Just an FYI, you should not be calling IdOpenSSLSetLibPath() on every HTTP request. It should be called only once, preferably at program startup. Indy does not load and unload OpenSSL on every request. It loads OpenSSL once and leaves it loaded for multiple requests to use. Off-hand, I don't see anything else wrong with your code. So the problem has to be something else preventing the HTTPS session from being established properly. Are you sure you are posting your HTTPS request to the correct URL to begin with? It sounds like that maybe that URL is not actually using HTTPS despite starting with "https://". You can verify that with a packet sniffer, like Wireshark, to look at the actual hello packets of the SSL/TLS handshake. Most likely, you will see the IOHandler receiving something other than an SSL/TLS ServerHello packet, which would account for the "unknown protocol" error.

That usually means you are trying to perform an SSL/TLS handshake on a non-SSL/TLS port. Did you remove the assignment of the PassThrough property, like I suggested? You should be letting TIdHTTP handle that property, do not touch it manually for HTTP at all. Can you show your updated code?

You are right, I misspoke. TIdTCPClient's Connect() method does call the Connected() method internally. Not true, otherwise you would not be getting the exception inside of TIdSSLIOHandlerSocketOpenSSL. The Connected() method checks for a nil IOHandler. So, the fact that you are getting an exception inside of the Readable() method of TIdSSLIOHandlerSocketOpenSSL means that your FSSLHandler is actually assigned to the TIdTCPClient's IOHandler. Also not true, because Readable() accesses the fSSLSocket object only when the fPassThrough member is False, and that member can't be False without an active fSSLSocket object, as the PassThrough property setter ensures that object's existence before setting fPassThrough to False. Your earlier crash report clearly shows your TTCPClient.Connect() method being called while your MainForm is being destroyed, so I still believe that your code is trying to access the TIdTCPClient and/or TIdSSLIOHandlerSocketOpenSSL while (or after) it is in a state of destruction. Your code is not cleaning up after itself properly during a shutdown. But you have not shown any of your actual code for TTCPAsyncThread or TMainForm, so noone can tell you why it is not cleaning up properly. You are likely missing some shutdown logic to stop the TTCPAsyncThread before the MainForm destroys the Indy components.

Why are you using sslvSSLv3? Nobody uses SSL v3.0 anymore, as it is no longer secure. You should be using TLS v1.0 at a minimum, preferably TLS v1.1 and/or TLS v1.2 instead. Get rid of this line completely, as you should not be using the SSLOptions.Method property at all: IdSSLIOHandlerSocketOpenSSL1.sslOptions.Method := sslvSSLv3; And then change this line: IdSSLIOHandlerSocketOpenSSL1.sslOptions.SSLVersions := [sslvSSLv3]; To this instead: IdSSLIOHandlerSocketOpenSSL1.sslOptions.SSLVersions := [sslvTLSv1, sslvTLSv1_1, sslvTLSv1_2]; Also, remove this line completely as well, as TIdHTTP will handle this property for you automatically based on whether it is requesting an HTTP or HTTPS url: IdSSLIOHandlerSocketOpenSSL1.PassThrough := false;

You did not show how you are creating and setting up your FSSLHandler, or how you are using TIdTCPClient in your TTCPAsyncThread, but the error report indicates that a nil pointer is being accessed inside of the IOHandler's PassThrough property setter method. The report also says that you are using Delphi 10.3 Rio. Are you using an up-to-date version of Indy 10 with it? Delphi 10.4 shipped with an updated Indy 10 snapshot from a few weeks ago. There were some changes made to the PassThrough setter back in August 2019, but I can't tell if you are using that version or not. The call stack trace shown in your error report makes no sense. It implies that your MainForm is trying to connect to the server while the Form is being destroyed? Typically, when an exception is logged, only the call stack of the thread that raised the exception should be logged. But the only way this error report's call stack trace makes sense is if it is actually logging call stacks from multiple threads at the same time, overlapping them. I think your code has a secondary worker thread that is trying to connect to the server at the same moment that your MainForm is destroying the Indy components. You need to shut down your TTCPAsyncThread completely before allowed the MainForm to destroy the Indy components. I'm also a little worried that Indy's ReadFromSource() method appears in this error report at all. TIdTCPClient's Connect() method doesn't call ReadFromSource(), which implies that something else in your code is trying to read from the TCP connection when it likely should not be. Are you, by chance, making any calls to TIdTCPClient's Connected() method to check the state of the TCP connection, in particular in the main thread? If so, you should not be doing that at all. It is hard to diagnose this error without seeing all of your relevant code that is using Indy, at least the connect and disconnect portions.

Still a work in progress. Mezen's pull request for a new SSLIOHandler for OpenSSL 1.1.x is still pending review, I just have not had time lately to review it yet, but you can download and try it for yourself.

If you are using HTTP for authentication, then why not just keep using HTTP for the file transfers, too? That is doable. However, there is nothing in the FTP protocol to handle such tokens, so that will have to be transmitted as the username/password/account for the session. But once authenticated, you can set the TIdFTPServerContext.HomeDir and TIdFTPServerContext.CurrentDir properties as needed. There is nothing in the FTP protocol to handle that. I would suggest simply deleting the token from the database as soon as a client logs in with it, making it a session token. That way, noone else can login with that same token, and it will be invalidated once the client disconnects. Yes, you can store custom data in the TIdFTPServerContext on a per-client basis. You definately do not want to query the database on every transfer. That will not work. There is nothing in the FTP protocol that would allow for such data on a per-request basis. But, the client can login with the token, and the server can validate it at the time of login and remember that the client is allowed to upload, and then the client can freely upload files as needed. Not necessary if the token is deleted as soon as it is redeemed during login. However, if would be trivial to remember the expire time on a per-client basis, and the server could validate the expiration on each file transfer, if needed. Same restrictions apply as above. Well, then now you are back to hitting the database on each file transfer, which is not a good thing for performance. You don't need to use CommandHandlers for this. TIdFTPServer does that internally for you. TIdFTPServer has OnUserLogin, OnUserAccount, OnStoreFile, and OnRetrieveFile events that you can use instead. Yes, a windows service would be a good idea. That is a very broad question with many possible different solutions.

Yes. You cannot bind to an IP that does not belong to the local device that your server is running on. Alternatively, you can bind to '0.0.0.0' to listen on all local IPv4 addresses with a single socket (use '::0' for IPv6). The TIdFTP.ExternalIP property is for use with active-mode file transfers when the FTP session passes through a proxy/router. In active mode, an FTP client has to open a listening socket to receive an inbound TCP connection from the FTP server. As such, it needs to tell the FTP server which IP to connect to, which can't be the local IP when the client is running behind a proxy/router. The TIdFTP.ServerHOST property is used with virtual FTP server hosting, it has nothing to do with file transfers. An FTP server can host multiple sites on the same server IP. So, to differentiate which site an FTP client wants to access, it must send a HOST command to the FTP server. When the TIdFTP.UseHOST property is True, TIdFTP sends the HOST command before performing user authentication. If the TIdFTP.ServerHOST property is blank, the TIdFTP.Host property is used. TIdFTPServer has an OnHostCheck event, and a TIdFTPServerContext.Host property, for handling HOST commands from clients. Only the TIdFTP.ExternalIP property deals with file transfers. The TIdFTP.ServerHOST property has nothing to do with that. The TIdFTP.DataPort property is the local port that TIdFTP will listen on, and the FTP server will connect to, for active-mode transfers. If it is 0 (the default), an available random ephemeral port will be chosen by the OS. If TIdFTP is running behind a proxy/router, you need to set the TIdFTP.ExternalIP property to the proxy/router's public IP, and enable Port Forwarding on your proxy/router to forward inbound traffic from the router's public IP/DataPort to TIdFTP's listening Binding.IP/DataPort. If TIdFTPServer is running behind a proxy/router, you don't need to set the TIdFTP.ExternalIP (unless TIdFTP is also behind a proxy/router), but you do need to enable Port Forwarding on your server's proxy/router to forward inbound traffic from the router's public IP/Port to TIdFTPServer's listening Binding.IP/Port (not DataPort), as well as port forwarding for any ports you setup in TIdFTPServer's DefaultDataPort or PASVBoundPort(Min|Max) properties. The TIdFTP.Host property is the hostname/IP that TIdFTP will connect to when you call TIdFTP.Connect(), so it can't be blank. Having TIdFTP connect to the proxy/router's public IP will not work, since there is no FTP server running at that IP. That is merely the proxy/router's public IP to the outside world. The TIdFTP.ServerHOST property has no effect on where TIdFTP.Connect() connects to.

Yes, it is an RTL function, but it has different implementations for different platforms, and they want the POSIX implementation to be inlined, as it is just a 1-line call to the POSIX unlink() function, so ideal for inlining: function DeleteFile(const FileName: string): Boolean; ... {$IFDEF POSIX} var M: TMarshaller; begin Result := unlink(M.AsAnsi(FileName, CP_UTF8).ToPointer) <> -1; end; {$ENDIF POSIX} The Windows implementation used to be a 1-line call as well, to the Win32 DeleteFile() function, and thus was inlined, too: function DeleteFile(const FileName: string): Boolean; {$IFDEF MSWINDOWS} begin Result := Winapi.Windows.DeleteFile(PChar(FileName)); end; {$ENDIF MSWINDOWS} But, it is no longer inlined. In XE, new logic was added to make SysUtils.DeleteFile() call the Win32 GetFileAttributes() and RemoveDirectory() functions when deleting a symbolic link to a directory (why that was added, I did not know): function DeleteFile(const FileName: string): Boolean; {$IFDEF MSWINDOWS} var Flags, LastError: Cardinal; begin Result := Winapi.Windows.DeleteFile(PChar(FileName)); if not Result then begin LastError := GetLastError; Flags := GetFileAttributes(PChar(FileName)); if (Flags <> INVALID_FILE_ATTRIBUTES) and (faSymLink and Flags <> 0) and (faDirectory and Flags <> 0) then begin Result := RemoveDirectory(PChar(FileName)); Exit; end; SetLastError(LastError); end; end; {$ENDIF MSWINDOWS} As such, the inline keyword for that implementation was removed in XE2.

It means the function in question has been marked with the 'inline' specifier, but the function uses code from the Posix.Unistd unit, and that unit is not in a 'uses' clause in scope of the code that is calling the function, so the function can't be inlined at the call site. You posted this in a VCL forum, but the VCL's version of DeleteFile() does not use any Posix units, so you must be using FMX's version of DeleteFile() instead.

That is how I feel for most things in the IOUtils unit! It is just bad implementations all around.

The WM_QUERYENDSESSION documentation clearly states that data saving should be deferred to the WM_ENDSESSION message, so you don't really need the MSG_SAVEDATA approach: And the WM_ENDSESSION documentation says: System shutdown can be aborted, in which case there would be no need to save any unsaved data, as long as your app has not been terminated yet. WM_QUERYENDSESSION is just asking for permission for the system to be shut down. The actual shutdown has not taken place yet. But, once an actual shutdown begins, unsaved data should be saved automatically. If you want to prompt the user whether unsaved data should be saved, do that in response to WM_CLOSE (or the TForm.OnCloseQuery event) instead: If you receive WM_ENDSESSION before receiving WM_CLOSE/OnCloseQuery then simply don't prompt the user.

No, because the as operator forces the compiler to create a hidden variable for the interface, before it is then passed to the const parameter. Had the code been written like this instead, THEN there would have been a problem: List.Add(TItem.Create); Correct.

That is not the reason. Your test functions may simply be holding a hidden local reference to the IItem that you create and add to the list, so the item is not fully released until after the list has been freed first. Try this instead to isolate that reference so it gets released sooner: procedure TestDestroyOnly; var List: TItemList; procedure AddItem; begin List.Add(TItem.Create as IItem); end; begin Writeln( 'Start Test - Destroy Only'); List := TItemList.Create; try AddItem; finally List.Free; end; Writeln( 'End Test - Destroy Only'); end; procedure TestWithExplicitClear; var List: TItemList; procedure AddItem; begin List.Add(TItem.Create as IItem); end; begin Writeln( 'Start Test - Explicit Clear'); List := TItemList.Create; try AddItem; List.Clear; finally List.Free; end; Writeln( 'End Test - Explicit Clear'); end;

All VCL TGraphic-derived classes have a SaveToFile() method, you don't need to save to a TFileStream manually: uses Vcl.Imaging.PngImage; procedure SaveBitmapToPNG(ABitmap: TBitmap; const AFileName: String; ACompresionLevel: Integer = 7); var img: TPngImage; begin img := TPngImage.Create; try img.CompressionLevel := ACompresionLevel; img.Assign(ABitmap); img.SaveToFile(AFileName); finally img.Free; end; end;