[WIP] lib/repo: Validate config more strictly #1739

mwleeds · 2018-09-28T16:51:33Z

Now that we're correctly reading negative values for the
"lock-timeout-secs" config key, throw an error if it's not within the
range of values that have semantic meaning. This mirrors the behavior
when other config keys have invalid values.

Now that we're correctly reading negative values for the "lock-timeout-secs" config key, throw an error if it's not within the range of values that have semantic meaning. This mirrors the behavior when other config keys have invalid values.

Currently the locking code checks if the value -1 was set for the config key "lock-timeout-secs" and if so, a thread trying to acquire a lock will block indefinitely. Positive values specify how long to attempt to acquire a lock in a non-blocking way (the attempt is made once every second). But when the value is read from the config file, g_ascii_strtoull() is used, which converts it to an unsigned integer. This commit makes libostree use g_ascii_strtoll() instead, so that it's possible to set that key to -1 as intended.

dbnicholson · 2018-09-28T17:10:58Z

I like this better than relying on an assert since that's a programmer error, not a user error. Actually, there are a bunch of potential errors not caught here that were in my original implementation in 5321998. See also @pwithnall's review in 5321998#r147145444.

mwleeds · 2018-09-28T18:15:16Z

That link didn't work for me but I think I found what you're referring to here: #1292 (comment)

cgwalters · 2018-09-28T19:06:50Z

@rh-atomic-bot delegate=dbnicholson

rh-atomic-bot · 2018-09-28T19:06:50Z

✌️ @dbnicholson can now approve this pull request

pwithnall

Looks good to me, but I’ll leave the final review to Dan.

pwithnall · 2018-09-29T09:22:59Z

Unfortunately it does not look so good to all the compilers:

src/libostree/ostree-repo.c: In function 'reload_core_config':
src/libostree/ostree-repo.c:2810:11: error: unused variable 'endptr' [-Werror=unused-variable]
     char *endptr;
           ^~~~~~
src/libostree/ostree-repo.c:2840:9: error: this 'if' clause does not guard... [-Werror=misleading-indentation]
         if (configured_lock_timeout < REPO_LOCK_DISABLED || configured_lock_timeout > G_MAXINT32 ||
         ^~
src/libostree/ostree-repo.c:2842:11: note: ...this statement, but the latter is misleadingly indented as if it were guarded by the 'if'
           return glnx_throw (error, "Invalid lock-timeout-secs '%s'", lock_timeout_seconds);
           ^~~~~~

dbnicholson

I think this is definitely the right idea with a few more tweaks. I'd also split this to one commit per conversion since the semantics for each config key are different.

dbnicholson · 2018-09-29T11:06:28Z

src/libostree/ostree-repo.c

-  { g_autofree char *tmp_expiry_seconds = NULL;
+  {
+    g_autofree char *tmp_expiry_seconds = NULL;
+    char *endptr;


Doesn't look like this is used.

dbnicholson · 2018-09-29T11:08:25Z

src/libostree/ostree-repo.c


        if (!ot_keyfile_get_value_with_default (self->config, "core", "lock-timeout-secs", "30",
                                                &lock_timeout_seconds, error))
          return FALSE;

-        self->lock_timeout_seconds = g_ascii_strtoll (lock_timeout_seconds, NULL, 10);
+        configured_lock_timeout = g_ascii_strtoll (lock_timeout_seconds, &endptr, 10);
+        if (configured_lock_timeout < REPO_LOCK_DISABLED || configured_lock_timeout > G_MAXINT32 ||


Why GMAXINT32? The timeout is a gint.

dbnicholson · 2018-09-29T11:17:51Z

src/libostree/ostree-repo.c

-        self->lock_timeout_seconds = g_ascii_strtoll (lock_timeout_seconds, NULL, 10);
+        configured_lock_timeout = g_ascii_strtoll (lock_timeout_seconds, &endptr, 10);
+        if (configured_lock_timeout < REPO_LOCK_DISABLED || configured_lock_timeout > G_MAXINT32 ||
+            *endptr != '\0');


Depending how pedantic you want to be, there's also the case of an empty value. I was raising an error before on lock_timeout == 0 && endptr == lock_timeout_str. You could detect that and set to the internal default rather than erroring or leaving it implicitly as 0.

I'm not totally sure about this, though. I don't have the code in front of me to see what happens.

pwithnall · 2018-09-29T12:18:43Z

Note: You could also potentially use g_ascii_string_to_[un]signed() rather than g_strto[u]ll(), if you depend on a new enough GLib version (I haven’ŧ checked).

mwleeds · 2018-09-29T22:41:35Z

Oops, didn't intend for y'all to spend time reviewing that last commit. I was pushing it to get it from one computer to another. But yeah, I'll follow-up and fix it.

openshift-ci · 2023-06-29T14:43:46Z

@mwleeds: The following tests failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
ci/prow/sanity	`dce18c9`	link	true	`/test sanity`
ci/prow/images	`dce18c9`	link	true	`/test images`
ci/prow/fcos-e2e	`dce18c9`	link	true	`/test fcos-e2e`

Full PR test history. Your PR dashboard.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

lib/repo: Validate lock-timeout-secs config

2af447d

Now that we're correctly reading negative values for the "lock-timeout-secs" config key, throw an error if it's not within the range of values that have semantic meaning. This mirrors the behavior when other config keys have invalid values.

WIP

dce18c9

pwithnall reviewed Sep 29, 2018

View reviewed changes

dbnicholson requested changes Sep 29, 2018

View reviewed changes

mwleeds changed the title ~~lib/repo: Validate lock-timeout-secs config~~ lib/repo: Validate config more strictly Sep 29, 2018

mwleeds changed the title ~~lib/repo: Validate config more strictly~~ [WIP] lib/repo: Validate config more strictly Oct 9, 2018

mwleeds mentioned this pull request Feb 28, 2019

ostree config set should not write out new config file if new set value is invalid #1827

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[WIP] lib/repo: Validate config more strictly #1739

[WIP] lib/repo: Validate config more strictly #1739

mwleeds commented Sep 28, 2018

dbnicholson commented Sep 28, 2018

mwleeds commented Sep 28, 2018

cgwalters commented Sep 28, 2018

rh-atomic-bot commented Sep 28, 2018

pwithnall left a comment

pwithnall commented Sep 29, 2018

dbnicholson left a comment

dbnicholson Sep 29, 2018

dbnicholson Sep 29, 2018

dbnicholson Sep 29, 2018

pwithnall commented Sep 29, 2018

mwleeds commented Sep 29, 2018

openshift-ci bot commented Jun 29, 2023

[WIP] lib/repo: Validate config more strictly #1739

Are you sure you want to change the base?

[WIP] lib/repo: Validate config more strictly #1739

Conversation

mwleeds commented Sep 28, 2018

dbnicholson commented Sep 28, 2018

mwleeds commented Sep 28, 2018

cgwalters commented Sep 28, 2018

rh-atomic-bot commented Sep 28, 2018

pwithnall left a comment

Choose a reason for hiding this comment

pwithnall commented Sep 29, 2018

dbnicholson left a comment

Choose a reason for hiding this comment

dbnicholson Sep 29, 2018

Choose a reason for hiding this comment

dbnicholson Sep 29, 2018

Choose a reason for hiding this comment

dbnicholson Sep 29, 2018

Choose a reason for hiding this comment

pwithnall commented Sep 29, 2018

mwleeds commented Sep 29, 2018

openshift-ci bot commented Jun 29, 2023