Skip to content

Fix Signature repr() #1155

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Aug 23, 2022
Merged

Conversation

JacobSwanson
Copy link
Contributor

@JacobSwanson JacobSwanson commented Aug 14, 2022

to_unicode() expects a non-NULL x argument (due to strlen(x));
however self->encoding may be NULL, especially if built with
build_signature() (i.e. if called from the commit.author property).

As a result, any repr() on a Signature with a NULL encoding will
cause a segfault.

Other functions manually map NULL to "utf-8" (see
Signature__encoding__get__()), so we'll do the same thing here.

@jdavid
Copy link
Member

jdavid commented Aug 17, 2022

Good catch.

I think it would be more correct to represent NULL as None, so in this case the repr looks like:

pygit2.Signature('Foo Ibáñez', '[email protected]', 1322174594, 60, None)

Then for this to work Signature_init has to be changed to use z instead of s:

    if (!PyArg_ParseTupleAndKeywords(
            args, kwds, "Os|Liz", keywords,
            &py_name, &email, &time, &offset, &encoding))

@JacobSwanson
Copy link
Contributor Author

Agreed that representation has better semantics. Updated to reflect!

Copy link
Member

@jdavid jdavid left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, please rebase, and squash into a single commit.

@JacobSwanson
Copy link
Contributor Author

Fixed up! Let me know if there is anything else I can do. Thanks

@JacobSwanson JacobSwanson requested a review from jdavid August 21, 2022 17:27
@jdavid
Copy link
Member

jdavid commented Aug 22, 2022

One more thing, pygit2/_pygit2.pyi needs to be updated. I think only _encoding and __init__ are concerned:

class Signature:
    _encoding: str | None
    ...
    def __init__(self, name: str, email: str, time: int, offset: int, encoding: str | None) -> None: ...

`to_unicode()` expects a non-NULL `x` argument (due to `strlen(x)`);
however `self->encoding` _may_ be NULL, especially if built with
`build_signature()` (i.e. if called from the `commit.author` property).

As a result, any `repr()` on a `Signature` with a `NULL` `encoding` will
cause a segfault.

While `encoding` defaults to "utf-8" on `__init__()`, the API now allows
users to explicitly initialize `encoding` to `None`.  Within class
methods, any use of a `None` `encoding` defaults to "utf-8".

Note some APIs like `to_unicode()` accept `NULL` for `encoding`, so we
do not perform explicit checks when used.

Additionally add `encoding` comparison to `richcompare()` and fix a test
that used default encoding.
@jdavid jdavid merged commit fbad877 into libgit2:master Aug 23, 2022
@jdavid
Copy link
Member

jdavid commented Aug 23, 2022

Merged, thanks!

@jdavid
Copy link
Member

jdavid commented Aug 24, 2022

FYI, after a second thought I've done follow up commit. The main change is that Signature._encoding will return utf-8 when internally it's NULL. This is to keep backwards compatibility, so users don't need to worry about handling None.

netbsd-srcmastr referenced this pull request in NetBSD/pkgsrc Sep 6, 2022
1.10.1 (2022-08-28)
-------------------------

- Fix segfault in ``Signature`` repr
  `#1155 <https://github.com/libgit2/pygit2/pull/1155>`_

- Linux and macOS wheels for Python 3.11
  `#1154 <https://github.com/libgit2/pygit2/pull/1154>`_


1.10.0 (2022-07-24)
-------------------------

- Upgrade to libgit2 1.5

- Add support for ``GIT_OPT_GET_OWNER_VALIDATION`` and
  ``GIT_OPT_SET_OWNER_VALIDATION``
  `#1150 <https://github.com/libgit2/pygit2/pull/1150>`_

- New ``untracked_files`` and ``ignored`` optional arguments for
  ``Repository.status(...)``
  `#1151 <https://github.com/libgit2/pygit2/pull/1151>`_


1.9.2 (2022-05-24)
-------------------------

- New ``Repository.create_commit_string(...)`` and
  ``Repository.create_commit_with_signature(...)``
  `#1142 <https://github.com/libgit2/pygit2/pull/1142>`_

- Linux and macOS wheels updated to libgit2 v1.4.3

- Remove redundant line
  `#1139 <https://github.com/libgit2/pygit2/pull/1139>`_


1.9.1 (2022-03-22)
-------------------------

- Type hints: added to C code and Branches/References
  `#1121 <https://github.com/libgit2/pygit2/pull/1121>`_
  `#1132 <https://github.com/libgit2/pygit2/pull/1132>`_

- New ``Signature`` supports ``str()`` and ``repr()``
  `#1135 <https://github.com/libgit2/pygit2/pull/1135>`_

- Fix ODB backend's read in big endian architectures
  `#1130 <https://github.com/libgit2/pygit2/pull/1130>`_

