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;