openbsd_pkg: Fix regexp matching crash

[zorun]

SUMMARY

Fix regexp crash caused by missing escaping of package name.

ISSUE TYPE

• Bugfix Pull Request

COMPONENT NAME

openbsd_pkg

ADDITIONAL INFORMATION

When a package name contains special characters (e.g. "g++"), they are
interpreted as part of the regexp.

This can lead to a crash with an error in the python re module, for
instance with "g++":

sre_constants.error: multiple repeat

Fix this by escaping the package name.

[zorun]

This bug is present since the beginning of this repo (I haven't checked in the previous ansible repo), so it would be nice to backport the fix to stable releases if it makes sense.

[ansibullbot]

cc @JoergFiedler @MacLemon @bcoca @dch @eest @jasperla @mekanix @opoplawski @overhacked @tuxillo
click here for bot help

[eest]

Hello @zorun, thanks for reporting this issue! It indeed looks like a bug and your fix looks good from a cursory review. I’ll get back to you when I have been able to properly review and test this locally 🙂.

[Ajpantuso]

Please add a changelog fragment. Unit tests for this fix would be nice as well to demonstrate it's effectiveness and avoid future regressions. If you don't want to write tests for the full package_present and package_latest functions I would wrap these regex tests into functions which just return a bool based on the search result. And since the last feature was added in community,general 1.3.0 I don't think backporting will be an issue. Thanks!

[zorun]

Hello @zorun, thanks for reporting this issue! It indeed looks like a bug and your fix looks good from a cursory review. I’ll get back to you when I have been able to properly review and test this locally slightly_smiling_face.

Thanks!

If you want to reproduce, I should mention that I triggered this bug when the package is not found. It triggered while trying to install g++ on OpenBSD/mips, where it does not exist. This was on gcc231 here: https://cfarm.tetaneutral.net/machines/list/

[zorun]

Please add a changelog fragment. Unit tests for this fix would be nice as well to demonstrate it's effectiveness and avoid future regressions. If you don't want to write tests for the full package_present and package_latest functions I would wrap these regex tests into functions which just return a bool based on the search result. And since the last feature was added in community,general 1.3.0 I don't think backporting will be an issue. Thanks!

I added a changelog entry, thanks for the pointer.

About tests, I didn't find any existing unit test for openbsd_pkg. Do you have pointers to documentation or a simple test I could use as a template?

[Ajpantuso]

About tests, I didn't find any existing unit test for openbsd_pkg. Do you have pointers to documentation or a simple test I could use as a template?

The Homebrew module has a simple unit test for one of it's functions that might help you get started.

[felixfontein]

FYI: 3.5.0 and 2.5.5 will be released tomorrow. @eest if you can confirm that this fix works by then, I'll gladly include it even if there are no tests yet. (The change looks safe enough to me that it's ok to add the tests later.)

[eest]

If you want to reproduce, I should mention that I triggered this bug when the package is not found. It triggered while trying to install g++ on OpenBSD/mips, where it does not exist. This was on gcc231 here: https://cfarm.tetaneutral.net/machines/list/

Thanks for the pointer, I don't have access to a mips machine but I could instead reproduce it on amd64 by adding a third plus sign. The result before your fix:

