Coverity is a persistent beast. Hot on the tails of fixing one leak,

it notices a slightly earlier leak.
(Which also suggests that marking bugs as RESOLVED isn't a great plan,
as it's not clear whether that calls off the dogs of war. I'd much
prefer it to be forced to scan again, and give a positve "all clear"
(or more accurately "nowt wrong I can see with that"))

p4raw-id: //depot/perl@27876

diff --git a/ext/Storable/Storable.xs b/ext/Storable/Storable.xs
index b77853c..e620b4b 100644 (file)
--- a/ext/Storable/Storable.xs
+++ b/ext/Storable/Storable.xs
@@ -656,6 +656,17 @@ static stcxt_t *Context_ptr = NULL;
        }                                                               \
   } STMT_END
 
+#define MBUF_SAFEPVREAD(x,s,z)                 \
+  STMT_START {                                 \
+       if ((mptr + (s)) <= mend) {             \
+               memcpy(x, mptr, s);             \
+               mptr += s;                      \
+       } else {                                \
+               Safefree(z);                    \
+               return (SV *) 0;                \
+       }                                       \
+  } STMT_END
+
 #define MBUF_PUTC(c)                           \
   STMT_START {                                         \
        if (mptr < mend)                                \
@@ -986,6 +997,16 @@ static const char byteorderstr_56[] = {BYTEORDER_BYTES_56, 0};
        }                                                                                       \
   } STMT_END
 
+#define SAFEPVREAD(x,y,z)                                      \
+  STMT_START {                                                 \
+       if (!cxt->fio)                                          \
+               MBUF_SAFEPVREAD(x,y,z);                         \
+       else if (PerlIO_read(cxt->fio, x, y) != y)       {      \
+               Safefree(z);                                    \
+               return (SV *) 0;                                \
+       }                                                       \
+  } STMT_END
+
 /*
  * This macro is used at retrieve time, to remember where object 'y', bearing a
  * given tag 'tagnum', has been retrieved. Next time we see an SX_OBJECT marker,
@@ -3963,6 +3984,7 @@ static SV *retrieve_blessed(pTHX_ stcxt_t *cxt, const char *cname)
        SV *sv;
        char buf[LG_BLESS + 1];         /* Avoid malloc() if possible */
        char *classname = buf;
+       char *malloced_classname = NULL;
 
        TRACEME(("retrieve_blessed (#%d)", cxt->tagnum));
        ASSERT(!cname, ("no bless-into class given here, got %s", cname));
@@ -3979,8 +4001,9 @@ static SV *retrieve_blessed(pTHX_ stcxt_t *cxt, const char *cname)
                RLEN(len);
                TRACEME(("** allocating %d bytes for class name", len+1));
                New(10003, classname, len+1, char);
+               malloced_classname = classname;
        }
-       READ(classname, len);
+       SAFEPVREAD(classname, len, malloced_classname);
        classname[len] = '\0';          /* Mark string end */
 
        /*
@@ -3990,8 +4013,7 @@ static SV *retrieve_blessed(pTHX_ stcxt_t *cxt, const char *cname)
        TRACEME(("new class name \"%s\" will bear ID = %d", classname, cxt->classnum));
 
        if (!av_store(cxt->aclass, cxt->classnum++, newSVpvn(classname, len))) {
-               if (classname != buf)
-                       Safefree(classname);
+               Safefree(malloced_classname);
                return (SV *) 0;
        }
 
@@ -4000,8 +4022,8 @@ static SV *retrieve_blessed(pTHX_ stcxt_t *cxt, const char *cname)
         */
 
        sv = retrieve(aTHX_ cxt, classname);    /* First SV which is SEEN will be blessed */
-       if (classname != buf)
-               Safefree(classname);
+       if (malloced_classname)
+               Safefree(malloced_classname);
 
        return sv;
 }
@@ -4153,6 +4175,7 @@ static SV *retrieve_hook(pTHX_ stcxt_t *cxt, const char *cname)
                 * on the stack.  Just like retrieve_blessed(), we limit the name to
                 * LG_BLESS bytes.  This is an arbitrary decision.
                 */
+               char *malloced_classname = NULL;
 
                if (flags & SHF_LARGE_CLASSLEN)
                        RLEN(len);
@@ -4162,9 +4185,10 @@ static SV *retrieve_hook(pTHX_ stcxt_t *cxt, const char *cname)
                if (len > LG_BLESS) {
                        TRACEME(("** allocating %d bytes for class name", len+1));
                        New(10003, classname, len+1, char);
+                       malloced_classname = classname;
                }
 
-               READ(classname, len);
+               SAFEPVREAD(classname, len, malloced_classname);
                classname[len] = '\0';          /* Mark string end */
 
                /*
@@ -4172,8 +4196,7 @@ static SV *retrieve_hook(pTHX_ stcxt_t *cxt, const char *cname)
                 */
 
                if (!av_store(cxt->aclass, cxt->classnum++, newSVpvn(classname, len))) {
-                       if (classname != buf)
-                               Safefree(classname);
+                       Safefree(malloced_classname);
                        return (SV *) 0;
                }
        }