[0/5] clean up snprintf use for string copying

  There are quite a few instances remaining in DPDK where snprintf is being
used for string copying. These were not being picked up by our existing
coccinelle script, but that can be fixed by editing the script and running
it against our code. In the process a bug was found and fixed in the
bonding pmd, where we were incorrectly specifiying the buffer length
parameter to snprintf.

The actual replacement was done in two phases - first replacing all
instances where only the snprintf line in question needed changing, then
fixing the other instances where we also needed to add in the header
include. [Using two stages allowed the header addition to be automated too,
since we had a list of files where every one needed the header inclusion]


Bruce Richardson (5):
  net/bonding: fix buffer length when printing strings
  devtools/cocci: make strlcpy replacement smarter
  devtools/cocci: create safer version of strlcpy script
  replace snprintf with strlcpy without adding extra include
  replace snprintf with strlcpy

 app/pdump/main.c                              |  4 +--
 app/proc-info/main.c                          |  2 +-
 app/test-acl/main.c                           |  3 +-
 app/test-bbdev/main.c                         |  5 ++-
 app/test-eventdev/evt_options.c               |  3 +-
 app/test-pmd/cmdline_flow.c                   | 13 +++----
 app/test/test_cmdline_etheraddr.c             |  3 +-
 app/test/test_cmdline_num.c                   |  3 +-
 app/test/test_compressdev.c                   |  2 +-
 app/test/test_eal_flags.c                     |  2 +-
 app/test/test_table_acl.c                     | 11 +++---
 devtools/cocci/strlcpy-with-header.cocci      | 12 +++++++
 devtools/cocci/strlcpy.cocci                  |  3 +-
 drivers/bus/dpaa/base/fman/of.c               |  7 ++--
 drivers/bus/pci/bsd/pci.c                     |  2 +-
 drivers/bus/pci/linux/pci_uio.c               |  3 +-
 drivers/bus/vdev/vdev.c                       |  2 +-
 .../crypto/aesni_mb/rte_aesni_mb_pmd_ops.c    |  3 +-
 .../aesni_mb/rte_aesni_mb_pmd_ops_compat.c    |  3 +-
 drivers/crypto/ccp/rte_ccp_pmd.c              |  6 ++--
 .../scheduler/rte_cryptodev_scheduler.c       |  8 ++---
 drivers/event/opdl/opdl_ring.c                |  3 +-
 drivers/net/af_packet/rte_eth_af_packet.c     |  5 +--
 drivers/net/ark/ark_pktchkr.c                 |  3 +-
 drivers/net/ark/ark_pktgen.c                  |  3 +-
 drivers/net/atlantic/atl_ethdev.c             |  5 +--
 drivers/net/bnx2x/bnx2x_ethdev.c              |  8 ++---
 drivers/net/bnxt/bnxt_stats.c                 | 36 +++++++++----------
 drivers/net/bonding/rte_eth_bond_pmd.c        | 23 ++++++------
 drivers/net/dpaa/dpaa_ethdev.c                |  8 ++---
 drivers/net/dpaa2/dpaa2_ethdev.c              |  7 ++--
 drivers/net/e1000/igb_ethdev.c                | 17 ++++-----
 drivers/net/ena/ena_ethdev.c                  | 11 +++---
 drivers/net/failsafe/failsafe_args.c          |  2 +-
 drivers/net/failsafe/failsafe_eal.c           |  6 ++--
 drivers/net/i40e/i40e_ethdev.c                | 12 +++----
 drivers/net/i40e/rte_pmd_i40e.c               |  5 +--
 drivers/net/ice/ice_ethdev.c                  | 11 +++---
 drivers/net/ixgbe/ixgbe_ethdev.c              | 35 +++++++++---------
 drivers/net/kni/rte_eth_kni.c                 |  3 +-
 drivers/net/liquidio/lio_ethdev.c             |  5 +--
 drivers/net/mvneta/mvneta_ethdev.c            |  3 +-
 drivers/net/mvpp2/mrvl_ethdev.c               |  5 +--
 drivers/net/pcap/rte_eth_pcap.c               |  8 ++---
 drivers/net/qede/qede_ethdev.c                | 22 ++++++------
 drivers/net/ring/rte_eth_ring.c               |  3 +-
 drivers/net/softnic/rte_eth_softnic_tap.c     |  2 +-
 drivers/net/tap/rte_eth_tap.c                 |  9 +++--
 drivers/net/virtio/virtio_user/vhost_user.c   |  3 +-
 .../net/virtio/virtio_user/virtio_user_dev.c  |  3 +-
 examples/cmdline/commands.c                   |  2 +-
 examples/cmdline/parse_obj_list.c             |  2 +-
 examples/ethtool/lib/rte_ethtool.c            |  8 ++---
 examples/exception_path/main.c                |  4 +--
 examples/ip_pipeline/kni.c                    |  2 +-
 examples/ip_pipeline/tap.c                    |  2 +-
 examples/l2fwd-crypto/main.c                  |  4 +--
 examples/l3fwd-power/main.c                   |  2 +-
 .../multi_process/simple_mp/mp_commands.c     |  2 +-
 examples/netmap_compat/bridge/bridge.c        |  4 +--
 examples/netmap_compat/lib/compat_netmap.c    |  2 +-
 examples/tep_termination/main.c               |  2 +-
 examples/vhost/main.c                         |  2 +-
 examples/vhost_scsi/scsi.c                    |  9 +++--
 examples/vm_power_manager/channel_manager.c   |  7 ++--
 examples/vm_power_manager/channel_monitor.c   | 14 ++++----
 lib/librte_acl/rte_acl.c                      |  3 +-
 lib/librte_bbdev/rte_bbdev.c                  |  3 +-
 lib/librte_cfgfile/rte_cfgfile.c              | 22 ++++++------
 lib/librte_cmdline/cmdline.c                  |  2 +-
 lib/librte_cmdline/cmdline_parse.c            |  5 +--
 lib/librte_cmdline/cmdline_parse_num.c        |  2 +-
 lib/librte_compressdev/rte_compressdev.c      |  5 +--
 lib/librte_compressdev/rte_compressdev_pmd.c  |  3 +-
 lib/librte_cryptodev/rte_cryptodev.c          |  4 +--
 lib/librte_cryptodev/rte_cryptodev_pmd.c      |  3 +-
 lib/librte_distributor/rte_distributor.c      |  2 +-
 lib/librte_distributor/rte_distributor_v20.c  |  2 +-
 lib/librte_eal/common/eal_common_memzone.c    |  2 +-
 lib/librte_eal/common/eal_common_options.c    |  3 +-
 lib/librte_eal/common/eal_common_tailqs.c     |  2 +-
 lib/librte_efd/rte_efd.c                      |  3 +-
 lib/librte_ethdev/rte_ethdev.c                |  6 ++--
 lib/librte_eventdev/rte_eventdev.c            |  4 +--
 lib/librte_flow_classify/rte_flow_classify.c  |  4 +--
 lib/librte_hash/rte_cuckoo_hash.c             |  2 +-
 lib/librte_hash/rte_fbk_hash.c                |  2 +-
 lib/librte_jobstats/rte_jobstats.c            |  3 +-
 lib/librte_kni/rte_kni.c                      |  6 ++--
 lib/librte_latencystats/rte_latencystats.c    |  5 +--
 lib/librte_lpm/rte_lpm.c                      |  4 +--
 lib/librte_lpm/rte_lpm6.c                     |  2 +-
 lib/librte_member/rte_member.c                |  3 +-
 lib/librte_mempool/rte_mempool.c              |  2 +-
 lib/librte_mempool/rte_mempool_ops.c          |  3 +-
 lib/librte_pdump/rte_pdump.c                  |  8 ++---
 lib/librte_pipeline/rte_pipeline.c            |  2 +-
 lib/librte_power/power_acpi_cpufreq.c         |  3 +-
 lib/librte_power/power_pstate_cpufreq.c       |  3 +-
 lib/librte_rawdev/rte_rawdev.c                |  3 +-
 lib/librte_reorder/rte_reorder.c              |  5 +--
 lib/librte_ring/rte_ring.c                    |  2 +-
 102 files changed, 302 insertions(+), 270 deletions(-)
 create mode 100644 devtools/cocci/strlcpy-with-header.cocci
  

