Hi Mark,
I noticed that interruptCallbackWaveform() will add a NULL terminator if less than "nelm" characters are copied, but callbackWfRead() etc. do not do this. I enclose a patch that adds this feature to them too, and also a couple of "paranoid" string termination checks elsewhere that may or may not be useful/necessary
Regards,
Freddie
> -----Original Message-----
> From: Mark Rivers [mailto:[email protected]]
> Sent: 07 November 2014 23:34
> To: Akeroyd, Freddie (STFC,RAL,ISIS); '[email protected]'
> Subject: RE: Asyn and stringin records
>
> As I was fixing the string termination problem that Freddie found, I
> discovered that there was another problem with interruptCallbackSi and
> interruptCallbackWaveform in devAsynOctet.c. Neither of those functions
> was locking the record with dbScanLock while the record fields were being
> modified. All of the other asyn device support does correctly lock records
> during the interrupt callbacks. The problem was only in devAsynOctet.c.
>
> I have fixed both problems and committed to SVN. The fixes will be in the
> R4-25 release of asyn. Meanwhile you can get them here:
>
> https://svn.aps.anl.gov/epics/asyn/trunk
>
> Mark
>
>
>
>
> -----Original Message-----
> From: Mark Rivers
> Sent: Friday, November 07, 2014 10:26 AM
> To: '[email protected]'; [email protected]
> Subject: RE: Asyn and stringin records
>
> Hi Freddie,
>
> This is the current code that handles the interrupt callbacks for the stringin
> record. You are definitely correct that it does not guaranteed that the string
> is null terminated.
>
> This is the code for periodic reads. It does guarantee that the string is null
> terminated.
>
> static void callbackSiRead(asynUser *pasynUser)
> {
> devPvt *pdevPvt = (devPvt *)pasynUser->userPvt;
> stringinRecord *psi = (stringinRecord *)pdevPvt->precord;
> size_t nBytesRead;
> asynStatus status;
> size_t len = sizeof(psi->val);
>
> status = readIt(pasynUser,psi->val,len,&nBytesRead);
> psi->time = pasynUser->timestamp;
> if(status==asynSuccess) {
> psi->udf = 0;
> if(nBytesRead==len) nBytesRead--;
> psi->val[nBytesRead] = 0;
> }
> finish((dbCommon *)psi);
> }
>
> This is the code for I/O Intr scanned records. It does NOT guarantee that the
> string is null terminated.
>
> static void interruptCallbackSi(void *drvPvt, asynUser *pasynUser,
> char *data,size_t numchars, int eomReason)
> {
> devPvt *pdevPvt = (devPvt *)drvPvt;
> stringinRecord *psi = (stringinRecord *)pdevPvt->precord;
> size_t num;
>
> pdevPvt->gotValue = 1;
> num = (numchars>=MAX_STRING_SIZE ? MAX_STRING_SIZE : numchars);
> strncpy(psi->val,data,num);
> psi->udf = 0;
> if(num<MAX_STRING_SIZE) psi->val[num] = 0;
> /* Set the status from pasynUser->auxStatus so I/O Intr scanned records
> can set alarms */
> if (pdevPvt->status == asynSuccess) pdevPvt->status = pasynUser-
> >auxStatus;
> psi->time = pasynUser->timestamp;
> scanIoRequest(pdevPvt->ioScanPvt);
> }
>
> I propose to change this to:
>
> static void interruptCallbackSi(void *drvPvt, asynUser *pasynUser,
> char *data,size_t numchars, int eomReason)
> {
> devPvt *pdevPvt = (devPvt *)drvPvt;
> stringinRecord *psi = (stringinRecord *)pdevPvt->precord;
> size_t maxChars = sizeof(psi->val)-1;
>
> pdevPvt->gotValue = 1;
> if (numchars > maxChars) numchars = maxChars;
> strncpy(psi->val,data,numchars);
> psi->val[numchars] = 0;
> psi->udf = 0;
> /* Set the status from pasynUser->auxStatus so I/O Intr scanned records
> can set alarms */
> if (pdevPvt->status == asynSuccess) pdevPvt->status = pasynUser-
> >auxStatus;
> psi->time = pasynUser->timestamp;
> scanIoRequest(pdevPvt->ioScanPvt);
> }
>
> Mark
>
> -----Original Message-----
> From: [email protected] [mailto:tech-talk-
> [email protected]] On Behalf Of [email protected]
> Sent: Friday, November 07, 2014 9:58 AM
> To: [email protected]
> Subject: Asyn and stringin records
>
> Hi,
>
> I have come across a difference in behaviour with asyn and the EPICS
> stringin record (asyn 4-22 / base 3.14.12.2 / win64) that occurs when the
> value received by the driver happens to be larger than the stringin
> maximum size. A record was changed from "periodic scan" to "I/O Intr"
> resulting in a crash due to a strcpy() (in monitor() from stringinRecord.c of
> base) being unable to find a NULL terminator and thus overwriting memory.
> Looking at devAsynOctet.c it seems that stringin records that are
> periodically scanned are always guaranteed to have a NULL terminated
> value, whereas ones processed via interruptCallbackSi() are NULL
> terminated only if the data received is less than 40 characters long (I noticed
> that in base 3.14.12.4 the stringinRecord.c monitor() routine now uses
> strncpy() and so does not require NULL termination). For my case I've just
> added NULL termination for "I/O Intr" too, but from looking at the stringin
> record reference manual page I wasn't sure if the ori!
> ginal I/O Instr stringin behaviour was actually more correct?
>
> Regards,
>
> Freddie
>
> --
> Scanned by iCritical.
--
Scanned by iCritical.
Index: devAsynOctet.c
===================================================================
--- devAsynOctet.c (revision 2455)
+++ devAsynOctet.c (working copy)
@@ -508,11 +508,12 @@
asynStatus status;
size_t nBytesRead;
long dbStatus;
- char raw[MAX_STRING_SIZE];
- char translate[MAX_STRING_SIZE];
+ char raw[MAX_STRING_SIZE+1];
+ char translate[MAX_STRING_SIZE+1];
size_t len = sizeof(psi->val);
dbStatus = dbGet(&pdevPvt->dbAddr,DBR_STRING,raw,0,0,0);
+ raw[MAX_STRING_SIZE] = 0;
if(dbStatus) {
asynPrint(pasynUser,ASYN_TRACE_ERROR,
"%s dbGet failed\n",psi->name);
@@ -573,12 +574,23 @@
return initDrvUser((devPvt *)pso->dpvt);
}
+/* implementation of strnlen() as i'm not sure it is available everywhere */
+static size_t my_strnlen(const char *str, size_t max_size)
+{
+ const char * end = (const char *)memchr(str, '\0', max_size);
+ if (end == NULL) {
+ return max_size;
+ } else {
+ return end - str;
+ }
+}
+
static void callbackSoWrite(asynUser *pasynUser)
{
devPvt *pdevPvt = (devPvt *)pasynUser->userPvt;
stringoutRecord *pso = (stringoutRecord *)pdevPvt->precord;
- writeIt(pasynUser,pso->val,strlen(pso->val));
+ writeIt(pasynUser,pso->val,my_strnlen(pso->val, sizeof(pso->val)));
finish((dbCommon *)pso);
}
@@ -601,12 +613,16 @@
waveformRecord *pwf = (waveformRecord *)pdevPvt->precord;
asynStatus status;
size_t nBytesRead;
+ char *pbuf = (char *)pwf->bptr;
status = writeIt(pasynUser,pdevPvt->buffer,pdevPvt->bufLen);
if(status==asynSuccess) {
status = readIt(pasynUser,pwf->bptr,(size_t)pwf->nelm,&nBytesRead);
pwf->time = pasynUser->timestamp;
- if(status==asynSuccess) pwf->nord = (epicsUInt32)nBytesRead;
+ if(status==asynSuccess) {
+ pwf->nord = (epicsUInt32)nBytesRead;
+ if (nBytesRead < pwf->nelm) pbuf[nBytesRead] = 0;
+ }
}
finish((dbCommon *)pwf);
}
@@ -631,10 +647,12 @@
asynStatus status;
size_t nBytesRead;
long dbStatus;
- char raw[MAX_STRING_SIZE];
- char translate[MAX_STRING_SIZE];
+ char raw[MAX_STRING_SIZE+1];
+ char translate[MAX_STRING_SIZE+1];
+ char *pbuf = (char *)pwf->bptr;
dbStatus = dbGet(&pdevPvt->dbAddr,DBR_STRING,raw,0,0,0);
+ raw[MAX_STRING_SIZE] = 0;
if(dbStatus) {
asynPrint(pasynUser,ASYN_TRACE_ERROR,
"%s dbGet failed\n",pwf->name);
@@ -647,7 +665,10 @@
if(status==asynSuccess) {
status = readIt(pasynUser,pwf->bptr,(size_t)pwf->nelm,&nBytesRead);
pwf->time = pasynUser->timestamp;
- if(status==asynSuccess) pwf->nord = (epicsUInt32)nBytesRead;
+ if(status==asynSuccess) {
+ pwf->nord = (epicsUInt32)nBytesRead;
+ if (nBytesRead < pwf->nelm) pbuf[nBytesRead] = 0;
+ }
}
finish((dbCommon *)pwf);
}
@@ -672,10 +693,14 @@
waveformRecord *pwf = (waveformRecord *)pdevPvt->precord;
size_t nBytesRead;
asynStatus status;
+ char *pbuf = (char *)pwf->bptr;
status = readIt(pasynUser,pwf->bptr,pwf->nelm,&nBytesRead);
pwf->time = pasynUser->timestamp;
- if(status==asynSuccess) pwf->nord = (epicsUInt32)nBytesRead;
+ if(status==asynSuccess) {
+ pwf->nord = (epicsUInt32)nBytesRead;
+ if (nBytesRead < pwf->nelm) pbuf[nBytesRead] = 0;
+ }
finish((dbCommon *)pwf);
}
- Replies:
- RE: Asyn and stringin records Mark Rivers
- References:
- Asyn and stringin records freddie.akeroyd
- RE: Asyn and stringin records Mark Rivers
- RE: Asyn and stringin records Mark Rivers
- Navigate by Date:
- Prev:
RE: Asyn and stringin records Mark Rivers
- Next:
Re: CA reference manual mystery function: ca_sg_pend Johnson, Andrew N.
- Index:
1994
1995
1996
1997
1998
1999
2000
2001
2002
2003
2004
2005
2006
2007
2008
2009
2010
2011
2012
2013
<2014>
2015
2016
2017
2018
2019
2020
2021
2022
2023
2024
- Navigate by Thread:
- Prev:
RE: Asyn and stringin records Mark Rivers
- Next:
RE: Asyn and stringin records Mark Rivers
- Index:
1994
1995
1996
1997
1998
1999
2000
2001
2002
2003
2004
2005
2006
2007
2008
2009
2010
2011
2012
2013
<2014>
2015
2016
2017
2018
2019
2020
2021
2022
2023
2024
|