[Rprotobuf-commits] r484 - in pkg: . inst inst/unitTests src
noreply at r-forge.r-project.org
noreply at r-forge.r-project.org
Wed Oct 3 21:03:43 CEST 2012
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
More information about the Rprotobuf-commits
mailing list