Experimental Physics and
Industrial Control System

Andrew Johnson <[email protected]> · Thu, 03 Jun 2010 16:38:44 +0100

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