Re: [devel] [PATCH 0/9] Review Request for mds: Add solution for TIPC buffer overflow [#1960]
Thanks Minh. I have no more comments.
Regards, Vu
On 9/23/19 7:48 AM, Minh Hon Chau wrote:
Hi all,
Below is the patch #10 that updates most of comments, it applies on
top of current patch #9.
This patch #10 does not use the shared_ptr and base:Mutex as comments
given by Gary and Vu, the reason is that it will cause a similar
problem reported in #2860 (user call exit() without properly doing mds
shutdown), unless those variables are allocated on the heap.
I would like to push the #1960 patches today if we don't have any more
comments. There are some other incremental improvements/fixes that
will be addressed in other tickets.
Thanks
Minh
---
src/mds/README | 2 +-
src/mds/mds_dt_tipc.c | 28 -
src/mds/mds_tipc_fctrl_intf.cc | 67
++--
src/mds/mds_tipc_fctrl_intf.h | 2 +-
src/mds/mds_tipc_fctrl_msg.cc | 44 +-
src/mds/mds_tipc_fctrl_msg.h | 22 +++--
src/mds/mds_tipc_fctrl_portid.cc | 46 ---
7 files changed, 137 insertions(+), 74 deletions(-)
diff --git a/src/mds/README b/src/mds/README
index 1b94632..0819bdc 100644
--- a/src/mds/README
+++ b/src/mds/README
@@ -182,7 +182,7 @@ TIPC portid state machine and its transition
kDisabled, // no flow control support at this state
kStartup, // a newly published portid starts at this state
-kTxProb, // txprob timer is running to confirm if the flow control
is supported
+kTxProb, // tx probation timer is running to confirm if the flow
control is supported
kEnabled // flow control support is confirmed, data flow is controlled
kRcvBuffOverflow // anticipating (or experienced) the receiver's
buffer overflow
diff --git a/src/mds/mds_dt_tipc.c b/src/mds/mds_dt_tipc.c
index 1b6c3f8..e7a7b48 100644
--- a/src/mds/mds_dt_tipc.c
+++ b/src/mds/mds_dt_tipc.c
@@ -247,6 +247,7 @@ uint32_t mdtm_tipc_init(NODE_ID nodeid, uint32_t
*mds_tipc_ref)
if (!get_tipc_port_id(tipc_cb.BSRsock, _id)) {
close(tipc_cb.Dsock);
close(tipc_cb.BSRsock);
+ *mds_tipc_ref = 0;
return NCSCC_RC_FAILURE;
}
*mds_tipc_ref = port_id.ref;
@@ -330,7 +331,7 @@ uint32_t mdtm_tipc_init(NODE_ID nodeid, uint32_t
*mds_tipc_ref)
}
/* Get tipc socket receive buffer size */
- int optval;
+ int optval = 0;
socklen_t optlen = sizeof(optval);
if (getsockopt(tipc_cb.BSRsock, SOL_SOCKET, SO_RCVBUF,
, ) != 0) {
@@ -350,12 +351,25 @@ uint32_t mdtm_tipc_init(NODE_ID nodeid, uint32_t
*mds_tipc_ref)
int acksize = -1;
if ((ptr = getenv("MDS_TIPC_FCTRL_ACKTIMEOUT")) != NULL) {
ackto = atoi(ptr);
+ if (ackto == 0) {
+ syslog(LOG_ERR, "MDTM:TIPC Invalid "
+ "MDS_TIPC_FCTRL_ACKTIMEOUT, using default
value");
+ ackto = -1;
+ }
}
if ((ptr = getenv("MDS_TIPC_FCTRL_ACKSIZE")) != NULL) {
acksize = atoi(ptr);
+ if (acksize == 0) {
+ syslog(LOG_ERR, "MDTM:TIPC Invalid "
+ "MDS_TIPC_FCTRL_ACKSIZE, using default
value");
+ acksize = -1;
+ }
}
- mds_tipc_fctrl_initialize(tipc_cb.BSRsock, port_id,
(uint64_t)optval,
+ mds_tipc_fctrl_initialize(tipc_cb.BSRsock, port_id, optval,
ackto, acksize, tipc_mcast_enabled);
+ } else {
+ syslog(LOG_ERR, "MDTM:TIPC Invalid value of"
+ "MDS_TIPC_FCTRL_ENABLED");
}
}
@@ -366,6 +380,7 @@ uint32_t mdtm_tipc_init(NODE_ID nodeid, uint32_t
*mds_tipc_ref)
close(tipc_cb.Dsock);
close(tipc_cb.BSRsock);
m_NCS_IPC_RELEASE(_cb.tmr_mbx, NULL);
+ mds_tipc_fctrl_shutdown();
return NCSCC_RC_FAILURE;
}
@@ -2528,7 +2543,7 @@ uint32_t mds_mdtm_send_tipc(MDTM_SEND_REQ *req)
*/
uint32_t status = 0;
uint32_t sum_mds_hdr_plus_mdtm_hdr_plus_len;
- uint16_t fctrl_seq_num = 0;
+ uint16_t fctrl_seq_num = 0;
int version = req->msg_arch_word & 0x7;
if (version > 1) {
sum_mds_hdr_plus_mdtm_hdr_plus_len =
@@ -2618,7 +2633,7 @@ uint32_t mds_mdtm_send_tipc(MDTM_SEND_REQ *req)
return NCSCC_RC_FAILURE;
}
/* if sndqueue is capable, then obtain the current sending
seq */
- if (mds_tipc_fctrl_sndqueue_capable(tipc_id, len,
_seq_num)
+ if (mds_tipc_fctrl_sndqueue_capable(tipc_id, _seq_num)
== NCSCC_RC_FAILURE){
m_MDS_LOG_ERR("FCTRL: Failed to send message len :%d", len);
return NCSCC_RC_FAILURE;
@@ -2717,10 +2732,10 @@ uint32_t mds_mdtm_send_tipc(MDTM_SEND_REQ *req)
}
/* if sndqueue is capable, then obtain the current
sending
Re: [devel] [PATCH 0/9] Review Request for mds: Add solution for TIPC buffer overflow [#1960]
Hi all,
Below is the patch #10 that updates most of comments, it applies on top
of current patch #9.
This patch #10 does not use the shared_ptr and base:Mutex as comments
given by Gary and Vu, the reason is that it will cause a similar problem
reported in #2860 (user call exit() without properly doing mds
shutdown), unless those variables are allocated on the heap.
I would like to push the #1960 patches today if we don't have any more
comments. There are some other incremental improvements/fixes that will
be addressed in other tickets.
Thanks
Minh
---
src/mds/README | 2 +-
src/mds/mds_dt_tipc.c | 28 -
src/mds/mds_tipc_fctrl_intf.cc | 67
++--
src/mds/mds_tipc_fctrl_intf.h | 2 +-
src/mds/mds_tipc_fctrl_msg.cc | 44 +-
src/mds/mds_tipc_fctrl_msg.h | 22 +++--
src/mds/mds_tipc_fctrl_portid.cc | 46 ---
7 files changed, 137 insertions(+), 74 deletions(-)
diff --git a/src/mds/README b/src/mds/README
index 1b94632..0819bdc 100644
--- a/src/mds/README
+++ b/src/mds/README
@@ -182,7 +182,7 @@ TIPC portid state machine and its transition
kDisabled, // no flow control support at this state
kStartup, // a newly published portid starts at this state
-kTxProb, // txprob timer is running to confirm if the flow control is
supported
+kTxProb, // tx probation timer is running to confirm if the flow
control is supported
kEnabled // flow control support is confirmed, data flow is controlled
kRcvBuffOverflow // anticipating (or experienced) the receiver's
buffer overflow
diff --git a/src/mds/mds_dt_tipc.c b/src/mds/mds_dt_tipc.c
index 1b6c3f8..e7a7b48 100644
--- a/src/mds/mds_dt_tipc.c
+++ b/src/mds/mds_dt_tipc.c
@@ -247,6 +247,7 @@ uint32_t mdtm_tipc_init(NODE_ID nodeid, uint32_t
*mds_tipc_ref)
if (!get_tipc_port_id(tipc_cb.BSRsock, _id)) {
close(tipc_cb.Dsock);
close(tipc_cb.BSRsock);
+ *mds_tipc_ref = 0;
return NCSCC_RC_FAILURE;
}
*mds_tipc_ref = port_id.ref;
@@ -330,7 +331,7 @@ uint32_t mdtm_tipc_init(NODE_ID nodeid, uint32_t
*mds_tipc_ref)
}
/* Get tipc socket receive buffer size */
- int optval;
+ int optval = 0;
socklen_t optlen = sizeof(optval);
if (getsockopt(tipc_cb.BSRsock, SOL_SOCKET, SO_RCVBUF,
, ) != 0) {
@@ -350,12 +351,25 @@ uint32_t mdtm_tipc_init(NODE_ID nodeid, uint32_t
*mds_tipc_ref)
int acksize = -1;
if ((ptr = getenv("MDS_TIPC_FCTRL_ACKTIMEOUT")) != NULL) {
ackto = atoi(ptr);
+ if (ackto == 0) {
+ syslog(LOG_ERR, "MDTM:TIPC Invalid "
+ "MDS_TIPC_FCTRL_ACKTIMEOUT, using default
value");
+ ackto = -1;
+ }
}
if ((ptr = getenv("MDS_TIPC_FCTRL_ACKSIZE")) != NULL) {
acksize = atoi(ptr);
+ if (acksize == 0) {
+ syslog(LOG_ERR, "MDTM:TIPC Invalid "
+ "MDS_TIPC_FCTRL_ACKSIZE, using default value");
+ acksize = -1;
+ }
}
- mds_tipc_fctrl_initialize(tipc_cb.BSRsock, port_id,
(uint64_t)optval,
+ mds_tipc_fctrl_initialize(tipc_cb.BSRsock, port_id, optval,
ackto, acksize, tipc_mcast_enabled);
+ } else {
+ syslog(LOG_ERR, "MDTM:TIPC Invalid value of"
+ "MDS_TIPC_FCTRL_ENABLED");
}
}
@@ -366,6 +380,7 @@ uint32_t mdtm_tipc_init(NODE_ID nodeid, uint32_t
*mds_tipc_ref)
close(tipc_cb.Dsock);
close(tipc_cb.BSRsock);
m_NCS_IPC_RELEASE(_cb.tmr_mbx, NULL);
+ mds_tipc_fctrl_shutdown();
return NCSCC_RC_FAILURE;
}
@@ -2528,7 +2543,7 @@ uint32_t mds_mdtm_send_tipc(MDTM_SEND_REQ *req)
*/
uint32_t status = 0;
uint32_t sum_mds_hdr_plus_mdtm_hdr_plus_len;
- uint16_t fctrl_seq_num = 0;
+ uint16_t fctrl_seq_num = 0;
int version = req->msg_arch_word & 0x7;
if (version > 1) {
sum_mds_hdr_plus_mdtm_hdr_plus_len =
@@ -2618,7 +2633,7 @@ uint32_t mds_mdtm_send_tipc(MDTM_SEND_REQ *req)
return NCSCC_RC_FAILURE;
}
/* if sndqueue is capable, then obtain the current sending
seq */
- if (mds_tipc_fctrl_sndqueue_capable(tipc_id, len, _seq_num)
+ if (mds_tipc_fctrl_sndqueue_capable(tipc_id, _seq_num)
== NCSCC_RC_FAILURE){
m_MDS_LOG_ERR("FCTRL: Failed to send message len :%d", len);
return NCSCC_RC_FAILURE;
@@ -2717,10 +2732,10 @@ uint32_t mds_mdtm_send_tipc(MDTM_SEND_REQ *req)
}
/* if sndqueue is capable, then obtain the current
sending seq */
if (mds_tipc_fctrl_sndqueue_capable(tipc_id,
- len +
