* [PATCH 1/4] kpartx: Improve portability of set_loop()
2017-06-13 16:33 [PATCH 0/4] Four multipath-tools patches Bart Van Assche
@ 2017-06-13 16:33 ` Bart Van Assche
2017-06-13 19:33 ` Martin Wilck
2017-06-13 16:33 ` [PATCH 2/4] libmultipath: Simplify assemble_map() Bart Van Assche
` (3 subsequent siblings)
4 siblings, 1 reply; 14+ messages in thread
From: Bart Van Assche @ 2017-06-13 16:33 UTC (permalink / raw)
To: Christophe Varoqui; +Cc: Bart Van Assche, dm-devel, Xose Vazquez Perez
Because macros like __x86_64__ are not defined by the C standard
and because it is easy to convert the int2ptr() macro into
standard C, use (void*)(uintptr_t) instead of int2ptr(). Inline
this macro because it only has one user. This patch does not
change any functionality.
Reported-by: Xose Vazquez Perez <xose.vazquez@gmail.com>
Signed-off-by: Bart Van Assche <bart.vanassche@sandisk.com>
Cc: Xose Vazquez Perez <xose.vazquez@gmail.com>
---
kpartx/lopart.c | 10 ++--------
1 file changed, 2 insertions(+), 8 deletions(-)
diff --git a/kpartx/lopart.c b/kpartx/lopart.c
index 70054459..f7ab91b5 100644
--- a/kpartx/lopart.c
+++ b/kpartx/lopart.c
@@ -21,6 +21,7 @@
#include <fcntl.h>
#include <errno.h>
#include <stdlib.h>
+#include <stdint.h>
#include <unistd.h>
#include <sys/ioctl.h>
#include <sys/stat.h>
@@ -37,13 +38,6 @@
#define LOOP_CTL_GET_FREE 0x4C82
#endif
-#if !defined (__alpha__) && !defined (__ia64__) && !defined (__x86_64__) \
- && !defined (__s390x__)
-#define int2ptr(x) ((void *) ((int) x))
-#else
-#define int2ptr(x) ((void *) ((long) x))
-#endif
-
static char *
xstrdup (const char *s)
{
@@ -249,7 +243,7 @@ int set_loop(const char *device, const char *file, int offset, int *loopro)
loopinfo.lo_encrypt_type = LO_CRYPT_NONE;
loopinfo.lo_encrypt_key_size = 0;
- if (ioctl (fd, LOOP_SET_FD, int2ptr(ffd)) < 0) {
+ if (ioctl(fd, LOOP_SET_FD, (void*)(uintptr_t)(ffd)) < 0) {
perror ("ioctl: LOOP_SET_FD");
close (fd);
close (ffd);
--
2.12.2
^ permalink raw reply related [flat|nested] 14+ messages in thread* Re: [PATCH 1/4] kpartx: Improve portability of set_loop()
2017-06-13 16:33 ` [PATCH 1/4] kpartx: Improve portability of set_loop() Bart Van Assche
@ 2017-06-13 19:33 ` Martin Wilck
0 siblings, 0 replies; 14+ messages in thread
From: Martin Wilck @ 2017-06-13 19:33 UTC (permalink / raw)
To: dm-devel
On Tue, 2017-06-13 at 09:33 -0700, Bart Van Assche wrote:
> Because macros like __x86_64__ are not defined by the C standard
> and because it is easy to convert the int2ptr() macro into
> standard C, use (void*)(uintptr_t) instead of int2ptr(). Inline
> this macro because it only has one user. This patch does not
> change any functionality.
>
> Reported-by: Xose Vazquez Perez <xose.vazquez@gmail.com>
> Signed-off-by: Bart Van Assche <bart.vanassche@sandisk.com>
> Cc: Xose Vazquez Perez <xose.vazquez@gmail.com>
> ---
> kpartx/lopart.c | 10 ++--------
> 1 file changed, 2 insertions(+), 8 deletions(-)
>
Reviewed-by: Martin Wilck <mwilck@suse.com>
--
Dr. Martin Wilck <mwilck@suse.com>, Tel. +49 (0)911 74053 2107
SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton
HRB 21284 (AG Nürnberg)
--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel
^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH 2/4] libmultipath: Simplify assemble_map()
2017-06-13 16:33 [PATCH 0/4] Four multipath-tools patches Bart Van Assche
2017-06-13 16:33 ` [PATCH 1/4] kpartx: Improve portability of set_loop() Bart Van Assche
@ 2017-06-13 16:33 ` Bart Van Assche
2017-06-13 19:32 ` Martin Wilck
2017-06-13 16:33 ` [PATCH 3/4] libmultipath/datacore: Remove dead code Bart Van Assche
` (2 subsequent siblings)
4 siblings, 1 reply; 14+ messages in thread
From: Bart Van Assche @ 2017-06-13 16:33 UTC (permalink / raw)
To: Christophe Varoqui; +Cc: Bart Van Assche, dm-devel
Introduce a macro for appending formatted text to the output buffer.
Eliminate the local variables 'shift' and 'freechar'. Move the
code for freeing a temporary buffer to the end of the function.
Handle snprintf() conversion errors. This patch avoids that gcc 7
reports the following warning:
dmparser.c:137:20: warning: '__builtin_snprintf' output truncated before the last format character [-Wformat-truncation=]
snprintf(p, 1, "\n");
^
Signed-off-by: Bart Van Assche <bart.vanassche@sandisk.com>
---
libmultipath/dmparser.c | 67 +++++++++++++++++++++++--------------------------
1 file changed, 31 insertions(+), 36 deletions(-)
diff --git a/libmultipath/dmparser.c b/libmultipath/dmparser.c
index 469e60d2..ba09dc72 100644
--- a/libmultipath/dmparser.c
+++ b/libmultipath/dmparser.c
@@ -45,6 +45,22 @@ merge_words (char ** dst, char * word, int space)
return 0;
}
+#define APPEND(p, end, args...) \
+({ \
+ int ret; \
+ \
+ ret = snprintf(p, end - p, ##args); \
+ if (ret < 0) { \
+ condlog(0, "%s: conversion error", mp->alias); \
+ goto err; \
+ } \
+ p += ret; \
+ if (p >= end) { \
+ condlog(0, "%s: params too small", mp->alias); \
+ goto err; \
+ } \
+})
+
/*
* Transforms the path group vector into a proper device map string
*/
@@ -52,10 +68,10 @@ int
assemble_map (struct multipath * mp, char * params, int len)
{
int i, j;
- int shift, freechar;
int minio;
int nr_priority_groups, initial_pg_nr;
char * p, * f;
+ const char *const end = params + len;
char no_path_retry[] = "queue_if_no_path";
char retain_hwhandler[] = "retain_attached_hw_handler";
struct pathgroup * pgp;
@@ -63,7 +79,6 @@ assemble_map (struct multipath * mp, char * params, int len)
minio = mp->minio;
p = params;
- freechar = len;
nr_priority_groups = VECTOR_SIZE(mp->pg);
initial_pg_nr = (nr_priority_groups ? mp->bestpg : 0);
@@ -86,29 +101,13 @@ assemble_map (struct multipath * mp, char * params, int len)
if (mp->retain_hwhandler == RETAIN_HWHANDLER_ON)
add_feature(&f, retain_hwhandler);
- shift = snprintf(p, freechar, "%s %s %i %i",
- f, mp->hwhandler,
- nr_priority_groups, initial_pg_nr);
-
- FREE(f);
-
- if (shift >= freechar) {
- condlog(0, "%s: params too small", mp->alias);
- return 1;
- }
- p += shift;
- freechar -= shift;
+ APPEND(p, end, "%s %s %i %i", f, mp->hwhandler, nr_priority_groups,
+ initial_pg_nr);
vector_foreach_slot (mp->pg, pgp, i) {
pgp = VECTOR_SLOT(mp->pg, i);
- shift = snprintf(p, freechar, " %s %i 1", mp->selector,
- VECTOR_SIZE(pgp->paths));
- if (shift >= freechar) {
- condlog(0, "%s: params too small", mp->alias);
- return 1;
- }
- p += shift;
- freechar -= shift;
+ APPEND(p, end, " %s %i 1", mp->selector,
+ VECTOR_SIZE(pgp->paths));
vector_foreach_slot (pgp->paths, pp, j) {
int tmp_minio = minio;
@@ -118,28 +117,24 @@ assemble_map (struct multipath * mp, char * params, int len)
tmp_minio = minio * pp->priority;
if (!strlen(pp->dev_t) ) {
condlog(0, "dev_t not set for '%s'", pp->dev);
- return 1;
+ goto err;
}
- shift = snprintf(p, freechar, " %s %d",
- pp->dev_t, tmp_minio);
- if (shift >= freechar) {
- condlog(0, "%s: params too small", mp->alias);
- return 1;
- }
- p += shift;
- freechar -= shift;
+ APPEND(p, end, " %s %d", pp->dev_t, tmp_minio);
}
}
- if (freechar < 1) {
- condlog(0, "%s: params too small", mp->alias);
- return 1;
- }
- snprintf(p, 1, "\n");
+ APPEND(p, end, "\n");
+ FREE(f);
condlog(3, "%s: assembled map [%s]", mp->alias, params);
return 0;
+
+err:
+ FREE(f);
+ return 1;
}
+#undef APPEND
+
int disassemble_map(vector pathvec, char *params, struct multipath *mpp,
int is_daemon)
{
--
2.12.2
^ permalink raw reply related [flat|nested] 14+ messages in thread* Re: [PATCH 2/4] libmultipath: Simplify assemble_map()
2017-06-13 16:33 ` [PATCH 2/4] libmultipath: Simplify assemble_map() Bart Van Assche
@ 2017-06-13 19:32 ` Martin Wilck
0 siblings, 0 replies; 14+ messages in thread
From: Martin Wilck @ 2017-06-13 19:32 UTC (permalink / raw)
To: Bart Van Assche, Christophe Varoqui; +Cc: dm-devel
On Tue, 2017-06-13 at 09:33 -0700, Bart Van Assche wrote:
> Introduce a macro for appending formatted text to the output buffer.
> Eliminate the local variables 'shift' and 'freechar'. Move the
> code for freeing a temporary buffer to the end of the function.
> Handle snprintf() conversion errors. This patch avoids that gcc 7
> reports the following warning:
>
> dmparser.c:137:20: warning: '__builtin_snprintf' output truncated
> before the last format character [-Wformat-truncation=]
> snprintf(p, 1, "\n");
> ^
>
> Signed-off-by: Bart Van Assche <bart.vanassche@sandisk.com>
> ---
> libmultipath/dmparser.c | 67 +++++++++++++++++++++++--------------
> ------------
> 1 file changed, 31 insertions(+), 36 deletions(-)
>
Reviewed-by: Martin Wilck <mwilck@suse.com>
--
Dr. Martin Wilck <mwilck@suse.com>, Tel. +49 (0)911 74053 2107
SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton
HRB 21284 (AG Nürnberg)
--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel
^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH 3/4] libmultipath/datacore: Remove dead code
2017-06-13 16:33 [PATCH 0/4] Four multipath-tools patches Bart Van Assche
2017-06-13 16:33 ` [PATCH 1/4] kpartx: Improve portability of set_loop() Bart Van Assche
2017-06-13 16:33 ` [PATCH 2/4] libmultipath: Simplify assemble_map() Bart Van Assche
@ 2017-06-13 16:33 ` Bart Van Assche
2017-06-13 19:31 ` Martin Wilck
2017-06-13 16:33 ` [PATCH 4/4] multipath: Fix a potential buffer overflow Bart Van Assche
2017-06-21 10:16 ` [PATCH 0/4] Four multipath-tools patches Christophe Varoqui
4 siblings, 1 reply; 14+ messages in thread
From: Bart Van Assche @ 2017-06-13 16:33 UTC (permalink / raw)
To: Christophe Varoqui; +Cc: Bart Van Assche, dm-devel
Remove those variables a value is assigned to but that are never
used. This patch avoids that gcc 7 reports the following warning:
datacore.c:98:22: warning: '
' directive output may be truncated writing 1 byte into a region of size between 0 and 8 [-Wformat-truncation=]
snprintf(vendor, 8, "%.8s\n", inqBuffp + 8);
Signed-off-by: Bart Van Assche <bart.vanassche@sandisk.com>
---
libmultipath/prioritizers/datacore.c | 10 +---------
1 file changed, 1 insertion(+), 9 deletions(-)
diff --git a/libmultipath/prioritizers/datacore.c b/libmultipath/prioritizers/datacore.c
index 36465ac4..59c98164 100644
--- a/libmultipath/prioritizers/datacore.c
+++ b/libmultipath/prioritizers/datacore.c
@@ -35,10 +35,6 @@
int datacore_prio (const char *dev, int sg_fd, char * args)
{
int k;
- char vendor[8];
- char product[32];
- char luname[32];
- char wwpn[32];
char sdsname[32];
unsigned char inqCmdBlk[INQ_CMD_LEN] = { INQ_CMD_CODE, 0, 0, 0, INQ_REPLY_LEN, 0 };
unsigned char inqBuff[INQ_REPLY_LEN];
@@ -95,11 +91,7 @@ int datacore_prio (const char *dev, int sg_fd, char * args)
if ((io_hdr.info & SG_INFO_OK_MASK) != SG_INFO_OK)
return 0;
- snprintf(vendor, 8, "%.8s\n", inqBuffp + 8);
- snprintf(product, 17, "%.16s", inqBuffp + 16);
- snprintf(luname, 21, "%.19s", inqBuffp + 36);
- snprintf(wwpn, 17, "%.16s", inqBuffp + 96);
- snprintf(sdsname, 17, "%.16s", inqBuffp + 112);
+ snprintf(sdsname, sizeof(sdsname), "%.16s", inqBuffp + 112);
if (strstr(sdsname , preferredsds))
return 1;
--
2.12.2
^ permalink raw reply related [flat|nested] 14+ messages in thread* Re: [PATCH 3/4] libmultipath/datacore: Remove dead code
2017-06-13 16:33 ` [PATCH 3/4] libmultipath/datacore: Remove dead code Bart Van Assche
@ 2017-06-13 19:31 ` Martin Wilck
0 siblings, 0 replies; 14+ messages in thread
From: Martin Wilck @ 2017-06-13 19:31 UTC (permalink / raw)
To: Bart Van Assche, Christophe Varoqui; +Cc: dm-devel
On Tue, 2017-06-13 at 09:33 -0700, Bart Van Assche wrote:
> Remove those variables a value is assigned to but that are never
> used. This patch avoids that gcc 7 reports the following warning:
>
> datacore.c:98:22: warning: '
> ' directive output may be truncated writing 1 byte into a region
> of size between 0 and 8 [-Wformat-truncation=]
> snprintf(vendor, 8, "%.8s\n", inqBuffp + 8);
>
> Signed-off-by: Bart Van Assche <bart.vanassche@sandisk.com>
> ---
> libmultipath/prioritizers/datacore.c | 10 +---------
> 1 file changed, 1 insertion(+), 9 deletions(-)
>
> diff --git a/libmultipath/prioritizers/datacore.c
> b/libmultipath/prioritizers/datacore.c
> index 36465ac4..59c98164 100644
> --- a/libmultipath/prioritizers/datacore.c
> +++ b/libmultipath/prioritizers/datacore.c
> @@ -35,10 +35,6 @@
> int datacore_prio (const char *dev, int sg_fd, char * args)
> {
> int k;
> - char vendor[8];
> - char product[32];
> - char luname[32];
> - char wwpn[32];
> char sdsname[32];
> unsigned char inqCmdBlk[INQ_CMD_LEN] = { INQ_CMD_CODE, 0, 0,
> 0, INQ_REPLY_LEN, 0 };
> unsigned char inqBuff[INQ_REPLY_LEN];
> @@ -95,11 +91,7 @@ int datacore_prio (const char *dev, int sg_fd,
> char * args)
> if ((io_hdr.info & SG_INFO_OK_MASK) != SG_INFO_OK)
> return 0;
>
> - snprintf(vendor, 8, "%.8s\n", inqBuffp + 8);
> - snprintf(product, 17, "%.16s", inqBuffp + 16);
> - snprintf(luname, 21, "%.19s", inqBuffp + 36);
> - snprintf(wwpn, 17, "%.16s", inqBuffp + 96);
> - snprintf(sdsname, 17, "%.16s", inqBuffp + 112);
> + snprintf(sdsname, sizeof(sdsname), "%.16s", inqBuffp + 112);
>
> if (strstr(sdsname , preferredsds))
> return 1;
I was wondering whether passing sizeof(sdsname) rather than 17 might
make a difference, and verified that that isn't the case.
Reviewed-by: Martin Wilck <mwilck@suse.com>
--
Dr. Martin Wilck <mwilck@suse.com>, Tel. +49 (0)911 74053 2107
SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton
HRB 21284 (AG Nürnberg)
--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel
^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH 4/4] multipath: Fix a potential buffer overflow
2017-06-13 16:33 [PATCH 0/4] Four multipath-tools patches Bart Van Assche
` (2 preceding siblings ...)
2017-06-13 16:33 ` [PATCH 3/4] libmultipath/datacore: Remove dead code Bart Van Assche
@ 2017-06-13 16:33 ` Bart Van Assche
2017-06-13 19:28 ` Martin Wilck
2017-06-21 10:16 ` [PATCH 0/4] Four multipath-tools patches Christophe Varoqui
4 siblings, 1 reply; 14+ messages in thread
From: Bart Van Assche @ 2017-06-13 16:33 UTC (permalink / raw)
To: Christophe Varoqui; +Cc: Bart Van Assche, dm-devel
Avoid that gcc 7 reports the following warning:
cli_handlers.c:1340:18: warning: '%d' directive writing between 1 and 3 bytes into a region of size 2 [-Wformat-overflow=]
sprintf(*reply,"%d",mpp->prflag);
Signed-off-by: Bart Van Assche <bart.vanassche@sandisk.com>
---
multipathd/cli_handlers.c | 14 ++++++--------
1 file changed, 6 insertions(+), 8 deletions(-)
diff --git a/multipathd/cli_handlers.c b/multipathd/cli_handlers.c
index 04c73866..460fea1f 100644
--- a/multipathd/cli_handlers.c
+++ b/multipathd/cli_handlers.c
@@ -1,6 +1,9 @@
/*
* Copyright (c) 2005 Christophe Varoqui
*/
+
+#define _GNU_SOURCE
+
#include "checkers.h"
#include "memory.h"
#include "vector.h"
@@ -1332,14 +1335,9 @@ cli_getprstatus (void * v, char ** reply, int * len, void * data)
condlog(3, "%s: prflag = %u", param, (unsigned int)mpp->prflag);
- *reply =(char *)malloc(2);
- *len = 2;
- memset(*reply,0,2);
-
-
- sprintf(*reply,"%d",mpp->prflag);
- (*reply)[1]='\0';
-
+ *len = asprintf(reply, "%d", mpp->prflag);
+ if (*len < 0)
+ return 1;
condlog(3, "%s: reply = %s", param, *reply);
--
2.12.2
^ permalink raw reply related [flat|nested] 14+ messages in thread* Re: [PATCH 4/4] multipath: Fix a potential buffer overflow
2017-06-13 16:33 ` [PATCH 4/4] multipath: Fix a potential buffer overflow Bart Van Assche
@ 2017-06-13 19:28 ` Martin Wilck
2017-06-13 19:53 ` Bart Van Assche
0 siblings, 1 reply; 14+ messages in thread
From: Martin Wilck @ 2017-06-13 19:28 UTC (permalink / raw)
To: Bart Van Assche, Christophe Varoqui; +Cc: dm-devel
Hi Bart,
On Tue, 2017-06-13 at 09:33 -0700, Bart Van Assche wrote:
> Avoid that gcc 7 reports the following warning:
>
> cli_handlers.c:1340:18: warning: '%d' directive writing between 1 and
> 3 bytes into a region of size 2 [-Wformat-overflow=]
> sprintf(*reply,"%d",mpp->prflag);
>
> Signed-off-by: Bart Van Assche <bart.vanassche@sandisk.com>
> ---
> multipathd/cli_handlers.c | 14 ++++++--------
> 1 file changed, 6 insertions(+), 8 deletions(-)
>
> diff --git a/multipathd/cli_handlers.c b/multipathd/cli_handlers.c
> index 04c73866..460fea1f 100644
> --- a/multipathd/cli_handlers.c
> +++ b/multipathd/cli_handlers.c
> @@ -1,6 +1,9 @@
> /*
> * Copyright (c) 2005 Christophe Varoqui
> */
> +
> +#define _GNU_SOURCE
> +
> #include "checkers.h"
> #include "memory.h"
> #include "vector.h"
> @@ -1332,14 +1335,9 @@ cli_getprstatus (void * v, char ** reply, int
> * len, void * data)
>
> condlog(3, "%s: prflag = %u", param, (unsigned int)mpp-
> >prflag);
>
> - *reply =(char *)malloc(2);
> - *len = 2;
> - memset(*reply,0,2);
> -
> -
> - sprintf(*reply,"%d",mpp->prflag);
> - (*reply)[1]='\0';
> -
> + *len = asprintf(reply, "%d", mpp->prflag);
> + if (*len < 0)
> + return 1;
>
> condlog(3, "%s: reply = %s", param, *reply);
>
how about this simpler patch, as prflag is actually a boolean?
diff --git a/multipathd/cli_handlers.c b/multipathd/cli_handlers.c
index 04c73866..c31ebd34 100644
--- a/multipathd/cli_handlers.c
+++ b/multipathd/cli_handlers.c
@@ -1337,7 +1337,7 @@ cli_getprstatus (void * v, char ** reply, int * len, void * data)
memset(*reply,0,2);
- sprintf(*reply,"%d",mpp->prflag);
+ sprintf(*reply, "%d", !!mpp->prflag);
(*reply)[1]='\0';
--
Dr. Martin Wilck <mwilck@suse.com>, Tel. +49 (0)911 74053 2107
SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton
HRB 21284 (AG Nürnberg)
--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel
^ permalink raw reply related [flat|nested] 14+ messages in thread* Re: [PATCH 4/4] multipath: Fix a potential buffer overflow
2017-06-13 19:28 ` Martin Wilck
@ 2017-06-13 19:53 ` Bart Van Assche
2017-06-13 20:18 ` Martin Wilck
0 siblings, 1 reply; 14+ messages in thread
From: Bart Van Assche @ 2017-06-13 19:53 UTC (permalink / raw)
To: Martin Wilck, Christophe Varoqui; +Cc: dm-devel@redhat.com
On 06/13/17 12:29, Martin Wilck wrote:
> how about this simpler patch, as prflag is actually a boolean?
>
> diff --git a/multipathd/cli_handlers.c b/multipathd/cli_handlers.c
> index 04c73866..c31ebd34 100644
> --- a/multipathd/cli_handlers.c
> +++ b/multipathd/cli_handlers.c
> @@ -1337,7 +1337,7 @@ cli_getprstatus (void * v, char ** reply, int * len, void * data)
> memset(*reply,0,2);
>
>
> - sprintf(*reply,"%d",mpp->prflag);
> + sprintf(*reply, "%d", !!mpp->prflag);
> (*reply)[1]='\0';
Hello Martin,
Every sprintf() call requires careful analysis to see whether or not it
triggers a buffer overflow. I really would like to get rid of that
sprintf() call.
Thanks,
Bart.
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 4/4] multipath: Fix a potential buffer overflow
2017-06-13 19:53 ` Bart Van Assche
@ 2017-06-13 20:18 ` Martin Wilck
2017-06-13 20:21 ` Bart Van Assche
0 siblings, 1 reply; 14+ messages in thread
From: Martin Wilck @ 2017-06-13 20:18 UTC (permalink / raw)
To: Bart Van Assche, Christophe Varoqui; +Cc: dm-devel@redhat.com
On Tue, 2017-06-13 at 12:53 -0700, Bart Van Assche wrote:
> On 06/13/17 12:29, Martin Wilck wrote:
> > how about this simpler patch, as prflag is actually a boolean?
> >
> > diff --git a/multipathd/cli_handlers.c b/multipathd/cli_handlers.c
> > index 04c73866..c31ebd34 100644
> > --- a/multipathd/cli_handlers.c
> > +++ b/multipathd/cli_handlers.c
> > @@ -1337,7 +1337,7 @@ cli_getprstatus (void * v, char ** reply, int
> > * len, void * data)
> > memset(*reply,0,2);
> >
> >
> > - sprintf(*reply,"%d",mpp->prflag);
> > + sprintf(*reply, "%d", !!mpp->prflag);
> > (*reply)[1]='\0';
>
> Hello Martin,
>
> Every sprintf() call requires careful analysis to see whether or not
> it
> triggers a buffer overflow. I really would like to get rid of that
> sprintf() call.
Then we could write
snprintf(*reply, 2, "%d", !!mpp->prflag);
without needing _GNU_SOURCE.
Martin
--
Dr. Martin Wilck <mwilck@suse.com>, Tel. +49 (0)911 74053 2107
SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton
HRB 21284 (AG Nürnberg)
--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 4/4] multipath: Fix a potential buffer overflow
2017-06-13 20:18 ` Martin Wilck
@ 2017-06-13 20:21 ` Bart Van Assche
2017-06-13 20:35 ` Martin Wilck
0 siblings, 1 reply; 14+ messages in thread
From: Bart Van Assche @ 2017-06-13 20:21 UTC (permalink / raw)
To: Martin Wilck, Christophe Varoqui; +Cc: dm-devel@redhat.com
On 06/13/17 13:18, Martin Wilck wrote:
> On Tue, 2017-06-13 at 12:53 -0700, Bart Van Assche wrote:
>> On 06/13/17 12:29, Martin Wilck wrote:
>>> how about this simpler patch, as prflag is actually a boolean?
>>>
>>> diff --git a/multipathd/cli_handlers.c b/multipathd/cli_handlers.c
>>> index 04c73866..c31ebd34 100644
>>> --- a/multipathd/cli_handlers.c
>>> +++ b/multipathd/cli_handlers.c
>>> @@ -1337,7 +1337,7 @@ cli_getprstatus (void * v, char ** reply, int
>>> * len, void * data)
>>> memset(*reply,0,2);
>>>
>>>
>>> - sprintf(*reply,"%d",mpp->prflag);
>>> + sprintf(*reply, "%d", !!mpp->prflag);
>>> (*reply)[1]='\0';
>>
>> Hello Martin,
>>
>> Every sprintf() call requires careful analysis to see whether or not
>> it
>> triggers a buffer overflow. I really would like to get rid of that
>> sprintf() call.
>
> Then we could write
>
> snprintf(*reply, 2, "%d", !!mpp->prflag);
>
> without needing _GNU_SOURCE.
Hello Martin,
There are already three other multipath-tools source files that #define
_GNU_SOURCE so I don't see what's wrong with using _GNU_SOURCE.
Bart.
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 4/4] multipath: Fix a potential buffer overflow
2017-06-13 20:21 ` Bart Van Assche
@ 2017-06-13 20:35 ` Martin Wilck
0 siblings, 0 replies; 14+ messages in thread
From: Martin Wilck @ 2017-06-13 20:35 UTC (permalink / raw)
To: Bart Van Assche, Christophe Varoqui; +Cc: dm-devel@redhat.com
Hello Bart,
On Tue, 2017-06-13 at 13:21 -0700, Bart Van Assche wrote:
>
> > > Hello Martin,
> > >
> > > Every sprintf() call requires careful analysis to see whether or
> > > not
> > > it
> > > triggers a buffer overflow. I really would like to get rid of
> > > that
> > > sprintf() call.
> >
> > Then we could write
> >
> > snprintf(*reply, 2, "%d", !!mpp->prflag);
> >
> > without needing _GNU_SOURCE.
>
> Hello Martin,
>
> There are already three other multipath-tools source files that
> #define
> _GNU_SOURCE so I don't see what's wrong with using _GNU_SOURCE.
Yes, I saw that. I haven't reviewed the reason why _GNU_SOURCE is used
in the other places. In general it's a thing I'd rather avoid for
portability reasons.
In this particular case, I think the problem at hand be easily solved
without resorting to _GNU_SOURCE.
But well, it's not a thing worth fighting about. May Christophe decide.
Martin
--
Dr. Martin Wilck <mwilck@suse.com>, Tel. +49 (0)911 74053 2107
SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton
HRB 21284 (AG Nürnberg)
--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 0/4] Four multipath-tools patches
2017-06-13 16:33 [PATCH 0/4] Four multipath-tools patches Bart Van Assche
` (3 preceding siblings ...)
2017-06-13 16:33 ` [PATCH 4/4] multipath: Fix a potential buffer overflow Bart Van Assche
@ 2017-06-21 10:16 ` Christophe Varoqui
4 siblings, 0 replies; 14+ messages in thread
From: Christophe Varoqui @ 2017-06-21 10:16 UTC (permalink / raw)
To: Bart Van Assche; +Cc: device-mapper development
[-- Attachment #1.1: Type: text/plain, Size: 950 bytes --]
The set is merged.
Thanks.
The _GNU_SOURCE define is no issue.
On Tue, Jun 13, 2017 at 6:33 PM, Bart Van Assche <bart.vanassche@sandisk.com
> wrote:
> Hello Christophe,
>
> This patch series consists of four small patches: one patch that improves
> portability of the kpartx source code and three patches that address
> compiler
> warnings reported by gcc 7. Please consider these patches for merging.
>
> Thanks,
>
> Bart.
>
> Bart Van Assche (4):
> kpartx: Improve portability of set_loop()
> libmultipath: Simplify assemble_map()
> libmultipath/datacore: Remove dead code
> multipath: Fix a potential buffer overflow
>
> kpartx/lopart.c | 10 ++----
> libmultipath/dmparser.c | 67 +++++++++++++++++-------------
> ------
> libmultipath/prioritizers/datacore.c | 10 +-----
> multipathd/cli_handlers.c | 14 ++++----
> 4 files changed, 40 insertions(+), 61 deletions(-)
>
> --
> 2.12.2
>
>
[-- Attachment #1.2: Type: text/html, Size: 1459 bytes --]
[-- Attachment #2: Type: text/plain, Size: 0 bytes --]
^ permalink raw reply [flat|nested] 14+ messages in thread