Skip to content

Fix AnsibleUnsafeText when copying files larger than SMALL_FILE_LIMIT#1110

Merged
moreati merged 3 commits into
mitogen-hq:masterfrom
bbc:unsafe-large-copy
Sep 2, 2024
Merged

Fix AnsibleUnsafeText when copying files larger than SMALL_FILE_LIMIT#1110
moreati merged 3 commits into
mitogen-hq:masterfrom
bbc:unsafe-large-copy

Conversation

@jrosser
Copy link
Copy Markdown

@jrosser jrosser commented Aug 28, 2024

Small files are carried in-band in the communication between controller and remote, with larger files being copied by falling back to transfer_file(). This second code path was missed in b822f20.

Reproducer extracted from original code that failed:

Make the files larger than 2^17 bytes to trigger a failure

- hosts: all
  vars:
    atftpd_path: /srv/tftp
    tftp_ipxe_binaries:
      x86_bios: ipxe/x86_64_undionly.kpxe
      x86_uefi: ipxe/x86_64_ipxe.efi
      arm64_uefi: ipxe/arm64_ipxe.efi
  tasks:
    - copy:
        src: "{{ item.value }}"
        dest: "{{ atftpd_path }}/ipxe_{{ item.key }}.bin"
      with_items:
        - "{{ tftp_ipxe_binaries | dict2items }}"

Exception:

