Repository: bazelbuild/bazel
Issue: pkg_deb fails when control fields contain unicode/utf-8 encoded characters
Labels: P4
team-Rules-Server

### Description of the problem:

pkg_deb fails when control fields contain unicode characters or the contents of utf-8 encoded files.

### Bugs: what's the simplest, easiest way to reproduce this bug? Please provide a minimal example if possible.

Using the following:
```shell
~/utf8-test$ ls
BUILD  WORKSPACE
```

WORKSPACE
```
```

BUILD
```
load("@bazel_tools//tools/build_defs/pkg:pkg.bzl", "pkg_tar", "pkg_deb")

pkg_tar(
  name = "data",
)

pkg_deb(
  name = "utf8-test",
  data = ":data",
  maintainer = "foo",
  description = "Unicode symbols ®, Δ, Й, ק ,م, ๗, あ,  叶, 葉, 말, ü and  é",
  package = "utf8-test",
  version = "1.0",
)
```

You will get an error message like the following:
```
~/utf8-test$ bazel build :utf8-test --verbose_failures
INFO: Invocation ID: 4831418c-4a4a-41f7-abf4-d18b50934194
INFO: Analysed target //:utf8-test (0 packages loaded, 0 targets configured).
INFO: Found 1 target...
ERROR: /home/user/utf8-test/BUILD:7:1: MakeDeb utf8-test_1.0_all.deb failed (Exit 1) make_deb failed: error executing command
  (cd /home/user/.cache/bazel/_bazel_user/e6e7bd4474c6bcca1dea3235a205b76e/execroot/__main__ && \
  exec env - \
  bazel-out/host/bin/external/bazel_tools/tools/build_defs/pkg/make_deb '--output=bazel-out/k8-fastbuild/bin/utf8-test_1.0_all.deb' '--changes=bazel-out/k8-fastbuild/bin/utf8-test_1.0_all.changes' '--data=bazel-out/k8-fastbuild/bin/data.tar' '--package=utf8-test' '--architecture=all' '--maintainer=foo' '--version=1.0' '--description=Reserved symbol®' '--distribution=unstable' '--urgency=medium')
Execution platform: @bazel_tools//platforms:host_platform

Use --sandbox_debug to see verbose messages from the sandbox: make_deb failed: error executing command
  (cd /home/user/.cache/bazel/_bazel_user/e6e7bd4474c6bcca1dea3235a205b76e/execroot/__main__ && \
  exec env - \
  bazel-out/host/bin/external/bazel_tools/tools/build_defs/pkg/make_deb '--output=bazel-out/k8-fastbuild/bin/utf8-test_1.0_all.deb' '--changes=bazel-out/k8-fastbuild/bin/utf8-test_1.0_all.changes' '--data=bazel-out/k8-fastbuild/bin/data.tar' '--package=utf8-test' '--architecture=all' '--maintainer=foo' '--version=1.0' '--description=Reserved symbol®' '--distribution=unstable' '--urgency=medium')
Execution platform: @bazel_tools//platforms:host_platform

Use --sandbox_debug to see verbose messages from the sandbox
Traceback (most recent call last):
  File "/home/user/.cache/bazel/_bazel_user/e6e7bd4474c6bcca1dea3235a205b76e/sandbox/linux-sandbox/1/execroot/__main__/bazel-out/host/bin/external/bazel_tools/tools/build_defs/pkg/make_deb.runfiles/bazel_tools/tools/build_defs/pkg/make_deb.py", line 342, in <module>
    main(FLAGS(sys.argv))
  File "/home/user/.cache/bazel/_bazel_user/e6e7bd4474c6bcca1dea3235a205b76e/sandbox/linux-sandbox/1/execroot/__main__/bazel-out/host/bin/external/bazel_tools/tools/build_defs/pkg/make_deb.runfiles/bazel_tools/tools/build_defs/pkg/make_deb.py", line 328, in main
    installedSize=GetFlagValue(FLAGS.installed_size))
  File "/home/user/.cache/bazel/_bazel_user/e6e7bd4474c6bcca1dea3235a205b76e/sandbox/linux-sandbox/1/execroot/__main__/bazel-out/host/bin/external/bazel_tools/tools/build_defs/pkg/make_deb.runfiles/bazel_tools/tools/build_defs/pkg/make_deb.py", line 195, in CreateDeb
    control = CreateDebControl(extrafiles=extrafiles, **kwargs)
  File "/home/user/.cache/bazel/_bazel_user/e6e7bd4474c6bcca1dea3235a205b76e/sandbox/linux-sandbox/1/execroot/__main__/bazel-out/host/bin/external/bazel_tools/tools/build_defs/pkg/make_deb.runfiles/bazel_tools/tools/build_defs/pkg/make_deb.py", line 163, in CreateDebControl
    f.addfile(tarinfo, fileobj=BytesIO(controlfile.encode('utf-8')))
UnicodeDecodeError: 'ascii' codec can't decode byte 0xc2 in position 136: ordinal not in range(128)
Target //:utf8-test failed to build
INFO: Elapsed time: 0.349s, Critical Path: 0.15s
INFO: 0 processes.
FAILED: Build did NOT complete successfully

```

