String Encoding UTF vs Latin1 and incompatibility between Soulseek.NET and Nicotine

[jackBonadies]

When requesting a file using Soulseek.NET from a client using Nicotine there is a filename encoding issue that can occur causing the transfer to fail.

For background - Soulseek.NET encodes strings in Latin1 by default and only if the encoding fails encodes strings in UTF8. Soulseek.NET decodes strings it receives from UTF8 and only if the decoding fails uses Latin1. Nicotine encodes strings using UTF8 (other than when doing a FileSearch, RoomSearch, or FolderContentsRequest) and decodes strings using UTF8 and only if that fails uses Latin1.

This means that if a Soulseek.NET client does a TransferRequest to a Nicotine client with a filename that is has a valid encoding in Latin1 that can be decoded in UTF8 but where the resulting string is different depending on the encoding, it will fail.

For example -
Good Case - I request file "Ü.jpg", Soulseek.NET encodes it using Latin1 (which succeeds as it is valid Latin1), Nicotine decodes it using UTF8 but it is not valid UTF8 and so it uses Latin1 as a fallback. The Nicotine client sees "Ü.jpg" and can send the file.
Bad Case - I request file "eÌe.jpg", Soulseek.NET encodes it using Latin1 (which succeeds as it is valid Latin1), Nicotine decodes it using UTF8 and it is valid UTF8, however interpreted as UTF8 it becomes "ée.jpg" and the transfer fails as the user does not have said file.

So basically, my question is, is there a downside to always encoding the request in UTF8? Concerns re SoulseekNS or others? Tagging @mathiascode as they likely have useful info with change made here: nicotine-plus/nicotine-plus@1328753

[mathiascode]

I did originally switch to sending messages encoded as latin1 by default, and I think I might have suggested this to @jpdillingham back then too. The idea was improved compatibility with Soulseek NS: nicotine-plus/nicotine-plus#760

Eventually I figured out that there were downsides with this approach, as mentioned in the linked commit: nicotine-plus/nicotine-plus@1328753

I continued using latin1 by default for search strings, since there were no apparent side effects, and Soulseek NS clients sent more search results.

It seems like I also enabled this for FolderContentsRequest messages later, since Soulseek NS clients were sending empty responses in some cases: nicotine-plus/nicotine-plus@574ff03

It also seems like I summarized my changes in a comment a while ago: #438 (comment)

[mathiascode]

is there a downside to always encoding the request in UTF8

If you want to get as many files as possible from Soulseek NS clients, then yes. That client is the only reason you would ever send a string encoded as latin1 today.

[jpdillingham]

I think I'll probably revert this behavior as well. I see that I 👍 your comment from a year ago but must have either misinterpreted or forgot to create an issue and it fell through the cracks.

Anymore my stance is generally that I want to maximize support for "modern" clients, and if I have to sacrifice compatibility with NS to do so, then I will.

[jackBonadies]

Thank you very much. Using UTF for transfers and falling back to Latin1 on certain failure messages seems to me the most robust solution.

I do want to make a note that for Soulseek.NET when sending a TransferRequest with Latin1 encoded filename to a Nicotine client in the failure case outlined above, that a timeout occurs rather than a TransferRejectedException. This is due to the fact that the QueueFailed response from the remote client will have the unintended filename (that resulted in UTF8 decoding a Latin1 encoded string) and therefore it will not match any download or WaitKey.

Here:

Soulseek.NET/src/Messaging/Handlers/PeerMessageHandler.cs

Line 328 in 50882a0

var download = SoulseekClient.DownloadDictionary.Values.FirstOrDefault(d => d.Username == connection.Username && d.Filename == uploadFailedResponse.Filename);

I don't know an elegant fix for this other than maybe checking the received filename both against the transfer Filename and the result of the transfer Filename Latin1 encoded and UTF8 decoded, but I just thought I would make note of it.

[jackBonadies]

For what its worth another potential solution could be, when reading a search response or browse response and decoding the filename, if utf8 decoding fails that means that the remote client must necessarily be soulseekNS. Therefore, add a member to the search response or browse response making note that we are talking a legacy client. When requesting a file from the client, request it with a Latin1 encoded string. In the case where the utf8 decoding does not fail but the string is Latin1 encoded, I believe this solution will still work (even if the string is different when utf8 decoding vs Latin1 decoding), as encoding with Latin1, decoding with UTF8, reencoding with UTF8, and finally decoding with Latin1 will result in the same string, even if the intermediate decoded UTF8 string is different.

