Experimental Physics and
Industrial Control System

<[email protected]> · Fri, 4 Jun 2010 09:27:35 +0100

From: Andrew Johnson [mailto:[email protected]] 
> > I'm in the middle of reworking read_reply() (rsrv/camessage.c) to 
> > implement support for autoresizing arrays (I hope to post my patches 
> > later 
> > this week), and I've encountered a curious bit of special case code 
> > that 
> > I'm about to erase ... so thought I'd better ask about it.
> 
> Ooh, careful...
> 
> > I'm puzzled by this commit (I've edited out all the rest of the diff, 
> > as it doesn't affect my changes):
> > 
> > revno: 10992
> > committer: Jeff Hill <[email protected]>
> > branch nick: B3.14
> > timestamp: Fri 2007-09-07 17:43:52 +0000
> > message:
> >   fix for mantis entry 300:
> >   'assert (size <= ntohs ( pMsg->m_postsize ))' failed in 
> > ..caserverio.c line 344
> > diff:
> > === modified file 'src/rsrv/camessage.c'
> > --- src/rsrv/camessage.c        2007-08-17 22:31:11 +0000
> > +++ src/rsrv/camessage.c        2007-09-07 17:43:52 +0000
> > @@ -586,16 +598,25 @@
> >              */
> >              if ( pevext->msg.m_dataType == DBR_STRING 
> >                  && pevext->msg.m_count == 1 ) {
> > -                /* add 1 so that the string terminator will be 
> > shipped */
> > -                strcnt = strlen ( (char *) pPayload ) + 1;
> > -                msgSize = strcnt;
> > +                char * pStr = (char *) pPayload;
> > +                size_t strcnt = strlen ( pStr );
> > +                if ( strcnt < payloadSize ) {
> > +                    payloadSize = ( ca_uint32_t ) ( strcnt + 1u );
> > +                }
> > +                else {
> > +                    pStr[payloadSize-1] = '\0';
> > +                    errlogPrintf ( 
> > +                        "caserver: read_reply: detected DBR_STRING 
> > w/o nill termination "
> > +                        "in response from db_get_field, 
> pPayload = \"%s\"\n",
> > +                        pStr );
> > +                }
> >              }
> >          }
> >          else {
> > -            memset ( pPayload, 0, msgSize );
> > +            memset ( pPayload, 0, payloadSize );
> >              cas_set_header_cid ( pClient, cacStatus );
> >             }
> > -        cas_commit_msg ( pClient, msgSize );
> > +        cas_commit_msg ( pClient, payloadSize );
> >      }
> >  
> >      /*
> > 
> > 
> > I'm quite baffled.  Is there any reason not to *always* ship the 
> > entire 40 characters of the EPICS string as a matter of course?  
> > Or is this a fine grained optimisation to avoid shipping an 
> > extra average 20 bytes?
> 
> String fields on the database side are not limited to 40 
> characters long; the db_access API can't zero-terminate the 
> source buffer.  Make sure that this code isn't protecting 
> against that kind of condition, which is easiest to test 
> using a record with a name longer than 40 characters and 
> accessing its .NAME field.

Ouch. OUCH.

So I take it that such long string handling only applies to single strings, not to arrays of strings?  Is there a limit on the length of such strings?

Unfortunately I see two nasty problems here, unless I'm missing something.

Firstly, the call to the client database goes through dbGetField() which provides no information about the actual buffer size, merely about the number of elements -- so I *have* to conclude that there's a hard-wired implicit limit on the string length somewhere in this API, and presumably the dbGetField() call must have to guarantee to null terminate the string it returns!

So let me see if I've got this right:  

dbGetField(dbaddr, DBR_STRING, buffer, popt, pn, pfl) copies up to MAX_STRING_LEN (some hard-wired constant I've not yet discovered) NULL terminated bytes to buffer[] if and ONLY IF *popt == 0 and *pn == 1.  If *pn > 1 then standard EPICS strings are copied, if *popt != 0 only predefined attributes are copied.  Is this right?  Is this specified anywhere?

I'm looking at the EPICS Application Developer's Guide, and I see some odd things.  The guide implies that the value is always copied, but the code for db_get_field would generate a stack overwrite if that was the case.  I see reference to a dbBufferSize routine in the documentation, but this is not used anywhere in EPICS base, so I imagine it's bogus.  I can find no reference to this horrible long string handling you mention.

Secondly, I can't see where enough room is allocated in the buffer passed to db_get_field() from read_field().  As far as I can tell, the buffer size allocation is computed by dbr_size_n() which, unless I'm hugely mistaken, allocates 40 bytes for a string.

My guess is that we get away with this (most of the time) because the buffer we use normally has plenty of room...  What am I missing?  This would seem a particularly evil Heisenbug if I'm reading this right.

> You also have the Mantis bug number that this was fixing, so 
> you should read the thread at 
> https://bugs.launchpad.net/epics-base/+bug/mantis-300 to 
> follow that discussion, in case it helps.
> 
> Jeff will probably not be able to read email until he gets 
> back to LANL next week.
> 
> - Andrew
> 
> PS: Could you re-post your patches by publishing a Bazaar 
> branch and proposing a merge?  It's going to have to happen 
> eventually, and it's easier to look at them that way IMHO...

Oh, ok.  I'll have to rework the string handling in db_get_field anyway, but I clearly need to understand it a lot better first.

-- 
This e-mail and any attachments may contain confidential, copyright and or privileged material, and are for the use of the intended addressee only. If you are not the intended addressee or an authorised recipient of the addressee please notify us of receipt by returning the e-mail and do not use, copy, retain, distribute or disclose the information in or attached to the e-mail.
Any opinions expressed within this e-mail are those of the individual and not necessarily of Diamond Light Source Ltd. 
Diamond Light Source Ltd. cannot guarantee that this e-mail or any attachments are free from viruses and we cannot accept liability for any damage which you may sustain as a result of software viruses which may be transmitted in or with the message.
Diamond Light Source Limited (company no. 4375679). Registered in England and Wales with its registered office at Diamond House, Harwell Science and Innovation Campus, Didcot, Oxfordshire, OX11 0DE, United Kingdom

Experimental Physics and Industrial Control System

Experimental Physics and
Industrial Control System