libgit2 / pygit2

Python bindings for libgit2
https://www.pygit2.org/
Other
1.61k stars 385 forks source link

Fix Signature repr() #1155

Closed JacobSwanson closed 2 years ago

JacobSwanson commented 2 years ago

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 commented 2 years ago

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', 'foo@bar.com', 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 commented 2 years ago

Agreed that representation has better semantics. Updated to reflect!

JacobSwanson commented 2 years ago

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

jdavid commented 2 years ago

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: ...
jdavid commented 2 years ago

Merged, thanks!

jdavid commented 2 years ago

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.