[jpdillingham]

I can make the change to try both encodings. I'll have a look around and see where else this might be affecting things.

I'm interested in your suggested solution of tracking legacy clients, but I'll need to think of a way to store this information. It will need to be offloaded to the application using the library (similar to how the endpoint cache works, I guess).

[jackBonadies]

Another proposed solution (again, FWIW), which is elegant in the sense that it is transparent to the remote client: When receiving a filename, make note of how you decode it (requires a boolean per filename). When sending the filename back to the remote client (for transfer request, folder contents) encode it in the same way you decoded it. In that sense the remote client will get the same byte sequence back and they will know what to do with it, it doesn't matter if we decoded it using a code page no one has ever heard of, as long as we encode it back using that same code page resulting in the same byte sequence the remote client will understand it. Because really the fundamental problem is a client decoding one way and encoding a different way resulting in a different byte sequence (in the problem case in the OP it was decoding in UTF8 and encoding in Latin1). In my testing as long as you send the remote client the same filename byte sequence they sent you, they will be able to understand the request.

[jackBonadies]

Is there a benefit to including the "raw filename" vs a boolean value? In my assessment using the boolean value (that states whether it was decoded in UTF8 or Latin1) one would always be able to yield that raw filename with the benefit that it is just a single bit per filename.

[jpdillingham]

There's a couple benefits I'm thinking of, but I haven't completely dismissed the boolean approach either (I need to test different scenarios). These are:

• Zero ambiguity. There may be scenarios where Latin1 -> UTF8 -> Latin1 may produce a byte array different from the original, especially if the original encoding isn't actually Latin1 but something compatible. This may be an unfounded concern.
• Developer experience/ergonomics: A single value representing the raw/canonical/bit perfect filename is easier to handle and reason about than a pairing of filename and encoding/boolean
• Minimal changes within the library: Switching from one string value to another would be quite a bit easier/would break a lot fewer tests

[jpdillingham]

There's a couple of flaws in this; the additional memory pressure (which is really pretty significant), and the fact that to retrieve folder contents, string manipulation is required, which means I must encode the provided bytes to a string at some point if I am going to support this functionality (and I do).

I'll keep thinking about this but I think the boolean approach, coupled with logic that will retry a few things with different encoding, is probably best. The boolean will inform the encoding that is attempted first, and the retry covers existing logic and/or clients that don't want to deal with the boolean.

[jackBonadies]

Zero ambiguity

In C# at least, any byte sequence decoded and reencoded in Latin1 results in the same byte string. This includes Latin1 undefined characters (0x00-0x1F, 0x7F-0x9F). In C# one just gets the code point for those characters which are then written to the byte.

For decoding a byte sequence and reencoding with UTF-8 I am not sure. If ExceptionFallback is not used (i.e. invalid UTF-8 is parsed), it can certainly result in a different byte sequence (i.e. any time the replacement character is encountered). But I cannot find any sort of proof online for the case where only valid UTF-8 is considered.

[jackBonadies]

I tried out the boolean method - it worked in the above outlined cases with both a soulseekNS client and modern clients. If you decide to go this method there is a hitch though. Right now we decode the full folder name and the filename separately. Meaning that it is possible to decode the full folder name and filename using different methods. One fix for this could be, if a file is encountered that is invalid UTF-8 to re-decode the whole message (i.e. the whole browse response or search response) via Latin1 so that it is consistent. Another fix could be to keep a bool for the directory name and a bool for the filename, but then the commands (i.e. TransferRequest) would have to be further refactored (split the field Filename into directory name and filename, add 2 boolean fields) and then ToByteArray() would have to write the directory string with the directory encoding, write a path separator (encoding wouldnt matter), write the filename string with the filename encoding.

[jpdillingham]

I made some changes in #778 and want to check my thinking. This PR is more about enablement and changing the default behavior; the fix for the root problem (ambiguity in filename encoding) will come later.

In this PR, I'm:

• Using UTF-8 to write all strings unless otherwise specified, removing the default to ISO-8859-1 but enabling it to be forced situationally
• Retaining the UTF-8 -> ISO-8859-1 "negotiation" when reading strings, but allowing one or the other to be forced situationally

