unix,windows: init all requests in fs calls #1509

cjihrig · 2017-08-25T14:08:44Z

Prior to this change, several of the fs functions checked for invalid arguments before initializing the fs request. If a consumer received a UV_EINVAL from one of these functions, and then called uv_fs_req_cleanup(), the application would crash, as the pointer being freed was not allocated. This commit makes sure that all fs functions initialize the request before returning.

Fixes: #1508

saghul · 2017-08-25T14:15:25Z

test/test-fs.c

  int r;

-  r = uv_fs_read(NULL, NULL, 0, NULL, 0, -1, NULL);
+  r = uv_fs_read(NULL, &read_req, 0, NULL, 0, -1, NULL);


hum, what should happen now if someone passes a NULL pointer then?

I guess nothing? Since uv_fs_cleanup would also do nothing if a NULL pointer is passed?

It crashes because none of the req init logic checks for NULL. That would happen for other calls too though. I think we should either crash consistently or update the init logic to handle NULL.

So the more I think about this, the more I think we should just assert on req for these functions. If we return UV_EINVAL in these cases, there is no way for the consumer to know if they can cleanup the request or not. It also seems pretty nonsensical to call any of these without a request.

@saghul I added a second commit to support NULL requests for all of the fs functions. The new logic is "if the request is NULL, immediately return UV_EINVAL. Otherwise, initialize the request before doing anything else." I think that should allow all requests to be cleaned up. PTAL

Please also add some tests which use a NULL req.

cjihrig · 2017-08-25T14:34:57Z

FWIW, green CI: https://ci.nodejs.org/view/libuv/job/libuv-test-commit/410/

cjihrig · 2017-08-30T12:33:07Z

@saghul ping

saghul

Sorry for the delay Colin! Left some comments.

saghul · 2017-09-01T08:53:01Z

src/unix/fs.c


 #define INIT(subtype)                                                         \
  do {                                                                        \
+    if (req == NULL)                                                          \


Then uv_fs_req_cleanup should check if req is null and not do anything: https://github.com/libuv/libuv/blob/v1.x/src/unix/fs.c#L1452

Same on Windows.

saghul · 2017-09-01T08:54:31Z

src/win/fs.c



 int uv_fs_close(uv_loop_t* loop, uv_fs_t* req, uv_file fd, uv_fs_cb cb) {
+  if (req == NULL)


I see every function looks like this now on Windows:

if (req == NULL) return UV_EINVAL; uv_fs_req_init(loop, req, UV_FS_FOO, cb);

Maybe use a macro to save some lines?

saghul · 2017-09-01T08:54:51Z

test/test-fs.c

  int r;

-  r = uv_fs_read(NULL, NULL, 0, NULL, 0, -1, NULL);
+  r = uv_fs_read(NULL, &read_req, 0, NULL, 0, -1, NULL);


Please also add some tests which use a NULL req.

cjihrig · 2017-09-02T19:22:31Z

@saghul addressed all of your comments.

CI: https://ci.nodejs.org/view/libuv/job/libuv-test-commit/422/

Update: CI was all 💚

saghul

LGTM. One minor nit (which you are free to ignore): I think I'd combine the last and 2nd commits into a single one.

Prior to this change, several of the fs functions checked for invalid arguments before initializing the fs request. If a consumer received a UV_EINVAL from one of these functions, and then called uv_fs_req_cleanup(), the application would crash, as the pointer being freed was not allocated. This commit makes sure that all fs functions initialize the request before returning. Fixes: libuv#1508 PR-URL: libuv#1509 Reviewed-By: Bartosz Sosnowski <[email protected]> Reviewed-By: Saúl Ibarra Corretgé <[email protected]>

This commit introduces an INIT macro to file system functions on Windows, similar to the one used on Unix platforms. The macro checks for NULL requests, and returns UV_EINVAL in such scenarios. This commit also adds support for passing NULL to uv_fs_req_cleanup(). In this scenario, the function is a no-op. Fixes: libuv#1508 PR-URL: libuv#1509 Reviewed-By: Bartosz Sosnowski <[email protected]> Reviewed-By: Saúl Ibarra Corretgé <[email protected]>

This commit adds a POST macro to the Windows fs functions, similar to the one used on Unix platforms. PR-URL: libuv#1509 Reviewed-By: Saúl Ibarra Corretgé <[email protected]>

Prior to this change, several of the fs functions checked for invalid arguments before initializing the fs request. If a consumer received a UV_EINVAL from one of these functions, and then called uv_fs_req_cleanup(), the application would crash, as the pointer being freed was not allocated. This commit makes sure that all fs functions initialize the request before returning. Fixes: #1508 PR-URL: #1509 Reviewed-By: Bartosz Sosnowski <[email protected]> Reviewed-By: Saúl Ibarra Corretgé <[email protected]>

This commit introduces an INIT macro to file system functions on Windows, similar to the one used on Unix platforms. The macro checks for NULL requests, and returns UV_EINVAL in such scenarios. This commit also adds support for passing NULL to uv_fs_req_cleanup(). In this scenario, the function is a no-op. Fixes: #1508 PR-URL: #1509 Reviewed-By: Bartosz Sosnowski <[email protected]> Reviewed-By: Saúl Ibarra Corretgé <[email protected]>

This commit adds a POST macro to the Windows fs functions, similar to the one used on Unix platforms. PR-URL: #1509 Reviewed-By: Saúl Ibarra Corretgé <[email protected]>

cjihrig · 2017-09-02T23:07:46Z

Squashed and landed as 7a0e64d, e539fc4, 9a4468f. Thanks!

saghul reviewed Aug 25, 2017

View reviewed changes

cjihrig mentioned this pull request Aug 25, 2017

fs: add fs.copyFile{Sync} nodejs/node#15034

Merged

4 tasks

bzoz approved these changes Aug 30, 2017

View reviewed changes

saghul requested changes Sep 1, 2017

View reviewed changes

cjihrig force-pushed the einvals branch from 0ce2f66 to 1d85807 Compare September 2, 2017 18:15

saghul approved these changes Sep 2, 2017

View reviewed changes

cjihrig force-pushed the einvals branch 2 times, most recently from 149d609 to d70385f Compare September 2, 2017 22:58

cjihrig added 3 commits September 2, 2017 19:01

windows: add POST macro to fs functions

b64a462

This commit adds a POST macro to the Windows fs functions, similar to the one used on Unix platforms. PR-URL: libuv#1509 Reviewed-By: Saúl Ibarra Corretgé <[email protected]>

cjihrig force-pushed the einvals branch from d70385f to b64a462 Compare September 2, 2017 23:03

cjihrig added a commit that referenced this pull request Sep 2, 2017

windows: add POST macro to fs functions

9a4468f

This commit adds a POST macro to the Windows fs functions, similar to the one used on Unix platforms. PR-URL: #1509 Reviewed-By: Saúl Ibarra Corretgé <[email protected]>

cjihrig closed this Sep 2, 2017

cjihrig deleted the einvals branch September 2, 2017 23:07

cjihrig mentioned this pull request Feb 16, 2018

src: unregister requests on invalid input #1747

Closed



		int uv_fs_close(uv_loop_t* loop, uv_fs_t* req, uv_file fd, uv_fs_cb cb) {
		if (req == NULL)

unix,windows: init all requests in fs calls #1509

unix,windows: init all requests in fs calls #1509

Uh oh!

Conversation

cjihrig commented Aug 25, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cjihrig commented Aug 25, 2017

Uh oh!

cjihrig commented Aug 30, 2017

Uh oh!

saghul left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cjihrig commented Sep 2, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

saghul left a comment

Choose a reason for hiding this comment

Uh oh!

cjihrig commented Sep 2, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

cjihrig commented Sep 2, 2017 •

edited

Loading