[Qemu-devel] strncpy: best avoided

[Jim Meyering]

[Hmm...  This was supposed to be the first in the series]

Given qemu's HACKING comments, I'm sure many here have read "man strncpy",
where it indicates it is often not the best function to use.

However, many of the uses of strncpy in qemu mistakenly fail to ensure
that the destination buffer is NUL-terminated.  The first 7 c-sets fix
a dozen or so buffer overrun errors due to the lack of NUL-termination
in buffers that are later used in a context that requires the terminating
NUL.

I audited all of the strndup uses in qemu and have replaced many with
uses of qemu's pstrcpy function (it guarantees NUL-termination and does
not zero-fill).  A few are easily/cleanly replaced by uses of memcpy,
and for the few remaining uses that are justified, I added comments
marking the use as justified, explaining that it's ok because uses of
the destination buffer (currently) do not require NUL-termination.
But see the note[0] below.

Some of these changes definitely count as trivial, while others look
trivial but required that I look into kernel sources to confirm that
NUL-termination is ok, but not required (e.g., for the SIOCGIFHWADDR
ioctl's ifr.ifr_name input: linux clears its last byte, up front).

Here's a Q&D classification of these change sets:

overrun                      block.c
several overruns             block/sheepdog.c
overrun                      block/vmdk.c
2 overruns                   hw/9pfs/virtio-9p-synth.c
overrun                      hw/lm32_hwsetup.h
overrun                      os-posix.c
overrun                      target-ppc/kvm.c

2-unchecked-strdup+comment   linux-user/elfload.c
simplify/avoid strncpy       ui/vnc-auth-sasl.c
snprintf+pstrcpy fragile     hw/bt-hci.c

2 x s/strncpy/memcpy/        hw/9pfs/virtio-9p-posix-acl.c
s/strncpy/memcpy/            hw/9pfs/virtio-9p-xattr-user.c
s/strncpy/memcpy/            hw/9pfs/virtio-9p-xattr.c
s/strncpy/memcpy/            hw/spapr_vscsi.c
s/strncpy/pstrcpy/ +Makefile libcacard/vcard_emul_nss.c
s/strncpy/pstrcpy            qga/commands-posix.c
s/strncpy/pstrcpy            target-i386/cpu.c
remove strzcpy definition    hw/acpi.c

comment                      block/qcow2.c
comment                      hw/r2d.c
comment                      hw/scsi-bus.c
comment                      HACKING

address@hidden


[0] I have mixed feelings about marking with comments the remaining,
justifiable uses of strncpy.  Note: I didn't finish that job.  On one
hand, it's good to document what I've done, presumably saving others
the work of auditing those uses.  On the other, it's ugly and not very
maintainable.

After writing the few comment-adding patches that merely say "this strncpy
use is justified...", I realized it might be better to use a different
symbol to indicate when one really does intend to use strncpy and realizes
the implications.  That would make it trivial to prohibit (mechanically)
any new direct use of "strncpy".  Then all "safe/intended" uses would
use the new symbol, say strNcpy or STRNCPY (with a comment explaining
the issues near the definition of the macro or static inline function),
and we wouldn't have to pollute the code with comments marking each use.
If there's interest (or no objection) I'll do that separately.