D7815: sha1dc: initial implementation of Python extension

indygreg (Gregory Szorc) phabricator at mercurial-scm.org
Wed Jan 8 23:29:08 EST 2020


indygreg added a comment.


  This seems mostly good to me.

INLINE COMMENTS

> cext.c:68
> +	if (SHA1DCFinal(hash_out, &ctx)) {
> +		PyErr_SetString(PyExc_OverflowError,
> +				"sha1 collision attack detected");

I'm not super keen on overloading `OverflowError` here. Or is this how `hashlib` works in the Python standard library?

> cext.c:92
> +	static const char hexdigit[] = "0123456789abcdef";
> +	for (int i = 0; i < 20; ++i) {
> +		hexhash[i * 2] = hexdigit[hash[i] >> 4];

I didn't verify this is correct because I didn't want to think too hard. Test coverage for parity with `hashlib` would be appreciated.

> cext.c:107
> +	}
> +	clone->ctx = self->ctx;
> +	return (PyObject *)clone;

Should this be a `memcpy` or some such? Or is this opaque type safe to copy by value? Test coverage for this demonstrating that a seeded hasher which is copied can properly diverge would be appreciated.

> cext.c:172
> +	sha1ctxType.tp_new = PyType_GenericNew;
> +	if (PyType_Ready(&sha1ctxType) < 0)
> +		return;

Nit: please always use braces.

REPOSITORY
  rHG Mercurial

CHANGES SINCE LAST ACTION
  https://phab.mercurial-scm.org/D7815/new/

REVISION DETAIL
  https://phab.mercurial-scm.org/D7815

To: durin42, #hg-reviewers
Cc: indygreg, spectral, mjpieters, mercurial-devel


More information about the Mercurial-devel mailing list