Re: [Patch] tabled: replace event_loopbreak with pipe
On 02/03/2010 10:02 PM, Pete Zaitcev wrote: As it turned out, event_loopbreak() does not awaken the thread that exectutes event_dispatch(), but our code expected that it would. One easily noticeable effect was that there was a noticeable delay between the state transition to DB master and listening on sockets. I knew the mysterious delay existed for a while, but never got around to investigate. For ncld API, I moved the processing of CLD packets to its own thread and suddenly everything else froze. Apparently the existing code only works because of extra packets of CLD protocol. For a fix, dispose of event_loopbreak and use a loopback pipe. Also gone is state_tdb_new. That thing was just disgusting. Notice that we still have one event_loopbreak remaining, because it works correctly thanks to UNIX signal awakening the polling thread. Signed-Off-By: Pete Zaitcev applied, with a few cosmetic changes (speling correction, use of 'switch') -- To unsubscribe from this list: send the line "unsubscribe hail-devel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[Patch] tabled: replace event_loopbreak with pipe
As it turned out, event_loopbreak() does not awaken the thread that
exectutes event_dispatch(), but our code expected that it would.
One easily noticeable effect was that there was a noticeable delay
between the state transition to DB master and listening on sockets.
I knew the mysterious delay existed for a while, but never got around
to investigate. For ncld API, I moved the processing of CLD packets
to its own thread and suddenly everything else froze. Apparently the
existing code only works because of extra packets of CLD protocol.
For a fix, dispose of event_loopbreak and use a loopback pipe.
Also gone is state_tdb_new. That thing was just disgusting.
Notice that we still have one event_loopbreak remaining, because it
works correctly thanks to UNIX signal awakening the polling thread.
Signed-Off-By: Pete Zaitcev
---
server/server.c | 90 +-
server/tabled.h |4 +-
2 files changed, 68 insertions(+), 26 deletions(-)
commit fb1aaf280f14e020e6a0110b828f4879b5eaa11e
Author: Master
Date: Wed Feb 3 19:48:42 2010 -0700
Replace event_loopbreak with ev_pipe.
diff --git a/server/server.c b/server/server.c
index e5580c5..b205340 100644
--- a/server/server.c
+++ b/server/server.c
@@ -89,7 +89,6 @@ static error_t parse_opt (int key, char *arg, struct
argp_state *state);
static const struct argp argp = { options, parse_opt, NULL, doc };
static bool server_running = true;
-static bool dump_stats;
static bool use_syslog = true;
int debugging = 0;
@@ -99,6 +98,12 @@ struct server tabled_srv = {
struct tabledb tdb;
+enum {
+ TT_CMD_DUMP,
+ TT_CMD_TDBST_MASTER,
+ TT_CMD_TDBST_SLAVE
+};
+
struct compiled_pat patterns[] = {
[pat_auth] =
{ "^AWS (\\w+):(\\S+)", 0, },
@@ -361,8 +366,8 @@ static void term_signal(int signo)
static void stats_signal(int signo)
{
- dump_stats = true;
- event_loopbreak();
+ static const unsigned char cmd = TT_CMD_DUMP;
+ write(tabled_srv.ev_pipe[1], &cmd, 1);
}
#define X(stat) \
@@ -1353,6 +1358,7 @@ static void tdb_checkpoint(int fd, short events, void
*userdata)
static void tdb_state_cb(enum db_event event)
{
+ unsigned char cmd;
switch (event) {
case TDB_EV_ELECTED:
@@ -1369,25 +1375,20 @@ static void tdb_state_cb(enum db_event event)
* This callback runs on the context of the replication
* manager thread, and calling any of our functions thus
* turns our program into a multi-threaded one. Instead
-* we do a loopbreak and postpone the processing.
+* we signal them main thread to do the processing.
*/
if (tabled_srv.state_tdb != ST_TDB_INIT &&
tabled_srv.state_tdb != ST_TDB_OPEN) {
if (event == TDB_EV_MASTER)
- tabled_srv.state_tdb_new = ST_TDB_MASTER;
+ cmd = TT_CMD_TDBST_MASTER;
else
- tabled_srv.state_tdb_new = ST_TDB_SLAVE;
- if (debugging) {
- applog(LOG_DEBUG, "TDB state > %s",
-
state_name_tdb[tabled_srv.state_tdb_new]);
- }
- event_loopbreak();
+ cmd = TT_CMD_TDBST_SLAVE;
+ write(tabled_srv.ev_pipe[1], &cmd, 1);
}
break;
default:
applog(LOG_WARNING, "API confusion with TDB, event 0x%x",
event);
tabled_srv.state_tdb = ST_TDB_OPEN; /* wrong, stub for now */
- tabled_srv.state_tdb_new = ST_TDB_INIT;
}
}
@@ -1727,6 +1728,8 @@ static void tdb_state_process(enum st_tdb new_state)
{
unsigned int db_flags;
+ if (debugging)
+ applog(LOG_DEBUG, "TDB state > %s", state_name_tdb[new_state]);
if ((new_state == ST_TDB_MASTER || new_state == ST_TDB_SLAVE) &&
tabled_srv.state_tdb == ST_TDB_ACTIVE) {
@@ -1744,6 +1747,38 @@ static void tdb_state_process(enum st_tdb new_state)
}
}
+static void internal_event(int fd, short events, void *userdata)
+{
+ unsigned char cmd;
+ ssize_t rrc;
+
+ rrc = read(tabled_srv.ev_pipe[0], &cmd, 1);
+ if (rrc < 0) {
+ applog(LOG_WARNING, "pipe read error: %s", strerror(errno));
+ abort();
+ }
+ if (rrc < 1) {
+ applog(LOG_WARNING, "pipe short read");
+ abort();
+ }
+
+ if (cmd == TT_CMD_DUMP) {
+ stats_dump();
+ } else if (cmd == TT_CMD_TDBST_MASTER) {
+ if (tabled_srv.state_tdb != ST_TDB_MASTER) {
+ tdb_state_process(ST_TDB_MASTER);
+ tabled_srv.state_tdb = ST_TDB_MASTER;
+ }
+ } else if (cmd
Re: [PATCHv2 1/2] cld: fix CLD_INODE_NAME_MAX woes
On Wed, Feb 3, 2010 at 2:27 PM, Jeff Garzik wrote: > On 02/03/2010 08:45 AM, Colin McCabe wrote: >> >> When we create a static buffer for an inode name, and treat it like a >> null-terminated string, it needs to be of length CLD_INODE_NAME_MAX + 1 so >> that it can hold the NULL-terminator. >> >> In cldc_del and cldc_open, we should check that the user-submitted inode >> name >> is less than or equal to CLD_INODE_NAME_MAX. Formerly we were just >> checking >> that it wasn't too big to fit in the packet. >> >> When copying the inode name out of struct cld_dirent_cur, use snprintf >> rather >> than strcpy to ensure that we never overflow the buffer. This isn't >> strictly >> necessary if all other checks are working perfectly, but it seems prudent. >> >> Signed-off-by: Colin McCabe > > applied, after s/snprintf/strncpy/ > > In general, too, you should never pass a variable string into snprintf, as > that may make a program vulnerable to printf format string attacks (user > supplies "%s" as a username, for example). Oh, how embarassing. I hate it when people do that, now I did it too. I tend to avoid strncpy because of how it always writes n bytes. If only glibc had picked up strlcpy. Colin -- To unsubscribe from this list: send the line "unsubscribe hail-devel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [Patch 4/7] tabled: retry conflicting locks
On 01/20/2010 05:56 PM, Pete Zaitcev wrote: Is there a way to cancel an outstanding lock request? How? You seem to think that there's no problem. Actually I think an cmo_close on a handle that has outstanding requests of any kind should drop them, so I was incorrect about killing the session being the only way. Maybe I can create some kind of ncld_open_locked() by using that feature. That ought to be good enough. CLOSE always removes outstanding locks, FWIW... always has. Jeff -- To unsubscribe from this list: send the line "unsubscribe hail-devel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCHv2 1/2] cld: fix CLD_INODE_NAME_MAX woes
On 02/03/2010 08:45 AM, Colin McCabe wrote: When we create a static buffer for an inode name, and treat it like a null-terminated string, it needs to be of length CLD_INODE_NAME_MAX + 1 so that it can hold the NULL-terminator. In cldc_del and cldc_open, we should check that the user-submitted inode name is less than or equal to CLD_INODE_NAME_MAX. Formerly we were just checking that it wasn't too big to fit in the packet. When copying the inode name out of struct cld_dirent_cur, use snprintf rather than strcpy to ensure that we never overflow the buffer. This isn't strictly necessary if all other checks are working perfectly, but it seems prudent. Signed-off-by: Colin McCabe applied, after s/snprintf/strncpy/ In general, too, you should never pass a variable string into snprintf, as that may make a program vulnerable to printf format string attacks (user supplies "%s" as a username, for example). A few other changes made to your XDR work: * "\n" removed from log messages, as that is appended as needed by log implementation * user_key() restored. that is our authentication hook, and it must be called, even though it merely returns the username passed to it at present. * msg type renamed back to msg op Jeff -- To unsubscribe from this list: send the line "unsubscribe hail-devel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCHv2 2/2] cld: kill CLD_MAX_PKT_MSG, add CLD_MAX_PAYLOAD_SZ
Get rid of CLD_MAX_PKT_MSG. It only existed so that we could use static arrays
in a few places.
Create CLD_MAX_PAYLOAD_SZ to represent the maximum size of a message that the
API user can GET or PUT. Reducing this constant could break users who
relied on the old maximum data size, so we should try not to do it often.
Signed-off-by: Colin McCabe
---
include/cld_msg.h |7 ---
include/cldc.h|4 ++--
lib/cldc.c| 24 +---
server/msg.c |7 +++
server/session.c | 12
5 files changed, 38 insertions(+), 16 deletions(-)
diff --git a/include/cld_msg.h b/include/cld_msg.h
index 6117be8..f24e4e0 100644
--- a/include/cld_msg.h
+++ b/include/cld_msg.h
@@ -34,9 +34,10 @@ enum {
CLD_MAX_SECRET_KEY = 128, /**< includes req. nul */
CLD_MAX_PKT_MSG_SZ = 1024,
- CLD_MAX_PKT_MSG = 128,
- CLD_MAX_MSG_SZ = CLD_MAX_PKT_MSG * 1024, /**< maximum total
- msg size, including all packets */
+ CLD_MAX_PAYLOAD_SZ = 131072, /**< maximum size of data that
users
+ can GET or PUT */
+ CLD_MAX_MSG_SZ = 196608, /**< maximum total
+ msg size, including all packets
*/
};
/** available RPC operations */
diff --git a/include/cldc.h b/include/cldc.h
index 0d72669..37d6eb6 100644
--- a/include/cldc.h
+++ b/include/cldc.h
@@ -73,10 +73,10 @@ struct cldc_msg {
int data_len;
int n_pkts;
- struct cldc_pkt_info *pkt_info[CLD_MAX_PKT_MSG];
+ uint8_t *data;
/* must be at end of struct */
- uint8_t data[0];
+ struct cldc_pkt_info *pkt_info[0];
};
/** an open file handle associated with a session */
diff --git a/lib/cldc.c b/lib/cldc.c
index dcc179c..0eec70f 100644
--- a/lib/cldc.c
+++ b/lib/cldc.c
@@ -260,7 +260,9 @@ static void cldc_msg_free(struct cldc_msg *msg)
if (!msg)
return;
- for (i = 0; i < CLD_MAX_PKT_MSG; i++)
+ free(msg->data);
+
+ for (i = 0; i < msg->n_pkts; i++)
free(msg->pkt_info[i]);
free(msg);
@@ -492,14 +494,23 @@ static struct cldc_msg *cldc_new_msg(struct cldc_session
*sess,
struct cldc_msg *msg;
struct cld_msg_hdr *hdr;
struct timeval tv;
- int i, data_left;
+ int n_pkts, i, data_left;
void *p;
gettimeofday(&tv, NULL);
- msg = calloc(1, sizeof(*msg) + msg_len);
+ n_pkts = msg_len / CLD_MAX_PKT_MSG_SZ;
+ n_pkts += ((msg_len % CLD_MAX_PKT_MSG_SZ) ? 1 : 0);
+
+ msg = calloc(1, sizeof(*msg) +
+ n_pkts * sizeof(struct cldc_pkt_info *));
if (!msg)
return NULL;
+ msg->data = calloc(1, msg_len);
+ if (!msg->data) {
+ free(msg);
+ return NULL;
+ }
__cld_rand64(&msg->xid);
@@ -512,8 +523,7 @@ static struct cldc_msg *cldc_new_msg(struct cldc_session
*sess,
msg->data_len = msg_len;
- msg->n_pkts = msg_len / CLD_MAX_PKT_MSG_SZ;
- msg->n_pkts += ((msg_len % CLD_MAX_PKT_MSG_SZ) ? 1 : 0);
+ msg->n_pkts = n_pkts;
p = msg->data;
data_left = msg_len;
@@ -542,7 +552,7 @@ static struct cldc_msg *cldc_new_msg(struct cldc_session
*sess,
data_left -= pkt_len;
}
- hdr = (struct cld_msg_hdr *) &msg->data[0];
+ hdr = (struct cld_msg_hdr *) msg->data;
memcpy(&hdr->magic, CLD_MSG_MAGIC, CLD_MAGIC_SZ);
hdr->op = op;
hdr->xid = msg->xid;
@@ -1099,7 +1109,7 @@ int cldc_put(struct cldc_fh *fh, const struct
cldc_call_opts *copts,
struct cldc_msg *msg;
struct cld_msg_put *put;
- if (!data || !data_len || data_len > CLD_MAX_MSG_SZ)
+ if (!data || !data_len || data_len > CLD_MAX_PAYLOAD_SZ)
return -EINVAL;
if (!fh->valid)
diff --git a/server/msg.c b/server/msg.c
index 301f698..62f8a71 100644
--- a/server/msg.c
+++ b/server/msg.c
@@ -757,6 +757,13 @@ void msg_put(struct msg_params *mp)
/* make sure additional input data as large as expected */
data_size = le32_to_cpu(msg->data_size);
+ if (data_size > CLD_MAX_PAYLOAD_SZ) {
+ HAIL_ERR(&srv_log, "%s: can't PUT %d bytes of data: "
+ "%d is the maximum.\n",
+ __func__, data_size, CLD_MAX_PAYLOAD_SZ);
+ resp_rc = CLE_BAD_PKT;
+ goto err_out_noabort;
+ }
if (mp->msg_len != (data_size + sizeof(*msg))) {
HAIL_INFO(&srv_log, "PUT len mismatch: msg len %zu, "
"wanted %zu + %u (== %zu)",
diff --git a/server/session.c b/server/session.c
index 9774fb5..f63f66a 100644
--- a/server/session.c
+++ b/server/session.c
@@ -623,13 +623,20 @@ bool sess_sendms
[PATCHv2 1/2] cld: fix CLD_INODE_NAME_MAX woes
v2: one part of this patch was originally accidentally mixed into patch 2
When we create a static buffer for an inode name, and treat it like a
null-terminated string, it needs to be of length CLD_INODE_NAME_MAX + 1 so
that it can hold the NULL-terminator.
In cldc_del and cldc_open, we should check that the user-submitted inode name
is less than or equal to CLD_INODE_NAME_MAX. Formerly we were just checking
that it wasn't too big to fit in the packet.
When copying the inode name out of struct cld_dirent_cur, use snprintf rather
than strcpy to ensure that we never overflow the buffer. This isn't strictly
necessary if all other checks are working perfectly, but it seems prudent.
Signed-off-by: Colin McCabe
---
include/cldc.h |2 +-
lib/cldc.c |4 ++--
tools/cldcli.c |1 +
3 files changed, 4 insertions(+), 3 deletions(-)
diff --git a/include/cldc.h b/include/cldc.h
index f1db7d2..0d72669 100644
--- a/include/cldc.h
+++ b/include/cldc.h
@@ -41,7 +41,7 @@ struct cldc_call_opts {
struct cld_msg_get_resp resp;
const char *buf;
unsigned int size;
- char inode_name[CLD_INODE_NAME_MAX];
+ char inode_name[CLD_INODE_NAME_MAX + 1];
} get;
} u;
};
diff --git a/lib/cldc.c b/lib/cldc.c
index 3dc565c..dcc179c 100644
--- a/lib/cldc.c
+++ b/lib/cldc.c
@@ -903,7 +903,7 @@ int cldc_del(struct cldc_session *sess, const struct
cldc_call_opts *copts,
return -EINVAL;
plen = strlen(pathname);
- if (plen > 65530)
+ if (plen > CLD_INODE_NAME_MAX)
return -EINVAL;
/* create DEL message */
@@ -974,7 +974,7 @@ int cldc_open(struct cldc_session *sess,
return -EINVAL;
plen = strlen(pathname);
- if (plen > 65530)
+ if (plen > CLD_INODE_NAME_MAX)
return -EINVAL;
/* create OPEN message */
diff --git a/tools/cldcli.c b/tools/cldcli.c
index c274e61..acf45d7 100644
--- a/tools/cldcli.c
+++ b/tools/cldcli.c
@@ -254,6 +254,7 @@ static int cb_ls_2(struct cldc_call_opts *copts_in, enum
cle_err_codes errc)
s = cldc_dirent_name(&dc);
strcpy(lsr.name, s);
+ snprintf(lsr.name, CLD_INODE_NAME_MAX + 1, s);
free(s);
write_from_thread(&lsr, sizeof(lsr));
--
1.6.2.5
--
To unsubscribe from this list: send the line "unsubscribe hail-devel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] Re: [PATCHv2] cld: use XDR for all messages
On Tue, Feb 2, 2010 at 10:35 PM, Jeff Garzik wrote: > On 01/29/2010 08:01 PM, Colin McCabe wrote: >> >> That seems reasonable. All of those functions could be looked at as >> "common." >> >> The intention was to group together a bunch of functions that were >> useful for packet formatting. Although the bulk of the formatting is >> done by XDR, there are some things that XDR doesn't do, like >> generating and checking SHA sums. cld_dump_buf and cld_pkt_hdr_to_str >> are purely for debugging purposes, but they seemed like something that >> would be generally useful for developers making packet format changes. >> I know that they were useful to me. > > I finally pushed out several changes split off from your main XDR patch, to > the upstream cld git repo. It took longer than expected because I would > make changes of my own along the way, which, each time, necessitated a > rediff+rebuild between "current cld" working tree and "cld + xdr" working > tree. > > The attached patch is what remains to be split up and committed. I have > definitely whittled it down, and along the way, moved and renamed some > things on my own. With regards to cld_fmt.*, my main objection was to > creating too many to-be-installed headers. One more header can be a pain > for us and for users, while one more source file in cld/lib/ is no big deal. > > Thus, the intention is to eliminate cld_fmt.h (newly renamed to cld_pkt.h) > altogether, while keeping the arrangement you created in cld_fmt.c (newly > renamed to lib/pkt.c). Yeah, after I looked at the v2 patch I realized it wasn't separated out well enough. So I was working on a v3 patch series that had the following changes: 1. kill CLD_MAX_PKT_MSG and add CLD_MAX_PAYLOAD_SZ 2. reformat Makefile.am files (to one file per line for nicer diffs) 3. move cld_msg macros into cld_common 4. some style issues 5. add cld_fmt.c (now called cld_pkt.c) 6. create cldc_call_opts_get_data and switch to using it in test/ and tool/ 7. the big XDR changes. It looks like 2, 3, 4, 5 have already come to pass (thanks Jeff!) I just sent out #1 and another patch that is related to CLD_INODE_NAME_MAX I think I'll send out a patch for #6 soon. > I will continue whittling down the patch until it just contains the XDR > changes themselves. In tools/Makefile.am, I don't think you need $(top_srcdir)/lib any more, since cld_msg_rpc.h is now an installed header. Colin -- To unsubscribe from this list: send the line "unsubscribe hail-devel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 2/2] cld: kill CLD_MAX_PKT_MSG, add CLD_MAX_PAYLOAD_SZ
Get rid of CLD_MAX_PKT_MSG. It only existed so that we could use static arrays
in a few places.
Create CLD_MAX_PAYLOAD_SZ to represent the maximum size of a message that the
API user can GET or PUT. Reducing this constant could break users who
relied on the old maximum data size, so we should try not to do it often.
Signed-off-by: Colin McCabe
---
include/cld_msg.h |7 ---
include/cldc.h|4 ++--
lib/cldc.c| 24 +---
server/msg.c |7 +++
server/session.c | 12
tools/cldcli.c|2 +-
6 files changed, 39 insertions(+), 17 deletions(-)
diff --git a/include/cld_msg.h b/include/cld_msg.h
index 6117be8..f24e4e0 100644
--- a/include/cld_msg.h
+++ b/include/cld_msg.h
@@ -34,9 +34,10 @@ enum {
CLD_MAX_SECRET_KEY = 128, /**< includes req. nul */
CLD_MAX_PKT_MSG_SZ = 1024,
- CLD_MAX_PKT_MSG = 128,
- CLD_MAX_MSG_SZ = CLD_MAX_PKT_MSG * 1024, /**< maximum total
- msg size, including all packets */
+ CLD_MAX_PAYLOAD_SZ = 131072, /**< maximum size of data that
users
+ can GET or PUT */
+ CLD_MAX_MSG_SZ = 196608, /**< maximum total
+ msg size, including all packets
*/
};
/** available RPC operations */
diff --git a/include/cldc.h b/include/cldc.h
index 0d72669..37d6eb6 100644
--- a/include/cldc.h
+++ b/include/cldc.h
@@ -73,10 +73,10 @@ struct cldc_msg {
int data_len;
int n_pkts;
- struct cldc_pkt_info *pkt_info[CLD_MAX_PKT_MSG];
+ uint8_t *data;
/* must be at end of struct */
- uint8_t data[0];
+ struct cldc_pkt_info *pkt_info[0];
};
/** an open file handle associated with a session */
diff --git a/lib/cldc.c b/lib/cldc.c
index dcc179c..0eec70f 100644
--- a/lib/cldc.c
+++ b/lib/cldc.c
@@ -260,7 +260,9 @@ static void cldc_msg_free(struct cldc_msg *msg)
if (!msg)
return;
- for (i = 0; i < CLD_MAX_PKT_MSG; i++)
+ free(msg->data);
+
+ for (i = 0; i < msg->n_pkts; i++)
free(msg->pkt_info[i]);
free(msg);
@@ -492,14 +494,23 @@ static struct cldc_msg *cldc_new_msg(struct cldc_session
*sess,
struct cldc_msg *msg;
struct cld_msg_hdr *hdr;
struct timeval tv;
- int i, data_left;
+ int n_pkts, i, data_left;
void *p;
gettimeofday(&tv, NULL);
- msg = calloc(1, sizeof(*msg) + msg_len);
+ n_pkts = msg_len / CLD_MAX_PKT_MSG_SZ;
+ n_pkts += ((msg_len % CLD_MAX_PKT_MSG_SZ) ? 1 : 0);
+
+ msg = calloc(1, sizeof(*msg) +
+ n_pkts * sizeof(struct cldc_pkt_info *));
if (!msg)
return NULL;
+ msg->data = calloc(1, msg_len);
+ if (!msg->data) {
+ free(msg);
+ return NULL;
+ }
__cld_rand64(&msg->xid);
@@ -512,8 +523,7 @@ static struct cldc_msg *cldc_new_msg(struct cldc_session
*sess,
msg->data_len = msg_len;
- msg->n_pkts = msg_len / CLD_MAX_PKT_MSG_SZ;
- msg->n_pkts += ((msg_len % CLD_MAX_PKT_MSG_SZ) ? 1 : 0);
+ msg->n_pkts = n_pkts;
p = msg->data;
data_left = msg_len;
@@ -542,7 +552,7 @@ static struct cldc_msg *cldc_new_msg(struct cldc_session
*sess,
data_left -= pkt_len;
}
- hdr = (struct cld_msg_hdr *) &msg->data[0];
+ hdr = (struct cld_msg_hdr *) msg->data;
memcpy(&hdr->magic, CLD_MSG_MAGIC, CLD_MAGIC_SZ);
hdr->op = op;
hdr->xid = msg->xid;
@@ -1099,7 +1109,7 @@ int cldc_put(struct cldc_fh *fh, const struct
cldc_call_opts *copts,
struct cldc_msg *msg;
struct cld_msg_put *put;
- if (!data || !data_len || data_len > CLD_MAX_MSG_SZ)
+ if (!data || !data_len || data_len > CLD_MAX_PAYLOAD_SZ)
return -EINVAL;
if (!fh->valid)
diff --git a/server/msg.c b/server/msg.c
index 301f698..62f8a71 100644
--- a/server/msg.c
+++ b/server/msg.c
@@ -757,6 +757,13 @@ void msg_put(struct msg_params *mp)
/* make sure additional input data as large as expected */
data_size = le32_to_cpu(msg->data_size);
+ if (data_size > CLD_MAX_PAYLOAD_SZ) {
+ HAIL_ERR(&srv_log, "%s: can't PUT %d bytes of data: "
+ "%d is the maximum.\n",
+ __func__, data_size, CLD_MAX_PAYLOAD_SZ);
+ resp_rc = CLE_BAD_PKT;
+ goto err_out_noabort;
+ }
if (mp->msg_len != (data_size + sizeof(*msg))) {
HAIL_INFO(&srv_log, "PUT len mismatch: msg len %zu, "
"wanted %zu + %u (== %zu)",
diff --git a/server/session.c b/server/session.c
index 9774fb5..f63f66a 100644
--- a/server/session.c
+++ b/server/session.c
@@ -623,1
[PATCH 1/2] cld: fix CLD_INODE_NAME_MAX woes
When we create a static buffer for an inode name, and treat it like a
null-terminated string, it needs to be of length CLD_INODE_NAME_MAX + 1 so
that it can hold the NULL-terminator.
In cldc_del and cldc_open, we should check that the user-submitted inode name
is less than or equal to CLD_INODE_NAME_MAX. Formerly we were just checking
that it wasn't too big to fit in the packet.
When copying the inode name out of struct cld_dirent_cur, use snprintf rather
than strcpy to ensure that we never overflow the buffer. This isn't strictly
necessary if all other checks are working perfectly, but it seems prudent.
Signed-off-by: Colin McCabe
---
include/cldc.h |2 +-
lib/cldc.c |4 ++--
2 files changed, 3 insertions(+), 3 deletions(-)
diff --git a/include/cldc.h b/include/cldc.h
index f1db7d2..0d72669 100644
--- a/include/cldc.h
+++ b/include/cldc.h
@@ -41,7 +41,7 @@ struct cldc_call_opts {
struct cld_msg_get_resp resp;
const char *buf;
unsigned int size;
- char inode_name[CLD_INODE_NAME_MAX];
+ char inode_name[CLD_INODE_NAME_MAX + 1];
} get;
} u;
};
diff --git a/lib/cldc.c b/lib/cldc.c
index 3dc565c..dcc179c 100644
--- a/lib/cldc.c
+++ b/lib/cldc.c
@@ -903,7 +903,7 @@ int cldc_del(struct cldc_session *sess, const struct
cldc_call_opts *copts,
return -EINVAL;
plen = strlen(pathname);
- if (plen > 65530)
+ if (plen > CLD_INODE_NAME_MAX)
return -EINVAL;
/* create DEL message */
@@ -974,7 +974,7 @@ int cldc_open(struct cldc_session *sess,
return -EINVAL;
plen = strlen(pathname);
- if (plen > 65530)
+ if (plen > CLD_INODE_NAME_MAX)
return -EINVAL;
/* create OPEN message */
--
1.6.2.5
--
To unsubscribe from this list: send the line "unsubscribe hail-devel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
