Skip to content
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.

Sign up for GitHub

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

PHP : Dispatch available descriptor specs in js_open_process function #963

Closed
wants to merge 5 commits into from
Closed

PHP : Dispatch available descriptor specs in js_open_process function #963

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
aadd426
Dispatch available descrpitor specs in proc open
Jan 20, 2024
f3af333
Dispatch available descriptor specs in proc open
Jan 20, 2024
db2edf7
Merge branch 'trunk' into dispatch-available-descriptor-spec-in-proc-…
mho22 Jan 20, 2024
1fe1aed
Dispatch available descriptor specs in proc open
Jan 21, 2024
2d1674c
Dispatch available descriptor specs in proc open
Jan 21, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
69 changes: 47 additions & 22 deletions packages/php-wasm/compile/php/php_wasm.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,18 +47,18 @@ int wasm_pclose_ret = -1;
/**
* Passes a message to the JavaScript module and writes the response
* data, if any, to the response_buffer pointer.
*
*
* @param message The message to pass into JavaScript.
* @param response_buffer The address where the response will be stored. The
* JS module will allocate a memory block for the response buffer and write
* its address to **response_buffer. The caller is responsible for freeing
* its address to **response_buffer. The caller is responsible for freeing
* that memory after use.
*
*
* @return The size of the response_buffer (it can contain null bytes).
*
* @note The caller should ensure that the memory allocated for response_buffer
* is freed after its use to prevent memory leaks. It's also recommended
* to handle exceptions and errors gracefully within the function to ensure
*
* @note The caller should ensure that the memory allocated for response_buffer
* is freed after its use to prevent memory leaks. It's also recommended
* to handle exceptions and errors gracefully within the function to ensure
* the stability of the system.
*/
extern size_t js_module_onMessage(const char *data, char **response_buffer);
Expand All @@ -71,13 +71,14 @@ extern size_t js_module_onMessage(const char *data, char **response_buffer);
// This wasm_popen function is called by PHP_FUNCTION(popen) thanks
// to a patch applied in the Dockerfile.
//
// The `js_popen_to_file` is defined in phpwasm-emscripten-library.js.
// The `js_popen_to_file` is defined in phpwasm-emscripten-library.js.
// It runs the `cmd` command and returns the path to a file that contains the
// output. The exit code is assigned to the exit_code_ptr.

