2014-10-05 21:44:06

by Rickard Strandqvist

Subject: [PATCH] gpu: drm: drm_dp_mst_topology.c: Fix improper use of strncat.

I have now eliminate the need to use the temporary string,
and therefore also the use of strncat.
And I think this code is clearer and more effective.

Signed-off-by: Rickard Strandqvist <[email protected]>
---
drivers/gpu/drm/drm_dp_mst_topology.c | 15 ++++++++-------
1 file changed, 8 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c b/drivers/gpu/drm/drm_dp_mst_topology.c
index ac3c273..c3f6571 100644
--- a/drivers/gpu/drm/drm_dp_mst_topology.c
+++ b/drivers/gpu/drm/drm_dp_mst_topology.c
@@ -997,17 +997,18 @@ static void build_mst_prop_path(struct drm_dp_mst_port *port,
struct drm_dp_mst_branch *mstb,
char *proppath)
{
- int i;
- char temp[8];
- snprintf(proppath, 255, "mst:%d", mstb->mgr->conn_base_id);
+ int i, len;
+ static const int proppath_len = 255;
+ memset(proppath, 0, proppath_len);
+ len = snprintf(proppath, proppath_len, "mst:%d", mstb->mgr->conn_base_id);
for (i = 0; i < (mstb->lct - 1); i++) {
int shift = (i % 2) ? 0 : 4;
int port_num = mstb->rad[i / 2] >> shift;
- snprintf(temp, 8, "-%d", port_num);
- strncat(proppath, temp, 255);
+ len += snprintf(&proppath[(len < proppath_len ? len : 0)],
+ proppath_len - len, "-%d", port_num);
}
- snprintf(temp, 8, "-%d", port->port_num);
- strncat(proppath, temp, 255);
+ snprintf(&proppath[(len < proppath_len ? len : 0)], proppath_len - len,
+ "-%d", port->port_num);
}

static void drm_dp_add_port(struct drm_dp_mst_branch *mstb,
--
1.7.10.4

2014-10-05 22:46:45

by Mateusz Guzik

Subject: Re: [PATCH] gpu: drm: drm_dp_mst_topology.c: Fix improper use of strncat.

On Sun, Oct 05, 2014 at 11:46:03PM +0200, Rickard Strandqvist wrote:
> I have now eliminate the need to use the temporary string,
> and therefore also the use of strncat.
> And I think this code is clearer and more effective.
>
The code is definitely less clear.

> Signed-off-by: Rickard Strandqvist <[email protected]>
> ---
> drivers/gpu/drm/drm_dp_mst_topology.c | 15 ++++++++-------
> 1 file changed, 8 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c b/drivers/gpu/drm/drm_dp_mst_topology.c
> index ac3c273..c3f6571 100644
> --- a/drivers/gpu/drm/drm_dp_mst_topology.c
> +++ b/drivers/gpu/drm/drm_dp_mst_topology.c
> @@ -997,17 +997,18 @@ static void build_mst_prop_path(struct drm_dp_mst_port *port,
> struct drm_dp_mst_branch *mstb,
> char *proppath)
> {
> - int i;
> - char temp[8];
> - snprintf(proppath, 255, "mst:%d", mstb->mgr->conn_base_id);
> + int i, len;
> + static const int proppath_len = 255;

Length should be passed by the caller.

> + memset(proppath, 0, proppath_len);

Why?
> + len = snprintf(proppath, proppath_len, "mst:%d", mstb->mgr->conn_base_id);
> for (i = 0; i < (mstb->lct - 1); i++) {
> int shift = (i % 2) ? 0 : 4;
> int port_num = mstb->rad[i / 2] >> shift;
> - snprintf(temp, 8, "-%d", port_num);
> - strncat(proppath, temp, 255);
> + len += snprintf(&proppath[(len < proppath_len ? len : 0)],
> + proppath_len - len, "-%d", port_num);

And if the space ran out you just overwrite from the start of the buffer
and provide negative length.

Since this function returns void (and so does its caller) it is quite
unclear what to do if the space ran out. I would just WARN.

> }
> - snprintf(temp, 8, "-%d", port->port_num);
> - strncat(proppath, temp, 255);
> + snprintf(&proppath[(len < proppath_len ? len : 0)], proppath_len - len,
> + "-%d", port->port_num);
> }
>
> static void drm_dp_add_port(struct drm_dp_mst_branch *mstb,

--
Mateusz Guzik