In the process of developing the IPTables::IPv4 Perl module, I've run up against
certain issues in libiptc itself that required some modding on my part to make
it work as it should. Also, in the process of analyzing/debugging, I've made a
couple minor changes to eliminate warnings from the tools I was using
(specifically valgrind). The changes I've made are:
- Setting iptc_fn in every externally-accessible function. I've run into cases
where incorrect errors were returned because a function previously called set
iptc_fn, but the one actually causing an error, called afterward, did not. This
causes confusing error messages to be returned.
- memset()'ing memory areas to be passed as arguments to setsockopt().
valgrind points out that not all the memory in these areas was being set to
useful values, potentially providing bad data to kernelspace.
- Adding refcounting on the sockfd file descriptor. If more than one table is
open at once, libiptc would happily overwrite the previous file descriptor, and
then close it before all open tables got a chance to commit or be released. This
corrects that, only close()'ing it when the last table handle is passed to
TC_FREE().
The patch is attached.
--
Derrik Pates
dpates@xxxxxxxxxx
--- /tmp/libiptc.c 2003-08-17 13:55:24.000000000 -0400
+++ libiptc/libiptc.c 2003-08-17 13:56:23.000000000 -0400
@@ -30,6 +30,7 @@
#endif
static int sockfd = -1;
+static int sockfd_use = 0;
static void *iptc_fn = NULL;
static const char *hooknames[]
@@ -253,30 +254,33 @@
iptc_fn = TC_INIT;
- if (sockfd != -1) {
- close(sockfd);
- sockfd = -1;
- }
-
if (strlen(tablename) >= TABLE_MAXNAMELEN) {
errno = EINVAL;
return NULL;
}
- sockfd = socket(TC_AF, SOCK_RAW, IPPROTO_RAW);
- if (sockfd < 0)
- return NULL;
+ if (sockfd_use == 0) {
+ sockfd = socket(TC_AF, SOCK_RAW, IPPROTO_RAW);
+ if (sockfd < 0)
+ return NULL;
+ }
+ sockfd_use++;
s = sizeof(info);
strcpy(info.name, tablename);
- if (getsockopt(sockfd, TC_IPPROTO, SO_GET_INFO, &info, &s) < 0)
+ if (getsockopt(sockfd, TC_IPPROTO, SO_GET_INFO, &info, &s) < 0) {
+ sockfd_use--;
return NULL;
+ }
if ((h = alloc_handle(info.name, info.size, info.num_entries))
== NULL) {
- close(sockfd);
- sockfd = -1;
+ sockfd_use--;
+ if (sockfd_use == 0) {
+ close(sockfd);
+ sockfd = -1;
+ }
return NULL;
}
@@ -310,8 +314,11 @@
if (getsockopt(sockfd, TC_IPPROTO, SO_GET_ENTRIES, &h->entries,
&tmp) < 0) {
- close(sockfd);
- sockfd = -1;
+ sockfd_use--;
+ if (sockfd_use == 0) {
+ close(sockfd);
+ sockfd = -1;
+ }
free(h);
return NULL;
}
@@ -323,8 +330,12 @@
void
TC_FREE(TC_HANDLE_T *h)
{
- close(sockfd);
- sockfd = -1;
+ iptc_fn = TC_FREE;
+ sockfd_use--;
+ if (sockfd_use == 0) {
+ close(sockfd);
+ sockfd = -1;
+ }
if ((*h)->cache_chain_heads)
free((*h)->cache_chain_heads);
free(*h);
@@ -343,6 +354,7 @@
void
TC_DUMP_ENTRIES(const TC_HANDLE_T handle)
{
+ iptc_fn = TC_DUMP_ENTRIES;
CHECK(handle);
printf("libiptc v%s. %u entries, %u bytes.\n",
@@ -483,6 +495,7 @@
/* Does this chain exist? */
int TC_IS_CHAIN(const char *chain, const TC_HANDLE_T handle)
{
+ iptc_fn = TC_IS_CHAIN;
return find_label(chain, handle) != NULL;
}
@@ -527,6 +540,7 @@
const char *
TC_FIRST_CHAIN(TC_HANDLE_T *handle)
{
+ iptc_fn = TC_FIRST_CHAIN;
if ((*handle)->cache_chain_heads == NULL
&& !populate_cache(*handle))
return NULL;
@@ -541,6 +555,7 @@
const char *
TC_NEXT_CHAIN(TC_HANDLE_T *handle)
{
+ iptc_fn = TC_NEXT_CHAIN;
(*handle)->cache_chain_iteration++;
if ((*handle)->cache_chain_iteration - (*handle)->cache_chain_heads
@@ -556,6 +571,7 @@
{
struct chain_cache *c;
+ iptc_fn = TC_FIRST_RULE;
c = find_label(chain, *handle);
if (!c) {
errno = ENOENT;
@@ -574,6 +590,7 @@
const STRUCT_ENTRY *
TC_NEXT_RULE(const STRUCT_ENTRY *prev, TC_HANDLE_T *handle)
{
+ iptc_fn = TC_NEXT_RULE;
if ((void *)prev + prev->next_offset
== (void *)(*handle)->cache_rule_end)
return NULL;
@@ -589,6 +606,7 @@
unsigned int off = 0;
STRUCT_ENTRY *start, *end;
+ iptc_fn = TC_NUM_RULES;
CHECK(*handle);
if (!find_label(&off, chain, *handle)) {
errno = ENOENT;
@@ -608,6 +626,7 @@
{
unsigned int pos = 0, chainindex;
+ iptc_fn = TC_GET_RULE;
CHECK(*handle);
if (!find_label(&pos, chain, *handle)) {
errno = ENOENT;
@@ -666,6 +685,7 @@
const char *TC_GET_TARGET(const STRUCT_ENTRY *e,
TC_HANDLE_T *handle)
{
+ iptc_fn = TC_GET_TARGET;
return target_name(*handle, e);
}
@@ -675,6 +695,7 @@
{
unsigned int i;
+ iptc_fn = TC_BUILTIN;
for (i = 0; i < NUMHOOKS; i++) {
if ((handle->info.valid_hooks & (1 << i))
&& handle->hooknames[i]
@@ -694,6 +715,7 @@
STRUCT_ENTRY *e;
int hook;
+ iptc_fn = TC_GET_POLICY;
hook = TC_BUILTIN(chain, *handle);
if (hook != 0)
start = (*handle)->info.hook_entry[hook-1];
@@ -1197,6 +1219,7 @@
STRUCT_ENTRY *entry,
TC_HANDLE_T *handle)
{
+ iptc_fn = TC_CHECK_PACKET;
errno = ENOSYS;
return NULL;
}
@@ -1234,6 +1257,7 @@
unsigned int i, end;
struct chain_cache *c;
+ iptc_fn = TC_ZERO_ENTRIES;
if (!(c = find_label(chain, *handle))) {
errno = ENOENT;
return 0;
@@ -1437,6 +1461,7 @@
{
struct chain_cache *c;
+ iptc_fn = TC_GET_REFERENCES;
if (!(c = find_label(chain, *handle))) {
errno = ENOENT;
return 0;
@@ -1627,6 +1652,7 @@
unsigned int i;
size_t counterlen;
+ iptc_fn = TC_COMMIT;
CHECK(*handle);
counterlen = sizeof(STRUCT_COUNTERS_INFO)
@@ -1645,6 +1671,7 @@
errno = ENOMEM;
return 0;
}
+ memset(repl, 0, sizeof(*repl) + (*handle)->entries.size);
/* These are the old counters we will get from kernel */
repl->counters = malloc(sizeof(STRUCT_COUNTERS)
@@ -1654,6 +1681,8 @@
errno = ENOMEM;
return 0;
}
+ memset(repl->counters, 0, sizeof(STRUCT_COUNTERS)
+ * (*handle)->info.num_entries);
/* These are the counters we're going to put back, later. */
newcounters = malloc(counterlen);
@@ -1663,6 +1692,7 @@
errno = ENOMEM;
return 0;
}
+ memset(newcounters, 0, counterlen);
strcpy(repl->name, (*handle)->info.name);
repl->num_entries = (*handle)->new_number;
