On 2023-09-28 at 14:25:23 -0700, Reinette Chatre wrote:
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@intel.com,
- Fenghua Yu fenghua.yu@intel.com
*/ +#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.
Right, sorry, then I guess initializing it to zero isn't a problem.
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 tried compiling and running by passing a value with scanf to write() (so the compiler doesn't know it's a negative one) just to check if it behaves and write() was returning a negative one. But yes, the fact it's unsigned means it's not a reliable way to catch this error.
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().
Okay, thanks a lot for pointing all this out. I'll do just a less than one check and move the initial value to zero.