$ ansible testservers -u root -m openbsd_pkg -a "name=g+++ state=present"
[WARNING]: You are running the development version of Ansible. You should only run Ansible from "devel" if you are modifying the Ansible engine, or trying out features under development. This is a rapidly changing source of
code and can become unstable at any point.
An exception occurred during task execution. To see the full traceback, use -vvv. The error was: re.error: multiple repeat at position 4
192.168.1.60 | FAILED! => {
    "changed": false,
    "module_stderr": "Shared connection to 192.168.1.60 closed.\r\n",
    "module_stdout": "Traceback (most recent call last):\r\n  File \"/root/.ansible/tmp/ansible-tmp-1628524855.9675233-81581-139529110244626/AnsiballZ_openbsd_pkg.py\", line 100, in <module>\r\n    _ansiballz_main()\r\n  File \"/root/.ansible/tmp/ansible-tmp-1628524855.9675233-81581-139529110244626/AnsiballZ_openbsd_pkg.py\", line 92, in _ansiballz_main\r\n    invoke_module(zipped_mod, temp_path, ANSIBALLZ_PARAMS)\r\n  File \"/root/.ansible/tmp/ansible-tmp-1628524855.9675233-81581-139529110244626/AnsiballZ_openbsd_pkg.py\", line 40, in invoke_module\r\n    runpy.run_module(mod_name='ansible_collections.community.general.plugins.modules.openbsd_pkg', init_globals=dict(_module_fqn='ansible_collections.community.general.plugins.modules.openbsd_pkg', _modlib_path=modlib_path),\r\n  File \"/usr/local/lib/python3.9/runpy.py\", line 210, in run_module\r\n    return _run_module_code(code, init_globals, run_name, mod_spec)\r\n  File \"/usr/local/lib/python3.9/runpy.py\", line 97, in _run_module_code\r\n    _run_code(code, mod_globals, init_globals,\r\n  File \"/usr/local/lib/python3.9/runpy.py\", line 87, in _run_code\r\n    exec(code, run_globals)\r\n  File \"/tmp/ansible_openbsd_pkg_payload_x95_4gkl/ansible_openbsd_pkg_payload.zip/ansible_collections/community/general/plugins/modules/openbsd_pkg.py\", line 653, in <module>\r\n  File \"/tmp/ansible_openbsd_pkg_payload_x95_4gkl/ansible_openbsd_pkg_payload.zip/ansible_collections/community/general/plugins/modules/openbsd_pkg.py\", line 605, in main\r\n  File \"/tmp/ansible_openbsd_pkg_payload_x95_4gkl/ansible_openbsd_pkg_payload.zip/ansible_collections/community/general/plugins/modules/openbsd_pkg.py\", line 244, in package_present\r\n  File \"/usr/local/lib/python3.9/re.py\", line 201, in search\r\n    return _compile(pattern, flags).search(string)\r\n  File \"/usr/local/lib/python3.9/re.py\", line 304, in _compile\r\n    p = sre_compile.compile(pattern, flags)\r\n  File \"/usr/local/lib/python3.9/sre_compile.py\", line 764, in compile\r\n    p = sre_parse.parse(p, flags)\r\n  File \"/usr/local/lib/python3.9/sre_parse.py\", line 948, in parse\r\n    p = _parse_sub(source, state, flags & SRE_FLAG_VERBOSE, 0)\r\n  File \"/usr/local/lib/python3.9/sre_parse.py\", line 443, in _parse_sub\r\n    itemsappend(_parse(source, state, verbose, nested + 1,\r\n  File \"/usr/local/lib/python3.9/sre_parse.py\", line 671, in _parse\r\n    raise source.error(\"multiple repeat\",\r\nre.error: multiple repeat at position 4\r\n",
    "msg": "MODULE FAILURE\nSee stdout/stderr for the exact error",
    "rc": 1
}

... and after your fix:

$ ansible testservers -u root -m openbsd_pkg -a "name=g+++ state=present"
[WARNING]: You are running the development version of Ansible. You should only run Ansible from "devel" if you are modifying the Ansible engine, or trying out features under development. This is a rapidly changing source of
code and can become unstable at any point.
192.168.1.60 | FAILED! => {
    "build": false,
    "changed": false,
    "msg": "Can't find g+++\n",
    "name": [
        "g+++"
    ],
    "state": "present"
}

A clear improvement 🙂 .

As for the other instance you fixed, given that g++ was already installed it could be verified by trying to run state=latest:

$ ansible testservers -u root -m openbsd_pkg -a "name=g++ state=latest"
[WARNING]: You are running the development version of Ansible. You should only run Ansible from "devel" if you are modifying the Ansible engine, or trying out features under development. This is a rapidly changing source of
code and can become unstable at any point.
An exception occurred during task execution. To see the full traceback, use -vvv. The error was: re.error: multiple repeat at position 4
192.168.1.60 | FAILED! => {
    "changed": false,
    "module_stderr": "Shared connection to 192.168.1.60 closed.\r\n",
    "module_stdout": "Traceback (most recent call last):\r\n  File \"/root/.ansible/tmp/ansible-tmp-1628528067.6084132-82123-205939577525348/AnsiballZ_openbsd_pkg.py\", line 100, in <module>\r\n    _ansiballz_main()\r\n  File \"/root/.ansible/tmp/ansible-tmp-1628528067.6084132-82123-205939577525348/AnsiballZ_openbsd_pkg.py\", line 92, in _ansiballz_main\r\n    invoke_module(zipped_mod, temp_path, ANSIBALLZ_PARAMS)\r\n  File \"/root/.ansible/tmp/ansible-tmp-1628528067.6084132-82123-205939577525348/AnsiballZ_openbsd_pkg.py\", line 40, in invoke_module\r\n    runpy.run_module(mod_name='ansible_collections.community.general.plugins.modules.openbsd_pkg', init_globals=dict(_module_fqn='ansible_collections.community.general.plugins.modules.openbsd_pkg', _modlib_path=modlib_path),\r\n  File \"/usr/local/lib/python3.9/runpy.py\", line 210, in run_module\r\n    return _run_module_code(code, init_globals, run_name, mod_spec)\r\n  File \"/usr/local/lib/python3.9/runpy.py\", line 97, in _run_module_code\r\n    _run_code(code, mod_globals, init_globals,\r\n  File \"/usr/local/lib/python3.9/runpy.py\", line 87, in _run_code\r\n    exec(code, run_globals)\r\n  File \"/tmp/ansible_openbsd_pkg_payload_twyguv_g/ansible_openbsd_pkg_payload.zip/ansible_collections/community/general/plugins/modules/openbsd_pkg.py\", line 653, in <module>\r\n  File \"/tmp/ansible_openbsd_pkg_payload_twyguv_g/ansible_openbsd_pkg_payload.zip/ansible_collections/community/general/plugins/modules/openbsd_pkg.py\", line 609, in main\r\n  File \"/tmp/ansible_openbsd_pkg_payload_twyguv_g/ansible_openbsd_pkg_payload.zip/ansible_collections/community/general/plugins/modules/openbsd_pkg.py\", line 298, in package_latest\r\n  File \"/usr/local/lib/python3.9/re.py\", line 201, in search\r\n    return _compile(pattern, flags).search(string)\r\n  File \"/usr/local/lib/python3.9/re.py\", line 304, in _compile\r\n    p = sre_compile.compile(pattern, flags)\r\n  File \"/usr/local/lib/python3.9/sre_compile.py\", line 764, in compile\r\n    p = sre_parse.parse(p, flags)\r\n  File \"/usr/local/lib/python3.9/sre_parse.py\", line 948, in parse\r\n    p = _parse_sub(source, state, flags & SRE_FLAG_VERBOSE, 0)\r\n  File \"/usr/local/lib/python3.9/sre_parse.py\", line 443, in _parse_sub\r\n    itemsappend(_parse(source, state, verbose, nested + 1,\r\n  File \"/usr/local/lib/python3.9/sre_parse.py\", line 671, in _parse\r\n    raise source.error(\"multiple repeat\",\r\nre.error: multiple repeat at position 4\r\n",
    "msg": "MODULE FAILURE\nSee stdout/stderr for the exact error",
    "rc": 1
}

