Fix handling of non-ASCII characters in archive entry file names

[fmeum]

When creating a PathFragment from a ZIP or TAR entry file name, the raw bytes of the name are now wrapped into a Latin-1 encoded String, which is how Bazel internally represents file paths.

Previously, ZIP entries as well as TAR entries with PAX headers would result in ordinary decoded Java strings, resulting in corrupted file names when passed to Bazel's file system operations.

Fixes #12986
Fixes bazel-contrib/rules_go#2771

[fmeum]

There doesn't seem to be an easy way to keep reading legacy tars in Latin-1 (that is, raw bytes) while still being able to distinguish them from UTF-8 names.

In order to preserve backwards compatibility with tars that use non-ASCII, non-UTF-8 file names without PAX headers, I could inject a custom Charset via CharsetProvider that somehow marks all strings it produces in a way that lets us distinguish them from those created by the default UTF-8 charset (e.g. via a WeakIdentityHashMap). Since that would definitely be a hack, I didn't want to do it without getting feedback on this point first.

[meteorcloudy]

@lberki Can you take a look here? I'm not quite familiar with encoding magic.

[lberki]

cc @tetromino (since he's been thnking about Unicode recently)

My professional opinion is that I don't know. What would his hack be useful for? I thought that what this change was doing was to parse the UTF-8 data into proper Unicode then convert that to Bazel's weird "UTF-8 bytes in ISO8859-1" encoding, but that seems doable without resorting to somehow tagging the Strings produced by it by (essentially) doing "if valid UTF-8, decode it as such, otherwise go directly to the weird Bazel encoding".

[fmeum]

It's more messy than that: The Latin-1 trick only works if all strings are encoded in Latin-1 from source to sink, otherwise the "opaque bag of bytes" property is irrecoverably lost.

But commons-compress breaks this invariant: While tar ustar headers respect the default string encoding (which can also be set via the constructor), the more modern tar pax headers always use UTF-8.

Since Bazel can't determine which encoding was used to produce the file names returned by commons-compress, I found the only non-hacky option to be to force UTF-8 for ustar headers as well, decode UTF-8 and then reencode as Latin-1.

As a result, this change would break users of tars with non-UTF-8 ustar headers. Previously, these were handled correctly via the Latin-1 trick (but any pax header with non-ASCII chars were broken), but now they would be decoded as UTF-8 and thus turn into garbage.

Ideally we could tweak commons-compress to return byte arrays or at least tell us which type of header it parser, but I didn't find a way to do this. We could raise an issue with them, hack around this limitation by tracking the strings via a custom charset or accept that life isn't fair and UTF-8 deserves to be the winner. ;⁠-⁠)

[lberki]

Well, actually, it's even more messy than that, if I understand correctly: the nice thing about the Latin-1 hack is that it works as long as the encoding on the input is the same as the encoding on the output. However, apparently, pax headers insist that they are UTF-8, which means that they constrain the output of Bazel to UTF-8 also.

Now I see where you are coming from: commons-compress doesn't tell whether there are pax headers and thus it doesn't tell you whether UTF-8 decoding was required. So what you could do is to keep track of Strings that were emitted by the Charset, thus you know which Strings went through the UTF-8 decoding and thus undo it. Is this correct?

I don't claim any knowledge of either commons-compress or the intricacies of tar files, but TarArchiveEntry seems to have some methods that have Pax in their name. Maybe those could be used to disambiguate instead of the hack you proposed?

[fmeum]

@tetromino Friendly ping

[fmeum]

@lberki Since I wanted to know whether the hack would work, I went ahead and implemented it. It does seem to work and I didn't even have to use a weak cache.

[fmeum]

@lberki @tetromino Friendly ping, I would really like to get this into Bazel 7. We currently have to maintain a number of hacks in rules_go just to be able to extract a Go SDK.

[brandjon]

My gut reaction to the encoding hack was pretty negative. Then I reread the code and description a couple times, and decided that the hack is only ugly because the apache api is insufficiently flexible for bad clients like us that want to do weird things with encodings. I'm ok with the hack provided it is sufficiently explained in comments, and doesn't leak outside this file.

I would like us to be pretty explicit in comments about what the format of the input headers and returned string are. I.e., Javadoc for the class CompressedTarFunction or its decompress() method, saying that USTAR headers are interpreted as latin-1, PAX headers are interpetted as UTF-8, and both types are turned into Java Strings holding the UTF-8 code units that represent the string content, in order to match our string behavior elsewhere.

One more question. We eventually want to fix Bazel's decoding of Starlark source files to be UTF-8 instead of latin-1. Does anything in this PR make that harder? It seems to me this PR is just preserving an existing behavior (USTAR) while fixing an erroneous one (PAX).

[fmeum]

I added more extensive comments, including Javadocs for the class.

One more question. We eventually want to fix Bazel's decoding of Starlark source files to be UTF-8 instead of latin-1. Does anything in this PR make that harder? It seems to me this PR is just preserving an existing behavior (USTAR) while fixing an erroneous one (PAX).

I don't think that it would make this harder. It does introduce one or two new places in which ISO_8859_1 would need to be replaced with UTF-8, but those are only related to prefix stripping, not the actual entry decoding logic that interfaces with commons-compress and relies on the custom decoder hack.

[lberki]

@tetromino is not available these days. I don't have any context on this pull request so I wouldn't be able to give it the attention it deserves, but let me try to find someone who has all the bits swapped into their brain.

[brandjon]

Thanks for the additional javadoc/comments.

[fmeum]

@bazel-io flag

[fmeum]

@brandjon Is this still on track for getting merged?

[meteorcloudy]

@brandjon is still fixing some internal breakages.

[fmeum]

@brandjon is still fixing some internal breakages.

Thanks for the update. I didn't know this code is used at Google - isn't it only relevant for external repos? Just curious.

[meteorcloudy]

Might be just some irrelevant breakages, Jon is OOO until next Tuesday, I can help merge this.

[brandjon]

Sorry, internal presubmits failed and then I went OOO. Thanks Yun for completing the merge!