942791 - Non-ASCII file names should be accessible in FTP directory listings regardless of the FEAT/UTF8 support

Status: RESOLVED FIXED

(Core Graveyard :: Networking: FTP, defect)

Description

[Masatoshi Kimura [:emk]]

See bug 942495.

Comment 1

[Masatoshi Kimura [:emk]]

On Nightly, the issue was not reproduced. Probably because the default encoding is windows-1252 which is has round-trip mapping from/to Unicode.

Comment 2

[Masatoshi Kimura [:emk]]

I confirmed I had to set [Options]-[Content]-[Advanced]-[Fallback Character Encoding] to Japanese to reproduce the issue.

Comment 3

[Masatoshi Kimura [:emk]]

Attachment: Ensure non-ASCII filenames are accessible in FTP directory listings

Comment 4

[Masatoshi Kimura [:emk]]

Attachment: Ensure non-ASCII filenames are accessible in FTP directory listings

The error recovery in the previous patch didn't work if the sequence contains a partial sequence (such as 0xE3 0x81). The new patch will give up and fallback if the sequence contains an invalid sequence as UTF-8.

Comment 5

[Masatoshi Kimura [:emk]]

Try run: https://tbpl.mozilla.org/?tree=Try&rev=a18b0647468a

Comment 6

[Masatoshi Kimura [:emk]]

Attachment: Revert bug 427089

The "fix" for bug 427089 is totally broken. It prevented access to files in <ftp://ftp.picosystems.net/> (from bug 942495).
I confirmed this didn't regress bug 427089.

Comment 7

[Masatoshi Kimura [:emk]]

Try run: https://tbpl.mozilla.org/?tree=Try&rev=2c5dfa4e775d

Comment 8

[Honza Bambas (:mayhemer)]

Comment on attachment 8338498 [details] [diff] [review]
Ensure non-ASCII filenames are accessible in FTP directory listings

Review of attachment 8338498 [details] [diff] [review]:
-----------------------------------------------------------------

Tested with ftp://ftp.picosystems.net/
Still 550 when switched to Shift_JIS, is that expected?

::: netwerk/streamconv/converters/nsIndexedToHTML.cpp
@@ +865,3 @@
>      // Adding trailing slash helps to recognize whether the URL points to a file
>      // or a directory (bug #214405).
> +    if ((type == nsIDirIndex::TYPE_DIRECTORY) &&(loc.Last() != '/')) {

add space after &&

@@ +884,5 @@
>          // Without it, the trailing '/' will be escaped, and links from within
>          // that directory will be incorrect
>          escFlags = esc_Forced | esc_OnlyASCII | esc_AlwaysCopy | esc_FileBaseName | esc_Colon | esc_Directory;
>      }
> +    NS_EscapeURL(loc.get(), loc.Length(), escFlags, escapeBuf);

to make clear what |escapeBuf| is please rename it to locEscaped

@@ +895,5 @@
> +        // Try to convert non-ASCII bytes to Unicode using UTF-8 decoder.
> +        nsCOMPtr<nsIUnicodeDecoder> decoder =
> +            mozilla::dom::EncodingUtils::DecoderForEncoding("UTF-8");
> +        decoder->SetInputErrorBehavior(nsIUnicodeDecoder::kOnError_Signal);
> +        int32_t len = escapeBuf.Length();

blank line over this line (try to split to blocks of logically belonging code)

@@ +898,5 @@
> +        decoder->SetInputErrorBehavior(nsIUnicodeDecoder::kOnError_Signal);
> +        int32_t len = escapeBuf.Length();
> +        int32_t outlen = 0;
> +        rv = decoder->GetMaxLength(escapeBuf.get(), len, &outlen);
> +        if (NS_FAILED(rv)) return rv;

return rv on its own line

@@ +899,5 @@
> +        int32_t len = escapeBuf.Length();
> +        int32_t outlen = 0;
> +        rv = decoder->GetMaxLength(escapeBuf.get(), len, &outlen);
> +        if (NS_FAILED(rv)) return rv;
> +        nsAutoArrayPtr<PRUnichar> outbuf(new PRUnichar[outlen + 1]);

! check outlen is in <0,PR_MAX_INT32-1>, if not, return NS_ERROR_FAILURE.

@@ +904,5 @@
> +        rv = decoder->Convert(escapeBuf.get(), &len, outbuf, &outlen);
> +        // Use the result only if the sequence is valid as UTF-8.
> +        if (rv == NS_OK) {
> +            outbuf[outlen] = 0;
> +            utf16URI.Append(outbuf);

do utf16URI.Append(outbuf, outlen);

Comment 9

[Masatoshi Kimura [:emk]]

(In reply to Honza Bambas (:mayhemer) from comment #8)
> Tested with ftp://ftp.picosystems.net/
> Still 550 when switched to Shift_JIS, is that expected?

Yes, that's expected. The FTP directory listing has a bug about switching the encoding from the menu which is out of scope of this bug. Please change the default fallback encoding from Options-Content and reload the page to test. Also the second patch is needed.
I'll fix other points.

Comment 10

[Masatoshi Kimura [:emk]]

Attachment: Ensure non-ASCII filenames are accessible in FTP directory listings, v2

> ! check outlen is in <0,PR_MAX_INT32-1>, if not, return NS_ERROR_FAILURE.

|+ 1| was needed to make room for the trailing null byte. It was changed to |utf16URI.Append(outbuf, outlen);|, so outlen will no longer overflow.

Comment 11

[Masatoshi Kimura [:emk]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/4c27e14de1af

Comment 12

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/4c27e14de1af

Comment 13

[Honza Bambas (:mayhemer)]

Comment on attachment 8339240 [details] [diff] [review]
Revert bug 427089

Stealing review on Michal's request.

Comment 14

[Honza Bambas (:mayhemer)]

(In reply to Masatoshi Kimura [:emk] from comment #6)
> Created attachment 8339240 [details] [diff] [review]
> Revert bug 427089
> 
> The "fix" for bug 427089 is totally broken. It prevented access to files in
> <ftp://ftp.picosystems.net/> (from bug 942495).
> I confirmed this didn't regress bug 427089.

How?  And I assume your fix is what fixes bug 427089 again?  What are other behavior changes when you back this patch out?  How this patch influences your patch?

Comment 15

[Masatoshi Kimura [:emk]]

By the patch of bug 427089, <ftp://ftp.picosystems.net/pub/DATEL_%E7%B7%8F%E5%90%88%E3%82%AB%E3%82%BF%E3%83%AD%E3%82%B0_1986_87.pdf> is treated as the same URL as <ftp://ftp.picosystems.net/pub/DATEL_%91%8D%8D%87%83J%83%5E%83%8D%83O_1986_87.pdf> which is the cause of the 550 error.
This (UTF-8 to system charset) conversion was needed before my patch part 1 because non-ASCII path parts in URL were decoded using the system charset, then encoded back to UTF-8. But my patch made the non-ASCII bytes transparent, so the double conversion is breaking the access to file names which happens to be a valid sequence as UTF-8.

Comment 16

[Masatoshi Kimura [:emk]]

> <ftp://ftp.picosystems.net/pub/DATEL_%E7%B7%8F%E5%90%88%E3%82%AB%E3%82%BF%E3%83%AD%E3%82%B0_1986_87.pdf> is treated as the same URL as <ftp://ftp.picosystems.net/pub/DATEL_%91%8D%8D%87%83J%83%5E%83%8D%83O_1986_87.pdf>

To be precise, it depends on the fallback charset. This is when the fallback charset is Shift_JIS.

Comment 17

[Honza Bambas (:mayhemer)]

Comment on attachment 8339240 [details] [diff] [review]
Revert bug 427089

Not locally tested.  FTP backout changes conform the original patch.

Comment 18

[Masatoshi Kimura [:emk]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/6882f119dcb1
We will need a test infrastructure for the FTP directory listing...

Comment 19

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/6882f119dcb1

Comment 20

[Masatoshi Kimura [:emk]]

Hm, the status was not changed because of the word "revert"?

Comment 21

[Masatoshi Kimura [:emk]]

Comment on attachment 8339240 [details] [diff] [review]
Revert bug 427089

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 427089
User impact if declined: Some files on FTP directory listings will not be accessible.
Testing completed (on m-c, etc.): landed on m-c, tested locally.
Risk to taking this patch (and alternatives if risky): pretty low
String or IDL/UUID changes made by this patch: none

Comment 22

[Masatoshi Kimura [:emk]]

Missed the 28 train.

Comment 23

[Masatoshi Kimura [:emk]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/0ae4bc68a553