Page 1 of 1

Bug (+patch): Code generated from NMODL might segfault

Posted: Thu Aug 07, 2014 11:24 am
Hi,

I couldn't find any information on the preferred way to submit bug reports and I hope this is the right place for it.

The code generated from a PROCEDURE statement combined with a TABLE in an NMODL file might segfault. For example the following code snippet

Code: Select all

``````PROCEDURE trates(v) {
TABLE ninf, ntau
DEPEND  celsius, temp, Ra, Rb, tha, qa

FROM vmin TO vmax WITH 199

rates(v): not consistently executed from here if usetable_hh == 1

:        tinc = -dt * tadj
:        nexp = 1 - exp(tinc/ntau)

}``````
produces the following C code

Code: Select all

``````static void _n_trates(double _lv){ int _i, _j;
double _xi, _theta;
if (!usetable) {
_f_trates(_lv); return;
}
_xi = _mfac_trates * (_lv - _tmin_trates);
_i = (int) _xi;
if (_xi <= 0.) {
ninf = _t_ninf[0];
ntau = _t_ntau[0];
return; }
if (_i >= 199) {
ninf = _t_ninf[199];
ntau = _t_ntau[199];
return; }
_theta = _xi - (double)_i;
ninf = _t_ninf[_i] + _theta*(_t_ninf[_i+1] - _t_ninf[_i]);
ntau = _t_ntau[_i] + _theta*(_t_ntau[_i+1] - _t_ntau[_i]);
}``````
Part of the problem is that _xi might be a number larger than the largest valid integer. In that case the result of the cast to int is undefined and might produce a negative number. The following boundary checks will evaluate to false (_xi is > 0, but _i is < 0 because of the failed conversion) and the access to _t_ninf and _t_ntau might be out of the array bounds which will either return some random value or lead to a segmentation fault.

To fix this I suggest performing the upper boundary check with _xi (like the lower boundary):

Code: Select all

``if (_xi >= 199) {``
Moreover, _lv might be nan (it is not clear to me why this happens, maybe there is some other deeper problem) which will make _xi also a nan. In that case the cast to int is again undefined, but also both of the boundary checks will evaluate to false. Thus, another check for nan is needed before the cast to int:

Code: Select all

``if (isnan(_xi)) { return NAN; }``
Overall, I suggest the following patch:

Code: Select all

``````--- src/nmodl/parsact.c.orig 2014-08-07 10:44:53.353886642 -0400
+++ src/nmodl/parsact.c   2014-08-06 13:29:09.399949755 -0400
@@ -846,9 +846,11 @@
Lappendstr(procfunc, "\n}\n");

/* table lookup */
-       Sprintf(buf, "_xi = _mfac_%s * (%s - _tmin_%s);\n _i = (int) _xi;\n",
+       Sprintf(buf, "_xi = _mfac_%s * (%s - _tmin_%s);\n",
fname, arg->name, fname);
Lappendstr(procfunc, buf);
+       Lappendstr(procfunc, "if (isnan(_xi)) { return NAN; }\n");
+       Lappendstr(procfunc, "_i = (int) _xi;\n");
Lappendstr(procfunc, "if (_xi <= 0.) {\n");
if (type == FUNCTION1) {
Sprintf(buf, "return _t_%s[0];\n", SYM(table->next)->name);
@@ -867,7 +869,7 @@
Lappendstr(procfunc, "return;");
}
Lappendstr(procfunc, "}\n");
-       Sprintf(buf, "if (_i >= %d) {\n", ntab);
+       Sprintf(buf, "if (_xi >= %d) {\n", ntab);
Lappendstr(procfunc, buf);
if (type == FUNCTION1) {
Sprintf(buf, "return _t_%s[%d];\n", SYM(table->next)->name, ntab);``````
So far this patch seems to have solved the problem I described here: http://www.neuron.yale.edu/phpBB/viewto ... f=2&t=3130

Re: Bug (+patch): Code generated from NMODL might segfault

Posted: Tue Aug 12, 2014 10:19 am
Thanks. The patch was slightly modified since 'nan' does not seem to be known on every machine and also so that TABLE would work in PROCEDURE as well.
http://www.neuron.yale.edu/hg/neuron/nr ... 71aeb65c11