[Rprotobuf-commits] r484 - in pkg: . inst inst/unitTests src

[noreply at r-forge.r-project.org]

Author: murray
Date: 2012-10-03 21:03:43 +0200 (Wed, 03 Oct 2012)
New Revision: 484

Modified:
   pkg/ChangeLog
   pkg/inst/NEWS.Rd
   pkg/inst/unitTests/runit.golden.message.R
   pkg/src/mutators.cpp
Log:
Fix another bug in mutators.cpp where LENGTH() is used on non-vectors
causing non-deterministic behavior because memory from random
location.  I think I've caught all bugs of this class in mutators.cpp
now, should have looked more carefully the first time I spotted this
bug in this file a few months ago.

This particular bug was triggered whenever set a repeated message
field in a protocol buffer to a single (not vector) Message object
(e.g. a STRSXP, not a VECSXP, hence LENGTH(x) was returning garbage).



Modified: pkg/ChangeLog
===================================================================
--- pkg/ChangeLog	2012-09-26 03:07:34 UTC (rev 483)
+++ pkg/ChangeLog	2012-10-03 19:03:43 UTC (rev 484)
@@ -1,3 +1,12 @@
+2012-10-03  Murray Stokely  <murray at FreeBSD.org>
+
+	* src/mutators.cpp (rprotobuf): Fix bug where LENGTH() is used on
+	  non-vectors when setting a repeated message field to a single
+	  Message object.  This caused non-deterministic behavior after
+	  memory was read from a random location.
+	* inst/unitTests/runit.golden.message.R (test.repeatedFields): Add
+	  tests for this case.
+
 2012-09-25  Dirk Eddelbuettel  <edd at debian.org>
 
 	* man/is_extension.Rd: Use \dontrun on examples as we cannot reload

Modified: pkg/inst/NEWS.Rd
===================================================================
--- pkg/inst/NEWS.Rd	2012-09-26 03:07:34 UTC (rev 483)
+++ pkg/inst/NEWS.Rd	2012-10-03 19:03:43 UTC (rev 484)
@@ -11,7 +11,9 @@
       \item fix a bug where NAs were silently treated as TRUE for logical/bool types
       \item fix a bug that caused crashes when adding vectors to optional fields
       \item fix bugs in readASCII that returned empty protocol buffers when the file or connection could not be opened
-      \item distinguish between non-existant and not-set fieldswith has() by returning NULL in the former case.
+      \item distinguish between non-existant and not-set fieldswith
+      has() by returning NULL in the former case.
+      \item fix a bug that caused non-deterministic behavior when setting a repeated message field in a protobuf to a single Message.
       \item add unit tests for all of the above.
     }
   }

Modified: pkg/inst/unitTests/runit.golden.message.R
===================================================================
--- pkg/inst/unitTests/runit.golden.message.R	2012-09-26 03:07:34 UTC (rev 483)
+++ pkg/inst/unitTests/runit.golden.message.R	2012-10-03 19:03:43 UTC (rev 484)
@@ -60,6 +60,26 @@
 	test <- new(protobuf_unittest.TestAllTypes)
 	test$add("repeated_int32", c(1:5))
 	checkEquals(test$repeated_int32, c(1:5))
+
+	test$repeated_int32 <- 1
+	checkEquals(test$repeated_int32, 1)
+
+        # Prior to RProtoBuf v0.2.5, this was not handled properly.
+        test.2 <- new(protobuf_unittest.TestAllTypes,
+                      repeated_string=c("foo", "bar"))
+        checkEquals(test.2$repeated_string, c("foo", "bar"))
+
+        # Versions of RProtoBuf <= 0.2.5 had non-deterministic behavior due to a
+        # memory management bug when setting a repeated field to a
+        # non-vector type (e.g. a Message).
+	test$repeated_foreign_message <- list(new(protobuf_unittest.ForeignMessage,
+						  c = 1),
+					      new(protobuf_unittest.ForeignMessage,
+						  c = 2))
+	checkEquals(length(test$repeated_foreign_message), 2)
+	test$repeated_foreign_message <- new(protobuf_unittest.ForeignMessage,
+					     c = 3)
+	checkEquals(length(test$repeated_foreign_message), 1)
 }
 
 test.repeated.bools <- function() {

Modified: pkg/src/mutators.cpp
===================================================================
--- pkg/src/mutators.cpp	2012-09-26 03:07:34 UTC (rev 483)
+++ pkg/src/mutators.cpp	2012-10-03 19:03:43 UTC (rev 484)
@@ -257,7 +257,7 @@
 void CHECK_values_for_enum( GPB::FieldDescriptor* field_desc, SEXP value ){
 	
 	const GPB::EnumDescriptor* enum_desc = field_desc->enum_type() ;
-    
+	// N.B. n undefined if TYPEOF(value) not a vector, but we catch that below.
 	int n = LENGTH(value) ;
 	
     switch( TYPEOF( value ) ){
@@ -389,7 +389,7 @@
 		// {{{ repeated fields
 		
 		// {{{ preliminary checks
-		int value_size = LENGTH(value);
+		int value_size = Rf_isVector(value) ? LENGTH(value) : 1;
 		// if the R type is RAWSXP and the cpp type is string or bytes, 
 		// then value_size is actually one because the raw vector
 		// is converted to a string
@@ -410,7 +410,7 @@
 			}
 		}
 		// }}}
-		
+		// The number of elements already in the repeated field.
 		int field_size = ref->FieldSize( *message, field_desc ) ;
 		
 		/* {{{ in case of messages or enum, we have to check that all values