Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Feature] Provide limit checks when reading global_prefs_override #4916

Open
Vulpine05 opened this issue Sep 7, 2022 · 4 comments · May be fixed by #5050
Open

[Feature] Provide limit checks when reading global_prefs_override #4916

Vulpine05 opened this issue Sep 7, 2022 · 4 comments · May be fixed by #5050

Comments

@Vulpine05
Copy link
Contributor

Vulpine05 commented Sep 7, 2022

Describe the problem
Preferences can be modified in three ways:

  1. Web preferences
  2. Locally, via the Manger
  3. Locally, by editing the global_prefs_override.xml file directly

When changing preferences via the project website or the Manager, those values are validated to be in the correct range. However, when BOINC is started, it will read the global_prefs_override file (if it exists), and store those values. Some of those preferences are currently validated, but not all. The example below shows the percentage of CPUs being changed to a range between 0 and 100:

boinc/lib/prefs.cpp

Lines 509 to 513 in 3fc6c02

if (xp.parse_double("max_ncpus_pct", max_ncpus_pct)) {
if (max_ncpus_pct < 0) max_ncpus_pct = 0;
if (max_ncpus_pct > 100) max_ncpus_pct = 100;
mask.max_ncpus_pct = true;
continue;

However, if suspend_cpu_usage were accidentally edited as a negative number, that negative number would be stored, and BOINC would never crunch any tasks.

Describe the solution you'd like
I feel this can be resolved with two steps:

  1. For any double variables that don't have limits set already, add them. These limits are likely to be simple, such as setting a negative number to 0. This will have to be evaluated on a case-by-case basis.
  2. The user should be aware that BOINC had to modify one of their values. I think the best thing would be to create a message in the notices tab. I would think this would be similar to how a message appears when there is incorrect data in an app_config file. The message doesn't have to be too specific, perhaps which preference was changed. I would assume if the user is editing their global_prefs_override file, they will know how to read through it and find the error.

Additional context
I have started to work on this solution, but with #4871 being in development, I stopped so I didn't have to redo large sections of code. I would be happy to continue developing it after that PR is merged. In the meantime, I'm looking for any feedback.

@AenBleidd
Copy link
Member

@davidpanderson, looks like a valid ticket and it would be nice to have it in the next release. Are you ok to wait for it implementation after #4871 is merged?

@davidpanderson
Copy link
Contributor

Not really needed. No one AFAIK manually edits global_prefs.xml;
it's not documented except for an ancient trac page.

@Vulpine05
Copy link
Contributor Author

Not really needed. No one AFAIK manually edits global_prefs.xml; it's not documented except for an ancient trac page.

Actually, there is documentation here and here.

@CharlieFenton
Copy link
Contributor

CharlieFenton commented Sep 8, 2022

Some projects customize / modify the standard web code. Coding errors could inadvertently allow users to set illegal values in their computer preferences when editing them on the project web site. Coding errors could also inadvertently cause bad values to be sent in the sched_reply message to the client even if the underlying values were OK. So it would be good to perform sanity checks on the computing prefs values in the sched_reply message as well as when parsing the global_prefs.xml and global_prefs_override.xml files. I believe the same validation code in the client could be used for all 3.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Backlog
Development

Successfully merging a pull request may close this issue.

4 participants