review of Agoric additions to xsnap

[dckc]

No description provided.

[dckc]

prereqs:

1. seccomp - snapshots over pipes rather than file put xsnap worker in a seccomp jail #2386
2. end-of-crank w.r.t. liveslots doing gc

[dckc]

@warner writes May 7, 2021, 11:36 PM:

you might look at our handler in

agoric-sdk/packages/xsnap/src/xsnap.c

Lines 1467 to 1498 in 79df84a

	static void fx_issueCommand(xsMachine *the)
	{
	int argc = xsToInteger(xsArgc);
	if (argc < 1) {
	mxTypeError("expected ArrayBuffer");
	}
	size_t length;
	char* buf = NULL;
	txSlot* arrayBuffer = &xsArg(0);
	length = fxGetArrayBufferLength(the, arrayBuffer);
	buf = malloc(length);
	fxGetArrayBufferData(the, arrayBuffer, 0, buf, length);
	int writeError = fxWriteNetString(toParent, "?", buf, length);
	free(buf);
	if (writeError != 0) {
	xsUnknownError(fxWriteNetStringError(writeError));
	}
	// read netstring
	size_t len;
	int readError = fxReadNetString(fromParent, &buf, &len);
	if (readError != 0) {
	xsUnknownError(fxReadNetStringError(readError));
	}
	xsResult = xsArrayBuffer(buf, len);
	}

@phoddie writes May 8, 2021, 12:28 AM:

• Generally don't take the address of slots - &xsArg(0) as used here is safe, but not always. Instead of calling fxGetArrayBufferLegth and fxGetArrayBufferData use xsGetArrayBufferLegth and xsGetArrayBufferData which accept the slot directly.

• Your call to malloc doesn't check for failure. Probably that's safe in your world. In our world, it would be a concern.

• The buffer allocated by fxReadString is orphaned. It is passed to xsArrayBuffer which copies the data, but doesn't take ownership of the pointer. There should be a free(buf) at the end of the function.

• In fxReadNetString, if there is a failure the buffer is freed. But then the freed pointer is then assigned to *dest. As long as everyone checks the return value, that's not a problem but.... seems unnecessarily risky.

p.s. I presume it's OK to share this.

cc @kriskowal

[dckc]

#3101 is landed but @warner expressed an interest in a more thorough review.

[dckc]

We should probably clean up these warnings. I'm not quite sure how to approach some of them:
https://gist.github.com/jessysaurusrex/e28c5ec0230c50c8af3adce8dee26fa5

p.s. for follow-up, see #2466