### What operating system are you running Bazel on?

Ubuntu 16.04 & 18.04

### What's the output of `bazel info release`?

`release 0.22.0`


###  Have you found anything relevant by searching the web?

No

### Any other information, logs, or outputs that you want to share?

Bazel used to work with the "®" symbol in the description field. It seems like the changes to make pkg defs Python 2/3 compatible here: https://github.com/bazelbuild/bazel/commit/6fff4da5f5e436551aa9294b58960da1d64af39a may have introduced the bug. Based on some initial StackOverflow searching it seems like using .decode("utf-8") on the input may be necessary. https://stackoverflow.com/a/16508962 If it's alright, I'd be happy to submit a patch on `tools/build_defs/pkg/make_deb.py` along those lines.



Issue comments:
If I'm understanding my build errors correctly, this change re-broke the py2/3 compatibility on make_deb.py that #6818 had previously fixed.

```
  File ".../bazel-out/k8-opt/bin/external/bazel_tools/tools/build_defs/pkg/make_deb.runfiles/bazel_tools/tools/build_defs/pkg/make_deb.py", line 301, in GetFlagValue
    flagvalue = flagvalue.decode('utf-8')
AttributeError: 'str' object has no attribute 'decode'
```

It's possible that the toolchain changes in 0.25.0 are also at fault and/or the only problem, but with the binary tagged `PY2AND3`, the code doesn't look correct to me on first read.
See https://github.com/bazelbuild/rules_pkg/issues/35

Pull request review comments:
Can you add 
￼   # -*- coding: utf-8 -*-
at the top of this file and build_test.sh 
I think it is safer to do the decode at the top of the method on the incoming value.
Or, better yet, the transform from bytes to unicode string should happen in GetFlagValue.
If you would like to do future maintainers a favor, you could clean up the formatting on the block above this.
The current formatting is an unreadable mess. How about.

  changesdata = ''.join(
      MakeDebianControlField(*x)
      for x in [
          ('Format', '1.8'),
          ('Date', time.ctime(timestamp)),
          ('Source', package),
          ('Binary', package),
          ('Architecture', architecture),
          ('Version', version),
          ('Distribution', distribution),
          ('Urgency', urgency),
          ('Maintainer', maintainer),
          ('Changed-By', maintainer),
          ('Description', '\n%s - %s' % (package, short_description)),
          ('Changes', (
              '\n%s (%s) %s; urgency=%s'
              '\nChanges are tracked in revision control.') % (
                  package, version, distribution, urgency)),
          ('Files', '\n' + ' '.join(
              [checksums['md5'], debsize, section, priority, deb_basename])),
          ('Checksums-Sha1', '\n' + ' '.join(
              [checksums['sha1'], debsize, deb_basename])),
          ('Checksums-Sha256', '\n' + ' '.join(
              [checksums['sha256'], debsize, deb_basename]))])

Or even:
  changesdata = ''.join(
      MakeDebianControlField('Format', '1.8'),
      MakeDebianControlField('Date', time.ctime(timestamp)),
      ...

If not, I'll clean it up after.
The decode() could be at line 295 
     return flagvalue.decode('utf-8')

But also we might want to add it at line 289
    flagvalue = flagvalue.decode('utf-8')
so that an @file usage can have a utf-8 path
Thanks for the review aiuto! Apologies it's taken me so long to get back to you. I'd be happy to add the encoding.

I was seeing `# -*- coding: utf-8 -*-` in other places in the codebase. (Mainly third_party/protobuf) Is that preferred or is `# -- coding: utf-8 --` alright?
That's fair. I'm happy to do it. I was trying to keep my initial changes minimal so it was easier to review.

I like the below approach so will go ahead and do that unless you prefer the other method:
```python
changesdata = ''.join([
MakeDebianControlField(...),
MakeDebianControlField(...),
...
])
```
Sounds good. I'll play around with this a little. I had a friend suggest setting `changesdata = u''` and `changesfile = u''` as well so they won't have to be overridden at each access point.
-*-.  My typo the firs time.
You might as well do the encoding just once. 
That works for me too. It is easier to follow than "MakeDebianControlField(*x)"
Doing some testing - it seems that doing:
```python
if flagvalue:
  flagvalue = flagvalue.decode('utf-8')
  ...
```
and removing the decode in: `return result.decode('utf-8').replace(...) + '\n\'` on line 145 will cause UnicodeDecodeErrors if one of the fields that doesn't use GetFlagValue(s) in CreateDeb (line 306) contains a utf-8 variable.

I could move the `flagvalue = flagvalue.decode('utf-8')` line further into the if statement nest so the `@file` directive will get it and leave the rest the same.

I agree. However, I get a TypeError from the `f.addfile(... BytesIO(controlfile))` line if I do the encoding at the top of the method. It will work if I do:
```python
controlfile = ''
  for values in DEBIAN_FIELDS:
    fieldname = values[0]
    key = fieldname[0].lower() + fieldname[1:].replace('-', '')
    if values[1] or (key in kwargs and kwargs[key]):
      controlfile += MakeDebianControlField(fieldname, kwargs[key], values[2])
  controlfile = controlfile.encode('utf-8')
...
```
and remove the encode() calls further down.

Do you have any idea why that would be? My guess would be the `controlfile += MakeDebianControlField(...)` line in the `for` loop but I'm unsure why that would override the encoding if it's set earlier. It seems like both this TypeError and the issues I run into `MakeDebianControlField` function result from the `installed_size` flag.
I think that is worth it. We have other outstanding bugs for not supporting UTF-8 file paths. So GetFlagValue needs to do that. AND the content should be interpreted as utf-8 encoded.

But I think I have the solution to what you observed.  We upgrade str to unicode str in MakeDebianControlField
I changed one of the non GetFlagValue() attributes to be unicode also, so the test would have a that case too.
     maintainer = "soméone@somewhere.com",

If you have extra tests, I would like to try them against my suggestion. If you think that change works. I also have it patched on top of your PR, so I could to the merge of yours.

Full diff: https://github.com/bazelbuild/bazel/compare/master...aiuto:deb

inline diff

*** 133,148 ****
  def MakeDebianControlField(name, value, wrap=False):
    """Add a field to a debian control file."""
    result = name + ': '
    if type(value) is list:
!     value = ', '.join(value)
    if wrap:
!     result += ' '.join(value.split('\n'))
      result = textwrap.fill(result,
                             break_on_hyphens=False,
                             break_long_words=False)
    else:
      result += value
!   return result.decode('utf-8').replace('\n', '\n ') + '\n'
  
  
  def CreateDebControl(extrafiles=None, **kwargs):
--- 133,150 ----
  def MakeDebianControlField(name, value, wrap=False):
    """Add a field to a debian control file."""
    result = name + ': '
+   if type(value) is str:
+     value = value.decode('utf-8')
    if type(value) is list:
!     value = u', '.join(value)
    if wrap:
!     result += u' '.join(value.split('\n'))
      result = textwrap.fill(result,
                             break_on_hyphens=False,
                             break_long_words=False)
    else:
      result += value
!   return result.replace('\n', '\n ') + '\n'
  
  
  def CreateDebControl(extrafiles=None, **kwargs):
***************
*** 295,303 ****
  
  def GetFlagValue(flagvalue, strip=True):
    if flagvalue:
      if flagvalue[0] == '@':
        with open(flagvalue[1:], 'r') as f:
!         flagvalue = f.read()
      if strip:
        return flagvalue.strip()
    return flagvalue
--- 297,306 ----
  
  def GetFlagValue(flagvalue, strip=True):
    if flagvalue:
+     flagvalue = flagvalue.decode('utf-8')
      if flagvalue[0] == '@':
        with open(flagvalue[1:], 'r') as f:
!         flagvalue = f.read().decode('utf-8')
      if strip:
        return flagvalue.strip()
    return flagvalue




Sounds good - if there are other outstanding utf-8 filepath bugs it'd be worth it to deal with that while we're here. 

Amusingly - that was the exact test case I was using. I'd be fine with you merging on top of my commits. I guess I didn't go far enough in changing things to unicode strings!

The one other thing I was noticing was that if the controlfile isn't utf-8 encoded when determining the size content will be dropped from it. I've been using `expect_log "Built-Using: Bazel"` to test for that as that seems to usually be the last line in the controlfile. However, I'm not sure how robust of a test that is.
Yes. I saw your fix for that. I'll try to commit this now. Having flakiness with the windows/jdk8 tests. So this may take a few tries to catch a clean build.
Looks like it passed. Thanks for the review!

Patch that resolved the issue:
diff --git a/tools/build_defs/pkg/BUILD b/tools/build_defs/pkg/BUILD
index f3c8b4c86e995f..c7e9e26997d5a6 100644
--- a/tools/build_defs/pkg/BUILD
+++ b/tools/build_defs/pkg/BUILD
@@ -1,3 +1,4 @@
+# -*- coding: utf-8 -*-￼
 licenses(["notice"])  # Apache 2.0
 
 filegroup(
@@ -213,7 +214,7 @@ pkg_deb(
         "dep1",
         "dep2",
     ],
-    description = "toto",
+    description = "toto ®, Й, ק ,م, ๗, あ, 叶, 葉, 말, ü and é",
     distribution = "trusty",
     maintainer = "someone@somewhere.com",
     make_deb = ":make_deb",
diff --git a/tools/build_defs/pkg/build_test.sh b/tools/build_defs/pkg/build_test.sh
index 99bae8e8fcbfa6..a1d920e8a24573 100755
--- a/tools/build_defs/pkg/build_test.sh
+++ b/tools/build_defs/pkg/build_test.sh
@@ -1,4 +1,5 @@
 #!/bin/bash
+# -*- coding: utf-8 -*-
 
 # Copyright 2015 The Bazel Authors. All rights reserved.
 #
@@ -181,7 +182,7 @@ function test_deb() {
   check_eq "-rwxr-xr-x" "$(get_deb_permission test-deb.deb ./usr/titi)"
   check_eq "-rw-r--r--" "$(get_deb_permission test-deb.deb ./etc/nsswitch.conf)"
   get_deb_description test-deb.deb >$TEST_log
-  expect_log "Description: toto"
+  expect_log "Description: toto ®, Й, ק ,م, ๗, あ, 叶, 葉, 말, ü and é"
   expect_log "Package: titi"
   expect_log "Depends: dep1, dep2"
 
diff --git a/tools/build_defs/pkg/make_deb.py b/tools/build_defs/pkg/make_deb.py
index 737c075a787721..1c9c84d68ab5b0 100644
--- a/tools/build_defs/pkg/make_deb.py
+++ b/tools/build_defs/pkg/make_deb.py
@@ -142,7 +142,7 @@ def MakeDebianControlField(name, value, wrap=False):
                            break_long_words=False)
   else:
     result += value
-  return result.replace('\n', '\n ') + '\n'
+  return result.decode('utf-8').replace('\n', '\n ') + '\n'
 
 
 def CreateDebControl(extrafiles=None, **kwargs):
@@ -159,7 +159,8 @@ def CreateDebControl(extrafiles=None, **kwargs):
   with gzip.GzipFile('control.tar.gz', mode='w', fileobj=tar, mtime=0) as gz:
     with tarfile.open('control.tar.gz', mode='w', fileobj=gz) as f:
       tarinfo = tarfile.TarInfo('control')
-      tarinfo.size = len(controlfile)
+      # Don't discard unicode characters when computing the size
+      tarinfo.size = len(controlfile.encode('utf-8'))
       f.addfile(tarinfo, fileobj=BytesIO(controlfile.encode('utf-8')))
       if extrafiles:
         for name, (data, mode) in extrafiles.items():
@@ -261,27 +262,31 @@ def CreateChanges(output,
   debsize = str(os.path.getsize(deb_file))
   deb_basename = os.path.basename(deb_file)
 
-  changesdata = ''.join(
-      MakeDebianControlField(*x)
-      for x in [('Format', '1.8'), ('Date', time.ctime(timestamp)), (
-          'Source', package
-      ), ('Binary', package
-         ), ('Architecture', architecture), ('Version', version), (
-             'Distribution', distribution
-         ), ('Urgency', urgency), ('Maintainer', maintainer), (
-             'Changed-By', maintainer
-         ), ('Description', '\n%s - %s' % (package, short_description)
-            ), ('Changes', ('\n%s (%s) %s; urgency=%s'
-                            '\nChanges are tracked in revision control.'
-                           ) % (package, version, distribution, urgency)
-               ), ('Files', '\n' + ' '.join(
-                   [checksums['md5'], debsize, section, priority, deb_basename])
-                  ), ('Checksums-Sha1', '\n' + ' '.join(
-                      [checksums['sha1'], debsize, deb_basename])
-                     ), ('Checksums-Sha256', '\n' + ' '.join(
-                         [checksums['sha256'], debsize, deb_basename]))])
+  changesdata = ''.join([
+    MakeDebianControlField('Format', '1.8'),
+    MakeDebianControlField('Date', time.ctime(timestamp)),
+    MakeDebianControlField('Source', package),
+    MakeDebianControlField('Binary', package),
+    MakeDebianControlField('Architecture', architecture),
+    MakeDebianControlField('Version', version),
+    MakeDebianControlField('Distribution', distribution),
+    MakeDebianControlField('Urgency', urgency),
+    MakeDebianControlField('Maintainer', maintainer),
+    MakeDebianControlField('Changed-By', maintainer),
+    MakeDebianControlField('Description', '\n%s - %s' % (
+      package, short_description)),
+    MakeDebianControlField('Changes', (
+      '\n%s (%s) %s; urgency=%s'
+      '\nChanges are tracked in revision control.') % (
+      package, version, distribution, urgency)),
+    MakeDebianControlField('Files', '\n' + ' '.join(
+      [checksums['md5'], debsize, section, priority, deb_basename])),
+    MakeDebianControlField('Checksums-Sha1', '\n' + ' '.join(
+      [checksums['sha1'], debsize, deb_basename])),
+    MakeDebianControlField('Checksums-Sha256', '\n' + ' '.join(
+      [checksums['sha256'], debsize, deb_basename]))])
   with open(output, 'w') as changes_fh:
-    changes_fh.write(changesdata)
+    changes_fh.write(changesdata.encode("utf-8"))
 
 
 def GetFlagValue(flagvalue, strip=True):


Test patch: