[openib-general] [PATCH] diags/saquery: fix node_desc.description as string usages

[Michael S. Tsirkin]

Quoting r. Sasha Khapyorsky <sashak at voltaire.com>:
> Subject: Re: [PATCH] diags/saquery: fix node_desc.description as string usages
> 
> On 13:44 Mon 30 Oct     , Michael S. Tsirkin wrote:
> > Quoting r. Sasha Khapyorsky <sashak at voltaire.com>:
> > > Subject: [PATCH] diags/saquery: fix node_desc.description as string usages
> > > 
> > > 
> > > node_desc.description buffer is received from the network and should
> > > not be NULL-terminated. In such cases using it as regular string in
> > > functions like strcmp() or printf() leads to segmentation faults.
> > > This patch fixes such usages.
> > > 
> > > Signed-off-by: Sasha Khapyorsky <sashak at voltaire.com>
> > > ---
> > >  diags/src/saquery.c |   22 ++++++++++++++++------
> > >  1 files changed, 16 insertions(+), 6 deletions(-)
> > > 
> > > diff --git a/diags/src/saquery.c b/diags/src/saquery.c
> > > index 5b4a85e..f5b23fd 100644
> > > --- a/diags/src/saquery.c
> > > +++ b/diags/src/saquery.c
> > > @@ -90,17 +90,21 @@ static void
> > >  print_node_desc(ib_node_record_t *node_record)
> > >  {
> > >  	ib_node_info_t *p_ni = &(node_record->node_info);
> > > +	ib_node_desc_t *p_nd = &(node_record->node_desc);
> > >  	if (p_ni->node_type == IB_NODE_TYPE_CA)
> > >  	{
> > > +		char desc[sizeof(p_nd->description) + 1];
> > > +		memcpy(desc, p_nd->description, sizeof(p_nd->description));
> > > +		desc[sizeof(desc) - 1] = '\0';
> > >  		printf("%6d  \"%s\"\n",
> > > -		       cl_ntoh16(node_record->lid),
> > > -		       node_record->node_desc.description);
> > > +		       cl_ntoh16(node_record->lid), desc);
> > >  	}
> > >  }
> > 
> > Would it not be simpler, and cleaner, to limit the string width in printf:
> >   		printf("%6d  \"%.*s\"\n",
> > 		       cl_ntoh16(node_record->lid),
> > 			sizeof(desc),
> > 			node_record->node_desc.description);
> 
> This would be simpler. However some web searching shows that not all
> printf() implementation permits not null terminated arrays even when
> precision is specified (some issues were reported even with glibc-2.3.2).

Hmm, couldn't find it. Warrants a comment then?

> OTOH I understand your concerns and hate this stupid copying. Originally
> wanted just to terminate node_desc.description array by '\0', but then
> potentially this array can be truncated.
> 
> Sasha
> 

-- 
MST