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

Suggestions for improvements to the NEURON Forum.
Post Reply
jgosmann
Posts: 3
Joined: Tue Jun 24, 2014 12:55 pm
Contact:

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

Post by jgosmann »

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
hines
Site Admin
Posts: 1682
Joined: Wed May 18, 2005 3:32 pm

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

Post by hines »

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
Post Reply