From 2340c6e6c1a3b34526108fc84f0220fadd052146 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Kriszti=C3=A1n=20L=C3=B6ki?= Date: Thu, 25 Feb 2021 14:54:07 +0100 Subject: [PATCH 1/5] Allow changing the BPTR field This fixes github issue #97: Reading into an aai record from a compress or histogram or subArray record could cause a segfault if the aai record was initialized before the linked one. --- modules/database/src/std/rec/compressRecord.c | 5 +++-- modules/database/src/std/rec/histogramRecord.c | 4 ++-- modules/database/src/std/rec/subArrayRecord.c | 4 ++-- 3 files changed, 7 insertions(+), 6 deletions(-) diff --git a/modules/database/src/std/rec/compressRecord.c b/modules/database/src/std/rec/compressRecord.c index 98e17985f..d7da7f9b6 100644 --- a/modules/database/src/std/rec/compressRecord.c +++ b/modules/database/src/std/rec/compressRecord.c @@ -106,7 +106,7 @@ static void monitor(compressRecord *prec) db_post_events(prec, &prec->nuse, monitor_mask); prec->ouse = prec->nuse; } - db_post_events(prec, prec->bptr, monitor_mask); + db_post_events(prec, (void*)&prec->val, monitor_mask); } static void put_value(compressRecord *prec, double *psource, int n) @@ -404,7 +404,6 @@ static long cvt_dbaddr(DBADDR *paddr) { compressRecord *prec = (compressRecord *) paddr->precord; - paddr->pfield = prec->bptr; paddr->no_elements = prec->nsam; paddr->field_type = DBF_DOUBLE; paddr->field_size = sizeof(double); @@ -426,6 +425,8 @@ static long get_array_info(DBADDR *paddr, long *no_elements, long *offset) epicsUInt32 off = prec->off; epicsUInt32 nuse = prec->nuse; + paddr->pfield = prec->bptr; + if (prec->balg == bufferingALG_FIFO) { epicsUInt32 nsam = prec->nsam; diff --git a/modules/database/src/std/rec/histogramRecord.c b/modules/database/src/std/rec/histogramRecord.c index 0e9724cb6..bc618df2e 100644 --- a/modules/database/src/std/rec/histogramRecord.c +++ b/modules/database/src/std/rec/histogramRecord.c @@ -291,7 +291,7 @@ static void monitor(histogramRecord *prec) } /* send out monitors connected to the value field */ if (monitor_mask) - db_post_events(prec, prec->bptr, monitor_mask); + db_post_events(prec, (void*)&prec->val, monitor_mask); return; } @@ -300,7 +300,6 @@ static long cvt_dbaddr(DBADDR *paddr) { histogramRecord *prec = (histogramRecord *) paddr->precord; - paddr->pfield = prec->bptr; paddr->no_elements = prec->nelm; paddr->field_type = DBF_ULONG; paddr->field_size = sizeof(epicsUInt32); @@ -312,6 +311,7 @@ static long get_array_info(DBADDR *paddr, long *no_elements, long *offset) { histogramRecord *prec = (histogramRecord *) paddr->precord; + paddr->pfield = prec->bptr; *no_elements = prec->nelm; *offset = 0; return 0; diff --git a/modules/database/src/std/rec/subArrayRecord.c b/modules/database/src/std/rec/subArrayRecord.c index 82e543ca7..b73dee16b 100644 --- a/modules/database/src/std/rec/subArrayRecord.c +++ b/modules/database/src/std/rec/subArrayRecord.c @@ -161,7 +161,6 @@ static long cvt_dbaddr(DBADDR *paddr) { subArrayRecord *prec = (subArrayRecord *) paddr->precord; - paddr->pfield = prec->bptr; paddr->no_elements = prec->malm; paddr->field_type = prec->ftvl; paddr->field_size = dbValueSize(prec->ftvl); @@ -174,6 +173,7 @@ static long get_array_info(DBADDR *paddr, long *no_elements, long *offset) { subArrayRecord *prec = (subArrayRecord *) paddr->precord; + paddr->pfield = prec->bptr; if (prec->udf) *no_elements = 0; else @@ -293,7 +293,7 @@ static void monitor(subArrayRecord *prec) monitor_mask = recGblResetAlarms(prec); monitor_mask |= (DBE_LOG|DBE_VALUE); - db_post_events(prec, prec->bptr, monitor_mask); + db_post_events(prec, (void*)&prec->val, monitor_mask); return; } From 4a0f488657e208ab2ed6aed17473d42d19fc9d2d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Kriszti=C3=A1n=20L=C3=B6ki?= Date: Thu, 25 Feb 2021 16:13:48 +0100 Subject: [PATCH 2/5] Fixed db_post_events to not use bptr --- modules/database/src/std/rec/histogramRecord.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/modules/database/src/std/rec/histogramRecord.c b/modules/database/src/std/rec/histogramRecord.c index bc618df2e..44b278f39 100644 --- a/modules/database/src/std/rec/histogramRecord.c +++ b/modules/database/src/std/rec/histogramRecord.c @@ -111,7 +111,7 @@ static void wdogCallback(epicsCallback *arg) if (prec->mcnt > 0){ dbScanLock((struct dbCommon *)prec); recGblGetTimeStamp(prec); - db_post_events(prec, prec->bptr, DBE_VALUE | DBE_LOG); + db_post_events(prec, (void*)&prec->val, DBE_VALUE | DBE_LOG); prec->mcnt = 0; dbScanUnlock((struct dbCommon *)prec); } From 1c566e21102e254a47974e2526847fa3d7117ecc Mon Sep 17 00:00:00 2001 From: Andrew Johnson Date: Sat, 27 Feb 2021 22:08:50 -0600 Subject: [PATCH 3/5] Modify aai to support pass-1 device initialization The Soft Channel device support requests pass-1 initialization. It no longer needs to initialize the INP link or allocate the array buffer itself, these are taken care of elsewhere. The record code uses PACT to remember that the device must be initialized again in pass 1. --- modules/database/src/std/dev/devAaiSoft.c | 12 ++++-------- modules/database/src/std/rec/aaiRecord.c | 24 ++++++++++++++++------- 2 files changed, 21 insertions(+), 15 deletions(-) diff --git a/modules/database/src/std/dev/devAaiSoft.c b/modules/database/src/std/dev/devAaiSoft.c index 55975bae0..aafdd1bcd 100644 --- a/modules/database/src/std/dev/devAaiSoft.c +++ b/modules/database/src/std/dev/devAaiSoft.c @@ -47,19 +47,15 @@ static long init_record(dbCommon *pcommon) aaiRecord *prec = (aaiRecord *)pcommon; DBLINK *plink = &prec->inp; - /* This is pass 0, link hasn't been initialized yet */ - dbInitLink(plink, DBF_INLINK); + /* Ask record to call us in pass 1 instead */ + if (prec->pact != 2) { + return 2; + } if (dbLinkIsConstant(plink)) { long nRequest = prec->nelm; long status; - /* Allocate a buffer, record support hasn't done that yet */ - if (!prec->bptr) { - prec->bptr = callocMustSucceed(nRequest, dbValueSize(prec->ftvl), - "devAaiSoft: buffer calloc failed"); - } - status = dbLoadLinkArray(plink, prec->ftvl, prec->bptr, &nRequest); if (!status) { prec->nord = nRequest; diff --git a/modules/database/src/std/rec/aaiRecord.c b/modules/database/src/std/rec/aaiRecord.c index c1410067c..157a48f5b 100644 --- a/modules/database/src/std/rec/aaiRecord.c +++ b/modules/database/src/std/rec/aaiRecord.c @@ -112,16 +112,18 @@ static long init_record(struct dbCommon *pcommon, int pass) prec->ftvl = DBF_UCHAR; prec->nord = (prec->nelm == 1); - /* we must call pdset->init_record in pass 0 - because it may set prec->bptr which must - not change after links are established before pass 1 - */ - + /* call pdset->init_record() in pass 0 so it can do its own + * memory allocation and set prec->bptr, which must be set by + * the end of pass 0. + */ if (pdset->common.init_record) { long status = pdset->common.init_record(pcommon); - /* init_record may set the bptr to point to the data */ - if (status) + if (status == 2) { + /* requesting pass 1 callback, remember to do that */ + prec->pact = 2; + } + else if (status) return status; } if (!prec->bptr) { @@ -132,6 +134,14 @@ static long init_record(struct dbCommon *pcommon, int pass) return 0; } + if (prec->pact == 2) { + /* device support asked for an init_record() callback in pass 1 */ + long status = pdset->common.init_record(pcommon); + if (status) + return status; + prec->pact = FALSE; + } + recGblInitSimm(pcommon, &prec->sscn, &prec->oldsimm, &prec->simm, &prec->siml); /* must have read_aai function defined */ From 6734918e6e188b1f846e603e7943c24bc6c1bed7 Mon Sep 17 00:00:00 2001 From: Andrew Johnson Date: Sat, 27 Feb 2021 22:19:48 -0600 Subject: [PATCH 4/5] Release notes and aai documentation updates --- documentation/RELEASE_NOTES.md | 15 +++++++ .../database/src/std/rec/aaiRecord.dbd.pod | 44 +++++++++++++------ 2 files changed, 45 insertions(+), 14 deletions(-) diff --git a/documentation/RELEASE_NOTES.md b/documentation/RELEASE_NOTES.md index eb9ba33fa..a8570f9e7 100644 --- a/documentation/RELEASE_NOTES.md +++ b/documentation/RELEASE_NOTES.md @@ -18,6 +18,21 @@ should also be read to understand what has changed since earlier releases. +### Fix aai's Device Support Initialization + +Krisztian Loki [reported](https://github.com/epics-base/epics-base/issues/97) +segfaults occurring when a Soft Channel aai record INP field was a DB link to +an array field of a compress record. This was caused by the aai record's +pass-0 device support initialization clashing with the semantics of the new +link support API. + +The aai record +[has been modified](https://github.com/epics-base/epics-base/pull/114) to +allow the Soft Channel device support to request a pass-1 initialization +callback. See the Device Support section of the Array Analogue Input Record +Reference pages in this release for the API changes, which are fully backwards +compatible for existing aai device support. + ### Priority inversion safe posix mutexes diff --git a/modules/database/src/std/rec/aaiRecord.dbd.pod b/modules/database/src/std/rec/aaiRecord.dbd.pod index 8eb8bad05..b3dd19216 100644 --- a/modules/database/src/std/rec/aaiRecord.dbd.pod +++ b/modules/database/src/std/rec/aaiRecord.dbd.pod @@ -151,10 +151,15 @@ for more information on simulation mode and its fields. static long init_record(aaiRecord *prec, int pass) -If device support includes C, it is called. +If device support includes an C routine it is called, but unlike +most record types this occurs in pass 0, which allows the device support to +allocate the array buffer itself. + +Since EPICS 7.0.5 the device support may return 2 to request a second call to +its C routine in pass 1. Checks if device support allocated array space. If not, space for the array is -allocated using NELM and FTVL. The array address is stored in the record. +allocated using NELM and FTVL. The array address is stored in BPTR. This routine initializes SIMM with the value of SIML if SIML type is CONSTANT link or creates a channel access link if SIML type is PV_LINK. VAL is likewise @@ -294,7 +299,7 @@ Scan forward link if necessary, set PACT FALSE, and return. %/* Declare Device Support Entry Table */ %struct aaiRecord; %typedef struct aaidset { - % dset common; /*init_record returns: (-1,0)=>(failure,success)*/ + % dset common; /*init_record returns: (-1,0,2)=>(failure,success,callback)*/ % long (*read_aai)(struct aaiRecord *prec); /*returns: (-1,0)=>(failure,success)*/ %} aaidset; %#define HAS_aaidset @@ -469,8 +474,19 @@ with C set to 1. long init_record(dbCommon *precord) -This routine is optional. If provided, it is called by the record support -C routine. +This routine is optional. +If provided, it is called by the record support's C routine in +pass 0. +The device support may allocate memory for the VAL field's array (enough space +for NELM elements of type FTVA) from its own memory pool if desired, and store +the pointer to this buffer in the BPTR field. +The record will use C for this memory allocation if BPTR has not been +set by this routine. +The routine must return 0 for success, -1 or a error status on failure. + +Since EPICS 7.0.5 if this routine returns 2 in pass 0, it will be called again +in pass 1 with the PACT field set to 2. +In pass 0 the PACT field is set to zero (FALSE). =head4 get_ioint_info @@ -485,7 +501,8 @@ provided for any device type that can use the ioEvent scanner. long read_aai(dbCommon *precord) -This routine must provide a new input value. It returns the following values: +This routine should provide a new input value. +It returns the following values: =over @@ -501,16 +518,15 @@ Other: Error. =head3 Device Support For Soft Records -The C<<< Soft Channel >>> device support module is provided to read values from -other records and store them in arrays. If INP is a constant link, then read_aai -does nothing. In this case, the record can be used to hold arrays written via -dbPuts. If INP is a database or channel access link, the new array value is read -from the link. NORD is set. +The C<<< Soft Channel >>> device support is provided to read values from other +records via the INP link, or to hold array values that are written into it. -This module places a value directly in VAL and NORD is set to the number of items -in the array. +If INP is a constant link the array value gets loaded from the link constant by +the C routine, which also sets NORD. +The C routine does nothing in this case. -If the INP link type is constant, then NORD is set to zero. +If INP is a database or channel access link, the C routine gets a +new array value from the link and sets NORD. =cut } From 6754404d0fc71b805c1825bfea42f7fc2356ad30 Mon Sep 17 00:00:00 2001 From: Andrew Johnson Date: Sun, 28 Feb 2021 15:02:27 -0600 Subject: [PATCH 5/5] Replace magic "2" with macro AAI_DEVINIT_PASS1 --- modules/database/src/std/dev/devAaiSoft.c | 4 ++-- modules/database/src/std/rec/aaiRecord.c | 6 +++--- modules/database/src/std/rec/aaiRecord.dbd.pod | 11 ++++++----- 3 files changed, 11 insertions(+), 10 deletions(-) diff --git a/modules/database/src/std/dev/devAaiSoft.c b/modules/database/src/std/dev/devAaiSoft.c index aafdd1bcd..f2e4a41f9 100644 --- a/modules/database/src/std/dev/devAaiSoft.c +++ b/modules/database/src/std/dev/devAaiSoft.c @@ -48,8 +48,8 @@ static long init_record(dbCommon *pcommon) DBLINK *plink = &prec->inp; /* Ask record to call us in pass 1 instead */ - if (prec->pact != 2) { - return 2; + if (prec->pact != AAI_DEVINIT_PASS1) { + return AAI_DEVINIT_PASS1; } if (dbLinkIsConstant(plink)) { diff --git a/modules/database/src/std/rec/aaiRecord.c b/modules/database/src/std/rec/aaiRecord.c index 157a48f5b..756b892ab 100644 --- a/modules/database/src/std/rec/aaiRecord.c +++ b/modules/database/src/std/rec/aaiRecord.c @@ -119,9 +119,9 @@ static long init_record(struct dbCommon *pcommon, int pass) if (pdset->common.init_record) { long status = pdset->common.init_record(pcommon); - if (status == 2) { + if (status == AAI_DEVINIT_PASS1) { /* requesting pass 1 callback, remember to do that */ - prec->pact = 2; + prec->pact = AAI_DEVINIT_PASS1; } else if (status) return status; @@ -134,7 +134,7 @@ static long init_record(struct dbCommon *pcommon, int pass) return 0; } - if (prec->pact == 2) { + if (prec->pact == AAI_DEVINIT_PASS1) { /* device support asked for an init_record() callback in pass 1 */ long status = pdset->common.init_record(pcommon); if (status) diff --git a/modules/database/src/std/rec/aaiRecord.dbd.pod b/modules/database/src/std/rec/aaiRecord.dbd.pod index b3dd19216..969f5800f 100644 --- a/modules/database/src/std/rec/aaiRecord.dbd.pod +++ b/modules/database/src/std/rec/aaiRecord.dbd.pod @@ -155,8 +155,8 @@ If device support includes an C routine it is called, but unlike most record types this occurs in pass 0, which allows the device support to allocate the array buffer itself. -Since EPICS 7.0.5 the device support may return 2 to request a second call to -its C routine in pass 1. +Since EPICS 7.0.5 the device support may return C to request +a second call to its C routine in pass 1. Checks if device support allocated array space. If not, space for the array is allocated using NELM and FTVL. The array address is stored in BPTR. @@ -299,10 +299,11 @@ Scan forward link if necessary, set PACT FALSE, and return. %/* Declare Device Support Entry Table */ %struct aaiRecord; %typedef struct aaidset { - % dset common; /*init_record returns: (-1,0,2)=>(failure,success,callback)*/ + % dset common; /*init_record returns: (-1,0,AAI_DEVINIT_PASS1)=>(failure,success,callback)*/ % long (*read_aai)(struct aaiRecord *prec); /*returns: (-1,0)=>(failure,success)*/ %} aaidset; %#define HAS_aaidset + %#define AAI_DEVINIT_PASS1 2 % field(VAL,DBF_NOACCESS) { prompt("Value") @@ -484,8 +485,8 @@ The record will use C for this memory allocation if BPTR has not been set by this routine. The routine must return 0 for success, -1 or a error status on failure. -Since EPICS 7.0.5 if this routine returns 2 in pass 0, it will be called again -in pass 1 with the PACT field set to 2. +Since EPICS 7.0.5 if this routine returns C in pass 0, it +will be called again in pass 1 with the PACT field set to C. In pass 0 the PACT field is set to zero (FALSE). =head4 get_ioint_info