Execution module - file.read binary=True fails with decoding errors

[rares-pop]

Description of Issue

Tried reading a binary file in Salt, in multiple versions:
2018.3, 2019.2.0, 2019.2.2, but fails with decoding exceptions.

Tried 2018.3 in both Windows and NILinuxRT.
Tried 2019.2.2 in arch linux.

All fails.

For example, using LANG=POSIX, I get this:

admin@rsctest-9035Syncf:/home# export LANG=POSIX
admin@rsctest-9035Syncf:/home# salt-call --local file.read /bin/bash True
[WARNING ] /usr/lib/python3.5/site-packages/salt/modules/nilrt_ip.py:276: DeprecationWarning: This method will be removed in future versions.  Use 'parser.read_file()' instead.
  config_parser.readfp(config_file)

[ERROR   ] An un-handled exception was caught by salt's global exception handler:
UnicodeDecodeError: 'ascii' codec can't decode byte 0xe0 in position 24: ordinal not in range(128)
Traceback (most recent call last):
  File "/usr/bin/salt-call", line 11, in <module>
    salt_call()
  File "/usr/lib/python3.5/site-packages/salt/scripts.py", line 410, in salt_call
    client.run()
  File "/usr/lib/python3.5/site-packages/salt/cli/call.py", line 57, in run
    caller.run()
  File "/usr/lib/python3.5/site-packages/salt/cli/caller.py", line 134, in run
    ret = self.call()
  File "/usr/lib/python3.5/site-packages/salt/cli/caller.py", line 212, in call
    ret['return'] = func(*args, **kwargs)
  File "/usr/lib/python3.5/site-packages/salt/modules/file.py", line 3564, in read
    return salt.utils.stringutils.to_unicode(file_obj.read())
  File "/usr/lib/python3.5/site-packages/salt/utils/stringutils.py", line 142, in to_unicode
    return _normalize(to_str(s, encoding, errors))
  File "/usr/lib/python3.5/site-packages/salt/utils/stringutils.py", line 102, in to_str
    raise exc  # pylint: disable=raising-bad-type
  File "/usr/lib/python3.5/site-packages/salt/utils/stringutils.py", line 95, in to_str
    return _normalize(s.decode(enc, errors))
UnicodeDecodeError: 'ascii' codec can't decode byte 0xe0 in position 24: ordinal not in range(128)
Traceback (most recent call last):
  File "/usr/bin/salt-call", line 11, in <module>
    salt_call()
  File "/usr/lib/python3.5/site-packages/salt/scripts.py", line 410, in salt_call
    client.run()
  File "/usr/lib/python3.5/site-packages/salt/cli/call.py", line 57, in run
    caller.run()
  File "/usr/lib/python3.5/site-packages/salt/cli/caller.py", line 134, in run
    ret = self.call()
  File "/usr/lib/python3.5/site-packages/salt/cli/caller.py", line 212, in call
    ret['return'] = func(*args, **kwargs)
  File "/usr/lib/python3.5/site-packages/salt/modules/file.py", line 3564, in read
    return salt.utils.stringutils.to_unicode(file_obj.read())
  File "/usr/lib/python3.5/site-packages/salt/utils/stringutils.py", line 142, in to_unicode
    return _normalize(to_str(s, encoding, errors))
  File "/usr/lib/python3.5/site-packages/salt/utils/stringutils.py", line 102, in to_str
    raise exc  # pylint: disable=raising-bad-type
  File "/usr/lib/python3.5/site-packages/salt/utils/stringutils.py", line 95, in to_str
    return _normalize(s.decode(enc, errors))
UnicodeDecodeError: 'ascii' codec can't decode byte 0xe0 in position 24: ordinal not in range(128)
admin@rsctest-9035Syncf:/home#

However, it fails using UTF-8 too.

Setup

N/A

Steps to Reproduce Issue

try running:
salt-call --local file.read /usr/bin/bash True

Versions Report

Salt Version:
           Salt: 2019.2.2
 
Dependency Versions:
           cffi: Not Installed
       cherrypy: Not Installed
       dateutil: Not Installed
      docker-py: Not Installed
          gitdb: Not Installed
      gitpython: Not Installed
          ioflo: Not Installed
         Jinja2: 2.10.3
        libgit2: Not Installed
        libnacl: Not Installed
       M2Crypto: 0.35.2
           Mako: Not Installed
   msgpack-pure: Not Installed
 msgpack-python: 0.6.2
   mysql-python: Not Installed
      pycparser: Not Installed
       pycrypto: 3.9.4
   pycryptodome: Not Installed
         pygit2: Not Installed
         Python: 2.7.17 (default, Oct 22 2019, 09:14:09)
   python-gnupg: Not Installed
         PyYAML: 5.1.2
          PyZMQ: 18.1.1
           RAET: Not Installed
          smmap: Not Installed
        timelib: Not Installed
        Tornado: 5.1.1
            ZMQ: 4.3.2
 
System Versions:
           dist:  
         locale: UTF-8
        machine: x86_64
        release: 5.4.2-arch1-1
         system: Linux
        version: Not Installed

[waynew]

Hey @rares-pop thanks for the report!

---

This will fail on any file that contains non-text (unicode) data. An easy way to test this:

 vim /tmp/broken
 ia<esc>
 :%!xxd
 ggf:2lre0<esc>
 :%!xxd -r
 :wq
 salt-call --local file.read /tmp/broken binary=True

This will break with the above issue. It happens because in the file.read function it's doing a .to_unicode

Either we need to update the docs to be explicit that we only support reading text files (despite being able to read them with the b flag), or we need to update the behavior to actually allow reading binary data.

The latter might be a much more involved fix, as Salt currently treats all internal data as unicode text.

[waynew]

@rares-pop what was your desired outcome here? Were you just trying to get the contents of a binary file available on the master? Or...?

For a workaround, you could feed the file to base64 - which has the disadvantage of making the file probably huge. Depending on your needs, there's also the whole push options, which does have other caveats.

[rares-pop]

@waynew - yeah, I mentioned 'reading binary files', so yeah, having non-ascii characters.

My particular use-case was to get a .so file from the minion to the master, to allow remote-debug on it. And that .so is binary.

I'm aware of cp.push stuff but I haven't tried it out, so I don't know if it works (it's marked as a security concern, and has to be explicitly enabled in master conf, so I wouldn't recommend it).

Hmm.. I haven't thought to base64, but I don't think it's feasible if we (or the clients) have to do it manually each time before we want to transfer the file. Maybe if salt would do that inline?
It won't work for my particular case either because we use a mongoDB returner where document sizes are limited to 16MB - https://docs.mongodb.com/manual/reference/limits/ (so increasing the file size would suck).

I was looking into fixing this issue myself in salt, but as you say, it's core is using unicodes, so it's not a trivial fix to enable this. We are work arounding this by using other non-salt mechanisms, but this behavior is still broken in salt.

I start to believe it might be a viable option to modify salt to do that base64 encoding inline, and document this behavior, so the function is not broken.

[waynew]

I don't hate that idea. It doesn't solve all of our underlying bytes vs. text issues, but it would be an improvement.

If it's written with a flag to guard it on the server side of the file client, and then the client side looks for some "encoding": "base64" value or something in order to know how to turn it back to bytes? That could work.

I don't think that based on priority we'll be able to get this done on the core team, but if someone wants to tackle it, I think this would be a great addition to the Phosphorus release 👍