RE: [PATCH] hv: kvp: Avoid reading past allocated blocks from KVP file

[Long Li <longli@microsoft.com>]

> From: Paul Meyer <Paul.Meyer@microsoft.com>
> 
> While reading in more than one block (50) of KVP records, the allocation goes
> per block, but the reads used the total number of allocated records (without
> resetting the pointer/stream). This causes the records buffer to overrun when
> the refresh reads more than one block over the previous capacity (e.g. reading
> more than 100 KVP records whereas the in-memory database was empty before).
> 
> Fix this by reading the correct number of KVP records from file each time.

Please drop this patch. I have sent a v2.

> 
> Signed-off-by: Paul Meyer <Paul.Meyer@microsoft.com>
> Reviewed-by: Long Li <longli@microsoft.com>
> ---
>  tools/hv/hv_kvp_daemon.c | 66 ++++++++----------------------------------------
>  1 file changed, 10 insertions(+), 56 deletions(-)
> 
> diff --git a/tools/hv/hv_kvp_daemon.c b/tools/hv/hv_kvp_daemon.c index
> eaa3bec..2094036 100644
> --- a/tools/hv/hv_kvp_daemon.c
> +++ b/tools/hv/hv_kvp_daemon.c
> @@ -193,11 +193,13 @@ static void kvp_update_mem_state(int pool)
>         for (;;) {
>                 readp = &record[records_read];
>                 records_read += fread(readp, sizeof(struct kvp_record),
> -                                       ENTRIES_PER_BLOCK * num_blocks,
> -                                       filep);
> +                               ENTRIES_PER_BLOCK * num_blocks - records_read,
> +                               filep);
> 
>                 if (ferror(filep)) {
> -                       syslog(LOG_ERR, "Failed to read file, pool: %d", pool);
> +                       syslog(LOG_ERR,
> +                               "Failed to read file, pool: %d; error: %d %s",
> +                                pool, errno, strerror(errno));
>                         exit(EXIT_FAILURE);
>                 }
> 
> @@ -224,15 +226,11 @@ static void kvp_update_mem_state(int pool)
>         fclose(filep);
>         kvp_release_lock(pool);
>  }
> +
>  static int kvp_file_init(void)
>  {
>         int  fd;
> -       FILE *filep;
> -       size_t records_read;
>         char *fname;
> -       struct kvp_record *record;
> -       struct kvp_record *readp;
> -       int num_blocks;
>         int i;
>         int alloc_unit = sizeof(struct kvp_record) * ENTRIES_PER_BLOCK;
> 
> @@ -246,61 +244,17 @@ static int kvp_file_init(void)
> 
>         for (i = 0; i < KVP_POOL_COUNT; i++) {
>                 fname = kvp_file_info[i].fname;
> -               records_read = 0;
> -               num_blocks = 1;
>                 sprintf(fname, "%s/.kvp_pool_%d", KVP_CONFIG_LOC, i);
>                 fd = open(fname, O_RDWR | O_CREAT | O_CLOEXEC, 0644 /* rw-r--r--
> */);
> 
>                 if (fd == -1)
>                         return 1;
> 
> -
> -               filep = fopen(fname, "re");
> -               if (!filep) {
> -                       close(fd);
> -                       return 1;
> -               }
> -
> -               record = malloc(alloc_unit * num_blocks);
> -               if (record == NULL) {
> -                       fclose(filep);
> -                       close(fd);
> -                       return 1;
> -               }
> -               for (;;) {
> -                       readp = &record[records_read];
> -                       records_read += fread(readp, sizeof(struct kvp_record),
> -                                       ENTRIES_PER_BLOCK,
> -                                       filep);
> -
> -                       if (ferror(filep)) {
> -                               syslog(LOG_ERR, "Failed to read file, pool: %d",
> -                                      i);
> -                               exit(EXIT_FAILURE);
> -                       }
> -
> -                       if (!feof(filep)) {
> -                               /*
> -                                * We have more data to read.
> -                                */
> -                               num_blocks++;
> -                               record = realloc(record, alloc_unit *
> -                                               num_blocks);
> -                               if (record == NULL) {
> -                                       fclose(filep);
> -                                       close(fd);
> -                                       return 1;
> -                               }
> -                               continue;
> -                       }
> -                       break;
> -               }
>                 kvp_file_info[i].fd = fd;
> -               kvp_file_info[i].num_blocks = num_blocks;
> -               kvp_file_info[i].records = record;
> -               kvp_file_info[i].num_records = records_read;
> -               fclose(filep);
> -
> +               kvp_file_info[i].num_blocks = 1;
> +               kvp_file_info[i].records = malloc(alloc_unit);
> +               kvp_file_info[i].num_records = 0;
> +               kvp_update_mem_state(i);
>         }
> 
>         return 0;
> --
> 2.7.4
> 
> _______________________________________________
> devel mailing list
> devel@linuxdriverproject.org
> https://na01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fdriverdev.li
> nuxdriverproject.org%2Fmailman%2Flistinfo%2Fdriverdev-
> devel&data=02%7C01%7Clongli%40microsoft.com%7C3d25aed8f1a14fb966170
> 8d52091db50%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C6364507
> 33196130349&sdata=7SZq7ER6YQo5ci6GmtPZUsL41g%2BERq2sswLeZNEb43k%
> 3D&reserved=0