Hi Maciej,
On 9/27/2023 11:46 PM, Maciej Wieczór-Retman wrote:
> On 2023-09-27 at 15:15:06 -0700, Reinette Chatre wrote:
>> On 9/22/2023 1:10 AM, Maciej Wieczor-Retman wrote:
>>> diff --git a/tools/testing/selftests/resctrl/resctrlfs.c b/tools/testing/selftests/resctrl/resctrlfs.c
>>> index 3a8111362d26..edc8fc6e44b0 100644
>>> --- a/tools/testing/selftests/resctrl/resctrlfs.c
>>> +++ b/tools/testing/selftests/resctrl/resctrlfs.c
>>> @@ -8,6 +8,7 @@
>>> * Sai Praneeth Prakhya <sai.praneeth.prakhya@xxxxxxxxx>,
>>> * Fenghua Yu <fenghua.yu@xxxxxxxxx>
>>> */
>>> +#include <fcntl.h>
>>> #include <limits.h>
>>>
>>> #include "resctrl.h"
>>> @@ -490,9 +491,8 @@ int write_bm_pid_to_resctrl(pid_t bm_pid, char *ctrlgrp, char *mongrp,
>>> */
>>> int write_schemata(char *ctrlgrp, char *schemata, int cpu_no, char *resctrl_val)
>>> {
>>> - char controlgroup[1024], schema[1024], reason[64];
>>> - int resource_id, ret = 0;
>>> - FILE *fp;
>>> + char controlgroup[1024], schema[1024], reason[128];
>>> + int resource_id, fd, schema_len = -1, ret = 0;
>>
>> I am trying to understand the schema_len initialization. Could
>> you please elaborate why you chose -1? I'm a bit concerned with
>> the robustness here with it being used as an unsigned integer
>> in write() and also the negative array index later.
>
> My idea was that if the initial value for schema_len was 0, then if
> resctrl_val wouldn't equal any of MBA_STR, MBM_STR, CAT_STR, CMT_STR
Ensuring that resctrl_val is equal to one of these seems to be the
first thing write_schemata() does.
> values schema_len would stay zero and write nothing.
Your alternative writes "-1". write() is declared as:
ssize_t write(int fd, const void *buf, size_t count);
note that "count" is size_t, which is an unsigned value. Providing
it -1 is thus a very large number and likely to cause overflow. In fact
if I even try to compile a program where the compiler can figure out
count will be -1 it fails the compile (stringop-overflow).
> I think it would be difficult to debug such an error because even later
> in ksft_print_msg the requested schema would get printed as if there was
> no error. In the case I mentioned above the function will just error out
> which I assume could be helpful.
You seem to rely on write() to cleanly catch giving it bad data.
> Other solutions that can accomplish the same goal would be checking
> write() not only for negative values but also for zero (since in
> here this is pretty much an error). Or checking schema_len for only
> positive values after the block of code where it gets assigned a
> value from sprintf.
>
> Are any of the above safer or more logical in your opinion?
There is no error checking on schema_len. After it has been initialized it
can be checked for errors and write_schemata() can be exited immediately if
an error was encountered without attempting the write().
Reinette
