The following series gets current master able to build on Fedora 28 + x86_64 using gcc 8.0.1. There were a number of build-breaking problems, mostly around correct string processing but all of them reflecting some issue in the code. Of the two major issues, one around struct alignment in dpaa I think is properly fixed by the patch 3/18 here, but for the apparently broken hash function cast in 1/18 I just stopped it breaking the build; unless I miss the trick it seems it needs fixing properly after some further discussion. Because of the kind of widespread string processing / off-by-one / buffer overflow issue fixed here, I think it'd be a really good idea to run this stuff through Coverity. They will give you a free account for OSS projects here https://scan.coverity.com/ --- Andy Green (18): lib: ret_table: workaround hash function cast error drivers: bus: pci: fix strncpy dangerous code drivers: bus: dpaa: fix inconsistent struct alignment drivers: net: axgbe: fix broken eeprom string comp drivers: net: nfp: nfpcore: fix strncpy misuse drivers: net: nfp: nfpcore fix off-by-one and no NUL on strncpy use drivers: net: nfp: don't memcpy out of source range drivers: net: nfp: fix buffer overflow in fw_name drivers: net: qede: fix strncpy constant and NUL drivers: net: qede: fix broken strncpy drivers:net:sfc: fix strncpy length drivers: net: sfc: fix another strncpy size and NUL drivers: net: vdev: readlink inputs cannot be aliased drivers: net: vdev: fix 3 x strncpy misuse test-pmd: can't find include app: fix sprintf overrun bug app: test-bbdev: strcpy ok for allocated string app: test-bbdev: strcpy ok for allocated string 2 app/proc-info/main.c | 9 +++++++-- app/test-bbdev/test_bbdev_vector.c | 5 +++-- app/test-pmd/Makefile | 1 + drivers/bus/dpaa/base/qbman/qman.c | 14 +++++++------- drivers/bus/dpaa/include/fsl_qman.h | 24 +++++++++++++----------- drivers/bus/pci/linux/pci.c | 3 ++- drivers/net/axgbe/axgbe_phy_impl.c | 4 ++-- drivers/net/nfp/nfp_net.c | 4 ++-- drivers/net/nfp/nfpcore/nfp_cpp_pcie_ops.c | 3 ++- drivers/net/nfp/nfpcore/nfp_resource.c | 6 +++--- drivers/net/qede/base/ecore_int.c | 10 ++++++---- drivers/net/qede/qede_main.c | 3 ++- drivers/net/sfc/sfc_ethdev.c | 9 ++++++--- drivers/net/vdev_netvsc/vdev_netvsc.c | 16 ++++++++++------ lib/librte_table/rte_table_hash_cuckoo.c | 2 +- 15 files changed, 67 insertions(+), 46 deletions(-) --
/home/agreen/projects/dpdk/lib/librte_table/rte_table_hash_cuckoo.c:110:16: error: cast between incompatible function types from ‘rte_table_hash_op_hash’ {aka ‘long unsigned int (*)(void *, void *, unsigned int, long unsigned int)’} to ‘uint32_t (*)(const void *, uint32_t, uint32_t)’ {aka ‘unsigned int (*)(const void *, unsigned int, unsigned int)’} [-Werror=cast-function-type] .hash_func = (rte_hash_function)(p->f_hash), The code seems to be quite broken. It's casting this typedef uint64_t (*rte_table_hash_op_hash)( void *key, void *key_mask, uint32_t key_size, uint64_t seed); to this typedef uint32_t (*rte_hash_function)(const void *key, uint32_t key_len, uint32_t init_val); if the definition with 4 args is later called with a pointer giving it three args, obviously it working is just an accident. I grepped around a bit and could not see it being cast back to the original type before use; the uses I saw have three args. I simply patch it to stop the build breaking, rather than fix it, since I am not sure what a fix should look like considering the whole code. --- lib/librte_table/rte_table_hash_cuckoo.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/librte_table/rte_table_hash_cuckoo.c b/lib/librte_table/rte_table_hash_cuckoo.c index dcb4fe978..eca72b506 100644 --- a/lib/librte_table/rte_table_hash_cuckoo.c +++ b/lib/librte_table/rte_table_hash_cuckoo.c @@ -107,7 +107,7 @@ rte_table_hash_cuckoo_create(void *params, struct rte_hash_parameters hash_cuckoo_params = { .entries = p->n_keys, .key_len = p->key_size, - .hash_func = (rte_hash_function)(p->f_hash), + .hash_func = (rte_hash_function)(void *)(p->f_hash), .hash_func_init_val = p->seed, .socket_id = socket_id, .name = p->name
In function ‘pci_get_kernel_driver_by_path’, inlined from ‘pci_scan_one.isra.1’ at /home/agreen/projects/dpdk/drivers/bus/pci/linux/pci.c:317:8: /home/agreen/projects/dpdk/drivers/bus/pci/linux/pci.c:57:3: error: ‘strncpy’ specified bound depends on the length of the source argument [-Werror=stringop-overflow=] strncpy(dri_name, name + 1, strlen(name + 1) + 1); --- drivers/bus/pci/linux/pci.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/drivers/bus/pci/linux/pci.c b/drivers/bus/pci/linux/pci.c index 4630a8057..b5bdfd33e 100644 --- a/drivers/bus/pci/linux/pci.c +++ b/drivers/bus/pci/linux/pci.c @@ -54,7 +54,8 @@ pci_get_kernel_driver_by_path(const char *filename, char *dri_name) name = strrchr(path, '/'); if (name) { - strncpy(dri_name, name + 1, strlen(name + 1) + 1); + strncpy(dri_name, name + 1, sizeof(dri_name) - 1); + dri_name[sizeof(dri_name) - 1] = '\0'; return 0; }
The actual descriptor for qm_mr_entry is 64-byte aligned. But the original code plays a trick, and puts a u8 common to the three descriptor subtypes in the union afterwards outside their structure definitions. Unfortunately since they compose a struct qm_fd with alignment 8, this trick destroys the ability of the compiler to understand what has happened, resulting in this kind of problem: /home/agreen/projects/dpdk/drivers/bus/dpaa/include/fsl_qman.h:354:3: error: alignment 1 of ‘struct <anonymous>’ is less than 8 [-Werror=packed-not-aligned] } __packed dcern; on gcc 8 / Fedora 28 out of the box. This patch moves the u8 verb into the structure definitions composed into the union, so the alignment of the parent struct containing the alignment 8 object can also be seen to be alignment 8 by the compiler. Uses of .verb are fixed up to use .ern.verb (the same offset of +0 inside all the structs in the union). The final struct layout should be unchanged. --- drivers/bus/dpaa/base/qbman/qman.c | 14 +++++++------- drivers/bus/dpaa/include/fsl_qman.h | 24 +++++++++++++----------- 2 files changed, 20 insertions(+), 18 deletions(-) diff --git a/drivers/bus/dpaa/base/qbman/qman.c b/drivers/bus/dpaa/base/qbman/qman.c index 2810fdd26..27d98cc10 100644 --- a/drivers/bus/dpaa/base/qbman/qman.c +++ b/drivers/bus/dpaa/base/qbman/qman.c @@ -314,9 +314,9 @@ static int drain_mr_fqrni(struct qm_portal *p) if (!msg) return 0; } - if ((msg->verb & QM_MR_VERB_TYPE_MASK) != QM_MR_VERB_FQRNI) { + if ((msg->ern.verb & QM_MR_VERB_TYPE_MASK) != QM_MR_VERB_FQRNI) { /* We aren't draining anything but FQRNIs */ - pr_err("Found verb 0x%x in MR\n", msg->verb); + pr_err("Found verb 0x%x in MR\n", msg->ern.verb); return -1; } qm_mr_next(p); @@ -483,7 +483,7 @@ static inline void qm_mr_pvb_update(struct qm_portal *portal) /* when accessing 'verb', use __raw_readb() to ensure that compiler * inlining doesn't try to optimise out "excess reads". */ - if ((__raw_readb(&res->verb) & QM_MR_VERB_VBIT) == mr->vbit) { + if ((__raw_readb(&res->ern.verb) & QM_MR_VERB_VBIT) == mr->vbit) { mr->pi = (mr->pi + 1) & (QM_MR_SIZE - 1); if (!mr->pi) mr->vbit ^= QM_MR_VERB_VBIT; @@ -832,7 +832,7 @@ static u32 __poll_portal_slow(struct qman_portal *p, u32 is) goto mr_done; swapped_msg = *msg; hw_fd_to_cpu(&swapped_msg.ern.fd); - verb = msg->verb & QM_MR_VERB_TYPE_MASK; + verb = msg->ern.verb & QM_MR_VERB_TYPE_MASK; /* The message is a software ERN iff the 0x20 bit is set */ if (verb & 0x20) { switch (verb) { @@ -1666,7 +1666,7 @@ int qman_retire_fq(struct qman_fq *fq, u32 *flags) */ struct qm_mr_entry msg; - msg.verb = QM_MR_VERB_FQRNI; + msg.ern.verb = QM_MR_VERB_FQRNI; msg.fq.fqs = mcr->alterfq.fqs; msg.fq.fqid = fq->fqid; #ifdef CONFIG_FSL_QMAN_FQ_LOOKUP @@ -2643,7 +2643,7 @@ int qman_shutdown_fq(u32 fqid) qm_mr_pvb_update(low_p); msg = qm_mr_current(low_p); while (msg) { - if ((msg->verb & + if ((msg->ern.verb & QM_MR_VERB_TYPE_MASK) == QM_MR_VERB_FQRN) found_fqrn = 1; @@ -2711,7 +2711,7 @@ int qman_shutdown_fq(u32 fqid) qm_mr_pvb_update(low_p); msg = qm_mr_current(low_p); while (msg) { - if ((msg->verb & QM_MR_VERB_TYPE_MASK) == + if ((msg->ern.verb & QM_MR_VERB_TYPE_MASK) == QM_MR_VERB_FQRL) orl_empty = 1; qm_mr_next(low_p); diff --git a/drivers/bus/dpaa/include/fsl_qman.h b/drivers/bus/dpaa/include/fsl_qman.h index e9793f30d..cf6dfce11 100644 --- a/drivers/bus/dpaa/include/fsl_qman.h +++ b/drivers/bus/dpaa/include/fsl_qman.h @@ -284,20 +284,20 @@ static inline dma_addr_t qm_sg_addr(const struct qm_sg_entry *sg) } while (0) /* See 1.5.8.1: "Enqueue Command" */ -struct qm_eqcr_entry { +struct __rte_aligned(8) qm_eqcr_entry { u8 __dont_write_directly__verb; u8 dca; u16 seqnum; u32 orp; /* 24-bit */ u32 fqid; /* 24-bit */ u32 tag; - struct qm_fd fd; + struct qm_fd fd; /* this has alignment 8 */ u8 __reserved3[32]; } __packed; /* "Frame Dequeue Response" */ -struct qm_dqrr_entry { +struct __rte_aligned(8) qm_dqrr_entry { u8 verb; u8 stat; u16 seqnum; /* 15-bit */ @@ -305,7 +305,7 @@ struct qm_dqrr_entry { u8 __reserved2[3]; u32 fqid; /* 24-bit */ u32 contextB; - struct qm_fd fd; + struct qm_fd fd; /* this has alignment 8 */ u8 __reserved4[32]; }; @@ -323,18 +323,19 @@ struct qm_dqrr_entry { /* "ERN Message Response" */ /* "FQ State Change Notification" */ struct qm_mr_entry { - u8 verb; union { struct { + u8 verb; u8 dca; u16 seqnum; u8 rc; /* Rejection Code */ u32 orp:24; u32 fqid; /* 24-bit */ u32 tag; - struct qm_fd fd; - } __packed ern; + struct qm_fd fd; /* this has alignment 8 */ + } __packed __rte_aligned(8) ern; struct { + u8 verb; #if __BYTE_ORDER__ == __ORDER_BIG_ENDIAN__ u8 colour:2; /* See QM_MR_DCERN_COLOUR_* */ u8 __reserved1:4; @@ -349,18 +350,19 @@ struct qm_mr_entry { u32 __reserved3:24; u32 fqid; /* 24-bit */ u32 tag; - struct qm_fd fd; - } __packed dcern; + struct qm_fd fd; /* this has alignment 8 */ + } __packed __rte_aligned(8) dcern; struct { + u8 verb; u8 fqs; /* Frame Queue Status */ u8 __reserved1[6]; u32 fqid; /* 24-bit */ u32 contextB; u8 __reserved2[16]; - } __packed fq; /* FQRN/FQRNI/FQRL/FQPN */ + } __packed __rte_aligned(8) fq; /* FQRN/FQRNI/FQRL/FQPN */ }; u8 __reserved2[32]; -} __packed; +} __packed __rte_aligned(8); #define QM_MR_VERB_VBIT 0x80 /* * ERNs originating from direct-connect portals ("dcern") use 0x20 as a verb
/home/agreen/projects/dpdk/drivers/net/axgbe/axgbe_phy_impl.c:576:6: error: ‘__builtin_memcmp_eq’ reading 16 bytes from a region of size 9 [-Werror=stringop-overflow=] if (memcmp(&sfp_eeprom->base[AXGBE_SFP_BASE_VENDOR_NAME], ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ AXGBE_BEL_FUSE_VENDOR, AXGBE_SFP_BASE_VENDOR_NAME_LEN)) --- drivers/net/axgbe/axgbe_phy_impl.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/drivers/net/axgbe/axgbe_phy_impl.c b/drivers/net/axgbe/axgbe_phy_impl.c index dfa908dd8..973177f69 100644 --- a/drivers/net/axgbe/axgbe_phy_impl.c +++ b/drivers/net/axgbe/axgbe_phy_impl.c @@ -574,11 +574,11 @@ static bool axgbe_phy_belfuse_parse_quirks(struct axgbe_port *pdata) struct axgbe_sfp_eeprom *sfp_eeprom = &phy_data->sfp_eeprom; if (memcmp(&sfp_eeprom->base[AXGBE_SFP_BASE_VENDOR_NAME], - AXGBE_BEL_FUSE_VENDOR, AXGBE_SFP_BASE_VENDOR_NAME_LEN)) + AXGBE_BEL_FUSE_VENDOR, strlen(AXGBE_BEL_FUSE_VENDOR))) return false; if (!memcmp(&sfp_eeprom->base[AXGBE_SFP_BASE_VENDOR_PN], - AXGBE_BEL_FUSE_PARTNO, AXGBE_SFP_BASE_VENDOR_PN_LEN)) { + AXGBE_BEL_FUSE_PARTNO, strlen(AXGBE_BEL_FUSE_PARTNO))) { phy_data->sfp_base = AXGBE_SFP_BASE_1000_SX; phy_data->sfp_cable = AXGBE_SFP_CABLE_ACTIVE; phy_data->sfp_speed = AXGBE_SFP_SPEED_1000;
--- drivers/net/nfp/nfpcore/nfp_cpp_pcie_ops.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/drivers/net/nfp/nfpcore/nfp_cpp_pcie_ops.c b/drivers/net/nfp/nfpcore/nfp_cpp_pcie_ops.c index 4e6c66624..9f6704a7f 100644 --- a/drivers/net/nfp/nfpcore/nfp_cpp_pcie_ops.c +++ b/drivers/net/nfp/nfpcore/nfp_cpp_pcie_ops.c @@ -846,7 +846,8 @@ nfp6000_init(struct nfp_cpp *cpp, const char *devname) memset(desc->busdev, 0, BUSDEV_SZ); - strncpy(desc->busdev, devname, strlen(devname)); + strncpy(desc->busdev, devname, sizeof(desc->busdev) - 1); + desc->busdev[sizeof(desc->busdev) - 1] = '\0'; ret = nfp_acquire_process_lock(desc); if (ret)
/home/agreen/projects/dpdk/drivers/net/nfp/nfpcore/nfp_resource.c:76:2: error: ‘strncpy’ output may be truncated copying 8 bytes from a string of length 8 [-Werror=stringop-truncation] strncpy(name_pad, res->name, sizeof(name_pad)); --- drivers/net/nfp/nfpcore/nfp_resource.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/drivers/net/nfp/nfpcore/nfp_resource.c b/drivers/net/nfp/nfpcore/nfp_resource.c index e1df2b2e1..54c6dcf54 100644 --- a/drivers/net/nfp/nfpcore/nfp_resource.c +++ b/drivers/net/nfp/nfpcore/nfp_resource.c @@ -65,15 +65,15 @@ struct nfp_resource { static int nfp_cpp_resource_find(struct nfp_cpp *cpp, struct nfp_resource *res) { - char name_pad[NFP_RESOURCE_ENTRY_NAME_SZ] = {}; + char name_pad[NFP_RESOURCE_ENTRY_NAME_SZ + 2]; struct nfp_resource_entry entry; uint32_t cpp_id, key; int ret, i; cpp_id = NFP_CPP_ID(NFP_RESOURCE_TBL_TARGET, 3, 0); /* Atomic read */ - memset(name_pad, 0, NFP_RESOURCE_ENTRY_NAME_SZ); - strncpy(name_pad, res->name, sizeof(name_pad)); + memset(name_pad, 0, sizeof(name_pad)); + strncpy(name_pad, res->name, sizeof(name_pad) - 1); /* Search for a matching entry */ if (!memcmp(name_pad, NFP_RESOURCE_TBL_NAME "\0\0\0\0\0\0\0\0", 8)) {
/home/agreen/projects/dpdk/drivers/net/nfp/nfp_net.c:669:2: error: ‘memcpy’ forming offset [5, 6] is out of the bounds [0, 4] of object ‘tmp’ with type ‘uint32_t’ {aka ‘unsigned int’} [-Werror=array-bounds] memcpy(&hw->mac_addr[0], &tmp, sizeof(struct ether_addr)); --- drivers/net/nfp/nfp_net.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/net/nfp/nfp_net.c b/drivers/net/nfp/nfp_net.c index 048324ec9..199aac40b 100644 --- a/drivers/net/nfp/nfp_net.c +++ b/drivers/net/nfp/nfp_net.c @@ -666,7 +666,7 @@ nfp_net_vf_read_mac(struct nfp_net_hw *hw) uint32_t tmp; tmp = rte_be_to_cpu_32(nn_cfg_readl(hw, NFP_NET_CFG_MACADDR)); - memcpy(&hw->mac_addr[0], &tmp, sizeof(struct ether_addr)); + memcpy(&hw->mac_addr[0], &tmp, 4); tmp = rte_be_to_cpu_32(nn_cfg_readl(hw, NFP_NET_CFG_MACADDR + 4)); memcpy(&hw->mac_addr[4], &tmp, 2);
/home/agreen/projects/dpdk/drivers/net/nfp/nfp_net.c: In function ‘nfp_pf_pci_probe’: /home/agreen/projects/dpdk/drivers/net/nfp/nfp_net.c:3160:23: error: ‘%s’ directive writing up to 99 bytes into a region of size 76 [-Werror=format-overflow=] sprintf(fw_name, "%s/%s.nffw", DEFAULT_FW_PATH, serial); --- drivers/net/nfp/nfp_net.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/net/nfp/nfp_net.c b/drivers/net/nfp/nfp_net.c index 199aac40b..d5f0e54e8 100644 --- a/drivers/net/nfp/nfp_net.c +++ b/drivers/net/nfp/nfp_net.c @@ -3144,7 +3144,7 @@ nfp_fw_upload(struct rte_pci_device *dev, struct nfp_nsp *nsp, char *card) struct nfp_cpp *cpp = nsp->cpp; int fw_f; char *fw_buf; - char fw_name[100]; + char fw_name[130]; char serial[100]; struct stat file_stat; off_t fsize, bytes;
--- drivers/net/qede/base/ecore_int.c | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/drivers/net/qede/base/ecore_int.c b/drivers/net/qede/base/ecore_int.c index f43781ba4..c809d84ef 100644 --- a/drivers/net/qede/base/ecore_int.c +++ b/drivers/net/qede/base/ecore_int.c @@ -1103,10 +1103,12 @@ static enum _ecore_status_t ecore_int_deassertion(struct ecore_hwfn *p_hwfn, OSAL_SNPRINTF(bit_name, 30, p_aeu->bit_name, num); - else - OSAL_STRNCPY(bit_name, - p_aeu->bit_name, - 30); + else { + strncpy(bit_name, + p_aeu->bit_name, + sizeof(bit_name) - 1); + bit_name[sizeof(bit_name) - 1] = '\0'; + } /* We now need to pass bitmask in its * correct position.
/home/agreen/projects/dpdk/drivers/net/qede/qede_main.c: In function ‘qed_slowpath_start’: /home/agreen/projects/dpdk/drivers/net/qede/qede_main.c:307:3: error: ‘strncpy’ output may be truncated copying 12 bytes from a string of length 127 [-Werror=stringop-truncation] strncpy((char *)drv_version.name, (const char *)params->name, ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ MCP_DRV_VER_STR_SIZE - 4); ~~~~~~~~~~~~~~~~~~~~~~~~~ --- drivers/net/qede/qede_main.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/drivers/net/qede/qede_main.c b/drivers/net/qede/qede_main.c index 2333ca073..243851da6 100644 --- a/drivers/net/qede/qede_main.c +++ b/drivers/net/qede/qede_main.c @@ -305,7 +305,8 @@ static int qed_slowpath_start(struct ecore_dev *edev, (params->drv_rev << 8) | (params->drv_eng); /* TBD: strlcpy() */ strncpy((char *)drv_version.name, (const char *)params->name, - MCP_DRV_VER_STR_SIZE - 4); + sizeof(drv_version.name) - 1); + drv_version.name[sizeof(drv_version.name) - 1] = '\0'; rc = ecore_mcp_send_drv_version(hwfn, hwfn->p_main_ptt, &drv_version); if (rc) {
--- drivers/net/sfc/sfc_ethdev.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/net/sfc/sfc_ethdev.c b/drivers/net/sfc/sfc_ethdev.c index e42d55350..e9bb283e0 100644 --- a/drivers/net/sfc/sfc_ethdev.c +++ b/drivers/net/sfc/sfc_ethdev.c @@ -742,7 +742,7 @@ sfc_xstats_get_names_by_id(struct rte_eth_dev *dev, char *name = xstats_names[nb_written++].name; strncpy(name, efx_mac_stat_name(sa->nic, i), - sizeof(xstats_names[0].name)); + sizeof(xstats_names[0].name) - 1); name[sizeof(xstats_names[0].name) - 1] = '\0'; }
--- drivers/net/sfc/sfc_ethdev.c | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/drivers/net/sfc/sfc_ethdev.c b/drivers/net/sfc/sfc_ethdev.c index e9bb283e0..bd5f17f33 100644 --- a/drivers/net/sfc/sfc_ethdev.c +++ b/drivers/net/sfc/sfc_ethdev.c @@ -662,10 +662,13 @@ sfc_xstats_get_names(struct rte_eth_dev *dev, for (i = 0; i < EFX_MAC_NSTATS; ++i) { if (EFX_MAC_STAT_SUPPORTED(port->mac_stats_mask, i)) { - if (xstats_names != NULL && nstats < xstats_count) + if (xstats_names != NULL && nstats < xstats_count) { strncpy(xstats_names[nstats].name, efx_mac_stat_name(sa->nic, i), - sizeof(xstats_names[0].name)); + sizeof(xstats_names[0].name) - 1); + xstats_names[0].name[ + sizeof(xstats_names[0].name) - 1] = '\0'; + } nstats++; } }
/home/agreen/projects/dpdk/drivers/net/vdev_netvsc/vdev_netvsc.c:335:2: error: passing argument 2 to restrict-qualified parameter aliases with argument 1 [-Werror=restrict] ret = readlink(buf, buf, size); ^~~ --- drivers/net/vdev_netvsc/vdev_netvsc.c | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/drivers/net/vdev_netvsc/vdev_netvsc.c b/drivers/net/vdev_netvsc/vdev_netvsc.c index c321a9f1b..c11794137 100644 --- a/drivers/net/vdev_netvsc/vdev_netvsc.c +++ b/drivers/net/vdev_netvsc/vdev_netvsc.c @@ -327,12 +327,13 @@ static int vdev_netvsc_sysfs_readlink(char *buf, size_t size, const char *if_name, const char *relpath) { + char in[160]; int ret; - ret = snprintf(buf, size, "/sys/class/net/%s/%s", if_name, relpath); - if (ret == -1 || (size_t)ret >= size) + ret = snprintf(in, sizeof(buf) - 1, "/sys/class/net/%s/%s", if_name, relpath); + if (ret == -1 || (size_t)ret >= sizeof(buf) - 1) return -ENOBUFS; - ret = readlink(buf, buf, size); + ret = readlink(in, buf, size); if (ret == -1) return -errno; if ((size_t)ret >= size - 1)
--- drivers/net/vdev_netvsc/vdev_netvsc.c | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/drivers/net/vdev_netvsc/vdev_netvsc.c b/drivers/net/vdev_netvsc/vdev_netvsc.c index c11794137..c36ec0f9a 100644 --- a/drivers/net/vdev_netvsc/vdev_netvsc.c +++ b/drivers/net/vdev_netvsc/vdev_netvsc.c @@ -182,7 +182,8 @@ vdev_netvsc_foreach_iface(int (*func)(const struct if_nameindex *iface, is_netvsc_ret = vdev_netvsc_iface_is_netvsc(&iface[i]) ? 1 : 0; if (is_netvsc ^ is_netvsc_ret) continue; - strncpy(req.ifr_name, iface[i].if_name, sizeof(req.ifr_name)); + strncpy(req.ifr_name, iface[i].if_name, sizeof(req.ifr_name) - 1); + req.ifr_name[sizeof(req.ifr_name) - 1] = '\0'; if (ioctl(s, SIOCGIFHWADDR, &req) == -1) { DRV_LOG(WARNING, "cannot retrieve information about" " interface \"%s\": %s", @@ -383,7 +384,8 @@ vdev_netvsc_device_probe(const struct if_nameindex *iface, DRV_LOG(DEBUG, "NetVSC interface \"%s\" (index %u) renamed \"%s\"", ctx->if_name, ctx->if_index, iface->if_name); - strncpy(ctx->if_name, iface->if_name, sizeof(ctx->if_name)); + strncpy(ctx->if_name, iface->if_name, sizeof(ctx->if_name) - 1); + ctx->if_name[sizeof(ctx->if_name) - 1] = '\0'; return 0; } if (!is_same_ether_addr(eth_addr, &ctx->if_addr)) @@ -581,7 +583,8 @@ vdev_netvsc_netvsc_probe(const struct if_nameindex *iface, goto error; } ctx->id = vdev_netvsc_ctx_count; - strncpy(ctx->if_name, iface->if_name, sizeof(ctx->if_name)); + strncpy(ctx->if_name, iface->if_name, sizeof(ctx->if_name) - 1); + ctx->if_name[sizeof(ctx->if_name) - 1] = '\0'; ctx->if_index = iface->if_index; ctx->if_addr = *eth_addr; ctx->pipe[0] = -1;
/home/agreen/projects/dpdk/app/test-pmd/cmdline.c:64:10: fatal error: rte_pmd_dpaa.h: No such file or directory #include <rte_pmd_dpaa.h> ^~~~~~~~~~~~~~~~ --- app/test-pmd/Makefile | 1 + 1 file changed, 1 insertion(+) diff --git a/app/test-pmd/Makefile b/app/test-pmd/Makefile index 60ae9b9c1..a0fdd0e11 100644 --- a/app/test-pmd/Makefile +++ b/app/test-pmd/Makefile @@ -13,6 +13,7 @@ APP = testpmd CFLAGS += -DALLOW_EXPERIMENTAL_API CFLAGS += -O3 CFLAGS += $(WERROR_FLAGS) +CFLAGS += -I$(RTE_SDK)/drivers/net/dpaa # # all source are stored in SRCS-y
/home/agreen/projects/dpdk/app/proc-info/main.c: In function ‘nic_xstats_display’: /home/agreen/projects/dpdk/app/proc-info/main.c:495:45: error: ‘%s’ directive writing up to 255 bytes into a region of size between 165 and 232 [-Werror=format-overflow=] sprintf(buf, "PUTVAL %s/dpdkstat-port.%u/%s-%s N:%" ^~ PRIu64"\n", host_id, port_id, counter_type, ~~~~~~~~~~~~ /home/agreen/projects/dpdk/app/proc-info/main.c:495:4: note: ‘sprintf’ output between 31 and 435 bytes into a destination of size 256 sprintf(buf, "PUTVAL %s/dpdkstat-port.%u/%s-%s N:%" ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ PRIu64"\n", host_id, port_id, counter_type, ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ xstats_names[i].name, values[i]); --- app/proc-info/main.c | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/app/proc-info/main.c b/app/proc-info/main.c index 539e13243..df46c235e 100644 --- a/app/proc-info/main.c +++ b/app/proc-info/main.c @@ -488,14 +488,19 @@ nic_xstats_display(uint16_t port_id) if (enable_collectd_format) { char counter_type[MAX_STRING_LEN]; char buf[MAX_STRING_LEN]; + size_t n; collectd_resolve_cnt_type(counter_type, sizeof(counter_type), xstats_names[i].name); - sprintf(buf, "PUTVAL %s/dpdkstat-port.%u/%s-%s N:%" + n = snprintf(buf, MAX_STRING_LEN, + "PUTVAL %s/dpdkstat-port.%u/%s-%s N:%" PRIu64"\n", host_id, port_id, counter_type, xstats_names[i].name, values[i]); - ret = write(stdout_fd, buf, strlen(buf)); + buf[sizeof(buf) - 1] = '\0'; + if (n > sizeof(buf) - 1) + n = sizeof(buf) - 1; + ret = write(stdout_fd, buf, n); if (ret < 0) goto err; } else {
--- app/test-bbdev/test_bbdev_vector.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/test-bbdev/test_bbdev_vector.c b/app/test-bbdev/test_bbdev_vector.c index addef0572..5ad2a6535 100644 --- a/app/test-bbdev/test_bbdev_vector.c +++ b/app/test-bbdev/test_bbdev_vector.c @@ -890,7 +890,7 @@ test_bbdev_vector_read(const char *filename, } memset(entry, 0, strlen(line) + 1); - strncpy(entry, line, strlen(line)); + strcpy(entry, line); /* check if entry ends with , or = */ if (entry[strlen(entry) - 1] == ','
--- app/test-bbdev/test_bbdev_vector.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/app/test-bbdev/test_bbdev_vector.c b/app/test-bbdev/test_bbdev_vector.c index 5ad2a6535..373f94984 100644 --- a/app/test-bbdev/test_bbdev_vector.c +++ b/app/test-bbdev/test_bbdev_vector.c @@ -912,7 +912,8 @@ test_bbdev_vector_read(const char *filename, } entry = entry_extended; - strncat(entry, line, strlen(line)); + /* entry has been allocated accordingly */ + strcpy(&entry[strlen(entry)], line); if (entry[strlen(entry) - 1] != ',') break;
Many thanks. I guess the most of below notes are applicable to many other patches in the series. Signed-off-by: , Fixes: and Cc: stable@dpdk.org tags are missing. See [1]. Changeset summary should start from "net/sfc: " I.e. something like: net/sfc: fix strncpy size and NUL (it looks like "another" is useless in the original subject) In general all patches should pass ./devtools/check-git-log.sh and ./devtools/checkpatches.sh (which requires path to Linux kernel checkpatches.pl). Andrew. [1] http://dpdk.org/doc/guides/contributing/patches.html#commit-messages-subject-line On 05/08/2018 07:30 AM, Andy Green wrote: > --- > drivers/net/sfc/sfc_ethdev.c | 7 +++++-- > 1 file changed, 5 insertions(+), 2 deletions(-) > > diff --git a/drivers/net/sfc/sfc_ethdev.c b/drivers/net/sfc/sfc_ethdev.c > index e9bb283e0..bd5f17f33 100644 > --- a/drivers/net/sfc/sfc_ethdev.c > +++ b/drivers/net/sfc/sfc_ethdev.c > @@ -662,10 +662,13 @@ sfc_xstats_get_names(struct rte_eth_dev *dev, > > for (i = 0; i < EFX_MAC_NSTATS; ++i) { > if (EFX_MAC_STAT_SUPPORTED(port->mac_stats_mask, i)) { > - if (xstats_names != NULL && nstats < xstats_count) > + if (xstats_names != NULL && nstats < xstats_count) { > strncpy(xstats_names[nstats].name, > efx_mac_stat_name(sa->nic, i), > - sizeof(xstats_names[0].name)); > + sizeof(xstats_names[0].name) - 1); > + xstats_names[0].name[ > + sizeof(xstats_names[0].name) - 1] = '\0'; > + } In fact strlcpy() should be used. > nstats++; > } > } >
On 05/08/2018 03:36 PM, Andrew Rybchenko wrote: > Many thanks. I guess the most of below notes are applicable to many other > patches in the series. > > Signed-off-by: , Fixes: and Cc: stable@dpdk.org tags are missing. See [1]. Everybody's project has different prejudices. > Changeset summary should start from "net/sfc: " > I.e. something like: > net/sfc: fix strncpy size and NUL Yeah if that's what you like. > (it looks like "another" is useless in the original subject) It captures my feeling at having to wade through making 18 fixes before I could compile the project on current Fedora. > In general all patches should pass ./devtools/check-git-log.sh and > ./devtools/checkpatches.sh > (which requires path to Linux kernel checkpatches.pl). Can you help me understand why adding CRLFs at 80 cols on the gcc errors I pasted helps anything at all? The patches actually fix problems in the code. If you don't care about Coverity, let me know and I will register this project there and send you fixes when I have time. > Andrew. > > [1] > http://dpdk.org/doc/guides/contributing/patches.html#commit-messages-subject-line > > On 05/08/2018 07:30 AM, Andy Green wrote: >> --- >> drivers/net/sfc/sfc_ethdev.c | 7 +++++-- >> 1 file changed, 5 insertions(+), 2 deletions(-) >> >> diff --git a/drivers/net/sfc/sfc_ethdev.c b/drivers/net/sfc/sfc_ethdev.c >> index e9bb283e0..bd5f17f33 100644 >> --- a/drivers/net/sfc/sfc_ethdev.c >> +++ b/drivers/net/sfc/sfc_ethdev.c >> @@ -662,10 +662,13 @@ sfc_xstats_get_names(struct rte_eth_dev *dev, >> >> for (i = 0; i < EFX_MAC_NSTATS; ++i) { >> if (EFX_MAC_STAT_SUPPORTED(port->mac_stats_mask, i)) { >> - if (xstats_names != NULL && nstats < xstats_count) >> + if (xstats_names != NULL && nstats < xstats_count) { >> strncpy(xstats_names[nstats].name, >> efx_mac_stat_name(sa->nic, i), >> - sizeof(xstats_names[0].name)); >> + sizeof(xstats_names[0].name) - 1); >> + xstats_names[0].name[ >> + sizeof(xstats_names[0].name) - 1] = '\0'; >> + } > > In fact strlcpy() should be used. Fair enough. Last time I looked it wasn't in glibc but seems it is now. -Andy >> nstats++; >> } >> } >> >
On Tue, May 08, 2018 at 12:29:38PM +0800, Andy Green wrote:
> In function ‘pci_get_kernel_driver_by_path’,
> inlined from ‘pci_scan_one.isra.1’ at /home/agreen/projects/dpdk/drivers/bus/pci/linux/pci.c:317:8:
> /home/agreen/projects/dpdk/drivers/bus/pci/linux/pci.c:57:3: error: ‘strncpy’ specified bound depends on the length of the source argument [-Werror=stringop-overflow=]
> strncpy(dri_name, name + 1, strlen(name + 1) + 1);
> ---
> drivers/bus/pci/linux/pci.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/bus/pci/linux/pci.c b/drivers/bus/pci/linux/pci.c
> index 4630a8057..b5bdfd33e 100644
> --- a/drivers/bus/pci/linux/pci.c
> +++ b/drivers/bus/pci/linux/pci.c
> @@ -54,7 +54,8 @@ pci_get_kernel_driver_by_path(const char *filename, char *dri_name)
>
> name = strrchr(path, '/');
> if (name) {
> - strncpy(dri_name, name + 1, strlen(name + 1) + 1);
> + strncpy(dri_name, name + 1, sizeof(dri_name) - 1);
> + dri_name[sizeof(dri_name) - 1] = '\0';
> return 0;
> }
While this fix is correct, a better fix would be to use strlcpy from
rte_string_fns.h.
strlcpy(dri_name, name + 1, sizeof(dri_name));
Regards,
/Bruce
On Tue, May 08, 2018 at 12:29:53PM +0800, Andy Green wrote:
>
> ---
> drivers/net/nfp/nfpcore/nfp_cpp_pcie_ops.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/net/nfp/nfpcore/nfp_cpp_pcie_ops.c b/drivers/net/nfp/nfpcore/nfp_cpp_pcie_ops.c
> index 4e6c66624..9f6704a7f 100644
> --- a/drivers/net/nfp/nfpcore/nfp_cpp_pcie_ops.c
> +++ b/drivers/net/nfp/nfpcore/nfp_cpp_pcie_ops.c
> @@ -846,7 +846,8 @@ nfp6000_init(struct nfp_cpp *cpp, const char *devname)
>
>
> memset(desc->busdev, 0, BUSDEV_SZ);
> - strncpy(desc->busdev, devname, strlen(devname));
> + strncpy(desc->busdev, devname, sizeof(desc->busdev) - 1);
> + desc->busdev[sizeof(desc->busdev) - 1] = '\0';
>
> ret = nfp_acquire_process_lock(desc);
> if (ret)
As with previous patch, a better fix is to use strlcpy. This would apply to
just about all uses of strncpy in the code.
/Bruce
On 05/08/2018 04:58 PM, Bruce Richardson wrote: > On Tue, May 08, 2018 at 12:29:53PM +0800, Andy Green wrote: >> >> --- >> drivers/net/nfp/nfpcore/nfp_cpp_pcie_ops.c | 3 ++- >> 1 file changed, 2 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/net/nfp/nfpcore/nfp_cpp_pcie_ops.c b/drivers/net/nfp/nfpcore/nfp_cpp_pcie_ops.c >> index 4e6c66624..9f6704a7f 100644 >> --- a/drivers/net/nfp/nfpcore/nfp_cpp_pcie_ops.c >> +++ b/drivers/net/nfp/nfpcore/nfp_cpp_pcie_ops.c >> @@ -846,7 +846,8 @@ nfp6000_init(struct nfp_cpp *cpp, const char *devname) >> >> >> memset(desc->busdev, 0, BUSDEV_SZ); >> - strncpy(desc->busdev, devname, strlen(devname)); >> + strncpy(desc->busdev, devname, sizeof(desc->busdev) - 1); >> + desc->busdev[sizeof(desc->busdev) - 1] = '\0'; >> >> ret = nfp_acquire_process_lock(desc); >> if (ret) > As with previous patch, a better fix is to use strlcpy. This would apply to > just about all uses of strncpy in the code. OK. But the strncpy() was already there, it's not introduced by the patch. I agree just doing it in one hit with strlcpy() is nicer. -Andy > /Bruce >
On Tue, May 08, 2018 at 04:18:41PM +0800, Andy Green wrote: > > > On 05/08/2018 03:36 PM, Andrew Rybchenko wrote: > > Many thanks. I guess the most of below notes are applicable to many other > > patches in the series. > > > > Signed-off-by: , Fixes: and Cc: stable@dpdk.org tags are missing. See [1]. > > Everybody's project has different prejudices. > > > Changeset summary should start from "net/sfc: " > > I.e. something like: > > net/sfc: fix strncpy size and NUL > > Yeah if that's what you like. > > > (it looks like "another" is useless in the original subject) > > It captures my feeling at having to wade through making 18 fixes before I > could compile the project on current Fedora. > > > In general all patches should pass ./devtools/check-git-log.sh and > > ./devtools/checkpatches.sh > > (which requires path to Linux kernel checkpatches.pl). > > Can you help me understand why adding CRLFs at 80 cols on the gcc errors I > pasted helps anything at all? The patches actually fix problems in the > code. > I don't think there is any need to wrap the error messages since they are pasted verbatim. > If you don't care about Coverity, let me know and I will register this > project there and send you fixes when I have time. > FYI: The DPDK project is already registered in coverity and scanned regularly. We do try and fix as many issues it flags as possible, but not everything gets dealt with as quickly as we'd like, sadly. https://scan.coverity.com/projects/dpdk-data-plane-development-kit Regards, /Bruce
On Tue, May 08, 2018 at 05:00:21PM +0800, Andy Green wrote:
>
>
> On 05/08/2018 04:58 PM, Bruce Richardson wrote:
> > On Tue, May 08, 2018 at 12:29:53PM +0800, Andy Green wrote:
> > >
> > > ---
> > > drivers/net/nfp/nfpcore/nfp_cpp_pcie_ops.c | 3 ++-
> > > 1 file changed, 2 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/net/nfp/nfpcore/nfp_cpp_pcie_ops.c b/drivers/net/nfp/nfpcore/nfp_cpp_pcie_ops.c
> > > index 4e6c66624..9f6704a7f 100644
> > > --- a/drivers/net/nfp/nfpcore/nfp_cpp_pcie_ops.c
> > > +++ b/drivers/net/nfp/nfpcore/nfp_cpp_pcie_ops.c
> > > @@ -846,7 +846,8 @@ nfp6000_init(struct nfp_cpp *cpp, const char *devname)
> > > memset(desc->busdev, 0, BUSDEV_SZ);
> > > - strncpy(desc->busdev, devname, strlen(devname));
> > > + strncpy(desc->busdev, devname, sizeof(desc->busdev) - 1);
> > > + desc->busdev[sizeof(desc->busdev) - 1] = '\0';
> > > ret = nfp_acquire_process_lock(desc);
> > > if (ret)
> > As with previous patch, a better fix is to use strlcpy. This would apply to
> > just about all uses of strncpy in the code.
>
> OK.
>
> But the strncpy() was already there, it's not introduced by the patch.
>
> I agree just doing it in one hit with strlcpy() is nicer.
>
> -Andy
>
Thanks for the fixes!
On 05/08/2018 11:18 AM, Andy Green wrote: > On 05/08/2018 03:36 PM, Andrew Rybchenko wrote: >> (it looks like "another" is useless in the original subject) > > It captures my feeling at having to wade through making 18 fixes > before I could compile the project on current Fedora. I see. >> In general all patches should pass ./devtools/check-git-log.sh and >> ./devtools/checkpatches.sh >> (which requires path to Linux kernel checkpatches.pl). > > Can you help me understand why adding CRLFs at 80 cols on the gcc > errors I pasted helps anything at all? The patches actually fix > problems in the code. Seeing GCC errors which patch fixes is useful to see. Yes, I agree that it is real problem in the code. > If you don't care about Coverity, let me know and I will register this > project there and send you fixes when I have time. dpdk is registered at Coverity and we get reports from time to time. > >> Andrew. >> >> [1] >> http://dpdk.org/doc/guides/contributing/patches.html#commit-messages-subject-line >> >> On 05/08/2018 07:30 AM, Andy Green wrote: >>> --- >>> drivers/net/sfc/sfc_ethdev.c | 7 +++++-- >>> 1 file changed, 5 insertions(+), 2 deletions(-) >>> >>> diff --git a/drivers/net/sfc/sfc_ethdev.c >>> b/drivers/net/sfc/sfc_ethdev.c >>> index e9bb283e0..bd5f17f33 100644 >>> --- a/drivers/net/sfc/sfc_ethdev.c >>> +++ b/drivers/net/sfc/sfc_ethdev.c >>> @@ -662,10 +662,13 @@ sfc_xstats_get_names(struct rte_eth_dev *dev, >>> for (i = 0; i < EFX_MAC_NSTATS; ++i) { >>> if (EFX_MAC_STAT_SUPPORTED(port->mac_stats_mask, i)) { >>> - if (xstats_names != NULL && nstats < xstats_count) >>> + if (xstats_names != NULL && nstats < xstats_count) { >>> strncpy(xstats_names[nstats].name, >>> efx_mac_stat_name(sa->nic, i), >>> - sizeof(xstats_names[0].name)); >>> + sizeof(xstats_names[0].name) - 1); >>> + xstats_names[0].name[ >>> + sizeof(xstats_names[0].name) - 1] = '\0'; >>> + } >> >> In fact strlcpy() should be used. > Fair enough. Last time I looked it wasn't in glibc but seems it is now. As far as I know it is not in glibc, but dpdk has internal fallback if the function is not available from external libs. Andrew.
> -----Original Message----- > From: dev <dev-bounces@dpdk.org> On Behalf Of Andy Green > Sent: Monday, May 7, 2018 11:30 PM > To: dev@dpdk.org > Subject: [dpdk-dev] [PATCH 09/18] drivers: net: qede: fix strncpy constant and > NUL > > > --- > drivers/net/qede/base/ecore_int.c | 10 ++++++---- > 1 file changed, 6 insertions(+), 4 deletions(-) > > diff --git a/drivers/net/qede/base/ecore_int.c > b/drivers/net/qede/base/ecore_int.c > index f43781ba4..c809d84ef 100644 > --- a/drivers/net/qede/base/ecore_int.c > +++ b/drivers/net/qede/base/ecore_int.c > @@ -1103,10 +1103,12 @@ static enum _ecore_status_t > ecore_int_deassertion(struct ecore_hwfn *p_hwfn, > OSAL_SNPRINTF(bit_name, 30, > p_aeu->bit_name, > num); > - else > - OSAL_STRNCPY(bit_name, > - p_aeu->bit_name, > - 30); > + else { > + strncpy(bit_name, > + p_aeu->bit_name, > + sizeof(bit_name) - 1); > + bit_name[sizeof(bit_name) - 1] > = '\0'; > + } I think you can retain OSAL_STRNCPY and just replace 30 with 'bit_name[sizeof(bit_name) - 1' and then set last byte with '\0' just like you did. Thanks, Shahed > > /* We now need to pass bitmask in its > * correct position.
On Tue, May 08, 2018 at 05:59:47PM +0000, dev-bounces@dpdk.org wrote:
>
>
> > -----Original Message-----
> > From: dev <dev-bounces@dpdk.org> On Behalf Of Andy Green
> > Sent: Monday, May 7, 2018 11:30 PM
> > To: dev@dpdk.org
> > Subject: [dpdk-dev] [PATCH 09/18] drivers: net: qede: fix strncpy constant and
> > NUL
> >
> >
> > ---
> > drivers/net/qede/base/ecore_int.c | 10 ++++++----
> > 1 file changed, 6 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/net/qede/base/ecore_int.c
> > b/drivers/net/qede/base/ecore_int.c
> > index f43781ba4..c809d84ef 100644
> > --- a/drivers/net/qede/base/ecore_int.c
> > +++ b/drivers/net/qede/base/ecore_int.c
> > @@ -1103,10 +1103,12 @@ static enum _ecore_status_t
> > ecore_int_deassertion(struct ecore_hwfn *p_hwfn,
> > OSAL_SNPRINTF(bit_name, 30,
> > p_aeu->bit_name,
> > num);
> > - else
> > - OSAL_STRNCPY(bit_name,
> > - p_aeu->bit_name,
> > - 30);
> > + else {
> > + strncpy(bit_name,
> > + p_aeu->bit_name,
> > + sizeof(bit_name) - 1);
> > + bit_name[sizeof(bit_name) - 1]
> > = '\0';
> > + }
>
> I think you can retain OSAL_STRNCPY and just replace 30 with 'bit_name[sizeof(bit_name) - 1' and then set last byte with '\0' just like you did.
Can that actually be fixed inside OSAL_STRNCPY itself, rather than having
each use needing to explicitly null-terminate?
> -----Original Message-----
> From: dev <dev-bounces@dpdk.org> On Behalf Of Bruce Richardson
> Sent: Tuesday, May 8, 2018 2:53 PM
> To: dev-bounces@dpdk.org
> Cc: Andy Green <andy@warmcat.com>; dev@dpdk.org
> Subject: Re: [dpdk-dev] [PATCH 09/18] drivers: net: qede: fix strncpy constant
> and NUL
>
> On Tue, May 08, 2018 at 05:59:47PM +0000, dev-bounces@dpdk.org wrote:
> >
> >
> > > -----Original Message-----
> > > From: dev <dev-bounces@dpdk.org> On Behalf Of Andy Green
> > > Sent: Monday, May 7, 2018 11:30 PM
> > > To: dev@dpdk.org
> > > Subject: [dpdk-dev] [PATCH 09/18] drivers: net: qede: fix strncpy
> > > constant and NUL
> > >
> > >
> > > ---
> > > drivers/net/qede/base/ecore_int.c | 10 ++++++----
> > > 1 file changed, 6 insertions(+), 4 deletions(-)
> > >
> > > diff --git a/drivers/net/qede/base/ecore_int.c
> > > b/drivers/net/qede/base/ecore_int.c
> > > index f43781ba4..c809d84ef 100644
> > > --- a/drivers/net/qede/base/ecore_int.c
> > > +++ b/drivers/net/qede/base/ecore_int.c
> > > @@ -1103,10 +1103,12 @@ static enum _ecore_status_t
> > > ecore_int_deassertion(struct ecore_hwfn *p_hwfn,
> > > OSAL_SNPRINTF(bit_name, 30,
> > > p_aeu->bit_name,
> > > num);
> > > - else
> > > - OSAL_STRNCPY(bit_name,
> > > - p_aeu->bit_name,
> > > - 30);
> > > + else {
> > > + strncpy(bit_name,
> > > + p_aeu->bit_name,
> > > + sizeof(bit_name) - 1);
> > > + bit_name[sizeof(bit_name) - 1]
> > > = '\0';
> > > + }
> >
> > I think you can retain OSAL_STRNCPY and just replace 30 with
> 'bit_name[sizeof(bit_name) - 1' and then set last byte with '\0' just like you did.
>
> Can that actually be fixed inside OSAL_STRNCPY itself, rather than having each
> use needing to explicitly null-terminate?
Although there is only instance of OSAL_STRNCPY, it makes sense to modify it.
Thanks,
Shahed
On 05/09/2018 04:02 AM, Shaikh, Shahed wrote: >> -----Original Message----- >> From: dev <dev-bounces@dpdk.org> On Behalf Of Bruce Richardson >> Sent: Tuesday, May 8, 2018 2:53 PM >> To: dev-bounces@dpdk.org >> Cc: Andy Green <andy@warmcat.com>; dev@dpdk.org >> Subject: Re: [dpdk-dev] [PATCH 09/18] drivers: net: qede: fix strncpy constant >> and NUL >> >> On Tue, May 08, 2018 at 05:59:47PM +0000, dev-bounces@dpdk.org wrote: >>> >>> >>>> -----Original Message----- >>>> From: dev <dev-bounces@dpdk.org> On Behalf Of Andy Green >>>> Sent: Monday, May 7, 2018 11:30 PM >>>> To: dev@dpdk.org >>>> Subject: [dpdk-dev] [PATCH 09/18] drivers: net: qede: fix strncpy >>>> constant and NUL >>>> >>>> >>>> --- >>>> drivers/net/qede/base/ecore_int.c | 10 ++++++---- >>>> 1 file changed, 6 insertions(+), 4 deletions(-) >>>> >>>> diff --git a/drivers/net/qede/base/ecore_int.c >>>> b/drivers/net/qede/base/ecore_int.c >>>> index f43781ba4..c809d84ef 100644 >>>> --- a/drivers/net/qede/base/ecore_int.c >>>> +++ b/drivers/net/qede/base/ecore_int.c >>>> @@ -1103,10 +1103,12 @@ static enum _ecore_status_t >>>> ecore_int_deassertion(struct ecore_hwfn *p_hwfn, >>>> OSAL_SNPRINTF(bit_name, 30, >>>> p_aeu->bit_name, >>>> num); >>>> - else >>>> - OSAL_STRNCPY(bit_name, >>>> - p_aeu->bit_name, >>>> - 30); >>>> + else { >>>> + strncpy(bit_name, >>>> + p_aeu->bit_name, >>>> + sizeof(bit_name) - 1); >>>> + bit_name[sizeof(bit_name) - 1] >>>> = '\0'; >>>> + } >>> >>> I think you can retain OSAL_STRNCPY and just replace 30 with >> 'bit_name[sizeof(bit_name) - 1' and then set last byte with '\0' just like you did. >> >> Can that actually be fixed inside OSAL_STRNCPY itself, rather than having each >> use needing to explicitly null-terminate? > > Although there is only instance of OSAL_STRNCPY, it makes sense to modify it. Doesn't it make more sense to get rid of OSAL_* that bring nothing at all to the party? #define OSAL_SPRINTF(name, pattern, ...) \ sprintf(name, pattern, ##__VA_ARGS__) #define OSAL_SNPRINTF(buf, size, format, ...) \ snprintf(buf, size, format, ##__VA_ARGS__) #define OSAL_STRLEN(string) strlen(string) #define OSAL_STRCPY(dst, string) strcpy(dst, string) #define OSAL_STRNCPY(dst, string, len) strncpy(dst, string, len) #define OSAL_STRCMP(str1, str2) strcmp(str1, str2) Do I miss the point or these are just cruft? -Andy > Thanks, > Shahed >
> >>> I think you can retain OSAL_STRNCPY and just replace 30 with > >> 'bit_name[sizeof(bit_name) - 1' and then set last byte with '\0' just like you > did. > >> > >> Can that actually be fixed inside OSAL_STRNCPY itself, rather than > >> having each use needing to explicitly null-terminate? > > > > Although there is only instance of OSAL_STRNCPY, it makes sense to modify it. > > Doesn't it make more sense to get rid of OSAL_* that bring nothing at all to the > party? > > #define OSAL_SPRINTF(name, pattern, ...) \ > sprintf(name, pattern, ##__VA_ARGS__) #define OSAL_SNPRINTF(buf, size, > format, ...) \ > snprintf(buf, size, format, ##__VA_ARGS__) #define OSAL_STRLEN(string) > strlen(string) #define OSAL_STRCPY(dst, string) strcpy(dst, string) #define > OSAL_STRNCPY(dst, string, len) strncpy(dst, string, len) #define > OSAL_STRCMP(str1, str2) strcmp(str1, str2) > > Do I miss the point or these are just cruft? Hi Andy, I'll send a cleanup patch for this. For now, you can go ahead with original patch. Thanks, Shahed Acked-by: Shahed Shaikh <shahed.shaikh@cavium.com> > > -Andy > > > Thanks, > > Shahed > >