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

[Richard Palethorpe <rpalethorpe@suse.de>]

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.

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


-- 
Thank you,
Richard.

-- 
Mailing list info: https://lists.linux.it/listinfo/ltp