* [PATCH v2] libmultipath: fix ontap prioritizer snprintf limits
@ 2024-08-07 22:59 Benjamin Marzinski
2024-08-08 8:55 ` Martin Wilck
0 siblings, 1 reply; 3+ messages in thread
From: Benjamin Marzinski @ 2024-08-07 22:59 UTC (permalink / raw)
To: Christophe Varoqui; +Cc: device-mapper development, Martin Wilck
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");
+ 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));
}
}
--
2.45.0
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH v2] libmultipath: fix ontap prioritizer snprintf limits
2024-08-07 22:59 [PATCH v2] libmultipath: fix ontap prioritizer snprintf limits Benjamin Marzinski
@ 2024-08-08 8:55 ` Martin Wilck
2024-08-08 14:23 ` Benjamin Marzinski
0 siblings, 1 reply; 3+ messages in thread
From: Martin Wilck @ 2024-08-08 8:55 UTC (permalink / raw)
To: Benjamin Marzinski, Christophe Varoqui; +Cc: device-mapper development
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
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH v2] libmultipath: fix ontap prioritizer snprintf limits
2024-08-08 8:55 ` Martin Wilck
@ 2024-08-08 14:23 ` Benjamin Marzinski
0 siblings, 0 replies; 3+ messages in thread
From: Benjamin Marzinski @ 2024-08-08 14:23 UTC (permalink / raw)
To: Martin Wilck; +Cc: Christophe Varoqui, device-mapper development
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
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2024-08-08 14:24 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2024-08-07 22:59 [PATCH v2] libmultipath: fix ontap prioritizer snprintf limits Benjamin Marzinski
2024-08-08 8:55 ` Martin Wilck
2024-08-08 14:23 ` Benjamin Marzinski
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.