wxCurl corrupts binary files when transferring via ftp

[deks-awash]

Describe the bug
Originally described in OpenCPN issue 4227, now closed. OpenCPN version 5.8.4. The weatherfax plugin uses wxCurlDownloadDialog to download images from the internet. Seemingly the only site using ftp in WeatherFaxInternetRetrievalis the Australian Bureau of Meteorology where gif images are downloaded from ftp.bom.gov.au, and these are corrupted. The dialog uses wxCurlFTP which is hard coded to use ASCII transfer mode.

To Reproduce
Steps to reproduce the behavior:

Enable the weatherfax plugin
Click on the weatherfax icon in the main toolbar
Select menu Retrieve > Internet
Under Servers choose "Australia BOM", Regions "Australia", then "MSLP 12hr Forecast" in main panel
Under "retrieve", click the "Selected" button
OpenCPN will either crash or display a very distorted fax on the map

Expected behavior
OpenCPN should not crash when downloading binary images from an ftp server, and downloaded binary images should not be corrupted

Desktop (please complete the following information if applicable):

OS: linux - Ubuntu 24.04 (lubuntu)
OS Version 24.04
OpenCpn version 5.8.4
weatherfax plugin version 1.9.111.1

Additional context
Finding the url of the file attempted to be downloaded as (n this case) ftp://ftp.bom.gov.au/anon/gen/difacs/IDX0131.gif and using curl to download it directly works fine - the gif can be displayed without issue. The weatherfax plugin uses lib/wxcurl/src.ftp.cpp from opencpn-libs (which the dialog apparently uses). The wxCurlFTP constructor sets m_tmMode(kASCII) and there seems to be no mechanism exposed upstream to change it. The simplest fix would be to set the transfer mode to binary by default (after all, http[s] downloads do this), but this might involve some regression testing. Otherwise, a change to test the url ending for a number of known binary file types (e.g. gif, png - perhaps configurable?) and set the transfer mode to binary accordingly.

[leamas]

Thanks, this sorts out the administrative issues.
Next step would be a PR here. Is this something you could do?

EDIT: I'd suggest testing for gif, png, etc and using binary. mode on those cases. Text mode handles the different eol conventions, we dont want to miss that.

[leamas]

Looked a little into this after sleeping on the problem. My preliminary conclusion is that weatherfax can be fixed without modifying opencpn-libs.

Outlines for such a fix.

1. Derive a custom dialog from wxCurlDownloadDialog, something like class FaxDownloadDlg: public wxCurlDownloadDialog
2. FaxDownloadDlg has access to the protected m_pThread variable
3. m_pThread has the curl handle available as the public method GetCurlSharedPtr()

Since the curl handle is available it means that a custom dialog could can set the transfer mode as required when initiating the transfer. This is probably the safest way to handle this, the application (here weatherfax) knows if the transfer should be binary or not. It would be a small, non-invasive fix to weatherfax.

Sorry for pointing to what might have been the wrong direction (opencpn-libs). Sigh.

[deks-awash]

Thanks. I could take a look (test) your proposed approach but it won't be for a week - I have to deal with a boat. -------- Original message --------From: leamas ***@***.***> Date: 1/12/24 10:39 pm (GMT+10:00) To: OpenCPN/opencpn-libs ***@***.***> Cc: deks-awash ***@***.***>, Author ***@***.***> Subject: Re: [OpenCPN/opencpn-libs] wxCurl corrupts binary files when transferring via ftp (Issue #17) Looked a little into this after sleeping on the problem. My preliminary conclusion is that weatherfax can be fixed without modifying opencpn-libs. Outlines for such a fix. Derive a custom dialog from wxCurlDownloadDialog, something like class FaxDownloadDlg: public wxCurlDownloadDialog FaxDownloadDlg has access to the protected m_pThread variable m_pThread has the curl handle available as the public method GetCurlSharedPtr() Since the curl handle is available it means that a custom dialog could set the transfer mode as required when initiating the transfer. This is probably the safest way to handle this, the application (here weatherfax) knows if the transfer should be binary or not. It would be a small, non-invasive fix to weatherfax. Sorry for pointing to what might have been the wrong direction (opencpn-libs). Sigh. —Reply to this email directly, view it on GitHub, or unsubscribe.You are receiving this because you authored the thread.Message ID: ***@***.***>

[leamas]

Fair enough. Our boats are a priority :)