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

1 files changed, 31 insertions, 8 deletions

diff --git a/ext/Storable/Storable.xs b/ext/Storable/Storable.xs
index b77853c875..e620b4b8bc 100644
--- 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;
 		}
 	}