[bug report] dm vdo: add statistics reporting

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hello Matthew Sakai,

The patch 25479d97b12c: "dm vdo: add statistics reporting" from Nov
16, 2023 (linux-next), leads to the following (unpublished) Smatch
static checker warning:

drivers/md/dm-vdo/funnel-workqueue.c:545 vdo_dump_completion_to_buffer() warn: why check scnprintf return value?
drivers/md/dm-vdo/message-stats.c:26 write_u64() warn: why check scnprintf return value?
drivers/md/dm-vdo/message-stats.c:43 write_u32() warn: why check scnprintf return value?
drivers/md/dm-vdo/message-stats.c:60 write_block_count_t() warn: why check scnprintf return value?
drivers/md/dm-vdo/message-stats.c:77 write_string() warn: why check scnprintf return value?
drivers/md/dm-vdo/message-stats.c:94 write_bool() warn: why check scnprintf return value?
drivers/md/dm-vdo/message-stats.c:111 write_u8() warn: why check scnprintf return value?

drivers/md/dm-vdo/message-stats.c
    99 static int write_u8(char *prefix,
    100                     u8 value,
    101                     char *suffix,
    102                     char **buf,
    103                     unsigned int *maxlen)
    104 {
    105         int count = scnprintf(*buf, *maxlen, "%s%u%s",
    106                               prefix == NULL ? "" : prefix,
    107                               value,
    108                               suffix == NULL ? "" : suffix);
    109         *buf += count;
    110         *maxlen -= count;
--> 111         if (count >= *maxlen)
                    ^^^^^^^^^^^^^^^^
snprintf() returns the number of bytes that would have been printed if
we had enough space and scnprintf() returns the number of bytes that
were printed.  (Both functions exclude the NUL terminator from the
returned count).

It seldom makes sense to check the return from scnprintf().  In this
case count can never be >= *maxlen.  The most it can be is *maxlen - 1.

    112                 return VDO_UNEXPECTED_EOF;
    113         return VDO_SUCCESS;
    114 }

The code in vdo_dump_completion_to_buffer() is almost basically correct.
It's just off by one in a totally harmless way and I wouldn't have
reported it if not for these other impossible conditions.

drivers/md/dm-vdo/funnel-workqueue.c
   538  void vdo_dump_completion_to_buffer(struct vdo_completion *completion, char *buffer,
   539                                     size_t length)
   540  {
   541          size_t current_length =
   542                  scnprintf(buffer, length, "%.*s/", TASK_COMM_LEN,
   543                            (completion->my_queue == NULL ? "-" : completion->my_queue->name));
   544  
   545          if (current_length < length - 1) {

The highest valid return is current_length == length - 1 but that's
treated as invalid.  If we changed to snprintf() then we could fix the
condition to allow the final character:

	current_length = snprintf(...);
	if (current_length <= length - 1) {
                          ^^^^

   546                  get_function_name((void *) completion->callback, buffer + current_length,
   547                                    length - current_length);
   548          }
   549  }

regards,
dan carpenter




[Index of Archives]     [DM Crypt]     [Fedora Desktop]     [ATA RAID]     [Fedora Marketing]     [Fedora Packaging]     [Fedora SELinux]     [Yosemite Discussion]     [KDE Users]     [Fedora Docs]

  Powered by Linux