Lack of Input Validation in Solver Parameters Allows Arbitrary Key-Value Pairs

[anilbeycorintis]

Describe the current issue

Happy New Year! 🎉

Thank you for developing and maintaining Firedrake—it’s an exceptional tool. We’ve noticed an issue that could enhance its robustness and usability.

Currently, the code snippet below runs without any errors, even with invalid or nonsensical solver_parameters. For example:

• A typo like "snes_rtolx" instead of "snes_rtol" is silently ignored, potentially causing incorrect solver behavior.
• Providing a parameter with the wrong type, such as None instead of a boolean like False, also doesn’t raise an error, leading to undefined behavior.

This lack of validation can result in unnoticed configuration errors, wasted resources, or incorrect results, particularly when such mistakes persist for extended periods.

Would it be possible to validate parameter names and types before creating the solver object? This improvement would greatly enhance reliability and help users avoid subtle but critical mistakes. Thank you!

import firedrake as fd

# Minimal problem setup
mesh = fd.UnitSquareMesh(1, 1)
V = fd.FunctionSpace(mesh, "CG", 1)
u = fd.Function(V)
v = fd.TestFunction(V)
F = fd.inner(fd.grad(u), fd.grad(v)) * fd.dx

# Arbitrary solver parameters without validation
variational_solver_parameters = {
    "homotopy_iterations": 10,
    "momotopy_iterations": 9,
    "snes_max_it": 10,
    "snes_max_iter": 2,
    "snes_maximum_it": 3,
    "snes_maximum_iter": 4,
    "snes_maximum_iterations": 5,
    "snes_max_its": 6,
    "is_this_a_valid_parameter?": True,
    "hello": "hi!",
    "answer_to_life_the_universe_and_everything": 42,
    "master_yoda": "validate_we_do_not?",
}

problem = fd.NonlinearVariationalProblem(F, u)
solver = fd.NonlinearVariationalSolver(
    problem,
    solver_parameters=variational_solver_parameters,
)
solver.solve()

# Code runs without error, showing no input validation on solver parameters.

Describe the solution you'd like
To address this issue, input validation should be implemented for solver parameters:

1. Key Validation:

   • The solver should only accept keys that are recognized as valid.
   • If an unrecognized key is provided, the solver should raise an error during initialization.

2. Type Validation:

   • The solver should check that the value provided for each parameter is of the correct type.
   • If the type is incorrect, the solver should raise a clear error before attempting to solve.

For example, the following input should raise an error:

solver_parameters = {
    "snes_max_it": "ten",  # Invalid type
    "unknown_key": 42,     # Invalid key
}

Additional info

This issue is a classic example of improper input validation, a vulnerability listed by CWE MITRE as CWE-20: Improper Input Validation. Addressing this not only improves user experience but also aligns Firedrake with best practices for software reliability.

[rckirby]

Thanks for the note on this -- I agree this is a challenge, and have wrangled with such parameter errors many times. However, our parameter dictionaries are really just providing a "pass-through" service to the PETSc options database, and dont' actually know what the list of valid parameters are. It also depends on what options PETSc was configured with. To validate, we would need each PETSc object (including third-party extensions!) to provide a list of what available options and their types/value ranges are supported. I think this is actually a quite hard problem that may be mostly beyond Firedrake's control?

At any rate, PETSc's -options_left parameter will at least print out warnings of unused parameters. This should catch your "unknown_key" error, but handling of invalid types/values would be at the mercy of individual PETSc objects (and different such objects may be more or less helpful in their error messages).

[anilbeycorintis]

Thank you so much, @rckirby, for your prompt response! I now have a much clearer understanding of the issue.

At any rate, PETSc's -options_left parameter will at least print out warnings of unused parameters. This should catch your "unknown_key" error.

Can this parameter be enabled directly from within Firedrake, or does it require calling PETSc explicitly?

Additionally, would you happen to know if there’s a way to call a PETSc function solely for input validation prior to creating Firedrake objects and running the simulation? If yes, that would be really helpful.

[connorjward]

Can this parameter be enabled directly from within Firedrake, or does it require calling PETSc explicitly?

I think -options_left is a bit buggy. Adding it to the parameters dict you provided does not produce any warnings.

I believe that it is enabled by default but only for command line options. Running a Firedrake script

$ python myscript.py -some_option_or_other

produces

WARNING! There are options you set that were not used!
WARNING! could be spelling mistake, etc!
There is one unused database option. It is:
Option left: name:-some_option_or_other (no value) source: command line

I think this only works at the outermost level and does not propagate through to the solver options dictionaries.

Additionally, would you happen to know if there’s a way to call a PETSc function solely for input validation prior to creating Firedrake objects and running the simulation? If yes, that would be really helpful.

Almost certainly not. The instantiation of Firedrake objects dynamically declares a lot of these parameters before which validation can't work.

[dham]

This is a bugbear of mine too. I would really love PETSc to fix this but it's a huge job so it's unlikely to happen, sadly.

[connorjward]