- Fix install with poetry
  `#1129 <https://github.com/libgit2/pygit2/pull/1129>`_
  `#1128 <https://github.com/libgit2/pygit2/issues/1128>`_

- Wheels: update to libgit2 v1.4.2

- Tests: fix testing ``parse_diff``
  `#1131 <https://github.com/libgit2/pygit2/pull/1131>`_

- CI: various fixes after migration to libgit2 v1.4


1.9.0 (2022-02-22)
-------------------------

- Upgrade to libgit2 v1.4

- Documentation, new recipes for committing and cloning
  `#1125 <https://github.com/libgit2/pygit2/pull/1125>`_


1.8.0 (2022-02-04)
-------------------------

- Rename ``RemoteCallbacks.progress(...)`` callback to ``.sideband_progress(...)``
  `#1120 <https://github.com/libgit2/pygit2/pull/1120>`_

- New ``Repository.merge_base_many(...)`` and ``Repository.merge_base_octopus(...)``
  `#1112 <https://github.com/libgit2/pygit2/pull/1112>`_

- New ``Repository.listall_stashes()``
  `#1117 <https://github.com/libgit2/pygit2/pull/1117>`_

- Code cleanup
  `#1118 <https://github.com/libgit2/pygit2/pull/1118>`_

Backward incompatible changes:

- The ``RemoteCallbacks.progress(...)`` callback has been renamed to
  ``RemoteCallbacks.sideband_progress(...)``. This matches the documentation,
  but may break existing code that still uses the old name.


1.7.2 (2021-12-06)
-------------------------

- Universal wheels for macOS
  `#1109 <https://github.com/libgit2/pygit2/pull/1109>`_


1.7.1 (2021-11-19)
-------------------------

- New ``Repository.amend_commit(...)``
  `#1098 <https://github.com/libgit2/pygit2/pull/1098>`_

- New ``Commit.message_trailers``
  `#1101 <https://github.com/libgit2/pygit2/pull/1101>`_

- Windows wheels for Python 3.10
  `#1103 <https://github.com/libgit2/pygit2/pull/1103>`_

- Changed: now ``DiffDelta.is_binary`` returns ``None`` if the file data has
  not yet been loaded, cf. `#962 <https://github.com/libgit2/pygit2/issues/962>`_

- Document ``Repository.get_attr(...)`` and update theme
  `#1017 <https://github.com/libgit2/pygit2/issues/1017>`_
  `#1105 <https://github.com/libgit2/pygit2/pull/1105>`_


1.7.0 (2021-10-08)
-------------------------

- Upgrade to libgit2 1.3.0
  `#1089 <https://github.com/libgit2/pygit2/pull/1089>`_

- Linux wheels now bundled with libssh2 1.10.0 (instead of 1.9.0)

- macOS wheels now include libssh2

- Add support for Python 3.10
  `#1092 <https://github.com/libgit2/pygit2/pull/1092>`_
  `#1093 <https://github.com/libgit2/pygit2/pull/1093>`_

- Drop support for Python 3.6

- New `pygit2.GIT_CHECKOUT_SKIP_LOCKED_DIRECTORIES`
  `#1087 <https://github.com/libgit2/pygit2/pull/1087>`_

- New optional argument ``location`` in ``Repository.applies(..)`` and
  ``Repository.apply(..)``
  `#1091 <https://github.com/libgit2/pygit2/pull/1091>`_

- Fix: Now the `flags` argument in `Repository.blame()` is passed through
  `#1083 <https://github.com/libgit2/pygit2/pull/1083>`_

- CI: Stop using Travis, move to GitHub actions

Caveats:

- Windows wheels for Python 3.10 not yet available.
netbsd-srcmastr referenced this pull request in NetBSD/pkgsrc Sep 6, 2022
1.10.1 (2022-08-28)
-------------------------

- Fix segfault in ``Signature`` repr
  `#1155 <https://github.com/libgit2/pygit2/pull/1155>`_

- Linux and macOS wheels for Python 3.11
  `#1154 <https://github.com/libgit2/pygit2/pull/1154>`_


1.10.0 (2022-07-24)
-------------------------

- Upgrade to libgit2 1.5

- Add support for ``GIT_OPT_GET_OWNER_VALIDATION`` and
  ``GIT_OPT_SET_OWNER_VALIDATION``
  `#1150 <https://github.com/libgit2/pygit2/pull/1150>`_

- New ``untracked_files`` and ``ignored`` optional arguments for
  ``Repository.status(...)``
  `#1151 <https://github.com/libgit2/pygit2/pull/1151>`_


1.9.2 (2022-05-24)
-------------------------

- New ``Repository.create_commit_string(...)`` and
  ``Repository.create_commit_with_signature(...)``
  `#1142 <https://github.com/libgit2/pygit2/pull/1142>`_

- Linux and macOS wheels updated to libgit2 v1.4.3

- Remove redundant line
  `#1139 <https://github.com/libgit2/pygit2/pull/1139>`_


1.9.1 (2022-03-22)
-------------------------

- Type hints: added to C code and Branches/References
  `#1121 <https://github.com/libgit2/pygit2/pull/1121>`_
  `#1132 <https://github.com/libgit2/pygit2/pull/1132>`_

- New ``Signature`` supports ``str()`` and ``repr()``
  `#1135 <https://github.com/libgit2/pygit2/pull/1135>`_

- Fix ODB backend's read in big endian architectures
  `#1130 <https://github.com/libgit2/pygit2/pull/1130>`_

- Fix install with poetry
  `#1129 <https://github.com/libgit2/pygit2/pull/1129>`_
  `#1128 <https://github.com/libgit2/pygit2/issues/1128>`_

- Wheels: update to libgit2 v1.4.2

- Tests: fix testing ``parse_diff``
  `#1131 <https://github.com/libgit2/pygit2/pull/1131>`_

- CI: various fixes after migration to libgit2 v1.4


1.9.0 (2022-02-22)
-------------------------

- Upgrade to libgit2 v1.4

- Documentation, new recipes for committing and cloning
  `#1125 <https://github.com/libgit2/pygit2/pull/1125>`_


1.8.0 (2022-02-04)
-------------------------

- Rename ``RemoteCallbacks.progress(...)`` callback to ``.sideband_progress(...)``
  `#1120 <https://github.com/libgit2/pygit2/pull/1120>`_

- New ``Repository.merge_base_many(...)`` and ``Repository.merge_base_octopus(...)``
  `#1112 <https://github.com/libgit2/pygit2/pull/1112>`_

- New ``Repository.listall_stashes()``
  `#1117 <https://github.com/libgit2/pygit2/pull/1117>`_

- Code cleanup
  `#1118 <https://github.com/libgit2/pygit2/pull/1118>`_

Backward incompatible changes:

- The ``RemoteCallbacks.progress(...)`` callback has been renamed to
  ``RemoteCallbacks.sideband_progress(...)``. This matches the documentation,
  but may break existing code that still uses the old name.


1.7.2 (2021-12-06)
-------------------------

- Universal wheels for macOS
  `#1109 <https://github.com/libgit2/pygit2/pull/1109>`_


1.7.1 (2021-11-19)
-------------------------

- New ``Repository.amend_commit(...)``
  `#1098 <https://github.com/libgit2/pygit2/pull/1098>`_

- New ``Commit.message_trailers``
  `#1101 <https://github.com/libgit2/pygit2/pull/1101>`_

- Windows wheels for Python 3.10
  `#1103 <https://github.com/libgit2/pygit2/pull/1103>`_

- Changed: now ``DiffDelta.is_binary`` returns ``None`` if the file data has
  not yet been loaded, cf. `#962 <https://github.com/libgit2/pygit2/issues/962>`_

- Document ``Repository.get_attr(...)`` and update theme
  `#1017 <https://github.com/libgit2/pygit2/issues/1017>`_
  `#1105 <https://github.com/libgit2/pygit2/pull/1105>`_


1.7.0 (2021-10-08)
-------------------------

- Upgrade to libgit2 1.3.0
  `#1089 <https://github.com/libgit2/pygit2/pull/1089>`_

- Linux wheels now bundled with libssh2 1.10.0 (instead of 1.9.0)

- macOS wheels now include libssh2

- Add support for Python 3.10
  `#1092 <https://github.com/libgit2/pygit2/pull/1092>`_
  `#1093 <https://github.com/libgit2/pygit2/pull/1093>`_

- Drop support for Python 3.6

- New `pygit2.GIT_CHECKOUT_SKIP_LOCKED_DIRECTORIES`
  `#1087 <https://github.com/libgit2/pygit2/pull/1087>`_

- New optional argument ``location`` in ``Repository.applies(..)`` and
  ``Repository.apply(..)``
  `#1091 <https://github.com/libgit2/pygit2/pull/1091>`_

- Fix: Now the `flags` argument in `Repository.blame()` is passed through
  `#1083 <https://github.com/libgit2/pygit2/pull/1083>`_

- CI: Stop using Travis, move to GitHub actions

Caveats:

- Windows wheels for Python 3.10 not yet available.
jorio added a commit to jorio/pygit2 that referenced this pull request Apr 1, 2023
`to_unicode(..., ..., NULL)` is equivalent to `to_unicode(..., ..., "strict")`. It returns NULL if the string cannot be decoded to the desired encoding.

PyUnicode_FromFormat will crash if we pass NULL pointers to be formatted as %R or %U.

Instead, let's use `to_unicode(..., ..., "replace")` so that we always get a non-NULL object to pass to PyUnicode_FromFormat.

This ties in with libgit2#1155.
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