Re: [PATCH v2] libmultipath: fix ontap prioritizer snprintf limits

[Benjamin Marzinski <bmarzins@redhat.com>]

On Thu, Aug 08, 2024 at 10:55:19AM +0200, Martin Wilck wrote:
> On Wed, 2024-08-07 at 18:59 -0400, Benjamin Marzinski wrote:
> > The ontap prioritizer functions dump_cdb() and process_sg_error()
> > both
> > incorrectly set the snprintf() limits larger than the available
> > space.
> > Instead of multiplying the number of elements to print by the size of
> > an
> > element to calculate the limit, they multiplied the number of
> > elements
> > to print by the maximum number of elements that the buffer could
> > hold.
> > 
> > Fix this by making these functions use strbufs instead.
> > 
> > Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
> > ---
> > 
> > changes in v2:
> > Use strbufs as suggested by Martin Wilck.
> > 
> >  libmultipath/prioritizers/ontap.c | 28 +++++++++++++++-------------
> >  1 file changed, 15 insertions(+), 13 deletions(-)
> > 
> > diff --git a/libmultipath/prioritizers/ontap.c
> > b/libmultipath/prioritizers/ontap.c
> > index 117886ea..b508371d 100644
> > --- a/libmultipath/prioritizers/ontap.c
> > +++ b/libmultipath/prioritizers/ontap.c
> > @@ -23,6 +23,7 @@
> >  #include "prio.h"
> >  #include "structs.h"
> >  #include "unaligned.h"
> > +#include "strbuf.h"
> >  
> >  #define INQUIRY_CMD	0x12
> >  #define INQUIRY_CMDLEN	6
> > @@ -35,32 +36,33 @@
> >  static void dump_cdb(unsigned char *cdb, int size)
> >  {
> >  	int i;
> > -	char buf[10*5+1];
> > -	char * p = &buf[0];
> > +	STRBUF_ON_STACK(buf);
> >  
> > -	condlog(0, "- SCSI CDB: ");
> > -	for (i=0; i<size; i++) {
> > -		p += snprintf(p, 10*(size-i), "0x%02x ", cdb[i]);
> > +	for (i = 0; i < size; i++) {
> > +		if (print_strbuf(&buf, "0x%02x ", cdb[i]) < 0) {
> > +			condlog(0, "ontap prio: failed to dump SCSI
> > CDB");
> 
> If print_strbuf() fails (very probably because it failed to allocate
> memory), condlog() is unlikely to succeed, either. I thought we agreed
> that trying to spit out error messages in this situation makes little
> sense.
> 
> Anyway, I guess the condlog call doesn't actually hurt, so:
> 
> Reviewed-by: Martin Wilck <mwilck@suse.com>
> 
> (If you consent, I'll edit the commit and remove the condlog call).

Feel free to remove them.

-Ben

> 
> Regards,
> Martin