-
Notifications
You must be signed in to change notification settings - Fork 4
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
BUG: A connection
object may become corrupt and re-referenced to another connection (PATCH)
#81
Comments
A proof-of-concept patchBelow is a patch that will protect against the above issues, e.g. > fh <- file("a.txt", open = "w+")
> cat("hello\n", file = fh)
> close(fh)
> fh2 <- file("b.txt", open = "w+")
> cat("world\n", file = fh2)
> close(fh)
Error in close.connection(fh) :
invalid connection (non-existing 'conn_id')
> cat("hello again\n", file = fh)
Error in cat("hello again\n", file = fh) :
invalid connection (non-existing 'conn_id') It also protects against serialization of connections, e.g. > fh2 <- file("b.txt", open = "w+")
> saveRDS(fh2, file = "fh2.rds")
> fh3 <- readRDS("fh2.rds")
> fh2 == fh3
[1] TRUE
> identical(fh2, fh3)
[1] FALSE
> attr(fh2, "conn_id")
<pointer: 0x78>
> attr(fh3, "conn_id")
<pointer: (nil)> #<== NIL because external pointer was lost when serialized
> isOpen(fh2)
[1] TRUE
> isOpen(fh3)
Error in isOpen(fh3) : invalid connection ('conn_id' is NULL) PatchThis patch introduces a new Index: src/include/R_ext/Connections.h
===================================================================
--- src/include/R_ext/Connections.h (revision 75521)
+++ src/include/R_ext/Connections.h (working copy)
@@ -92,6 +92,7 @@
size_t R_ReadConnection(Rconnection con, void *buf, size_t n);
size_t R_WriteConnection(Rconnection con, void *buf, size_t n);
Rconnection R_GetConnection(SEXP sConn); // added in R 3.3.0
+Rconnection R_GetConnection2(SEXP sConn); // added in R 3.6.0
#ifdef __cplusplus
}
Index: src/library/tools/src/gramRd.c
===================================================================
--- src/library/tools/src/gramRd.c (revision 75521)
+++ src/library/tools/src/gramRd.c (working copy)
@@ -4435,9 +4435,11 @@
PushState();
- ifile = asInteger(CAR(args)); args = CDR(args);
+ ifile = asInteger(CAR(args));
- con = getConnection(ifile);
+ con = R_GetConnection2(CAR(args));
+ args = CDR(args);
+
wasopen = con->isopen;
source = CAR(args); args = CDR(args);
/* encoding is unused */
Index: src/library/utils/src/io.c
===================================================================
--- src/library/utils/src/io.c (revision 75521)
+++ src/library/utils/src/io.c (working copy)
@@ -352,7 +352,7 @@
error(_("invalid quote symbol set"));
i = asInteger(file);
- data.con = getConnection(i);
+ data.con = R_GetConnection2(file);
if(i == 0) {
data.ttyflag = 1;
} else {
@@ -852,7 +852,7 @@
data.skipNul = skipNul;
i = asInteger(file);
- data.con = getConnection(i);
+ data.con = R_GetConnection2(file);
data.ttyflag = (i == 0);
data.wasopen = data.con->isopen;
if(!data.wasopen) {
@@ -1076,7 +1076,8 @@
/* this is going to be a connection open or openable for writing */
if(!inherits(CAR(args), "connection"))
error(_("'file' is not a connection"));
- con = getConnection(asInteger(CAR(args))); args = CDR(args);
+ con = R_GetConnection2(CAR(args));
+ args = CDR(args);
if(!con->canwrite)
error(_("cannot write to this connection"));
wasopen = con->isopen;
Index: src/main/builtin.c
===================================================================
--- src/main/builtin.c (revision 75521)
+++ src/main/builtin.c (working copy)
@@ -565,7 +565,7 @@
file = CAR(args);
ifile = asInteger(file);
- con = getConnection(ifile);
+ con = R_GetConnection2(file);
if(!con->canwrite) /* if it is not open, we may not know yet */
error(_("cannot write to this connection"));
args = CDR(args);
Index: src/main/connections.c
===================================================================
--- src/main/connections.c (revision 75521)
+++ src/main/connections.c (working copy)
@@ -178,6 +178,21 @@
}
+Rconnection getConnection2(int n, void *id)
+{
+ Rconnection con = NULL;
+
+ if(n < 0 || n >= NCONNECTIONS || n == NA_INTEGER ||
+ !(con = Connections[n]))
+ error(_("invalid connection"));
+ if (!id)
+ error(_("invalid connection ('conn_id' is NULL)"));
+ else if (con->id != id)
+ error(_("invalid connection (non-existing 'conn_id')"));
+
+ return con;
+}
+
attribute_hidden
int getActiveSink(int n)
{
@@ -2894,7 +2909,7 @@
checkArity(op, args);
if(!inherits(CAR(args), "rawConnection"))
error(_("'con' is not a rawConnection"));
- con = getConnection(asInteger(CAR(args)));
+ con = R_GetConnection2(CAR(args));
if(!con->canwrite)
error(_("'con' is not an output rawConnection"));
this = con->private;
@@ -3318,7 +3333,7 @@
checkArity(op, args);
if(!inherits(CAR(args), "textConnection"))
error(_("'con' is not a textConnection"));
- con = getConnection(asInteger(CAR(args)));
+ con = R_GetConnection2(CAR(args));
if(!con->canwrite)
error(_("'con' is not an output textConnection"));
this = con->private;
@@ -3462,7 +3477,7 @@
if(!inherits(CAR(args), "connection"))
error(_("'con' is not a connection"));
i = asInteger(CAR(args));
- con = getConnection(i);
+ con = R_GetConnection2(CAR(args));
if(i < 3) error(_("cannot open standard connections"));
if(con->isopen) {
warning(_("connection is already open"));
@@ -3491,7 +3506,7 @@
int rw, res;
checkArity(op, args);
- con = getConnection(asInteger(CAR(args)));
+ con = R_GetConnection2(CAR(args));
rw = asInteger(CADR(args));
res = con->isopen != FALSE;
switch(rw) {
@@ -3510,7 +3525,7 @@
checkArity(op, args);
if(!inherits(CAR(args), "connection"))
error(_("'con' is not a connection"));
- con = getConnection(asInteger(CAR(args)));
+ con = R_GetConnection2(CAR(args));
return ScalarLogical(con->incomplete != FALSE);
}
@@ -3521,7 +3536,7 @@
checkArity(op, args);
if(!inherits(CAR(args), "connection"))
error(_("'con' is not a connection"));
- con = getConnection(asInteger(CAR(args)));
+ con = R_GetConnection2(CAR(args));
return ScalarLogical(con->canseek != FALSE);
}
@@ -3611,7 +3626,7 @@
error(_("cannot close 'output' sink connection"));
if(i == R_ErrorCon)
error(_("cannot close 'message' sink connection"));
- Rconnection con = getConnection(i);
+ Rconnection con = R_GetConnection2(CAR(args));
int status = con_close1(con);
free(Connections[i]);
Connections[i] = NULL;
@@ -3634,7 +3649,7 @@
checkArity(op, args);
if(!inherits(CAR(args), "connection"))
error(_("'con' is not a connection"));
- con = getConnection(asInteger(CAR(args)));
+ con = R_GetConnection2(CAR(args));
if(!con->isopen) error(_("connection is not open"));
where = asReal(CADR(args));
origin = asInteger(CADDR(args));
@@ -3657,7 +3672,7 @@
checkArity(op, args);
if(!inherits(CAR(args), "connection"))
error(_("'con' is not a connection"));
- con = getConnection(asInteger(CAR(args)));
+ con = R_GetConnection2(CAR(args));
con->truncate(con);
return R_NilValue;
}
@@ -3669,7 +3684,7 @@
checkArity(op, args);
if(!inherits(CAR(args), "connection"))
error(_("'con' is not a connection"));
- con = getConnection(asInteger(CAR(args)));
+ con = R_GetConnection2(CAR(args));
if(con->canwrite) con->fflush(con);
return R_NilValue;
}
@@ -3795,7 +3810,8 @@
checkArity(op, args);
if(!inherits(CAR(args), "connection"))
error(_("'con' is not a connection"));
- con = getConnection(asInteger(CAR(args))); args = CDR(args);
+ con = R_GetConnection2(CAR(args));
+ args = CDR(args);
n = asVecSize(CAR(args)); args = CDR(args);
if(n == -999)
error(_("invalid '%s' argument"), "n");
@@ -3923,7 +3939,7 @@
if(!inherits(CADR(args), "connection"))
error(_("'con' is not a connection"));
con_num = asInteger(CADR(args));
- con = getConnection(con_num);
+ con = R_GetConnection2(CADR(args));
sep = CADDR(args);
if(!isString(sep)) error(_("invalid '%s' argument"), "sep");
useBytes = asLogical(CADDDR(args));
@@ -4076,7 +4092,7 @@
bytes = RAW(CAR(args));
nbytes = XLENGTH(CAR(args));
} else {
- con = getConnection(asInteger(CAR(args)));
+ con = R_GetConnection2(CAR(args));
if(con->text) error(_("can only read from a binary connection"));
}
@@ -4335,7 +4351,7 @@
if(TYPEOF(CADR(args)) == RAWSXP) {
isRaw = TRUE;
} else {
- con = getConnection(asInteger(CADR(args)));
+ con = R_GetConnection2(CADR(args));
if(con->text) error(_("can only write to a binary connection"));
wasopen = con->isopen;
if(!con->canwrite) error(_("cannot write to this connection"));
@@ -4676,7 +4692,7 @@
bytes = RAW(CAR(args));
nbytes = LENGTH(CAR(args));
} else {
- con = getConnection(asInteger(CAR(args)));
+ con = R_GetConnection2(CAR(args));
if(!con->canread)
error(_("cannot read from this connection"));
}
@@ -4756,7 +4772,7 @@
if(TYPEOF(CADR(args)) == RAWSXP) {
isRaw = TRUE;
} else {
- con = getConnection(asInteger(CADR(args)));
+ con = R_GetConnection2(CADR(args));
if(!con->canwrite)
error(_("cannot write to this connection"));
wasopen = con->isopen;
@@ -4955,7 +4971,7 @@
stext = CAR(args);
if(!isString(stext))
error(_("invalid '%s' argument"), "data");
- con = getConnection(asInteger(CADR(args)));
+ con = R_GetConnection2(CADR(args));
newLine = asLogical(CADDR(args));
if(newLine == NA_LOGICAL)
error(_("invalid '%s' argument"), "newLine");
@@ -4994,7 +5010,7 @@
Rconnection con = NULL;
checkArity(op, args);
- con = getConnection(asInteger(CAR(args)));
+ con = R_GetConnection2(CAR(args));
return ScalarInteger(con->nPushBack);
}
@@ -5004,7 +5020,7 @@
Rconnection con = NULL;
checkArity(op, args);
- con = getConnection(asInteger(CAR(args)));
+ con = R_GetConnection2(CAR(args));
if(con->nPushBack > 0) {
for(j = 0; j < con->nPushBack; j++) free(con->PushBack[j]);
@@ -5173,12 +5189,14 @@
SEXP attribute_hidden
do_getconnection(SEXP call, SEXP op, SEXP args, SEXP env)
{
- SEXP ans, class;
+ SEXP input, conn_id, ans, class;
int what;
+ void *id;
Rconnection con;
checkArity(op, args);
- what = asInteger(CAR(args));
+ input = CAR(args);
+ what = asInteger(input);
if (what == NA_INTEGER)
error(_("there is no connection NA"));
if (what < 0 || what >= NCONNECTIONS || !Connections[what])
@@ -5185,6 +5203,18 @@
error(_("there is no connection %d"), what);
con = Connections[what];
+
+ if (what > 2) {
+ conn_id = getAttrib(input, R_ConnIdSymbol);
+ if (conn_id != R_NilValue) {
+ id = R_ExternalPtrAddr(conn_id);
+ if (!id)
+ error(_("invalid connection ('conn_id' is NULL)"));
+ else if (con->id != id)
+ error(_("invalid connection (non-existing 'conn_id')"));
+ }
+ }
+
PROTECT(ans = ScalarInteger(what));
PROTECT(class = allocVector(STRSXP, 2));
SET_STRING_ELT(class, 0, mkChar(con->class));
@@ -5203,6 +5233,7 @@
checkArity(op, args);
Rcon = getConnection(asInteger(CAR(args)));
+ /* Rcon = R_GetConnection2(CAR(args)); */
PROTECT(ans = allocVector(VECSXP, 7));
PROTECT(names = allocVector(STRSXP, 7));
SET_STRING_ELT(names, 0, mkChar("description"));
@@ -5516,6 +5547,22 @@
return getConnection(asInteger(sConn));
}
+Rconnection R_GetConnection2(SEXP sConn) {
+ int n;
+ SEXP conn_id;
+
+ if (!isInteger(sConn)) error(_("invalid connection (non-integer value)"));
+
+ n = asInteger(sConn);
+
+ if (!inherits(sConn, "connection")) return getConnection(n);
+
+ conn_id = getAttrib(sConn, R_ConnIdSymbol);
+ if (conn_id == R_NilValue) return getConnection(n);
+
+ return getConnection2(n, R_ExternalPtrAddr(conn_id));
+}
+
/* ------------------- (de)compression functions --------------------- */
/* Code for gzcon connections is modelled on gzio.c from zlib 1.2.3 */
@@ -5789,7 +5836,8 @@
checkArity(op, args);
if(!inherits(CAR(args), "connection"))
error(_("'con' is not a connection"));
- incon = getConnection(icon = asInteger(CAR(args)));
+ incon = R_GetConnection2(CAR(args));
+ icon = asInteger(CAR(args));
level = asInteger(CADR(args));
if(level == NA_INTEGER || level < 0 || level > 9)
error(_("'level' must be one of 0 ... 9"));
@@ -6045,7 +6093,7 @@
PROTECT(val = allocVector(LGLSXP, nsock));
for (i = 0; i < nsock; i++) {
- Rconnection conn = getConnection(asInteger(VECTOR_ELT(insock, i)));
+ Rconnection conn = R_GetConnection2(VECTOR_ELT(insock, i));
Rsockconn scp = conn->private;
if (strcmp(conn->class, "sockconn") != 0)
error(_("not a socket connection"));
Index: src/main/dcf.c
===================================================================
--- src/main/dcf.c (revision 75521)
+++ src/main/dcf.c (working copy)
@@ -88,6 +88,7 @@
file = CAR(args);
con = getConnection(asInteger(file));
+ con = R_GetConnection2(file);
wasopen = con->isopen;
if(!wasopen) {
if(!con->open(con)) error(_("cannot open the connection"));
Index: src/main/deparse.c
===================================================================
--- src/main/deparse.c (revision 75521)
+++ src/main/deparse.c (working copy)
@@ -386,6 +386,7 @@
int ifile = asInteger(CADR(args));
if (ifile != 1) {
Rconnection con = getConnection(ifile);
+ con = R_GetConnection2(CADR(args));
RCNTXT cntxt;
Rboolean wasopen = con->isopen;
if(!wasopen) {
@@ -472,6 +473,7 @@
}
else {
Rconnection con = getConnection(INTEGER(file)[0]);
+ con = R_GetConnection2(file);
Rboolean wasopen = con->isopen;
RCNTXT cntxt;
if(!wasopen) {
Index: src/main/saveload.c
===================================================================
--- src/main/saveload.c (revision 75521)
+++ src/main/saveload.c (working copy)
@@ -2335,6 +2335,7 @@
list = CAR(args);
con = getConnection(asInteger(CADR(args)));
+ con = R_GetConnection2(CADR(args));
if (TYPEOF(CADDR(args)) != LGLSXP)
error(_("'ascii' must be logical"));
@@ -2439,6 +2440,7 @@
checkArity(op, args);
con = getConnection(asInteger(CAR(args)));
+ con = R_GetConnection2(CAR(args));
wasopen = con->isopen;
if(!wasopen) {
Index: src/main/scan.c
===================================================================
--- src/main/scan.c (revision 75521)
+++ src/main/scan.c (working copy)
@@ -925,6 +925,7 @@
int ii = asInteger(file);
data.con = getConnection(ii);
+ data.con = R_GetConnection2(file);
if(ii == 0) {
data.atStart = FALSE;
data.ttyflag = 1;
Index: src/main/serialize.c
===================================================================
--- src/main/serialize.c (revision 75521)
+++ src/main/serialize.c (working copy)
@@ -2486,6 +2486,7 @@
object = CAR(args);
con = getConnection(asInteger(CADR(args)));
+ con = R_GetConnection2(CADR(args));
if (TYPEOF(CADDR(args)) != LGLSXP)
error(_("'ascii' must be logical"));
@@ -2554,6 +2555,7 @@
checkArity(op, args);
con = getConnection(asInteger(CAR(args)));
+ con = R_GetConnection2(CAR(args));
/* Now we need to do some sanity checking of the arguments.
A filename will already have been opened, so anything
@@ -2649,6 +2651,7 @@
SEXP (*hook)(SEXP, SEXP);
struct bconbuf_st bbs;
Rconnection con = getConnection(asInteger(icon));
+ con = R_GetConnection2(icon);
int version;
if (Sversion == R_NilValue)
@@ -2842,6 +2845,7 @@
}
else {
Rconnection con = getConnection(asInteger(icon));
+ con = R_GetConnection2(icon);
R_InitConnOutPStream(&out, con, type, version, hook, fun);
R_Serialize(object, &out);
return R_NilValue;
@@ -2869,6 +2873,7 @@
return R_Unserialize(&in);
} else {
Rconnection con = getConnection(asInteger(icon));
+ con = R_GetConnection2(icon);
R_InitConnInPStream(&in, con, R_pstream_any_format, hook, fun);
return R_Unserialize(&in);
}
Index: src/main/source.c
===================================================================
--- src/main/source.c (revision 75521)
+++ src/main/source.c (working copy)
@@ -221,9 +221,11 @@
R_ParseError = 0;
R_ParseErrorMsg[0] = '\0';
- ifile = asInteger(CAR(args)); args = CDR(args);
+ ifile = asInteger(CAR(args));
- con = getConnection(ifile);
+ con = R_GetConnection2(CAR(args));
+ args = CDR(args);
+
wasopen = con->isopen;
num = asInteger(CAR(args)); args = CDR(args);
if (num == 0) |
connection
object may become corrupt and re-referenced to another connectionconnection
object may become corrupt and re-referenced to another connection (PATCH)
I've posted a summary (with patch connections.patch.txt) of this to R-devel 'PATCH: Asserting that 'connection' used has not changed + R_GetConnection2()' on 2018-10-30. |
Background
In R, connections are indexed by their (zero-based) row indices in the table of available connections. For example,
Issue
The problem with the current implementation where connections are referred to by their index (only), is that (i) the table of connections changes over time and (ii) connection indices are recycled. Because an
connection
object holds the connection row index, it means that the actual underlying connection that aconnection
object refers to may change over its lifetime.For example, if we close the above
fh
connection (for writing to filea.txt
) and then create another connection 'fh2' (for writing to fileb.txt
), then bothfh
andfh2
will effectively refer to the same connection. This is a proof of concept code example:Next, in the same R session, continue by creating a new file connection:
So far so good, but because
fh
still refers to connection nbr 3, it will now work as an alias to the newfh2
connection;Suggestion
Internally, R connections are represented by an C-level
Rconn
struct. This structure has a fieldvoid *id
that initated at zero and then incremented by one (= +4 bytes) for each new connection created (viainit_con()
).Now, it turns out that the value of the unique
id
field is recorded in and exposed to us via (non-documented) attributeconn_id
through thevoid *ex_ptr
field (e.g. https://github.com/wch/r-source/blob/tags/R-3-5-1/src/main/connections.c#L5439-L5440 for file connections). For example,In other words, thanks to attribute
conn_id
aconnection
object does indeed carry all information needed to uniquely identify the underlying connection. It is just that the current R implementation does not make use of it (other than for garbage collection/finalizers).Patch
I've got a prototype where R internally asserts that each
connection
used refers to the same connection as it was originally created for by validating against the internalid
field. I'll post a patch "soon".References
The text was updated successfully, but these errors were encountered: