Skip to content

Simplify loop in REST encoding and fix an off by one bug in the allocation #4950

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

Closed
wants to merge 1 commit into from

Conversation

herwinw
Copy link
Contributor

@herwinw herwinw commented Mar 31, 2023

The logic of the code was pretty much like this:

while (alloc <= limit) {
  // Allocate `alloc` bytes, move the old buffer over to the new buffer, free old buffer
  len = func(current + used, alloc - used, 1, userdata);
  if (!len) {
    return; // This means we're finished
  }
  alloc = alloc * 2; // Double the buffer size and try again
}
// If we get here, we ran out of buffer space and cause an error

This means once the last part has been written by func, the returned length is larger than zero, so we run the loop once again, allocate a bigger chunk of memory, move the contents and free the old buffer, then call func again to have it return a 0 immediately so we can return the buffer we already had in the previous iteration.

Another effect of this logic is that the limit is effectively cut in half: the limit argument is 8K, but if the output is more than 4K and less than 8K, the loop gets into another iteration, but now alloc is more than limit, so it gets into the error state because it thinks it does not have enough space. We ran into this issue with JSON encoding (which is the most verbose output in the module) and EAP-TLS (which add a lot of virtual attributes with information about the certificate).

@herwinw herwinw changed the title Simplify behaviour in REST encoding Simplify loop in REST encoding and fix an off by one bug in the allocation Mar 31, 2023
@herwinw herwinw force-pushed the rlm_rest_size_limit branch 3 times, most recently from 727c70d to e4b6b69 Compare March 31, 2023 11:47
@arr2036
Copy link
Member

arr2036 commented Mar 31, 2023

Those functions get passed to CURLOPT_READFUNCTION so need that specific signature.

@herwinw herwinw force-pushed the rlm_rest_size_limit branch from e4b6b69 to 46d52fa Compare April 3, 2023 07:31
@herwinw
Copy link
Contributor Author

herwinw commented Apr 3, 2023

Those functions get passed to CURLOPT_READFUNCTION so need that specific signature.

I've removed the first commit, so it keeps the void pointers, and just added one more cast to rlm_rest_request_t*

@arr2036
Copy link
Member

arr2036 commented Apr 3, 2023

Final issue is we need commits to be signed for this branch. Can you sign it, then I'd be happy to merge. Thanks!

@herwinw herwinw force-pushed the rlm_rest_size_limit branch from 46d52fa to 60a1a84 Compare April 3, 2023 16:34
The old code still ran one more loop once the request has been finished.
This not only included a new memory allocation + move + free, but it
also resulted in the limit argument being effectively cut in half.
@herwinw herwinw force-pushed the rlm_rest_size_limit branch 2 times, most recently from 2e27980 to 81f61e7 Compare April 3, 2023 16:42
@arr2036
Copy link
Member

arr2036 commented Apr 3, 2023

It's now come up with a signature error, saying the commit email doesn't match the GPG signature email and still won't let me merge the commit :(

@herwinw
Copy link
Contributor Author

herwinw commented Apr 3, 2023

It's now come up with a signature error, saying the commit email doesn't match the GPG signature email and still won't let me merge the commit :(

I know, I've been trying to push a new signed commit a few times, but it keeps returning this error, even though they're the same addresses.
I'll just keep continue trying, it might accidentally match some time.

@arr2036
Copy link
Member

arr2036 commented Apr 4, 2023

Did you add your GPG key to GitHub? That may the issue if not...

@mcnewton
Copy link
Member

Merged manually.

@mcnewton mcnewton closed this May 19, 2023
@herwinw herwinw deleted the rlm_rest_size_limit branch June 16, 2023 17:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants