[LTP] [PATCH v3 04/16] API/cgroup: Implement tst_cg_load_config

[Luke Nowakowski-Krijger]

Hi Richard,

On Mon, Mar 7, 2022 at 2:43 AM Richard Palethorpe <rpalethorpe@suse.de>
wrote:

> Hello Luke,
>
> Really great work for the patch series in general! However see comments
> below.
>
> Luke Nowakowski-Krijger <luke.nowakowskikrijger@canonical.com> writes:
>
> > Implement tst_cg_load_config which consumes the state given by
> > tst_cg_print_config to update the internal test state to reflect
> > the given config.
> >
> > This allows for programs using the cgroup C API to load and reload
> > state, allowing functionality such as calling tst_cg_require and
> > tst_cg_cleanup to function properly between programs or between
> > invocations of a binary using the C API.
> >
> > Signed-off-by: Luke Nowakowski-Krijger <
> luke.nowakowskikrijger@canonical.com>
> > ---
> > v2: Add root null check in parse_root_config.
> >     Remove checking for ltp_drain_dir key from config as it was
> >     redundant.
> >     Remove unsued variable in parse_ctrl_config.
> >     Cleanup some compiler warnings.
> > v3: Rewrite to consume each line of the config with a scanf to make
> >     the parsing much simpler while using new config variables.
> >
> >  include/tst_cgroup.h |  7 +++++
> >  lib/tst_cgroup.c     | 62 ++++++++++++++++++++++++++++++++++++++++++++
> >  2 files changed, 69 insertions(+)
> >
> > diff --git a/include/tst_cgroup.h b/include/tst_cgroup.h
> > index 87e55f4df..9bad0d366 100644
> > --- a/include/tst_cgroup.h
> > +++ b/include/tst_cgroup.h
> > @@ -121,6 +121,13 @@ void tst_cg_scan(void);
> >   */
> >  void tst_cg_print_config(void);
> >
> > +/* Load the config printed out by tst_cg_print_config and configure the
> internal
> > + * libary state to match the config. Used to allow tst_cg_cleanup to
> properly
> > + * cleanup mounts and directories created by tst_cg_require between
> program
> > + * invocations.
> > + */
> > +void tst_cg_load_config(const char *const config);
> > +
> >  /* Ensure the specified controller is available in the test's default
> >   * CGroup, mounting/enabling it if necessary. Usually this is not
> >   * necesary use tst_test.needs_cgroup_controllers instead.
> > diff --git a/lib/tst_cgroup.c b/lib/tst_cgroup.c
> > index 8f95064b3..90d91d94e 100644
> > --- a/lib/tst_cgroup.c
> > +++ b/lib/tst_cgroup.c
> > @@ -366,6 +366,68 @@ static struct cgroup_root *cgroup_find_root(const
> char *const mnt_path)
> >       return NULL;
> >  }
> >
> > +static void parse_config(const char *const config_entry)
>
> cgroup_parse_config_line perhaps? or cgroup_parse_ctrl?
>
> > +{
> > +     const char ctrl_name[32], mnt_path[PATH_MAX],
> > test_dir_name[NAME_MAX + 1];
>
> These buffers are not const.
>
> I have lsp and clangd setup in emacs which warns about this. It's easy
> to miss the warning in the compiler output.
>
> > +     int ver, we_require_it, we_mounted_it, ltp_dir_we_created_it;
> > +     struct cgroup_root *root;
> > +     struct cgroup_ctrl *ctrl;
> > +
> > +     sscanf(config_entry, CONFIG_FORMAT,
> > +             ctrl_name, &ver, &we_require_it, mnt_path, &we_mounted_it,
> > +             &ltp_dir_we_created_it, test_dir_name);
>
> You need to run 'make check-tst_cgroup' in ltp/lib. The sscanf return
> value is not checked which could result in segfaults and other confusing
> errors if parsing fails.
>
> > +
> > +     if (!(ctrl = cgroup_find_ctrl(ctrl_name)))
>
> check_patch.pl forbids assignments in if statements (it makes static
> analysis difficult). Again this is caught with 'make check'.
>
> > +             tst_brk(TBROK, "Could not find ctrl from config. Ctrls
> changing between calls?");
> > +
> > +     ctrl->we_require_it = we_require_it;
> > +
> > +     if (!(root = cgroup_find_root(mnt_path)))
> > +             tst_brk(TBROK, "Could not find root from config. Config
> possibly malformed?");
> > +
> > +     if (we_mounted_it)
> > +             root->we_mounted_it = 1;
> > +
> > +     if (!root->ltp_dir.dir_name) {
> > +             cgroup_dir_mk(&root->mnt_dir, cgroup_ltp_dir,
> &root->ltp_dir);
> > +             cgroup_dir_mk(&root->ltp_dir, cgroup_ltp_drain_dir,
> &root->drain_dir);
> > +             if (ltp_dir_we_created_it) {
> > +                     root->ltp_dir.we_created_it = 1;
> > +                     root->drain_dir.we_created_it = 1;
> > +             }
> > +     }
> > +
> > +     if (!root->test_dir.dir_name && strcmp(test_dir_name, "NULL")) {
> > +             strncpy(cgroup_test_dir, test_dir_name, NAME_MAX);
>
> I think this could result in an unterminated string. strncpy does not
> add a null char after the string that was copied. Note also that the
> format string passed to sscanf does not limit the field width so it
> could overwrite root and ctrl on the stack if the input is long enough.
>
>
Thank you for pointing all these things out. I forgot about all the
shenanigans that can happen when you don't sanity check string stuff :)

A few solutions to creating the scanff format are:
1) pre-processor trick to concatenate defines into a string, something like
this
#define _tostr(str) #str
#define tostr(str) _tostr(str)
#define CONFIG_FORMAT "%" tostr(CTRL_NAME_MAX) "s\t%d\t%d\t%"
tostr(PATH_MAX) "s\t%d\t%d\t%" tostr(NAME_MAX) "s"

2) just hardcode the lengths to avoid all of this nonsense above

3) just create the format at runtime

I'm more or less asking what you think is best to avoid sending out all of
the patches and then discussing this / if there is a better way.



> > +             cgroup_dir_mk(&root->ltp_dir, cgroup_test_dir,
> &root->test_dir);
> > +             root->test_dir.we_created_it = 1;
> > +     }
> > +}
> > +
> > +/* Load the test state config provided by tst_cg_print_config
> > + *
> > + * This will reload some internal tst_cgroup state given by the config
> > + * that might otherwise have been lost between calls or between
> different
> > + * processes. In particular this is used by testcases/lib/tst_cgctl to
> > + * provide access to this C api to shell scripts.
> > + *
> > + * The config keeps track of the minimal state needed for tst_cg_cleanup
> > + * to cleanup mounts and directories created by tst_cg_require.
> > + */
> > +void tst_cg_load_config(const char *const config)
> > +{
> > +     char temp_config[BUFSIZ];
> > +     char *line;
> > +
> > +     if (strlen(config) >= BUFSIZ)
> > +             tst_brk(TBROK, "Config has exceeded buffer size?");
> > +     strncpy(temp_config, config, BUFSIZ);
>
> Again, this won't copy the null byte from config if strlen(config) ==
> BUFSIZ. You could use memcpy here anyway if you know the string length.
>
> > +
> > +     line = strtok(temp_config, "\n");
> > +     /* Make sure to consume the header. */
> > +     for (line = strtok(NULL, "\n"); line; line = strtok(NULL, "\n"))
> > +             parse_config(line);
> > +}
> >
> >  /* Determine if a mounted cgroup hierarchy is unique and record it if
> so.
> >   *
>
>
I will take all of the other things you mentioned and clean them up. Thanks
for the review!


>
> --
> Thank you,
> Richard.
>

Thanks,
- Luke
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.linux.it/pipermail/ltp/attachments/20220316/bd226fa5/attachment.htm>