> On Apr 3, 2019, at 9:45 AM, Bruce Richardson <bruce.richardson@intel.com> wrote:
> 
> There are quite a few instances remaining in DPDK where snprintf is being
> used for string copying. These were not being picked up by our existing
> coccinelle script, but that can be fixed by editing the script and running
> it against our code. In the process a bug was found and fixed in the
> bonding pmd, where we were incorrectly specifiying the buffer length
> parameter to snprintf.
> 
> The actual replacement was done in two phases - first replacing all
> instances where only the snprintf line in question needed changing, then
> fixing the other instances where we also needed to add in the header
> include. [Using two stages allowed the header addition to be automated too,
> since we had a list of files where every one needed the header inclusion]
> 
> 
> Bruce Richardson (5):
>  net/bonding: fix buffer length when printing strings
>  devtools/cocci: make strlcpy replacement smarter
>  devtools/cocci: create safer version of strlcpy script
>  replace snprintf with strlcpy without adding extra include
>  replace snprintf with strlcpy
> 

Should we not be testing the return values from strlcpy and snprintf, which means we need to create a macro or inline function. We could use a macro and only enable with DEBUG support if we think performance or code size if a problem.

I am surprised none of the tools are catching these types of problems.

Not to make Bruce do that change for this patch, but we need to look at it for a later patch IMO.

