Repository navigation
Improve GF2Field construction, validation and error messages - #1962
Conversation
- GF2Field checks irreducibility by default (also in C++), and a reducible modulus error names an irreducible factor and suggests a working modulus. - GF2Field accepts polynomial strings like "x^8 + x^4 + x^3 + x^2 + 1"; repeated terms are rejected. - Circuit builder GF(2^m) helpers take `field: km.GF2Field | None` only. - Better validation: out of range degrees, non-element imul/idiv constants, and field/register degree mismatches give actionable ValueErrors. - Rejected init_gf2_* calls no longer leave stray resets or allocations in the builder (sizes are validated before any operation is emitted). - Fix stale docstrings/comments in the GF(2) arithmetic helpers.
There was a problem hiding this comment.
Code Review
This pull request enhances Galois Field arithmetic in kickmix by adding support for algebraic polynomial string parsing, integrating prime factor data for 2^m - 1 up to m=512 to validate and retrieve primitive elements, and allowing direct integration with the Python galois library. It also introduces stricter validation to reject reducible moduli and report their factors, alongside comprehensive test coverage. Review feedback identifies two high-severity issues: a missing type check in py_int_to_field_element that could cause crashes when non-integers are passed, and an integer overflow vulnerability in the polynomial exponent parsing loop that could bypass maximum bit limits.
| GF2Poly py_int_to_field_element(const pybind11::object &value_obj, const GF2Field &field, const char *name) { | ||
| GF2Poly value = py_int_to_gf2_poly(pybind11::reinterpret_borrow<pybind11::int_>(value_obj), name); |
There was a problem hiding this comment.
The function py_int_to_field_element uses pybind11::reinterpret_borrow<pybind11::int_> on value_obj without verifying that it is actually a Python integer. If a non-integer type (such as a float, string, or list) is passed from Python, this will bypass type checking and cause undefined behavior or crashes in C++ when py_int_to_gf2_poly is called. We should validate the input type using is_py_int first.
GF2Poly py_int_to_field_element(const pybind11::object &value_obj, const GF2Field &field, const char *name) {
if (!is_py_int(value_obj)) {
throw std::invalid_argument(std::string(name) + " must be an int.");
}
GF2Poly value = py_int_to_gf2_poly(pybind11::reinterpret_borrow<pybind11::int_>(value_obj), name);| size_t exp = 0; | ||
| for (char c : exp_str) { | ||
| if (c < '0' || c > '9') { | ||
| throw std::invalid_argument( | ||
| "Invalid exponent in polynomial term \"" + std::string(token) + "\" in \"" + | ||
| std::string(raw_text) + "\". Expected a non-negative integer, e.g. \"x^4 + x + 1\"."); | ||
| } | ||
| exp = exp * 10 + static_cast<size_t>(c - '0'); | ||
| if (exp >= MAX_BITS) { | ||
| throw std::invalid_argument( | ||
| "Polynomial term \"" + std::string(token) + | ||
| "\" has exponent >= " + std::to_string(MAX_BITS) + " (maximum supported field degree is " + | ||
| std::to_string(GF2_MAX_DEGREE) + ")."); | ||
| } | ||
| } |
There was a problem hiding this comment.
The exponent parsing loop is vulnerable to integer overflow if exp_str is extremely long (e.g., containing more digits than can fit in a size_t). If exp overflows and wraps around to a small value less than MAX_BITS, the validation check exp >= MAX_BITS will be bypassed, leading to incorrect parsing of the polynomial. We should prevent overflow by checking if exp has already exceeded MAX_BITS inside the loop.
size_t exp = 0;
for (char c : exp_str) {
if (c < '0' || c > '9') {
throw std::invalid_argument(
"Invalid exponent in polynomial term \"" + std::string(token) + "\" in \"" +
std::string(raw_text) + "\". Expected a non-negative integer, e.g. \"x^4 + x + 1\".");
}
if (exp >= MAX_BITS) {
continue;
}
exp = exp * 10 + static_cast<size_t>(c - '0');
}
if (exp >= MAX_BITS) {
throw std::invalid_argument(
"Polynomial term \"" + std::string(token) +
"\" has exponent >= " + std::to_string(MAX_BITS) + " (maximum supported field degree is " +
std::to_string(GF2_MAX_DEGREE) + ").");
}| 2 | ||
| >>> aes_field = km.GF2Field(8, modulus=0x11B) | ||
| >>> hex(aes_field.modulus) | ||
| '0x11b' |
There was a problem hiding this comment.
I liked it being an integer... but I'm not vetoing.
|
|
||
| # (in class kickmix.GF2Field) | ||
| @staticmethod | ||
| def from_galois( |
There was a problem hiding this comment.
Because this is a simple conversion, it would be more pythonic to do this via the initializer. For example, consider that you write int("55") instead of int.from_str("55").
field: km.GF2Field | Noneonly.primitive_element