dbPutString can leave strings unterminated

[Bruce Hill]

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.

[mdavidsaver (mdavidsaver)]

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.

[Andrew Johnson (anj)]

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).

[Andrew Johnson (anj)]

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

[Bruce Hill (bhill)]

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
arvScreens/Basler_acA1300-30gm-features.arvScreens/Basler_acA1300-30gm-features.
w/ OVAL as
arvScreens/Basler_acA1300-30gm-features.

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 3.14.12.4, 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.

Regards,
- Bruce

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

[Andrew Johnson (anj)]

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.