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.

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

Possible typo in examples (LWS_PRE bytes padding after buffer end) #2629

Closed
nick87720z opened this issue Apr 29, 2022 · 10 comments
Closed

Possible typo in examples (LWS_PRE bytes padding after buffer end) #2629

nick87720z opened this issue Apr 29, 2022 · 10 comments

Comments

@nick87720z
Copy link
Contributor

While learning earlier example about sending dynamic http, I noticed, that end of writeable area inside buffer is also padded by LWS_PRE, althoug total buffe size is LWS_PRE + 2048 - this means, at most 2048 - LWS_PRE bytes are sent by single lws_write(). If such padding was required, would be more logical to name it like LWS_POST or so. My guess, end value was meaned to be like start + sizeof(buf) - LWS_PRE - 1 or just buf + sizeof(buf) - 1 (without -LWS_PRE in second case).

@lws-team
Copy link
Member

It's a bit hard to understand what you're describing without a diff or links to the lines, but the http-server-dynamic cb buffer looks like this

	uint8_t buf[LWS_PRE + 2048], *start = &buf[LWS_PRE], *p = start,
		*end = &buf[sizeof(buf) - LWS_PRE - 1];

That is we can write starting from LWS_PRE bytes inside the buffer, and we have 2048 bytes there we can use, starting at p / start..

You should note well that with a 16-byte LWS_PRE, sizeof(buf) is going to be 2064 bytes. So between start and end + 1 will be 2048 bytes. So unless I miss the point, it is correct already.

@nick87720z
Copy link
Contributor Author

Must be either end = &buf[ sizeof(buf) - 1] or end = &start[ sizeof(buf) - LWS_PRE - 1 ]. If shorter - either replace &buf with &start or remove - LWS_PRE in the end initializer. I see, that real data space is meaned to be 2048. Setting end to end of entire buf (as I propose) would make this to be true: start - end == 2048, while current code makes it rather start - end = 2048 - LWS_PRE.

@lws-team
Copy link
Member

Yes the real usable space is 2048 starting at start, with LWS_PRE reserved for lws to add framing behind that. start is passed into the api.

I don't think anything needs fixing here. If it still feels wrong, you can prove it by printing the pointer offsets / sizeof arithmetic numbers.

@nick87720z
Copy link
Contributor Author

nick87720z commented Apr 29, 2022

Shift by LWS_PRE must be applied only to start, which would make end - start result to sizeof(buf) - LWS_PRE. However, current code shifts end pointer back by same value, which sets end - start to sizeof(buf) - 2 * LWS_PRE.
Is later how it must be?

Schematically: how it must be:

buf      start               end = buf + sizeof(buf)
 |         |===== data =======|
 [=LWS_PRE=|=== 2048 bytes ===]
 |====== sizeof( buf) ========|

And how it is now:

buf      start     end       buf + sizeof(buf)
 |         |= data =|=LWS_PRE=]
 [=LWS_PRE=|=== 2048 bytes ===]
 |====== sizeof( buf) ========|

Which doesn't look correct.

@lws-team
Copy link
Member

Mm but look closely, end is set referenced to buf, not start. So it must allow for the reserved LWS_PRE area from buf. Please print out the real pointers to satisfy yourself (or me, if I am wrong) of this.

@nick87720z
Copy link
Contributor Author

nick87720z commented Apr 29, 2022

Mm but look closely, end is set referenced to buf, not start

This is why it should NOT be decreased by LSW_PRE. Subtraction would be needed only with start as base - to keep end at actual buffer end. Second case (how it's now) demonstrates that.

Here's one more side for my explanation:
if start is shifted by +x and end is shifted by +y, than total changee for end - start range is +(y - x). Substituting x = LWS_PRE and y = -LWS_PRE, we get y - x => -LWS_PRE - LWS_PRE => -2 * LWS_PRE (correct is -LWS_PRE).

  • Need to slightly edit previous (extending graphs).

@nick87720z
Copy link
Contributor Author

nick87720z commented Apr 29, 2022

I got exact measures - pointers and diffs (they are per-byte pointers):

Code, right after declaration:

        fprintf( stderr, "DEBUG: buf=%p start=%p: end=%p\n: (start - buf)=%zi: (end - buf)=%zi: (end - start)=%zi\n",
                 buf, start, end,
                 start - buf, end - buf, end - start);

Output:

DEBUG: buf=0x7ffc156a9bd0 start=0x7ffc156a9be0: end=0x7ffc156aa3cf
: (start - buf)=16: (end - buf)=2047: (end - start)=2031

@lws-team
Copy link
Member

Oh... I think you are right... end needs to be the end. Thanks for showing it clearly.

@nick87720z
Copy link
Contributor Author

nick87720z commented Apr 29, 2022

Some other examples are correct. I did not check all, only this one. And one, which is correct, is minimal-http-server-form-post-lwsac (just took first line after grep in http-server dir).
More exactly - these http examples have same mistake (just looked for same pattern at callback start):

minimal-http-server-h2-long-poll
minimal-http-server-custom-headers
minimal-http-server-dynamic-edit

lws-team pushed a commit that referenced this issue May 3, 2022
Several examples trim their buffer with an extra LWS_PRE from the end...
actually end should point to end the end of buf without a second LWS_PRE
reservation.

#2629
lws-team pushed a commit that referenced this issue May 3, 2022
Several examples trim their buffer with an extra LWS_PRE from the end...
actually end should point to end the end of buf without a second LWS_PRE
reservation.

#2629
lws-team pushed a commit that referenced this issue May 4, 2022
Several examples trim their buffer with an extra LWS_PRE from the end...
actually end should point to end the end of buf without a second LWS_PRE
reservation.

#2629
lws-team pushed a commit that referenced this issue May 4, 2022
Several examples trim their buffer with an extra LWS_PRE from the end...
actually end should point to end the end of buf without a second LWS_PRE
reservation.

#2629
@lws-team
Copy link
Member

lws-team commented May 4, 2022

A patch for this is on main now, thanks.

lws-team pushed a commit that referenced this issue May 17, 2022
Several examples trim their buffer with an extra LWS_PRE from the end...
actually end should point to end the end of buf without a second LWS_PRE
reservation.

#2629
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

No branches or pull requests

2 participants