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

Thread-safe Content.Chunk#slice #9141

Closed
gregw opened this issue Jan 9, 2023 · 2 comments · Fixed by #9142
Closed

Thread-safe Content.Chunk#slice #9141

gregw opened this issue Jan 9, 2023 · 2 comments · Fixed by #9142
Assignees
Labels
Bug For general bugs on Jetty side

Comments

@gregw
Copy link
Contributor

gregw commented Jan 9, 2023

Jetty version(s)
12

Description

The implementation of Content.Chunk#slice(int,int,boolean) is not threadsafe.

If multiple threads are slicing off the same chunk, then incorrect results may occur as each thread modifies the pointers of the original chunk. It should be able to use the ByteBuffer#slice(int,int) method to slice without modifying the original buffer.

@gregw gregw added the Bug For general bugs on Jetty side label Jan 9, 2023
@gregw
Copy link
Contributor Author

gregw commented Jan 9, 2023

I think the implementation can be something like:

        default Chunk slice(int position, int limit, boolean last)
        {
            ByteBuffer sourceBuffer = getByteBuffer();
            if (position == limit)
                return last && limit == sourceBuffer.limit() ? EOF : EMPTY;
            return from(sourceBuffer.slice(position, limit - position), last, this);
        }

Note that if we slice an empty chunk of a non empty last chunk, then I think it should be last only if it is slicing at the limit. Hence the extra check on EOF vs EMPTY.

sbordet added a commit that referenced this issue Jan 9, 2023
* Changed Content.Chunk.slice(int, int, boolean) to have the same parameters as ByteBuffer.slice(int, int) for consistency.
* Updated Chunk.slice(int, int, boolean) javadocs.
* Update code that was calling Chunk.slice(int, int, boolean).

Signed-off-by: Simone Bordet <simone.bordet@gmail.com>
@sbordet sbordet linked a pull request Jan 9, 2023 that will close this issue
sbordet added a commit that referenced this issue Jan 9, 2023
* Changed Content.Chunk.slice(int, int, boolean) to have the same parameters as ByteBuffer.slice(int, int) for consistency.
* Updated Chunk.slice(int, int, boolean) javadocs.
* Update code that was calling Chunk.slice(int, int, boolean).

Signed-off-by: Simone Bordet <simone.bordet@gmail.com>
@gregw
Copy link
Contributor Author

gregw commented Jan 9, 2023

Actually, the last is passed in, so it doesn't matter about the limit. The implementation should be just:

        default Chunk slice(int position, int limit, boolean last)
        {
            ByteBuffer sourceBuffer = getByteBuffer();
            if (position == limit)
                return last ? EOF : EMPTY;
            return from(sourceBuffer.slice(position, limit - position), last, this);
        }

sbordet added a commit that referenced this issue Jan 10, 2023
* Fixes #9141 - Thread-safe Content.Chunk#slice

* Changed Content.Chunk.slice(int, int, boolean) to have the same parameters as ByteBuffer.slice(int, int) for consistency.
* Updated Chunk.slice(int, int, boolean) javadocs.
* Update code that was calling Chunk.slice(int, int, boolean).

Signed-off-by: Simone Bordet <simone.bordet@gmail.com>
@sbordet sbordet closed this as completed Jan 10, 2023
gregpoulos pushed a commit to gregpoulos/jetty.project that referenced this issue Jan 16, 2023
…x-document-modules

* upstream/jetty-12.0.x:
  Issue jetty#9167 - making assumption in flaky test
  jetty 12.0.x cleanup duplicate osgi pom metadata (jetty#9093)
  Jetty 12 - Add tests in util/resource for alternate FileSystem implementations (jetty#9149)
  Cleanup non-retainable `Retainable`s (jetty#9159)
  Fixes retainability of special Chunks (jetty#9073)
  TCK: Dispatch forward and includes attributes do not meet the spec (jetty#9074)
  re-enable h3 tests (jetty#8773)
  More fundamental test case
  Reorganization of jetty-client classes. (jetty#9127)
  Removing @disabled from SslUploadTest
  Removing @disabled from jetty-start
  jetty#9134 added test
  ee10: DefaultServlet: Replace checks for isStreaming() by !isWriting()
  jetty#9078 make HttpContent.getByteBuffer() implementations return new ByteBuffer instances and document that contract
  Fixes jetty#9141 - Thread-safe Content.Chunk#slice (jetty#9142)
  Remove `@Disabled` from `jetty-jmx` (jetty#9143)
  Bump maven.version from 3.8.6 to 3.8.7
  Bump maven.version from 3.8.6 to 3.8.7
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug For general bugs on Jetty side
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants