Lwt: can `getgrgid` and `getgrnam` handle ERANGE by retrying with a bigger buffer?

[dwang20151005]

I saw the following test failure:

[2020-09-04 12:53:37.834749-04:00] Testing library 'unix'...

[2020-09-04 12:53:37.834749-04:00] .................................SS.....................................................................................................EE.............................................................................
[2020-09-04 12:53:37.834749-04:00] Test 'getgrgid and Unix.getgrgid' in suite 'lwt_unix' raised 'Unix.Unix_error(Unix.ERANGE, "getgrgid", "")'

[2020-09-04 12:53:37.834749-04:00] Test 'getgrnam and Unix.getgrnam' in suite 'lwt_unix' raised 'Unix.Unix_error(Unix.ERANGE, "getgrnam", "tech")'

I think this is because lwt trusts sysconf(_SC_GETGR_R_SIZE_MAX) to
predict the buffer required, and does not handle ERANGE:

https://github.com/ocsigen/lwt/blob/master/src/unix/unix_c/unix_get_pw_gr_nam_id_job.c#L82

man 3 getgrgid suggests retrying with a bigger buffer:

The call

    sysconf(_SC_GETGR_R_SIZE_MAX)

returns  either -1, without changing errno, or an initial suggested size for buf.  (If
this size is too small, the call fails with ERANGE, in which case the caller can retry
with a larger buffer.)

This is what the corresponding Core code does:

https://github.com/janestreet/core/blob/master/src/core_unix.ml#L1880

I wish I could offer you a PR, but unix_get_pw_gr_nam_id_job.c is
more C than I can handle. I patched my lwt locally with a minimum
value I know to be sufficient for now:

diff --git a/external/lwt/src/unix/unix_get_pw_gr_nam_id_job.c b/external/lwt/src/unix/unix_get_pw_gr_nam_id_job.c
--- a/external/lwt/src/unix/unix_get_pw_gr_nam_id_job.c
+++ b/external/lwt/src/unix/unix_get_pw_gr_nam_id_job.c
@@ -81,6 +81,7 @@ static value alloc_group_entry(struct gr
     {                                                                  \
         size_t buffer_size = sysconf(_SC_##CONF##_R_SIZE_MAX);         \
         if (buffer_size == (size_t)-1) buffer_size = 16384;            \
+        if (buffer_size < 16 * 1024) buffer_size = 1024 * 16;          \
         job->buffer = (char *)lwt_unix_malloc(buffer_size);            \
         job->result = FUNC##_r(job->ARG, &job->entry, job->buffer,     \
                                buffer_size, &job->ptr);                \

[raphael-proust]

We should definitely retry with a bigger buffer. I don't know if your diff is the solution we want. I'd think we would like to retry after doubling (or some other factor) the size of the buffer.

The C part is also a bit much for me so I'm tempted to make a solution within the OCaml side.

[dwang20151005]

I don't know if your diff is the solution we want.

I really hope it is not!

I only meant, I found a quick hack that works for my use case, so no urgency from me on a fix upstream.

[aantron]

@raphael-proust This will be annoying to fix on the OCaml side, because the code is inside a worker thread.

The C is pretty straightforward in the worker thread function. It's just four lines of straight-line code. You can convert it to a loop that tries with ever-growing buffer sizes (with some bound) as long as it keeps getting ERANGE. Just don't forget to free buffers that turned out to be too small after each failing iteration :) @dwang20151005, could you try creating a PR along these lines?

[dwang20151005]

Sorry to be unhelpful, but, no, not if it requires me to learn C. I opened
the issue because it was easy for me to report the bug once I understood
it. I have a workaround that is good enough to continue with my work.

On Wed, Sep 16, 2020 at 11:24 AM Anton Bachin [e-mail]
wrote:

@raphael-proust https://github.com/raphael-proust This will be annoying
to fix on the OCaml side, because the code is inside a worker thread.

The C is pretty straightforward in the worker thread function. It's just
four lines of straight-line code. You can convert it to a loop that tries
with ever-growing buffer sizes (with some bound) as long as it keeps
getting ERANGE. Just don't forget to free buffers that turned out to be
too small after each failing iteration :) @dwang20151005
https://github.com/dwang20151005, could you try creating a PR along
these lines?

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
https://github.com/ocsigen/lwt/issues/803#issuecomment-693480161, or
unsubscribe
https://github.com/notifications/unsubscribe-auth/ADSJX7AGKN7CPNZNAXW4HMDSGDKEFANCNFSM4Q2GYE5Q
.

[raphael-proust]

I'll try to make a solution along those lines. I think I have enough C left in me for a draft.

FWIW, I started working on a solution where the OCaml side does the control flow (catching ERANGE and retrying) and the C side just takes one additional parameter (factor) which is used to multiply the buffer size. https://gist.github.com/raphael-proust/78ae49b40757b1475d9e4e18c51cfdab is the WIP, but it doesn't work and I'm guessing it has to do with either multiplication and different int representations or OCaml-C FFI argument passing.

[aantron]

@raphael-proust If I understood the OCaml solution correctly, then it has the drawback of doing multiple round trip communications with worker threads from the thread pool that actually run the system calls. It's also much more complicated than a simple loop, which is really ultimately the same as it would be in Java or any other language with "C-like" syntax, except for the need to deallocate too-small buffers manually. I'll be happy to do a review to help become sure in the C code :)

[raphael-proust]

@dwang20151005 Would you be able to test the fix in #808 ?

[dwang20151005]

Can you tell me the tip revision?

I'm happy to do the testing, but I'm afraid I don't typically use git or github. I figured out how to clone the repo. I don't know where a github pull request lists its tip revision. (I hope you understand what I mean by the tip of the pull request; google does not.)

[raphael-proust]

You can get a whole tarball at https://github.com/raphael-proust/lwt/archive/fix-getgrgid-getgrname.zip
Or you need to get to the fix-getgrgid-getgrname branch on my own fork of the project: [email protected]:raphael-proust/lwt.git
The HEAD is 19cc947308b294d74574005cddeac7547bc524a2

[dwang20151005]

No tests failed, but two tests in unix were skipped and I don't know how to show you which tests were skipped:

9:28:50 % dune build @default @runtest                      
File "test/ppx/main.ml", line 9, characters 0-46:
9 | let%lwt structure_let_result = Lwt.return true
    ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
Warning 22: let%lwt should not be used at the module item level.
Replace let%lwt x = e by let x = Lwt_main.run (e)
File "test/ppx/main.ml", line 9, characters 0-46:
9 | let%lwt structure_let_result = Lwt.return true
    ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
Warning 22: let%lwt should not be used at the module item level.
Replace let%lwt x = e by let x = Lwt_main.run (e)
        main alias test/core/runtest
Testing library 'core'...
..........................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................SSSSSSSSSSSSSSSSSSSSSSS................................................................................................................
Ok. 650 tests ran, 23 tests skipped in 0.01 seconds
        main alias test/ppx/runtest
Testing library 'ppx'...
................
Ok. 16 tests ran, 0 tests skipped in 1.20 seconds
        main alias test/react/runtest
Testing library 'react'...
...........
Ok. 11 tests ran, 0 tests skipped in 4.50 seconds
        main alias test/unix/runtest
Testing library 'unix'...
.................................SS....................................................................................................................................................................................
Ok. 213 tests ran, 2 tests skipped in 6.28 seconds
Done: 1085/1087 (jobs: 1)%                                                                            

[raphael-proust]

Based on the skipping policy within the test, I'm guessing that you you have Lwt_unix.IO_vectors.system_limit = None and that you skipped readv: iovecs exceeding limit and writev: iovecs exceeding limit.

The getgrgid and getgrnam tests are under an ~only_if:(fun () -> not Sys.win32) and there is a lot of tests like that so you'd have skipped much more than two if you had skipped those ones.

Thanks!

[dwang20151005]

Thanks for the quick fix! I don't know when you would close this issue (now that you have a fix? after the merge?) but I'll let you do it whenever you want.

[raphael-proust]

Merging the PR closed the issue because the description of the PR included Fixes #803 :)