@JHopeCollins realised why options_left does not work for solver parameter dictionaries. When we solve we only temporarily insert options into the options database before popping them back out after the solve. This means that they aren't present at PETSc finalisation for PETSc to complain about.

[colinjcotter]

Would it help to just not pop the options after the solve? On 14 Jan 2025, at 16:04, David A. Ham ***@***.***> wrote:  This is a bugbear of mine too. I would really love PETSc to fix this but it's a huge job so it's unlikely to happen, sadly. — Reply to this email directly, view it on GitHub<#3968 (comment)>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/ABOSV4SOA7QWWKDXLVLJ2BT2KUYSPAVCNFSM6AAAAABVC5WSLSVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDKOJQGM2TENBQGE>. You are receiving this because you are subscribed to this thread.Message ID: ***@***.***>

[dham]

@JHopeCollins realised why options_left does not work for solver parameter dictionaries. When we solve we only temporarily insert options into the options database before popping them back out after the solve. This means that they aren't present at PETSc finalisation for PETSc to complain about.

Could we perform the unused check on the options at the time that we pop them?

[JHopeCollins]

There's a number of questions/thoughts I had about the way the options manager handles options.

1. I don't know what the motivation is for inserting/removing the options from parameters dictionary from the global Options - why not just put them in and leave them, like Colin said. In docstring says:
   This ensures that the options database has the relevant entries for the duration of the with block, before removing them afterwards. This is a much more robust way of dealing with the fixed-size options database than trying to clear it out using destructors.
   But this doesn't explain why you want to remove them.

2. Why do we grab and parse the entire set of command line options to find the ones we need? Why not use PETSc.Options(prefix) and let PETSc do the parsing?

3. There's also a comment about flag options not DTRT, but I don't know Wrong Thing they do instead, and I suspect it may be fixed by doing 2.

[JHopeCollins]

Could we perform the unused check on the options at the time that we pop them?

This is a good idea, and should be relatively easy to do. If the option hasn't been used we could keep it in the global dictionary so it still gets picked up in PETSc Finalize.

[connorjward]

Could we perform the unused check on the options at the time that we pop them?

This is a good idea, and should be relatively easy to do. If the option hasn't been used we could keep it in the global dictionary so it still gets picked up in PETSc Finalize.

But this pollutes the global state in potentially quite a strange way.

[JHopeCollins]

Could keep a _firedrake_list_of_unused_optionsand put these back in the dictionary just before PETSc_Finalize?

[JHopeCollins]

