[YARN-9420] Avoid potentially dangerous filename concatenation in native code (cgroups-operations.c)

• Type: Improvement
• Status: Open
• Priority: Minor
• Resolution: Unresolved
• Assignee: Unassigned
• Reporter: Szilard Nemeth
• Created: 27/Mar/19 19:34
• Updated: 27/Mar/19 19:34

Description

In cgroups-operations.c, in function get_cgroups_path_to_write, at the end of the function, there's a string formatting operation:

if (snprintf(buffer, MAX_PATH_LEN, "%s/%s/%s/%s/%s.%s",
 cgroups_root, hierarchy_name, yarn_hierarchy_name,
 group_id, hierarchy_name, param_name) < 0) {
 fprintf(ERRORFILE, "Failed to print output path.\n");
 failed = 1;
 goto cleanup;
 }

This functions is being called from just one function: update_cgroups_parameters
All calls of update_cgroups_parameters look like this (note that only the last parameter differs):

update_cgroups_parameters_func_p("devices", "deny",
 container_id, param_value);

So essentially, get_cgroups_path_to_write will have these arguments:
1. hierarchy_name: "devices"
2. param_name: "allow"
3. group_id: container_id

An example of a full path:

/var/lib/yarn-ce/cgroups/devices/hadoop-yarn/c_1/devices.deny

, where:
1. cgroups_root = "/var/lib/yarn-ce/cgroups"
2. hierarchy_name = "devices"
3. yarn_hierarchy_name = "/hadoop-yarn"
4. group_id = "c_1"
5. param_name = "deny"

The problem is that the last bit of the format string ("%s.%s") relies on the fact that the variable hierarchy_name holds the value "devices", so it can be reused in the path and for the filename as well ("devices.deny").
It would be more clear if param_name would hold the whole filename as is, e.g. "devices.allow", instead of manually constructing it from 2 strings.