dbPutString can leave strings unterminated

Bug #1563191 reported by Bruce Hill
This bug affects 1 person
Affects Status Importance Assigned to Milestone
Fix Released
Fix Released
Fix Released
Fix Released

Bug Description

If a db file initializes a stringin or stringout record w/ a constant string > 40 char,
the string does not get terminated.

This, coupled w/ a strcpy(prec->oval,prec->val) in the init_record() function of
both stringin and stringout can crash the ioc if the memory in oval wasn't pre-initialized to 0's.

The problem code is in dbStatic/dbStaticLib.c in dbPutString():
    switch (pflddes->field_type) {
    case DBF_STRING:
 if(!pfield) return(S_dbLib_fieldNotFound);
 strncpy((char *)pfield, pstring,pflddes->size);

should be
        strncpy((char *)pfield, pstring, pflddes->size-1 );
        ((char *)pfield)[pflddes->size] = 0;

It might also be a good idea to change the strcpy calls in stringinRecord and stringoutRecord to strncpy.

Revision history for this message
mdavidsaver (mdavidsaver) wrote :

Attempted a fix with http://bazaar.launchpad.net/~epics-core/epics-base/3.14/revision/12620. This moves the field size test up before the string copy. Also made the change to "size-1".

Bruce could you please verify that this fixes the crash you see? If it doesn't please attach a .db file demonstrating the issue. This is clearly a bug, but from a quick test I couldn't actually cause a crash.

Revision history for this message
Andrew Johnson (anj) wrote :

Bruce, how did you trigger this bug — by setting DOL to a too-long string in a .db file, or did the string come from some device support at runtime?

Setting a too-long DOL string triggers a different strcpy() buffer overflow in recGblInitConstantLink() for me, which Michael's commit doesn't fix.

I can't replicate your problem by setting the VAL field directly in a .db file, that gets rejected when the database is loaded. The other place where we call dbPutString() is in dbPutFieldLink() but that seems to protected already, my attempts to caput a too-long string to a link field get ignored (without reporting any errors anywhere though).

Revision history for this message
Andrew Johnson (anj) wrote :

I just committed a fix for recGblInitConstantLink() to the 3.14 branch, it's the one after Micheal's.

Revision history for this message
Bruce Hill (bhill) wrote : Re: [Bug 1563191] Re: dbPutString can leave strings unterminated

Yes, Michael's patch fixes it and I can confirm the bug still
exists if I ifdef back to the prior version of dbStaticLib.c

I'm not sure why I haven't seen this before, as I'm sure I've
run into this issue of the stringin or stringout initializer being
over 40 characters before.

Perhaps we haven't see this problem before as the memory segment that
the record gets created in is probably all zeroes in many cases.

In one test yesterday I left the string unterminated by using the
existing dbStaticLib.c code, but changed the stringin init_record()
to use strncpy. It didn't crash, but a dbpr showed VAL as
w/ OVAL as

The db file I tested w/ is just this, loaded after my usual db files:
record(stringin, "$(P)$(R)CamModelScreen") {
   field(VAL, "arvScreens/Basler_acA1300-30gm-features.edl")
   field(PINI, "YES")

The IOC is based on, w/ a dynamically linked RHEL5 executable
using ADCore, ffmpegServer, ADProsilica, aravisGigE, and quite a few other
related modules including our EVR module. The host is running kernel
2.6.18-164.15.1.el5 #1 SMP and has several other IOC's running on it.
Most of our ioc's have used static linking, so perhaps the dynamic linking
in this ioc makes a difference, as we've been using this kernel for years.

I can confirm that I get an error msg on dbLoadRecords:
dbLoadRecords( db/j.db "P=TST:GIGE:BASLER2,R=:,PORT=CAM" )
Can't set "TST:GIGE:BASLER2:CamModelScreen.VAL" to "arvScreens/Basler_acA1300-30gm-features.edl"
Error at or before ")" in file "db/j.db" line 3

but the unterminated val array has already been created even though it doesn't crash
until the iocInit() call.

- Bruce

Bruce Hill
Member Technical Staff
SLAC National Accelerator Lab
2575 Sand Hill Road M/S 10
Menlo Park, CA 94025

Revision history for this message
Andrew Johnson (anj) wrote :

Thanks Bruce, that makes sense now. I just posted a patch to the 3.14.12 Known Problems page which includes both Michael's and my fixes.

Andrew Johnson (anj)
no longer affects: epics-base/3.12
no longer affects: epics-base/3.13
To post a comment you must log in.
This report contains Public information  
Everyone can see this information.

Other bug subscribers

Remote bug watches

Bug watches keep track of this bug in other bug trackers.