The full traceback is:
Traceback (most recent call last):
  File "/root/venv/lib/python3.10/site-packages/ansible/executor/task_executor.py", line 119, in run
    item_results = self._run_loop(items)
  File "/root/venv/lib/python3.10/site-packages/ansible/executor/task_executor.py", line 334, in _run_loop
    res = self._execute(variables=task_vars)
  File "/root/venv/lib/python3.10/site-packages/ansible/executor/task_executor.py", line 636, in _execute
    result = self._handler.run(task_vars=vars_copy)
  File "/home/ubuntu/mitogen/ansible_mitogen/mixins.py", line 146, in run
    return super(ActionModuleMixin, self).run(tmp, task_vars)
  File "/root/venv/lib/python3.10/site-packages/ansible/plugins/action/copy.py", line 522, in run
    module_return = self._copy_file(source_full, source_rel, content, content_tempfile, dest, task_vars, follow)
  File "/root/venv/lib/python3.10/site-packages/ansible/plugins/action/copy.py", line 305, in _copy_file
    remote_path = self._transfer_file(source_full, tmp_src)
  File "/root/venv/lib/python3.10/site-packages/ansible/plugins/action/__init__.py", line 556, in _transfer_file
    self._connection.put_file(local_path, remote_path)
  File "/home/ubuntu/mitogen/ansible_mitogen/connection.py", line 1129, in put_file
    self.get_chain().call(
  File "/home/ubuntu/mitogen/ansible_mitogen/connection.py", line 466, in call
    return self._rethrow(recv)
  File "/home/ubuntu/mitogen/ansible_mitogen/connection.py", line 452, in _rethrow
    return recv.get().unpickle()
  File "/home/ubuntu/mitogen/mitogen/core.py", line 1009, in unpickle
    raise obj
mitogen.core.CallError: mitogen.core.StreamError: cannot unpickle 'ansible.utils.unsafe_proxy'/'AnsibleUnsafeText'
  File "<stdin>", line 3860, in _dispatch_one
  File "<stdin>", line 3843, in _parse_request
  File "<stdin>", line 998, in unpickle
  File "<stdin>", line 789, in find_class
  File "<stdin>", line 899, in _find_global

fatal: [foobar]: FAILED! => {
    "msg": "Unexpected failure during module execution: mitogen.core.StreamError: cannot unpickle 'ansible.utils.unsafe_proxy'/'AnsibleUnsafeText'\n  File \"<stdin>\", line 3860, in _dispatch_one\n  File \"<stdin>\", line 3843, in _parse_request\n  File \"<stdin>\", line 998, in unpickle\n  File \"<stdin>\", line 789, in find_class\n  File \"<stdin>\", line 899, in _find_global\n",
    "stdout": ""
}

@moreati
Copy link
Copy Markdown
Member

moreati commented Aug 29, 2024

refs #1046

@moreati
Copy link
Copy Markdown
Member

moreati commented Aug 29, 2024

related to #1087. @jrosser thank for these PRs.

  1. May I rebase your branches and add commits to incorporate tests and changlog entries?
  2. Would you like an entry in https://github.com/mitogen-hq/mitogen/blob/master/docs/contributors.rst ?

@jrosser
Copy link
Copy Markdown
Author

jrosser commented Aug 29, 2024

@moreati Yes please do rebase / adjust any of my PR as needed to get them suitable to merge. Adding https://github.com/jrosser to the contributors list would be nice - thankyou.

@jrosser
Copy link
Copy Markdown
Author

jrosser commented Aug 29, 2024

@moreati If you have a test case for this I can update the branch with your changes as I did in #1087, alternatively we can close this PR and start a new new one from my personal github account so that I can permit you access to the branch.

@moreati
Copy link
Copy Markdown
Member

moreati commented Aug 30, 2024

If you have a test case for this I can update the branch with your changes

In progress.

Notes to self

  • SMALL_FILE_LIMIT is currently 126976 bytes == 124 kiB
  • ag 'large|512mb' tests identifies several existing tests that handle a large file
  • A seeded random.Random.randbytes() should reproducibly produce the same pseudorandom bytes.

@moreati
Copy link
Copy Markdown
Member

moreati commented Aug 30, 2024

Like #1087 this only occurs in a with_items: loop, when the destination has a file extension. It does not matter if the source filename has an extension. Unlike #1087 the task immediately fails, and ansible-playbook exits with a non-zero status.

- hosts: localhost
  gather_facts: false
  vars:
    chonkers:
      - {src: ~/tmp/150kiB_seed_1234_v1,      dest: /tmp/150kiB_seed_1234_v1}
      - {src: ~/tmp/150kiB_seed_1234_v1.copy, dest: /tmp/150kiB_seed_1234_v1}
      - {src: ~/tmp/150kiB_seed_1234_v1,      dest: /tmp/150kiB_seed_1234_v1.data}
      - {src: ~/tmp/150kiB_seed_1234_v1.copy, dest: /tmp/150kiB_seed_1234_v1.data}
  tasks:
    - copy:
        src: "{{ item.src }}"
        dest: "{{ item.dest }}"
        mode: u=rw,go=r
      with_items: "{{ chonkers }}"

    - name: Cleanup 1
      file:
        path: "{{ item.dest }}"
        state: absent
      with_items: "{{ chonkers }}"
➜  mitogen git:(master) ✗ git show                                                                                                                                           
commit 84a4fcdf008cd2b90506b5ae4762826c524030bf (HEAD -> master, upstream/master, upstream/HEAD, origin/master, origin/HEAD)
Merge: c95d4112 5ab872f2
Author: Alex Willmer <alex@moreati.org.uk>
Date:   Fri Aug 30 08:02:46 2024 +0100

    Merge pull request #1087 from bbc/unsafe-chmod
    
    ansible_mitogen: Handle unsafe paths in _remote_chmod

➜  mitogen git:(master) ✗ ANSIBLE_STRATEGY=mitogen_linear ANSIBLE_STRATEGY_PLUGINS=ansible_mitogen/plugins/strategy ~/tmp/v312/bin/ansible-playbook ~/tmp/issue1110_repro.yml
[WARNING]: No inventory was parsed, only implicit localhost is available
[WARNING]: provided hosts list is empty, only localhost is available. Note that the implicit localhost does not match 'all'

PLAY [localhost] ***************************************************************************************************************************************************************************************************************************

TASK [copy] ********************************************************************************************************************************************************************************************************************************
ok: [localhost] => (item={'src': '~/tmp/150kiB_seed_1234_v1', 'dest': '/tmp/150kiB_seed_1234_v1'})
ok: [localhost] => (item={'src': '~/tmp/150kiB_seed_1234_v1.copy', 'dest': '/tmp/150kiB_seed_1234_v1'})
ERROR! [mux  68294] 09:48:51.595685 E mitogen.[local.68296]: raw pickle was: b'\x80\x02(X!\x00\x00\x00kintha-68295-1f78b8f40-548a2df07fq\x00X\x16\x00\x00\x00ansible_mitogen.targetq\x01NX\r\x00\x00\x00transfer_fileq\x02)cmitogen.core\nKwargs\nq\x03}q\x04(X\x07\x00\x00\x00contextq\x05cmitogen.core\n_unpickle_context\nq\x06K\x00N\x86q\x07Rq\x08X\x07\x00\x00\x00in_pathq\tX#\x00\x00\x00/Users/alex/tmp/150kiB_seed_1234_v1q\nX\x08\x00\x00\x00out_pathq\x0bcansible.utils.unsafe_proxy\nAnsibleUnsafeText\nq\x0cXM\x00\x00\x00/Users/alex/.ansible/tmp/ansible_mitogen_action_adffb98f18aac450/.source.dataq\r\x85q\x0eRq\x0fu\x85q\x10Rq\x11tq\x12.'
An exception occurred during task execution. To see the full traceback, use -vvv. The error was:   File "<stdin>", line 899, in _find_global
fatal: [localhost]: FAILED! => 
  msg: |-
    Unexpected failure during module execution: mitogen.core.StreamError: cannot unpickle 'ansible.utils.unsafe_proxy'/'AnsibleUnsafeText'
      File "<stdin>", line 3860, in _dispatch_one
      File "<stdin>", line 3843, in _parse_request
      File "<stdin>", line 998, in unpickle
      File "<stdin>", line 789, in find_class
      File "<stdin>", line 899, in _find_global
  stdout: ''

PLAY RECAP *********************************************************************************************************************************************************************************************************************************
localhost                  : ok=0    changed=0    unreachable=0    failed=1    skipped=0    rescued=0    ignored=0   

➜  mitogen git:(master) ✗ echo $?
2

moreati and others added 2 commits August 30, 2024 16:51
This is in anticipation of mitogen-hq#1110, which only exhibits inside a with_items:
loop. For this refactor `loop:` is used, to confirm the refactored tests are
still correct. A subsequent commit will change them to with_items.

The content of the files and their SHA1 checksums are unchanged.
Small files are carried in-band in the communication between
controller and remote, with larger files being copied by falling back
to a more traditional ansible put_file mechanism. This large
file code path was missed in b822f20.
@moreati
Copy link
Copy Markdown
Member

moreati commented Aug 30, 2024

Confirm that the loop driven version of tests/ansible/integration/copy.yml in moreati@ce1acce fails if all loop: keywords are replaced by with_items: and this PR has not been applied.

TASK [Copy sourced files src={{ item.src }}, dest={{ item.dest }}, mode=u=rw,go=r] ***********************************
Friday 30 August 2024  15:57:00 +0000 (0:00:00.059)       0:00:18.424 ********* 
changed: [target-debian10-1] => (item=/tmp/copy-tiny-file.out)
changed: [target-debian11-2] => (item=/tmp/copy-tiny-file.out)
changed: [target-ubuntu2004-3] => (item=/tmp/copy-tiny-file.out)
ERROR! [mux  140446] 15:57:00.997443 E mitogen.[ssh.localhost:2202]: raw pickle was: b'\x80\x02(X%\x00\x00\x00uv2404-142265-721cfe0e1080-18e27186e7q\x00X\x16\x00\x00\x00ansible_mitogen.targetq\x01NX\r\x00\x00\x00transfer_fileq\x02)cmitogen.core\nKwargs\nq\x03}q\x04(X\x07\x00\x00\x00contextq\x05cmitogen.core\n_unpickle_context\nq\x06K\x00N\x86q\x07Rq\x08X\x07\x00\x00\x00in_pathq\tX\x14\x00\x00\x00/tmp/copy-large-fileq\nX\x08\x00\x00\x00out_pathq\x0bcansible.utils.unsafe_proxy\nAnsibleUnsafeText\nq\x0cX]\x00\x00\x00/home/mitogen__has_sudo_nopw/.ansible/tmp/ansible_mitogen_action_78b7c26ed6a4d40c/.source.outq\r\x85q\x0eRq\x0fu\x85q\x10Rq\x11tq\x12.'
ERROR! [mux  140446] 15:57:01.000550 E mitogen.[ssh.localhost:2201]: raw pickle was: b'\x80\x02(X%\x00\x00\x00uv2404-142264-721cfe0e1080-18e27183daq\x00X\x16\x00\x00\x00ansible_mitogen.targetq\x01NX\r\x00\x00\x00transfer_fileq\x02)cmitogen.core\nKwargs\nq\x03}q\x04(X\x07\x00\x00\x00contextq\x05cmitogen.core\n_unpickle_context\nq\x06K\x00N\x86q\x07Rq\x08X\x07\x00\x00\x00in_pathq\tX\x14\x00\x00\x00/tmp/copy-large-fileq\nX\x08\x00\x00\x00out_pathq\x0bcansible.utils.unsafe_proxy\nAnsibleUnsafeText\nq\x0cX]\x00\x00\x00/home/mitogen__has_sudo_nopw/.ansible/tmp/ansible_mitogen_action_92f38c8edfa56781/.source.outq\r\x85q\x0eRq\x0fu\x85q\x10Rq\x11tq\x12.'
ERROR! [mux  140446] 15:57:01.007620 E mitogen.[ssh.localhost:2203]: raw pickle was: b'\x80\x02(X%\x00\x00\x00uv2404-142266-721cfe0e1080-18e271a183q\x00X\x16\x00\x00\x00ansible_mitogen.targetq\x01NX\r\x00\x00\x00transfer_fileq\x02)cmitogen.core\nKwargs\nq\x03}q\x04(X\x07\x00\x00\x00contextq\x05cmitogen.core\n_unpickle_context\nq\x06K\x00N\x86q\x07Rq\x08X\x07\x00\x00\x00in_pathq\tX\x14\x00\x00\x00/tmp/copy-large-fileq\nX\x08\x00\x00\x00out_pathq\x0bcansible.utils.unsafe_proxy\nAnsibleUnsafeText\nq\x0cX]\x00\x00\x00/home/mitogen__has_sudo_nopw/.ansible/tmp/ansible_mitogen_action_19afbf54168d85d7/.source.outq\r\x85q\x0eRq\x0fu\x85q\x10Rq\x11tq\x12.'
An exception occurred during task execution. To see the full traceback, use -vvv. The error was:   File "<stdin>", line 899, in _find_global
fatal: [target-debian11-2]: FAILED! => 
  msg: |-
    Unexpected failure during module execution: mitogen.core.StreamError: cannot unpickle 'ansible.utils.unsafe_proxy'/'AnsibleUnsafeText'
      File "<stdin>", line 3860, in _dispatch_one
      File "<stdin>", line 3843, in _parse_request
      File "<stdin>", line 998, in unpickle
      File "<stdin>", line 789, in find_class
      File "<stdin>", line 899, in _find_global
  stdout: ''
An exception occurred during task execution. To see the full traceback, use -vvv. The error was:   File "<stdin>", line 899, in _find_global
fatal: [target-debian10-1]: FAILED! => 
  msg: |-
    Unexpected failure during module execution: mitogen.core.StreamError: cannot unpickle 'ansible.utils.unsafe_proxy'/'AnsibleUnsafeText'
      File "<stdin>", line 3860, in _dispatch_one
      File "<stdin>", line 3843, in _parse_request
      File "<stdin>", line 998, in unpickle
      File "<stdin>", line 789, in find_class
      File "<stdin>", line 899, in _find_global
  stdout: ''
An exception occurred during task execution. To see the full traceback, use -vvv. The error was:   File "<stdin>", line 899, in _find_global
fatal: [target-ubuntu2004-3]: FAILED! => 
  msg: |-
    Unexpected failure during module execution: mitogen.core.StreamError: cannot unpickle 'ansible.utils.unsafe_proxy'/'AnsibleUnsafeText'
      File "<stdin>", line 3860, in _dispatch_one
      File "<stdin>", line 3843, in _parse_request
      File "<stdin>", line 998, in unpickle
      File "<stdin>", line 789, in find_class
      File "<stdin>", line 899, in _find_global
  stdout: ''

The bug was fixed in a previous commit by Jonathan Rosser. This adds testing.
The bug is only triggered when the copy module is used inside a `with_items:`
loop and the destination filename has an extension. A `loop:` loop is not
sufficient.

refs mitogen-hq#1110
@moreati
Copy link
Copy Markdown
Member

moreati commented Aug 30, 2024

@jrosser I think https://github.com/moreati/mitogen/tree/unsafe-large-copy is ready for you to update your branch with. It turns out the test suite already had tests for large file copies, but they weren't inside a with_items: loop so didn't catch this.

@moreati moreati merged commit 289a780 into mitogen-hq:master Sep 2, 2024
jadacyrus pushed a commit to jadacyrus/mitogen that referenced this pull request Jan 28, 2025
This is in anticipation of mitogen-hq#1110, which only exhibits inside a with_items:
loop. For this refactor `loop:` is used, to confirm the refactored tests are
still correct. A subsequent commit will change them to with_items.

The content of the files and their SHA1 checksums are unchanged.
jadacyrus pushed a commit to jadacyrus/mitogen that referenced this pull request Jan 28, 2025
The bug was fixed in a previous commit by Jonathan Rosser. This adds testing.
The bug is only triggered when the copy module is used inside a `with_items:`
loop and the destination filename has an extension. A `loop:` loop is not
sufficient.

refs mitogen-hq#1110
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants