Re: [Qemu-devel] [PATCH v5 1/4] firmware: introduce sysfs driver for QEMU's fw_cfg device

On 11/24/15 18:38, Eric Blake wrote:

On 11/24/2015 09:55 AM, Gabriel L. Somlo wrote:

quoted

On Tue, Nov 24, 2015 at 04:14:50AM +0800, kbuild test robot wrote:

quoted

quoted

   drivers/firmware/qemu_fw_cfg.c: In function 'fw_cfg_cmdline_set':

quoted

quoted

drivers/firmware/qemu_fw_cfg.c:510:7: warning: format '%lli' expects argument of type 'long long int *', but argument 3 has type 'phys_addr_t *' [-Wformat=]

          &ctrl_off, &data_off, &consumed);
          ^

Oh, I think I know why this happened:

quoted

So, I could use u64 instead of phys_addr_t and resource_size_t, and
keep "%lli" (or "%Li"), but then I'd have to check if the parsed value

%Li is not POSIX.  Don't use it (stick with %lli).

quoted

would overflow a 32-bit address value on arches where phys_addr_t is
u32, which would make things a bit more messy and awkward.

I'm planning on #ifdef-ing the format string instead:

#ifdef CONFIG_PHYS_ADDR_T_64BIT
#define PH_ADDR_SCAN_FMT "@%Li%n:%Li:%Li%n"
#else
#define PH_ADDR_SCAN_FMT "@%li%n:%li:%li%n"
#endif

A more typical approach is akin to <inttypes.h>; have PH_ADDR_FMT
defined to either "lli" or "li", then write sscanf(str, "@%"PH_ADDR_FMT
"%n:..., ...), using PH_ADDR_FMT multiple times.

quoted

...
        processed = sscanf(str, PH_ADDR_SCAN_FMT,
                           &base, &consumed,
                           &ctrl_off, &data_off, &consumed);

Umm, why are you passing &consumed to more than one sscanf() %?  That's
(probably) undefined behavior.

[In general, sscanf() is a horrid interface to use for parsing integers
- it has undefined behavior if the input text would trigger integer
overflow, making it safe to use ONLY on text that you control and can
guarantee won't overflow. By the time you've figured out if untrusted
text meets the requirement for safe parsing via sscanf(), you've
practically already parsed it via safer strtol() and friends.]

Yes, but this is the kernel, which may or may not follow POSIX
semantics. (And may or may not curse at POSIX in the process, either
way! :))

Laszlo
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html