| Message ID | 20240807225910.3266004-1-bmarzins@redhat.com (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Delegated to: | christophe varoqui |
| Headers | show |
| Series | [v2] libmultipath: fix ontap prioritizer snprintf limits | expand |
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). Regards, Martin
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
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"); + return; + } } - condlog(0, "%s", buf); + condlog(0, "- SCSI CDB: %s", get_strbuf_str(&buf)); } static void process_sg_error(struct sg_io_hdr *io_hdr) { int i; - char buf[128*5+1]; - char * p = &buf[0]; + STRBUF_ON_STACK(buf); condlog(0, "- masked_status=0x%02x, host_status=0x%02x, " "driver_status=0x%02x", io_hdr->masked_status, io_hdr->host_status, io_hdr->driver_status); if (io_hdr->sb_len_wr > 0) { - condlog(0, "- SCSI sense data: "); - for (i=0; i<io_hdr->sb_len_wr; i++) { - p += snprintf(p, 128*(io_hdr->sb_len_wr-i), "0x%02x ", - io_hdr->sbp[i]); + for (i = 0; i < io_hdr->sb_len_wr; i++) { + if (print_strbuf(&buf, "0x%02x ", io_hdr->sbp[i]) < 0) { + condlog(0, "ontap prio: failed to dump SCSI sense data"); + return; + } } - condlog(0, "%s", buf); + condlog(0, "- SCSI sense data: %s", get_strbuf_str(&buf)); } }
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(-)