Regards,
Keith
  

On Wed, Apr 03, 2019 at 03:57:49PM +0100, Wiles, Keith wrote:
> 
> 
> > On Apr 3, 2019, at 9:45 AM, Bruce Richardson <bruce.richardson@intel.com> wrote:
> > 
> > There are quite a few instances remaining in DPDK where snprintf is being
> > used for string copying. These were not being picked up by our existing
> > coccinelle script, but that can be fixed by editing the script and running
> > it against our code. In the process a bug was found and fixed in the
> > bonding pmd, where we were incorrectly specifiying the buffer length
> > parameter to snprintf.
> > 
> > The actual replacement was done in two phases - first replacing all
> > instances where only the snprintf line in question needed changing, then
> > fixing the other instances where we also needed to add in the header
> > include. [Using two stages allowed the header addition to be automated too,
> > since we had a list of files where every one needed the header inclusion]
> > 
> > 
> > Bruce Richardson (5):
> >  net/bonding: fix buffer length when printing strings
> >  devtools/cocci: make strlcpy replacement smarter
> >  devtools/cocci: create safer version of strlcpy script
> >  replace snprintf with strlcpy without adding extra include
> >  replace snprintf with strlcpy
> > 
> 
> Should we not be testing the return values from strlcpy and snprintf, which means we need to create a macro or inline function. We could use a macro and only enable with DEBUG support if we think performance or code size if a problem.
> 

Yes, I think in some/many cases we should be checking the return value, but
unfortunately I don't think it's the case that we always should or should
not do so. In some cases, truncation is fine. Therefore, I don't think an
automated solution can work here, and I don't see much point in wrapping
any of these functions in macros, since the action to take on truncation
probably varies from place to place.

> I am surprised none of the tools are catching these types of problems.
> 
Yes. It would be useful to have a tool to flag this, so we can see under
what circumstances it may be a problem.

> Not to make Bruce do that change for this patch, but we need to look at it for a later patch IMO.
> 
Phew! :-)
  

On Wed, 3 Apr 2019 14:57:49 +0000
"Wiles, Keith" <keith.wiles@intel.com> wrote:

> > On Apr 3, 2019, at 9:45 AM, Bruce Richardson <bruce.richardson@intel.com> wrote:
> > 
> > There are quite a few instances remaining in DPDK where snprintf is being
> > used for string copying. These were not being picked up by our existing
> > coccinelle script, but that can be fixed by editing the script and running
> > it against our code. In the process a bug was found and fixed in the
> > bonding pmd, where we were incorrectly specifiying the buffer length
> > parameter to snprintf.
> > 
> > The actual replacement was done in two phases - first replacing all
> > instances where only the snprintf line in question needed changing, then
> > fixing the other instances where we also needed to add in the header
> > include. [Using two stages allowed the header addition to be automated too,
> > since we had a list of files where every one needed the header inclusion]
> > 
> > 
> > Bruce Richardson (5):
> >  net/bonding: fix buffer length when printing strings
> >  devtools/cocci: make strlcpy replacement smarter
> >  devtools/cocci: create safer version of strlcpy script
> >  replace snprintf with strlcpy without adding extra include
> >  replace snprintf with strlcpy
> >   
> 
> Should we not be testing the return values from strlcpy and snprintf, which means we need to create a macro or inline function. We could use a macro and only enable with DEBUG support if we think performance or code size if a problem.
> 
> I am surprised none of the tools are catching these types of problems.
> 
> Not to make Bruce do that change for this patch, but we need to look at it for a later patch IMO.
> 
> Regards,
> Keith
> 

In a lot of cases the error checking should be earlier in the input path.
Detecting overflow late in making a string is unhelpful.  But rejecting
input where the name is too long to start with is safer and better.

It is one of those cases where static checkers tell you there is a
problem but the right solution is farther back in the code, not just
right where the error is reported.
  

03/04/2019 16:45, Bruce Richardson:
> There are quite a few instances remaining in DPDK where snprintf is being
> used for string copying. These were not being picked up by our existing
> coccinelle script, but that can be fixed by editing the script and running
> it against our code. In the process a bug was found and fixed in the
> bonding pmd, where we were incorrectly specifiying the buffer length
> parameter to snprintf.
> 
> The actual replacement was done in two phases - first replacing all
> instances where only the snprintf line in question needed changing, then
> fixing the other instances where we also needed to add in the header
> include. [Using two stages allowed the header addition to be automated too,
> since we had a list of files where every one needed the header inclusion]
> 
> 
> Bruce Richardson (5):
>   net/bonding: fix buffer length when printing strings
>   devtools/cocci: make strlcpy replacement smarter
>   devtools/cocci: create safer version of strlcpy script
>   replace snprintf with strlcpy without adding extra include
>   replace snprintf with strlcpy

Applied (with 2 rebase changes), thanks