Experimental Physics and
Industrial Control System

"Jeff Hill" <[email protected]> · Tue, 22 Jun 2010 09:53:41 -0600

Hi Andrew,

Concerning mantis 300. Presumably, db_get_field wouldn?t return more than
MAX_STRING_SIZE characters for a DBR_STRING, and my current conjecture is
that this bug occurred because it wrote exactly MAX_STRING_SIZE characters,
with no nill termination at the end, into RSRV's buffer? Is that presumption
correct? Furthermore, wouldn?t the correct behavior be to (artificially) add
a nill termination to exactly the end of RSRV's MAX_STRING_SIZE buffer in
that situation? 

I do see Michael's confusion on this issue, as the code that I added is
nanny-ing the situation ( to use your terminology :-) and probably wouldn?t
be necessary if db_get_field was strictly obeying the terms of the interface
definition.

Admittedly, we have a weaker level of confirmation for this particular bug.
I did not personally reproduce the bug. I relied instead on publishing the
patch, and then receiving confirmation from the impacted user that it was in
fact fixing his issue.

Jeff
______________________________________________________
Jeffrey O. Hill           Email        [email protected]
LANL MS H820              Voice        505 665 1831
Los Alamos NM 87545 USA   FAX          505 665 5107

Message content: TSPA

> -----Original Message-----
> From: Andrew Johnson [mailto:[email protected]]
> Sent: Thursday, June 03, 2010 9:39 AM
> To: Michael Abbott
> Cc: EPICS core; Jeff Hill
> Subject: Re: Hacking rsrv/camessage.c
> 
> Hi Michael,
> 
> > 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.
> 
> 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...

Experimental Physics and Industrial Control System

Experimental Physics and
Industrial Control System