Skip to content

Improve GF2Field construction, validation and error messages - #1962

Merged
tanujkhattar merged 4 commits into
quantumlib:mainfrom
tanujkhattar:gf2_field_improvements
Oct 15, 2026
Merged

tanujkhattar merged 4 commits into
quantumlib:mainfrom
tanujkhattar:gf2_field_improvements

Conversation

@tanujkhattar

@tanujkhattar tanujkhattar commented Oct 3, 2026 •

Copy link
Copy Markdown
Collaborator
  • 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.
  • Better docstrings/comments in the GF(2) arithmetic helpers.
  • GF2Field also exposes a primitive_element

- 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.

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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.

Comment on lines +183 to +184
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);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

high

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);

Comment on lines +106 to +120
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) + ").");
}
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

security-high high

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'

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I liked it being an integer... but I'm not vetoing.

Comment thread docs/kickmix/api_reference.md Outdated

# (in class kickmix.GF2Field)
@staticmethod
def from_galois(

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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").

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

removed

@tanujkhattar
tanujkhattar added this pull request to the merge queue Oct 15, 2026
Merged via the queue into quantumlib:main with commit 242addf Oct 15, 2026
28 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants