Hello to all,
I suspect I found a bug in the agent/helpers/table_iterator.c, related
to GET requests of several variables - it doesn't show for one variable
or for get nexts.
The summary: iinfo->free_data_context gets called more than one time for
the same data context.
Setup:
- an iterator info which has free_data_context
- make a request for N different rows in the table, by directly
specifying the index and its columns; i.e., request more than one
attribute for the same index
What's happening:
- the netsnmp_table_iterator_helper_handler goes like this, around
line 454:
index_search =
(iinfo->get_first_data_point) (&callback_loop_context,
&callback_data_context,
index_search, iinfo);
/* loop over each data point */
while(index_search) {
/* remember to free this later */
free_this_index_search = index_search;
/* compare against each request*/
for(request = requests ; request; request = request->next) {
.....
switch(reqinfo->mode) {
case MODE_GET:
case MODE_SET_RESERVE1:
if (snmp_oid_compare(myname, myname_len,
request->requestvb->name,
request->requestvb->name_length) == 0) {
.....
request_count--; /* One less to look for */
} else {
if (iinfo->free_data_context && callback_data_context) {
(iinfo->free_data_context)(callback_data_context,
iinfo);
}
}
break;
The ..... have been deleted as not of importance. Not that, for the
first data context as returned by get_first_data_point, we have N
requests to match against it. Suppose the first fails, then the
free_data_context is called against it. Then the code breaks and goes
back to the for(request =...) and comes back to switch(reqinfo->mode).
But this time, the callback_data_context has been free()'d and is
invalid.
If what I understand from the code is correct, the agent should only
free the data_context when the for(request =...) is finished, no during
the for, because we might use the same data_context more than once.
This means that any request for more rows in a table which uses
free_data_context corrupts the memory and possibly crashes the agent.
Also, at line 587 in the same function, a comparison is made between
callback_data_context and last_loop_context, which represent different
types; I think this should be callback_loop_context and
last_loop_context.
Attached is a patch which seems to correct this problem by moving the
free just before the get_next_data_point. There is one problem
remaining, one extra free() is made by netsnmp_free_ti_cache
(table_iterator.c:246), but I don't understand this code.
The tests and debugging have been made with a module of my own and
valgrind.
Regards,
Iustin Pop
--- table_iterator.c.old 2004-10-13 22:52:24.000000000 +0300
+++ table_iterator.c 2004-10-13 22:59:07.000000000 +0300
@@ -331,6 +331,7 @@
netsnmp_table_registration_info *table_reg_info = NULL;
int i;
netsnmp_data_list *ldata;
+ void *free_this_data_context;
iinfo = (netsnmp_iterator_info *) handler->myvoid;
if (!iinfo || !reginfo || !reqinfo)
@@ -453,6 +454,8 @@
/* loop over each data point */
while(index_search) {
+ free_this_data_context = NULL;
+
/* remember to free this later */
free_this_index_search = index_search;
@@ -484,10 +487,7 @@
callback_loop_context, iinfo);
request_count--; /* One less to look for */
} else {
- if (iinfo->free_data_context && callback_data_context) {
- (iinfo->free_data_context)(callback_data_context,
- iinfo);
- }
+ free_this_data_context = callback_data_context;
}
break;
@@ -552,10 +552,7 @@
request_count--;
} else {
- if (iinfo->free_data_context && callback_data_context) {
- (iinfo->free_data_context)(callback_data_context,
- iinfo);
- }
+ free_this_data_context = callback_data_context;
}
break;
@@ -574,6 +571,14 @@
}
}
+ /* We have checked all requests against this data context,
+ * and we can free it now (if required)
+ */
+ if(free_this_data_context && iinfo->free_data_context) {
+ (iinfo->free_data_context)(free_this_data_context,
+ iinfo);
+ }
+
/* Is there any point in carrying on? */
if (!request_count)
break;
@@ -584,7 +589,7 @@
&callback_data_context,
index_search, iinfo);
if (iinfo->free_loop_context && last_loop_context &&
- callback_data_context != last_loop_context) {
+ callback_loop_context != last_loop_context) {
(iinfo->free_loop_context) (last_loop_context, iinfo);
last_loop_context = NULL;
}