In principle I might be ok with "polluting" global state (which we would also do if we don't pop the options after each solve) because I don't think PETSc expects/wants you to ever have multiple objects with the same prefix but different options.

[wence-]

But this doesn't explain why you want to remove them.

By default, every new solver in firedrake has a unique prefix. If you never pop the options out of the database after running the solve then it grows unboundedly -> BAD (used to be really bad because the options database was fixed size, so you would get a crash). Now you just get a hard-to-diagnose memory leak.

You could solve that by changing the firedrake defaults so that every solver by default has the same (empty) prefix.

But now, a solve with options specifies pollutes another solve without options

[wence-]

What you could do is implement a per-solve options_left with https://petsc.org/release/manualpages/Sys/PetscOptionsUsed/, which you can use to query if a particular solve used all the options specified in the solver_parameters dictionary.

Unfortunately, this isn't quite perfect, because for reasons (I think it could be fixed), any PETSc option that is only inspected via petsc4py doesn't get marked as used (or at least it didn't used to)

[JHopeCollins]

Oh yes, I knew there was something else I had about the options manager. If you don't provide the manager a prefix then it generates that unique one, and then also ignores any command line options - this makes sense because the unique prefix isn't stable if you later modify the code.

However, last time I checked, if you do try to pass a command line option to that unique prefix, then it is both ignored by the options manager, and also doesn't show up in -options_left report. I didn't get to the bottom of why.

What if the options manager only inserted options permanently into the global options if you also provide a prefix? If you don't provide a prefix then it generates the unique one and does the insertion/removal as it currently does.

[JHopeCollins]

What you could do is implement a per-solve options_left with https://petsc.org/release/manualpages/Sys/>PetscOptionsUsed/, which you can use to query if a particular solve used all the options specified in the >solver_parameters dictionary.

Unfortunately, this isn't quite perfect, because for reasons (I think it could be fixed), any PETSc option that is only >inspected via petsc4py doesn't get marked as used (or at least it didn't used to)

A per-solve options_left could be really handy for dealing with the parameters dictionary though, if the petsc4py bug is/can be easily fixed.

[anilbeycorintis]

Great to see solutions being suggested. Thank you.

A per-solve options_left could be really handy for dealing with the parameters dictionary though, if the petsc4py bug is/can be easily fixed.

Is this solution something to be implemented within firedrake or is it something that the users of firedrake can apply?

[dham]

Great to see solutions being suggested. Thank you.

A per-solve options_left could be really handy for dealing with the parameters dictionary though, if the petsc4py bug is/can be easily fixed.

Is this solution something to be implemented within firedrake or is it something that the users of firedrake can apply?

As far as I can see, PetscOptionsUsed is not currently exposed by petsc4py, so that needs to change. It may also be necessary to deal with the petsc4py potential bug listed above. Once that were done, I think the actual firedrake change is one line in firedrake/petsc.py.

[anilbeycorintis]

Great to see solutions being suggested. Thank you.

A per-solve options_left could be really handy for dealing with the parameters dictionary though, if the petsc4py bug is/can be easily fixed.

Is this solution something to be implemented within firedrake or is it something that the users of firedrake can apply?

As far as I can see, PetscOptionsUsed is not currently exposed by petsc4py, so that needs to change. It may also be necessary to deal with the petsc4py potential bug listed above. Once that were done, I think the actual firedrake change is one line in firedrake/petsc.py.

Ah great, so I could contact the petsc4py maintainers, right?

[connorjward]

Great to see solutions being suggested. Thank you.

A per-solve options_left could be really handy for dealing with the parameters dictionary though, if the petsc4py bug is/can be easily fixed.

Is this solution something to be implemented within firedrake or is it something that the users of firedrake can apply?

As far as I can see, PetscOptionsUsed is not currently exposed by petsc4py, so that needs to change. It may also be necessary to deal with the petsc4py potential bug listed above. Once that were done, I think the actual firedrake change is one line in firedrake/petsc.py.

Ah great, so I could contact the petsc4py maintainers, right?

Yes you could create an issue on PETSc GitLab (and please link to it here). Adding new bindings is usually straightforward. I would be interested in getting the PETSc devs insights on how difficult it would be to address the issue of petsc4py not marking options as used.

[anilbeycorintis]

Unfortunately, this isn't quite perfect, because for reasons (I think it could be fixed), any PETSc option that is only inspected via petsc4py doesn't get marked as used (or at least it didn't used to)

While creating a minimal reproducer for the petsc issue, I noticed the options that are inspected (i.e. read) in Python are actually considered used (at least they don't show up in the list of unused parameters). @wence- this is what you were referring to, right? Can we consider that particular issue of petsc4py fixed now?

from petsc4py import PETSc

snes = PETSc.SNES().create()

options = PETSc.Options()
solver_parameters = {
    "snes_max_it": 10,  # valid option
    "read_param": "55", # inspected option
    "unread_param": 33  # uninspected option
}

for key, value in solver_parameters.items():
    options[key] = value

c1 = options['read_param']  # reading/inspecting

snes.setFromOptions()
# > WARNING! There are options you set that were not used!
# > WARNING! could be spelling mistake, etc!
# > There is one unused database option. It is:
# > Option left: name:-unread_param value: 33 source: code

The version of petsc4py I use is 3.22.2

❯ pip list | grep petsc
petsc4py                      3.22.2

[wence-]

I think that might be good enough. But what about if you use the programmatic interface from python options.getInt or whatever it's called?

[anilbeycorintis]

I just tried that, it behaves the same as reading it via index access.

from petsc4py import PETSc

snes = PETSc.SNES().create()

options = PETSc.Options()
solver_parameters = {
    "snes_max_it": 10,  # valid option
    "read_param": "55", # inspected option
    "unread_param": 33  # uninspected option
}

for key, value in solver_parameters.items():
    options[key] = value

read_value = options.getString("read_param") 

snes.setFromOptions()
# OUTPUT:
# > WARNING! There are options you set that were not used!
# > WARNING! could be spelling mistake, etc!
# > There is one unused database option. It is:
# > Option left: name:-unread_param value: 33 source: code

[anilbeycorintis]

Here is the petsc issue: https://gitlab.com/petsc/petsc/-/issues/1704

[dham]

I see Matt Knepley seems to have implemented it immediately. Does his fix work for you?

[anilbeycorintis]

I confirm it does work fine. The new used method does work as expected and it does not change the state of Options left parameters.

from petsc4py import PETSc

snes = PETSc.SNES().create()

options = PETSc.Options()
solver_parameters = {
    "snes_max_it": 10,  # valid option
    "read_param": "55", # inspected option
    "unread_param": 33  # uninspected option
}

for key, value in solver_parameters.items():
    options[key] = value

read_value = options.getString("read_param") 
print(f"read_param is used?: {options.used('read_param')}")  # prints true
print(f"unread_param is used?: {options.used('unread_param')}")  # prints false

snes.setFromOptions()
# > WARNING! There are options you set that were not used!
# > WARNING! could be spelling mistake, etc!
# > There is one unused database option. It is:
# > Option left: name:-unread_param value: 33 source: code
# NOTE: these results are not affected after calling options.used().

[anilbeycorintis]

The change is merged to petsc 🎉
https://gitlab.com/petsc/petsc/-/merge_requests/8090

Looking forward to the new Firedrake bindings.

[dham]

https://github.com/firedrakeproject/firedrake/tree/warn_unused_options is a completely untested implementation of the Firedrake part of the change. We need to merge PETSc main into our PETSc fork in order to test it (or externally build PETSc main).

[dham]

Good news: we now have a PR #3999 .

Bad news: merging it depends on us fixing #3998