[prev in list] [next in list] [prev in thread] [next in thread]
List: ssic-linux-devel
Subject: [SSI-devel] Code of a quite stunning level of ugliness.
From: John Hughes <john () Calva ! COM>
Date: 2009-03-24 11:54:01
Message-ID: 49C8C9D9.6000204 () Calva ! COM
[Download RAW message or body]
[Attachment #2 (multipart/alternative)]
In the clusterised IPC stuff there is the idea of a "node-id pair",
which associates IPC id's with the owning node.
To make life interesting there is no structure to these things, they're
just allocated as chunks of memory that happen to be filled with ints.
We have code like this (cluster/ssi/ipc/namesvr_func.c):
int
ipcname_gettotal(int service,
char **node_id_pairs, /* [OUT] nodenum-id pairs */
int *size) /* [INOUT] the number of ipc structs */
^^^ the comment is not quite true, on input size==-1 means just count
the pairs and don't allocate memory. on output size contains *the
number of int's* allocated, not the number of pairs!
{
ipc_obj_db_t *odbp;
char *buf;
int idx=0, count;
...
for (count=0; count < odbp->iodb_size; count++) {
if (odbp->iodb_active[count] != NULL)
idx = idx + 2; <== Note! +2!
}
if ((idx <= 0) || (*size == -1)) goto done;
^^^ so if size == -1 we just return "size"
*node_id_pairs = (char*)kmalloc(idx * sizeof(int),GFP_KERNEL);
^^^ otherwise we allocate an array of pairs to return to the caller.
...
buf = *node_id_pairs;
for (count=0; count < odbp->iodb_size; count++) {
if (odbp->iodb_active[count] == NULL)
continue;
*((int *)buf) = odbp->iodb_active[count]->svr_node;
buf += sizeof(int);
*((int *)buf) = odbp->iodb_active[count]->io_id;
buf += sizeof(int);
^^^ Eww.
}
done:
NSC_IPC_RDUNLOCK(odbp);
*size = idx;
return 0;
}
Callers of this code are pretty dodgy too, for example ipc/mem.c
char *node_id_pairs=NULL;
char *tmp_pairs=NULL;
int size=0, node_num=0;
bzero(viewstr, 10);
if (!local_view) {
size = -1;
cli_ipcname_gettotal(NAME_SERVICE_SHM, &node_id_pairs, &size);
^^^ gets number of int's to allocate
if (size <= 0) goto done;
node_id_pairs = (char *)kmalloc(size * sizeof(int), GFP_KERNEL);
^^^ allocates 'em
if (node_id_pairs == NULL)
goto done;
memset(node_id_pairs, 0, size * sizeof(int));
^^^ zeroes out the list
cli_ipcname_gettotal(NAME_SERVICE_SHM, &node_id_pairs, &size);
^^^ *THEN OVERWRITES THE LIST WITH A NEW COPY ALLOCATED IN
ipcname_gettotal!*
tmp_pairs = (char *)node_id_pairs;
}
[Attachment #5 (text/html)]
<!DOCTYPE html PUBLIC "-//W3C//DTD HTML 4.01 Transitional//EN">
<html>
<head>
</head>
<body bgcolor="#ffffff" text="#000000">
In the clusterised IPC stuff there is the idea of a "node-id pair",
which associates IPC id's with the owning node.<br>
<br>
To make life interesting there is no structure to these things, they're
just allocated as chunks of memory that happen to be filled with
ints. We have code like this (cluster/ssi/ipc/namesvr_func.c):<br>
<pre>int
ipcname_gettotal(int service,
char **node_id_pairs, /* [OUT] nodenum-id pairs */
int *size) /* [INOUT] the number of ipc structs */
</pre>
^^^ the comment is not quite true, on input size==-1 means just count
the pairs and don't allocate memory. on output size contains <b>the
number of int's</b> allocated, not the number of pairs!<br>
<pre>{
ipc_obj_db_t *odbp;
char *buf;
int idx=0, count;
...
for (count=0; count < odbp->iodb_size; count++) {
if (odbp->iodb_active[count] != NULL)
idx = idx + 2; <== Note! +2!
}
if ((idx <= 0) || (*size == -1)) goto done;
</pre>
^^^ so if size == -1 we just return "size"<br>
<pre> *node_id_pairs = (char*)kmalloc(idx * sizeof(int),GFP_KERNEL);
</pre>
^^^ otherwise we allocate an array of pairs to return to the caller.<br>
<pre>...
buf = *node_id_pairs;
for (count=0; count < odbp->iodb_size; count++) {
if (odbp->iodb_active[count] == NULL)
continue;
*((int *)buf) = odbp->iodb_active[count]->svr_node;
buf += sizeof(int);
*((int *)buf) = odbp->iodb_active[count]->io_id;
buf += sizeof(int);
</pre>
^^^ Eww.<br>
<pre> }
done:
NSC_IPC_RDUNLOCK(odbp);
*size = idx;
return 0;
}
</pre>
Callers of this code are pretty dodgy too, for example ipc/mem.c<br>
<pre> char *node_id_pairs=NULL;
char *tmp_pairs=NULL;
int size=0, node_num=0;
bzero(viewstr, 10);
if (!local_view) {
size = -1;
cli_ipcname_gettotal(NAME_SERVICE_SHM, &node_id_pairs, &size);
</pre>
^^^ gets number of int's to allocate<br>
<pre> if (size <= 0) goto done;
node_id_pairs = (char *)kmalloc(size * sizeof(int), GFP_KERNEL);
</pre>
^^^ allocates 'em<br>
<pre> if (node_id_pairs == NULL)
goto done;
memset(node_id_pairs, 0, size * sizeof(int));
</pre>
^^^ zeroes out the list<br>
<pre> cli_ipcname_gettotal(NAME_SERVICE_SHM, &node_id_pairs, &size);
</pre>
^^^ <b>THEN OVERWRITES THE LIST WITH A NEW COPY ALLOCATED IN
ipcname_gettotal!</b><br>
<pre> tmp_pairs = (char *)node_id_pairs;
}
</pre>
<br>
</body>
</html>
------------------------------------------------------------------------------
Apps built with the Adobe(R) Flex(R) framework and Flex Builder(TM) are
powering Web 2.0 with engaging, cross-platform capabilities. Quickly and
easily build your RIAs with Flex Builder, the Eclipse(TM)based development
software that enables intelligent coding and step-through debugging.
Download the free 60 day trial. http://p.sf.net/sfu/www-adobe-com
_______________________________________________
ssic-linux-devel mailing list
ssic-linux-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/ssic-linux-devel
[prev in list] [next in list] [prev in thread] [next in thread]
Configure |
About |
News |
Add a list |
Sponsored by KoreLogic