EMSCRIPTEN_KEEPALIVE FILE *wasm_popen(const char *cmd, const char *mode)
{
FILE *fp;

if (*mode == 'r') {
uint8_t last_exit_code;
char *file_path = js_popen_to_file(cmd, mode, &last_exit_code);
Expand All @@ -90,8 +91,10 @@ EMSCRIPTEN_KEEPALIVE FILE *wasm_popen(const char *cmd, const char *mode)
int current_procopen_call_id = ++procopen_call_id;
char *device_path = js_create_input_device(current_procopen_call_id);
int stdin_childend = current_procopen_call_id;
int stdin_parentend = open(device_path, O_WRONLY);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Interesting! What does this line change?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I actually add a child and parent in every descriptors found. No matter which one it is. So I had to find a parent end for the stdin in wasm_popen to use the same parameters for js_open_process. And when I analyzed the proc_open7.4.c file I found out that for the stdin parentend it was that. You can find this in proc_open7.4.c line 489. I copy pasted. I dont think the parentend is used in the js_open_process but it allows the function to work.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Gotcha! It seems like leaning on the multi-arg signature would remove the need for adding this line?

Copy link
Collaborator

@adamziel adamziel Jan 22, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, could the open() call be avoided by assigning NULL instead of stdin_parentend?

Copy link
Contributor Author

@mho22 mho22 Jan 22, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep ! I tried it right now and it works without that unused stdin_parentend. I will remove that part.

fp = fopen(device_path, mode);


php_file_descriptor_t stdout_pipe[2];
php_file_descriptor_t stderr_pipe[2];
if (0 != pipe(stdout_pipe) || 0 != pipe(stderr_pipe)) {
Expand All @@ -100,18 +103,41 @@ EMSCRIPTEN_KEEPALIVE FILE *wasm_popen(const char *cmd, const char *mode)
return 0;
}

int **descv = malloc(sizeof(int *) * 3);

int *stdin = malloc(sizeof(int) * 3);
int *stdout = malloc(sizeof(int) * 3);
int *stderr = malloc(sizeof(int) * 3);

stdin[0] = 0;
stdin[1] = stdin_childend;
stdin[2] = stdin_parentend;

stdout[0] = 1;
stdout[1] = stdout_pipe[0];
stdout[2] = stdout_pipe[1];

stderr[0] = 2;
stderr[1] = stderr_pipe[0];
stderr[2] = stderr_pipe[1];

descv[0] = stdin;
descv[1] = stdout;
descv[2] = stderr;

// the wasm way {{{
js_open_process(
cmd,
stdin_childend,
// stdout. @TODO: Pipe to /dev/null
stdout_pipe[0],
stdout_pipe[1],
// stderr. @TODO: Pipe to /dev/null
stderr_pipe[0],
stderr_pipe[1]
cmd,
descv,
Copy link
Collaborator

@adamziel adamziel Jan 22, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice! TIL you can pass arrays like this.

Edit: A-ha, you still decode that on the JS side by looping over the memory. Gotcha!

Copy link
Collaborator

@adamziel adamziel Jan 22, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thinking about this more, this seems to add quite a few lines of code and some indirection that needs to be mentally processed when reading the code. What do you think about preserving the long js_open_process signature and, whenever a descriptor is missing, passing NULLs directly as function args?

Copy link
Contributor Author

@mho22 mho22 Jan 22, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes! What is passed is the array pointer in C, and I had to loop over the memory to find all the data in JS.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the php method, you can add more or less than 3 descriptors. If you want to have the same behavior, this will need a dynamic array. Keeping the old way will prevent adding new descriptors, but you're the boss. :).

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A-ha! I missed the fact PHP actually uses those additional descriptors. Indeed this is what the documentation says:

The file descriptor numbers are not limited to 0, 1 and 2 - you may specify any valid file descriptor number and it will be passed to the child process. This allows your script to interoperate with other scripts that run as "co-processes". In particular, this is useful for passing passphrases to programs like PGP, GPG and openssl in a more secure manner. It is also useful for reading status information provided by those programs on auxiliary file descriptors.

This is really a good spot, thank you! Let's use the array, then. I just checked, and this can actually be handled by Playground as Node.js child_process.spawn() supports arbitrary file descriptors via the stdio option.

3
);
// }}}

free(stdin);
free(stdout);
free(stderr);

free(descv);
}
else
{
Expand All @@ -130,12 +156,12 @@ EMSCRIPTEN_KEEPALIVE FILE *wasm_popen(const char *cmd, const char *mode)
* * passthru()
* * system()
* * shell_exec()
*
* The wasm_php_exec function is called thanks
*
* The wasm_php_exec function is called thanks
* to -Dphp_exec=wasm_php_exec in the Dockerfile and also a
* small patch that removes php_exec and marks wasm_php_exec()
* small patch that removes php_exec and marks wasm_php_exec()
* as external.
*
*
* {{{
*/

Expand Down Expand Up @@ -169,7 +195,7 @@ static size_t handle_line(int type, zval *array, char *buf, size_t bufl) {
* will throw an EWOULDBLOCK error when trying to read from a
* blocking pipe. This function overrides that behavior and
* instead waits for the pipe to become readable.
*
*
* @see https://github.com/WordPress/wordpress-playground/issues/951
* @see https://github.com/emscripten-core/emscripten/issues/13214
*/
Expand Down Expand Up @@ -1616,4 +1642,3 @@ int EMSCRIPTEN_KEEPALIVE del_callback(zend_function *fptr)
return vrzno_del_callback(fptr);
}
#endif

187 changes: 113 additions & 74 deletions packages/php-wasm/compile/php/phpwasm-emscripten-library.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -243,21 +243,11 @@ const LibraryExample = {
* purposes of PHP's proc_open() function.
*
* @param {int} command Command to execute (string pointer).
* @param {int} stdinFd Child process end of the stdin pipe (the one to read from).
* @param {int} stdoutChildFd Child process end of the stdout pipe (the one to write to).
* @param {int} stdoutParentFd PHP's end of the stdout pipe (the one to read from).
* @param {int} stderrChildFd Child process end of the stderr pipe (the one to write to).
* @param {int} stderrParentFd PHP's end of the stderr pipe (the one to read from).
* @param {int} descriptors Descriptor specs (int array pointer, [ number, child, parent ] ).
* @param {int} descriptorsLength Descriptor length.
* @returns {int} 0 on success, 1 on failure.
*/
js_open_process: function (
command,
stdinFd,
stdoutChildFd,
stdoutParentFd,
stderrChildFd,
stderrParentFd
) {
js_open_process: function (command, descriptors, descriptorsLength) {
if (!PHPWASM.proc_fds) {
PHPWASM.proc_fds = {};
}
Expand All @@ -270,6 +260,20 @@ const LibraryExample = {
return 0;
}

if (descriptorsLength < 2) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This could use a comment to explain why short-circuit in this case. Is this what native PHP does? Or is this a limitation of this implementation?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh no, I added this condition because I thought it had no sense to add only one descriptor. Sorry, subjective thought. What I know too is that when you add only one element in a dynamic array, the array has to add 16 to find the first element in memory. Let me give you an explanation about this : [ elements added in array -> pointer offset ] :

1 -> 16
2 -> 16
3 -> 16
4 -> 24
5 -> 24
6 -> 32
7 -> 32
8 -> 48
9 -> 48
...

This means you can understand how to find the correct data in the memory even if it is dynamic. But there is a minimum of 16. And that if( descriptorsLength < 2 ) allows to avoid 'if one element then + 16 instead of 8.

I hope I explained this well.

Copy link
Collaborator

@adamziel adamziel Jan 22, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh no, I added this condition because I thought it had no sense to add only one descriptor. Sorry, subjective thought.

All good! I just checked and it seems like you can even pass zero descriptors and proc_open will still work:

$pipes = []; proc_open('echo "test123" > out', [], $pipes);

return 1;
}
var pointersStart = descriptors + ((descriptorsLength >> 1) + 1) * 8;

var std = {};
for (var i = 0; i < descriptorsLength; i++) {
var ptr = pointersStart + i * 16;
std[HEAPU8[ptr]] = {
child: HEAPU8[ptr + 4],
parent: HEAPU8[ptr + 8],
};
}

let cp;
try {
cp = PHPWASM.spawnProcess(cmdstr);
Expand Down Expand Up @@ -318,51 +322,84 @@ const LibraryExample = {
}
};
}
PHPWASM.proc_fds[stdoutParentFd] = new EventEmitter();
PHPWASM.proc_fds[stdoutParentFd].stdinFd = stdinFd;
PHPWASM.proc_fds[stderrParentFd] = new EventEmitter();
PHPWASM.proc_fds[stderrParentFd].stdinFd = stdinFd;

const stdoutStream = SYSCALLS.getStreamFromFD(stdoutChildFd);
cp.on('exit', function (data) {
PHPWASM.proc_fds[stdoutParentFd].exited = true;
PHPWASM.proc_fds[stdoutParentFd].emit('data');
PHPWASM.proc_fds[stderrParentFd].exited = true;
PHPWASM.proc_fds[stderrParentFd].emit('data');

if (std[1]) {
PHPWASM.proc_fds[std[1].parent] = new EventEmitter();
if (std[0]) {
PHPWASM.proc_fds[std[1].parent].stdinFd = std[0].child;
}
}

if (std[2]) {
PHPWASM.proc_fds[std[2].parent] = new EventEmitter();
if (std[0]) {
PHPWASM.proc_fds[std[2].parent].stdinFd = std[0].child;
}
}

const stdoutStream = std[1]
? SYSCALLS.getStreamFromFD(std[1].child)
: null;
cp.on('exit', function () {
if (std[1]) {
PHPWASM.proc_fds[std[1].parent].exited = true;
PHPWASM.proc_fds[std[1].parent].emit('data');
}

if (std[2]) {
PHPWASM.proc_fds[std[2].parent].exited = true;
PHPWASM.proc_fds[std[2].parent].emit('data');
}
});

// Pass data from child process's stdout to PHP's end of the stdout pipe.
cp.stdout.on('data', function (data) {
PHPWASM.proc_fds[stdoutParentFd].hasData = true;
PHPWASM.proc_fds[stdoutParentFd].emit('data');
stdoutStream.stream_ops.write(
stdoutStream,
data,
0,
data.length,
0
);
if (std[1]) {
PHPWASM.proc_fds[std[1].parent].hasData = true;
PHPWASM.proc_fds[std[1].parent].emit('data');
}

if (stdoutStream) {
stdoutStream.stream_ops.write(
stdoutStream,
data,
0,
data.length,
0
);
}
});

// Pass data from child process's stderr to PHP's end of the stdout pipe.
const stderrStream = SYSCALLS.getStreamFromFD(stderrChildFd);
const stderrStream = std[2]
? SYSCALLS.getStreamFromFD(std[2].child)
: null;
cp.stderr.on('data', function (data) {
PHPWASM.proc_fds[stderrParentFd].hasData = true;
PHPWASM.proc_fds[stderrParentFd].emit('data');
stderrStream.stream_ops.write(
stderrStream,
data,
0,
data.length,
0
);
if (std[2]) {
PHPWASM.proc_fds[std[2].parent].hasData = true;
PHPWASM.proc_fds[std[2].parent].emit('data');
}

if (stderrStream) {
stderrStream.stream_ops.write(
stderrStream,
data,
0,
data.length,
0
);
}
});

// Pass data from stdin fd to child process's stdin.
if (PHPWASM.input_devices && stdinFd in PHPWASM.input_devices) {
if (
PHPWASM.input_devices &&
std[0] &&
std[0].child in PHPWASM.input_devices
) {
// It is a "pipe". By now it is listed in `callback_pipes`.
// Let's listen to anything it outputs and pass it to the child process.
PHPWASM.input_devices[stdinFd].onData(function (data) {
PHPWASM.input_devices[std[0].child].onData(function (data) {
if (!data) return;
const dataStr = new TextDecoder('utf-8').decode(data);
cp.stdin.write(dataStr);
Expand All @@ -372,37 +409,39 @@ const LibraryExample = {

// It is a file descriptor.
// Let's pass the already read contents to the child process.
const stdinStream = SYSCALLS.getStreamFromFD(stdinFd);
if (!stdinStream.node) {
return 0;
}

// Pipe the entire stdinStream to cp.stdin
const CHUNK_SIZE = 1024;
const buffer = Buffer.alloc(CHUNK_SIZE);
let offset = 0;

while (true) {
const bytesRead = stdinStream.stream_ops.read(
stdinStream,
buffer,
0,
CHUNK_SIZE,
offset
);
if (bytesRead === null || bytesRead === 0) {
break;
}
try {
cp.stdin.write(buffer.subarray(0, bytesRead));
} catch (e) {
console.error(e);
return 1;
if (std[0]) {
const stdinStream = SYSCALLS.getStreamFromFD(std[0].child);
if (!stdinStream.node) {
return 0;
}
if (bytesRead < CHUNK_SIZE) {
break;

// Pipe the entire stdinStream to cp.stdin
const CHUNK_SIZE = 1024;
const buffer = Buffer.alloc(CHUNK_SIZE);
let offset = 0;

while (true) {
const bytesRead = stdinStream.stream_ops.read(
stdinStream,
buffer,
0,
CHUNK_SIZE,
offset
);
if (bytesRead === null || bytesRead === 0) {
break;
}
try {
cp.stdin.write(buffer.subarray(0, bytesRead));
} catch (e) {
console.error(e);
return 1;
}
if (bytesRead < CHUNK_SIZE) {
break;
}
offset += bytesRead;
}
offset += bytesRead;
}

return 0;
Expand Down
Loading
Loading