sel record crashes IOC when NVL link read illegal value

[Dirk Zimoch]

When SELM is "Specified" and NVL points to a record that provides a value which is out of range [0...11], the record support crashes the IOC. The reason is a missing sanity check in fetch_values() before the value is used to increment two pointers.

The bug exists in all versions of base I checked, including 3.13.*

Additional information:
Fix:
----------------------------------------------------------
Index: selRecord.c
===================================================================
RCS file: /cvs/G/EPICS/base-3.14.8/src/rec/selRecord.c,v
retrieving revision 1.1.1.1
diff -c -r1.1.1.1 selRecord.c
*** selRecord.c 10 Mar 2006 14:12:49 -0000 1.1.1.1
--- selRecord.c 20 Jul 2007 13:49:19 -0000
***************
*** 416,421 ****
--- 416,422 ----
        /* fetch the select index */
        status=dbGetLink(&(psel->nvl),DBR_USHORT,&(psel->seln),0,0);
        if (!RTN_SUCCESS(status)) return(status);
+ if (psel->seln >= 12) return -1;

        plink += psel->seln;
        pvalue += psel->seln;
----------------------------------------------------------

BTW: I think, SELN should have the PP attribute so that when SELM is "Specified" and NVL is not used/constant, one can caput to SELN.

Original Mantis Bug: mantis-295
    http://www.aps.anl.gov/epics/mantis/view_bug_page.php?f_id=295

[Andrew Johnson (anj)]

I committed this fix to CVS:

Index: selRecord.c
===================================================================
RCS file: /net/phoebus/epicsmgr/cvsroot/epics/base/src/rec/selRecord.c,v
retrieving revision 1.17.2.2
diff -u -b -r1.17.2.2 selRecord.c
--- selRecord.c 29 Aug 2005 19:49:39 -0000 1.17.2.2
+++ selRecord.c 20 Jul 2007 19:15:37 -0000
@@ -346,6 +346,10 @@
     pvalue = &psel->a;
     switch (psel->selm){
     case (selSELM_Specified):
+ if (psel->seln >= SEL_MAX) {
+ recGblSetSevr(psel,SOFT_ALARM,MAJOR_ALARM);
+ return(0);
+ }
        val = *(pvalue+psel->seln);
        break;
     case (selSELM_High_Signal):
@@ -417,6 +421,11 @@
        status=dbGetLink(&(psel->nvl),DBR_USHORT,&(psel->seln),0,0);
        if (!RTN_SUCCESS(status)) return(status);

+ if (psel->seln >= SEL_MAX) {
+ recGblSetSevr(psel,SOFT_ALARM,MAJOR_ALARM);
+ return(-1);
+ }
+
        plink += psel->seln;
        pvalue += psel->seln;

I am reluctant to set pp for SELN since this could break existing control systems that don't expect it to be pp. I agree that it would make more sense for it to be pp, but making the change could cause some subtle bugs when upgrading an older control system.

[Andrew Johnson (anj)]

Feedback from Dirk.

[Andrew Johnson (anj)]

I decided to simplify the error paths in record processing, and also made a bad SELN generate an INVALID alarm on request from Dirk. My new patch against the original R3.14.9 release is as follows:

Index: selRecord.c
===================================================================
RCS file: /net/phoebus/epicsmgr/cvsroot/epics/base/src/rec/selRecord.c,v
retrieving revision 1.17.2.2
retrieving revision 1.17.2.4
diff -u -b -r1.17.2.2 -r1.17.2.4
--- selRecord.c 29 Aug 2005 19:49:39 -0000 1.17.2.2
+++ selRecord.c 30 Jul 2007 16:18:04 -0000 1.17.2.4
@@ -1,19 +1,16 @@
 /*************************************************************************\
-* Copyright (c) 2002 The University of Chicago, as Operator of Argonne
+* Copyright (c) 2007 UChicago Argonne LLC, as Operator of Argonne
 * National Laboratory.
 * Copyright (c) 2002 The Regents of the University of California, as
 * Operator of Los Alamos National Laboratory.
-* EPICS BASE Versions 3.13.7
-* and higher are distributed subject to a Software License Agreement found
+* EPICS BASE is distributed subject to a Software License Agreement found
 * in file LICENSE that is included with this distribution.
 \*************************************************************************/
-/* recSel.c */
-/* base/src/rec $Id: selRecord.c,v 1.17.2.2 2005/08/29 19:49:39 anj Exp $ */
+/* $Id: selRecord.c,v 1.17.2.4 2007/07/30 16:18:04 anj Exp $ */

-/* recSel.c - Record Support Routines for Select records */
+/* selRecord.c - Record Support Routines for Select records */
 /*
  * Original Author: Bob Dalesio
- * Current Author: Marty Kraimer
  * Date: 6-2-89
  */

@@ -82,7 +79,7 @@
 #define SEL_MAX 12

 static void checkAlarms();
-static int do_sel();
+static void do_sel();
 static int fetch_values();
 static void monitor();

@@ -119,9 +116,7 @@
 {
     psel->pact = TRUE;
     if ( RTN_SUCCESS(fetch_values(psel)) ) {
- if( !RTN_SUCCESS(do_sel(psel)) ) {
- recGblSetSevr(psel,CALC_ALARM,INVALID_ALARM);
- }
+ do_sel(psel);
     }

     recGblGetTimeStamp(psel);
@@ -333,7 +328,7 @@
     return;
 }

-static int do_sel(struct selRecord *psel)
+static void do_sel(struct selRecord *psel)
 {
     double *pvalue;
     struct link *plink;
@@ -346,6 +341,10 @@
     pvalue = &psel->a;
     switch (psel->selm){
     case (selSELM_Specified):
+ if (psel->seln >= SEL_MAX) {
+ recGblSetSevr(psel,SOFT_ALARM,INVALID_ALARM);
+ return;
+ }
        val = *(pvalue+psel->seln);
        break;
     case (selSELM_High_Signal):
@@ -384,8 +383,8 @@
        val = order[psel->seln];
        break;
     default:
- recGblSetSevr(psel,SOFT_ALARM,MAJOR_ALARM);
- return(-1);
+ recGblSetSevr(psel,CALC_ALARM,INVALID_ALARM);
+ return;
     }
     if (!isinf(val)){
        psel->val=val;
@@ -394,7 +393,7 @@
        recGblSetSevr(psel,UDF_ALARM,MAJOR_ALARM);
        /* If UDF is TRUE this alarm will be overwritten by checkAlarms*/
     }
- return(0);
+ return;
 }

 /*
@@ -415,7 +414,8 @@
     if(psel->selm == selSELM_Specified) {
        /* fetch the select index */
        st...

[Andrew Johnson (anj)]

Fixed in R3.14.10.

[Andrew Johnson (anj)]

R3.14.10 released.