Still to do:

• Force ISO-8859-1 when sending search requests and requests for folder contents

In a subsequent PR:

• Figure out how to pass filenames around without making any assumptions about encoding

[jackBonadies]

Force ISO-8859-1 when sending requests for folder contents

In my understanding, though I am yet to test this, isn't this not any better than forcing ISO-8859-1 when sending a TransferRequest? Reason being in that if we send a directory name that is the bad case in the OP (i.e. valid UTF8 but with characters outside ascii) to a remote nicotine client, they will decode it as UTF8 and so the directory string will be different and it will fail to find the folder in shares.

I have not tested this but looking at the code it would seem that that would be the case...

[mathiascode]

Which version of Nicotine+ did you test with, and which OS and file system? I tried your example file names, downloaded them with slskd from a Nicotine+ client running the latest version, but couldn't reproduce the issue.

[jackBonadies]

I tried it again using slskd (latest) and downloading from my Nicotine+ 3.2.0 machine on Windows. On investigation I realized that when I copied and pasted the filename, it did not paste properly. "eÌe.jpg" works fine, since when encoded using Latin1 does not produce a valid UTF string and therefore an error occurs and the fallback Latin1 is used. The filename I found in the wild that prompted this discussion actually had the sequence "eÌ\u0081e" so an unprintable character between I and e. Sorry about that. Please use the string "för" - no unprintable characters, and when encoded via Latin1 and decoded via UTF8 it results in the sting "för" which is valid but wrong. I just tested this and slskd displayed an error File Not Shared.

[mathiascode]

Thanks! I've made a change in Nicotine+ to only use latin-1 in the FolderContentsRequest message if the response contains no files: nicotine-plus/nicotine-plus@be901b7

Edit: I need to add some protection to prevent endless messages if the folder is actually empty.

[jpdillingham]

I have put a lot of thought and testing into this in the last few days, with most of those changes being visible in the history of #778.

I have:

• Added a diagnostic message when the 'preferred' encoding can't be used to encode/decode a string
• Added the ability to override default encoding when building network messages at the lowest level (but this is never used anywhere)
• Changed the logic to encode all outgoing strings as UTF-8 by default, only falling back to ISO-8859-1 if UTF-8 fails

These changes ensure that modern clients are maximally compatible by defaulting to UTF-8, and break compatibility for some files served by Soulseek NS clients.

I have put a lot of thought and investigation into what it will take to detect encoding and retain that information along with search and browse responses, and allow clients to pass it back in to ensure correct behavior, and frankly it just isn't something I'm interested in doing at this point.

To do this I would need to pass encoding along with strings all over the library, refactor a ton of tested and hardened code, break a large chunk of my ~2100 unit tests, and burn what would likely be several weeks of time that could otherwise be spent developing features for slskd.

I'm not saying I'll never do this, and if someone wanted to take on the work I'd be happy to review a proposal, but my thinking at this time is that it's simply not worth the disruption or the time.

[jpdillingham]

Note that I have seen the discussion around using ISO-8859-1 encoding for search requests, and I still need to do some investigation to see if that's worthwhile.

In general though, I'm not planning to invest any more time on efforts to improve support for Soulseek NS.

[jackBonadies]

Thanks for looking into this btw. I also did some testing and experimenting and just wanted to note:

Sending back the same byte string someone sent you does not guarantee that there will not be an encoding issue. Currently the latest non dev branch of Nicotine encodes as Latin1 by default and decodes as Unicode by default meaning if you try to download the file "för" from a Nicotine user, if you take note of how you decoded and encode it the same way, sending back the same bytestring, they will still read their own string back incorrectly (as "för"). So if the end client does not encode and decode the same way, then sending back the same bytestring is no good. So even with the taking note of encoding method, you will still have to retry with encoding it in Latin1. Other than this case, sending back the same bytestring will always work. No question however that its a major refactoring effort and pain for minimal benefit.

[mathiascode]

I've shifted my efforts towards reducing the remaining number of Soulseek NS users out there. I've asked Nir about preventing Soulseek NS clients from connecting to the server, and reported a vulnerability that would hopefully get us there sooner. If this happens, I'll remove the workarounds from Nicotine+ and go UTF-8 only.