[UNDERTOW-2072] DefaultByteBufferPool.getBuffer() may return null #1333

baranowb · 2022-06-10T05:52:38Z

https://issues.redhat.com/browse/UNDERTOW-2072

e-ts · 2022-06-10T11:31:43Z

Hi @baranowb, are you sure this change actually removes the race? If I understand Java Memory Model correctly, referenceCountUpdate.compareAndSet(0,1) will act as a publish and getBuffers reading 0 will act as acquire. This only provides ordering between actions before publish and actions after acquire. The subsequent write to buffer after publish would still race with the read of buffer, before acquire.

See for example the section JMM Interpretation: Roach Motel on https://shipilev.net/blog/2014/jmm-pragmatics/, and the subsequent quiz examples.

core/src/main/java/io/undertow/server/DefaultByteBufferPool.java

fl4via · 2022-06-15T12:58:47Z

core/src/main/java/io/undertow/server/DefaultByteBufferPool.java

@@ -256,20 +256,26 @@ private static class DefaultPooledBuffer implements PooledByteBuffer {

        @Override
        public ByteBuffer getBuffer() {
+            final ByteBuffer tmp = this.buffer;
            if(referenceCount == 0) {


maybe we should throw exception if tmp is null

baranowb · 2022-06-15T13:22:34Z

Hi @baranowb, are you sure this change actually removes the race? If I understand Java Memory Model correctly, referenceCountUpdate.compareAndSet(0,1) will act as a publish and getBuffers reading 0 will act as acquire. This only provides ordering between actions before publish and actions after acquire. The subsequent write to buffer after publish would still race with the read of buffer, before acquire.

See for example the section JMM Interpretation: Roach Motel on https://shipilev.net/blog/2014/jmm-pragmatics/, and the subsequent quiz examples.

Oh, Im fairly sure it does not, the problem here is that this part of code wasnt synced for reason and Im sort of forced to accept some trade off - try to mitigate the problem as best as it can be done. Though Im not 100% happy with even this PR, I will change it a bit.

fl4via

There is just a small change pending, other than that it looks very good @baranowb . I'll add it myself to make it possible adding this commit to the 2.3.0.Beta1 release

fl4via · 2022-09-17T09:58:54Z

core/src/main/java/io/undertow/server/DefaultByteBufferPool.java

                    leakDetector.closed = true;
                }
-                pool.freeInternal(buffer);
-                this.buffer = null;
+                if (tmp != null) {


assert tmp != null
If the referenceCountUpdaters is 1, it means it is not null.

core/src/main/java/io/undertow/server/DefaultByteBufferPool.java

fl4via · 2022-09-17T21:50:10Z

@baranowb I need to figure out why my proposed commit brokes the tests. I'll update this PR as soon as I'm done with my investigation.

fl4via · 2022-10-12T08:22:30Z

this PR still breaks CI and it will require further investigation

fl4via · 2022-10-12T08:26:26Z

The test failure is:
Running io.undertow.websockets.extensions.WebSocketExtensionBasicTestCase
[ERROR] Tests run: 3, Failures: 2, Errors: 0, Skipped: 1, Time elapsed: 310.17 s <<< FAILURE! - in io.undertow.websockets.extensions.WebSocketExtensionBasicTestCase
[ERROR] testExtensionsHeaders(io.undertow.websockets.extensions.WebSocketExtensionBasicTestCase) Time elapsed: 10.094 s <<< FAILURE!
I canceled the workflow because it seems this causes the CI to hang (or it was just a bad coincidence...)

ropalka · 2023-01-18T20:04:23Z

core/src/main/java/io/undertow/server/DefaultByteBufferPool.java

@@ -153,7 +154,6 @@ public PooledByteBuffer allocate() {
                local.allocationDepth++;
            }
        }
-        buffer.clear();


This shouldn't be deleted. E.g. thread local cached buffers will not be cleared anymore.

Great catch @ropalka !

ropalka · 2023-01-18T20:05:07Z

core/src/main/java/io/undertow/server/DefaultByteBufferPool.java

@@ -139,6 +139,7 @@ public PooledByteBuffer allocate() {
            buffer = queue.poll();
            if (buffer != null) {
                currentQueueLengthUpdater.decrementAndGet(this);
+                buffer.clear();


This doesn't clear all buffers created / reused in this method, see below comment.

…omicIntegerFieldUpdater.addAndGet method by the original volatile read statement, it is more efficient and there is no gain in concurrency safety when using the former, as far as I could find out when investigating this. Also, I removed the check for temp being null before invoking freeInternal, because it must be non-null if referenceCount was 1 when we retrieved it. Signed-off-by: Flavia Rainone <frainone@redhat.com>

fl4via · 2023-04-23T10:05:47Z

I need to see if that fixes the PR so it can be merged. However, it seems that github actions is down, the checks are queued forever. Because I'll be away on PTO until May 2nd, I'm passing the ball to you, @ropalka ! If it passes, it is ready to be merged, unless there are other issues that need to be fixed in the PR. In that case, if someone wants to give it a try, feel free to do so. See you all on the way back!

carterkozak reviewed Jun 10, 2022

View reviewed changes

core/src/main/java/io/undertow/server/DefaultByteBufferPool.java Outdated Show resolved Hide resolved

fl4via requested changes Jun 15, 2022

View reviewed changes

baranowb force-pushed the UNDERTOW-2072 branch from ed92d3b to 0b6d8bb Compare August 1, 2022 10:29

fl4via added under verification Currently being verified (running tests, reviewing) before posting a review to contributor and removed waiting PR update Awaiting PR update(s) from contributor before merging labels Aug 11, 2022

fl4via requested changes Sep 17, 2022

View reviewed changes

fl4via force-pushed the UNDERTOW-2072 branch from 0b6d8bb to 15a178e Compare September 17, 2022 10:47

fl4via added waiting CI check Ready to be merged but waiting for CI check and removed waiting PR update Awaiting PR update(s) from contributor before merging labels Sep 17, 2022

fl4via force-pushed the UNDERTOW-2072 branch 5 times, most recently from 374984c to 24bd330 Compare September 17, 2022 10:56

fl4via approved these changes Sep 17, 2022

View reviewed changes

fl4via removed the next release This PR will be merged before next release or has already been merged (for payload double check) label Sep 17, 2022

fl4via force-pushed the UNDERTOW-2072 branch from 24bd330 to 02bddf2 Compare October 12, 2022 04:20

fl4via added waiting PR update Awaiting PR update(s) from contributor before merging failed CI Introduced new regession(s) during CI check and removed waiting CI check Ready to be merged but waiting for CI check labels Oct 12, 2022

baranowb force-pushed the UNDERTOW-2072 branch from 02bddf2 to 7c9ca9d Compare January 18, 2023 11:32

ropalka requested changes Jan 18, 2023

View reviewed changes

baranowb force-pushed the UNDERTOW-2072 branch 3 times, most recently from 01ae7b4 to f10109b Compare January 25, 2023 07:40

fl4via force-pushed the UNDERTOW-2072 branch from f10109b to 02bddf2 Compare April 23, 2023 07:35

fl4via added waiting CI check Ready to be merged but waiting for CI check and removed failed CI Introduced new regession(s) during CI check labels Apr 23, 2023

baranowb and others added 2 commits April 23, 2023 07:03

[UNDERTOW-2072] DefaultByteBufferPool.getBuffer() may return null

365fe37

fl4via force-pushed the UNDERTOW-2072 branch from 24ede6c to e38c9e1 Compare April 23, 2023 10:03

fl4via removed the waiting PR update Awaiting PR update(s) from contributor before merging label Apr 23, 2023

fl4via added next release This PR will be merged before next release or has already been merged (for payload double check) and removed waiting CI check Ready to be merged but waiting for CI check labels Oct 11, 2023

fl4via merged commit e92f244 into undertow-io:master Oct 11, 2023

fl4via mentioned this pull request Oct 11, 2023

[UNDERTOW-2305][UNDERTOW-2271][UNDERTOW-2072] CVE-2023-3223 Backport bug fixes #1523

Merged

fl4via removed the next release This PR will be merged before next release or has already been merged (for payload double check) label Oct 11, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[UNDERTOW-2072] DefaultByteBufferPool.getBuffer() may return null #1333

[UNDERTOW-2072] DefaultByteBufferPool.getBuffer() may return null #1333

baranowb commented Jun 10, 2022

e-ts commented Jun 10, 2022

fl4via Jun 15, 2022

baranowb commented Jun 15, 2022 •

edited

Loading

fl4via left a comment

fl4via Sep 17, 2022

fl4via commented Sep 17, 2022

fl4via commented Oct 12, 2022

fl4via commented Oct 12, 2022

ropalka Jan 18, 2023

fl4via Apr 23, 2023

ropalka Jan 18, 2023

fl4via commented Apr 23, 2023

[UNDERTOW-2072] DefaultByteBufferPool.getBuffer() may return null #1333

[UNDERTOW-2072] DefaultByteBufferPool.getBuffer() may return null #1333

Conversation

baranowb commented Jun 10, 2022

e-ts commented Jun 10, 2022

fl4via Jun 15, 2022

Choose a reason for hiding this comment

baranowb commented Jun 15, 2022 • edited Loading

fl4via left a comment

Choose a reason for hiding this comment

fl4via Sep 17, 2022

Choose a reason for hiding this comment

fl4via commented Sep 17, 2022

fl4via commented Oct 12, 2022

fl4via commented Oct 12, 2022

ropalka Jan 18, 2023

Choose a reason for hiding this comment

fl4via Apr 23, 2023

Choose a reason for hiding this comment

ropalka Jan 18, 2023

Choose a reason for hiding this comment

fl4via commented Apr 23, 2023

baranowb commented Jun 15, 2022 •

edited

Loading