... and again with the fix:

$ ansible testservers -u root -m openbsd_pkg -a "name=g++ state=latest"
[WARNING]: You are running the development version of Ansible. You should only run Ansible from "devel" if you are modifying the Ansible engine, or trying out features under development. This is a rapidly changing source of
code and can become unstable at any point.
192.168.1.60 | SUCCESS => {
    "build": false,
    "changed": false,
    "name": [
        "g++"
    ],
    "state": "latest"
}

I have verified upgrades still work as expected by installing g++ on a 6.8 machine, running sysupgrade(8) to upgrade it to 6.9, and then upgrading the package via state=latest (changed: true followed by changed: false):

$ ansible testservers -u root -m openbsd_pkg -a "name=g++ state=latest"
[WARNING]: You are running the development version of Ansible. You should only run Ansible from "devel" if you
are modifying the Ansible engine, or trying out features under development. This is a rapidly changing source
of code and can become unstable at any point.
192.168.1.62 | CHANGED => {
    "build": false,
    "changed": true,
    "name": [
        "g++"
    ],
    "state": "latest"
}

$ ansible testservers -u root -m openbsd_pkg -a "name=g++ state=latest"
[WARNING]: You are running the development version of Ansible. You should only run Ansible from "devel" if you
are modifying the Ansible engine, or trying out features under development. This is a rapidly changing source
of code and can become unstable at any point.
192.168.1.62 | SUCCESS => {
    "build": false,
    "changed": false,
    "name": [
        "g++"
    ],
    "state": "latest"
}

Since the module documentation at https://docs.ansible.com/ansible/latest/collections/community/general/openbsd_pkg_module.html states python >= 2.5 as the version requirement I took a look at the old docs to make sure the escape function is present in older versions of python, and it is: https://docs.python.org/2/library/re.html#re.escape

As for why there are no tests in the code I think this is because the majority of this module was written at a time when there was no testing infrastructure in place (at least that I was aware about). I have run the fixed verson of the code through my own testing playbook at https://github.com/eest/openbsd_pkg-tests which succeeded.

@felixfontein you have my OK to merge this PR.
@zorun thanks again for finding and fixing this!

[patchback]

Backport to stable-2: 💚 backport PR created

✅ Backport PR branch: patchback/backports/stable-2/56b5be0630e226f20cf461b2ab9c722de5e34483/pr-3161

Backported as #3185

🤖 @patchback
I'm built with octomachinery and
my source is open — https://github.com/sanitizers/patchback-github-app.

[patchback]

Backport to stable-3: 💚 backport PR created

✅ Backport PR branch: patchback/backports/stable-3/56b5be0630e226f20cf461b2ab9c722de5e34483/pr-3161

Backported as #3186

🤖 @patchback
I'm built with octomachinery and
my source is open — https://github.com/sanitizers/patchback-github-app.

[felixfontein]

@zorun thanks a lot for fixing this!
@Ajpantuso @eest thanks a lot for reviewing and testing this!

[zorun]

Excellent, thanks for the in-depth review @eest !