You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@mina.apache.org by "David Rosenstrauch (JIRA)" <ji...@apache.org> on 2009/02/26 18:13:02 UTC
[jira] Created: (DIRMINA-664) EMPTY_* IoBuffer constants can be
made mutable and cause data errors
EMPTY_* IoBuffer constants can be made mutable and cause data errors
--------------------------------------------------------------------
Key: DIRMINA-664
URL: https://issues.apache.org/jira/browse/DIRMINA-664
Project: MINA
Issue Type: Bug
Components: Core
Affects Versions: 2.0.0-M4
Environment: All?
Reporter: David Rosenstrauch
Priority: Minor
The EMPTY_* constants in the IoBuffer class can be made mutable (by using the setAutoExpand(true) method call) which can result in those constant buffers no longer being empty. This can obviously cause a multitude of data errors.
See this JUnit test case for an example:
import junit.framework.TestCase;
import org.apache.mina.core.buffer.IoBuffer;
public class TestIoBuffer extends TestCase {
public void testIoBufferAllocate() {
IoBuffer buf = IoBuffer.allocate(0).setAutoExpand(true);
buf.putInt(1234);
buf.flip();
buf = IoBuffer.allocate(0);
assertEquals(0, buf.remaining());
}
public void testEmptyIoBuffer() {
IoBuffer buf = IoBuffer.EMPTY_BUFFER.setAutoExpand(true);
buf.putInt(1234);
buf.flip();
buf = IoBuffer.EMPTY_BUFFER;
assertEquals(0, buf.remaining());
}
}
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Commented: (DIRMINA-664) EMPTY_* IoBuffer constants can be
made mutable and cause data errors
Posted by "David Rosenstrauch (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/DIRMINA-664?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12677237#action_12677237 ]
David Rosenstrauch commented on DIRMINA-664:
--------------------------------------------
That's not going to fix it. It doesn't matter if you declare the constant(s) as private. The fact you're handing out the same (mutable) "constant" buffer repeatedly is the problem. Someone can modify the object, and then it's no longer an empty buffer.
Take, for example, the case where someone calls IoBuffer.allocate(0).setAutoExpand(true). As per the code:
public static IoBuffer allocate(int capacity, boolean direct) {
if (capacity == 0) {
return direct ? EMPTY_DIRECT_BUFFER : EMPTY_HEAP_BUFFER;
}
... it will return either EMPTY_DIRECT_BUFFER or EMPTY_HEAP_BUFFER. Both of those are private fields. But it doesn't matter. If I then set that "constant" "empty" buffer that was returned to me to be expandable and write to it, then any subsequent calls to IoBuffer.allocate(0) will return that buffer - which is no longer an empty buffer.
So I don't think you have any choice but to get rid of the static constants.
As far as the expandable functionality, just my IMO, but I'd request that you not remove that as it's extremely useful. It can often be difficult to know ahead of time exactly how large of a buffer you're going to need (e.g., if the data can vary in length), and stepping through your object graph to compute the size needed might require almost as much coding and CPU cycles as it would take to write the object graph to the buffer in the first place. So it's extremely useful to have an expandable buffer.
> EMPTY_* IoBuffer constants can be made mutable and cause data errors
> --------------------------------------------------------------------
>
> Key: DIRMINA-664
> URL: https://issues.apache.org/jira/browse/DIRMINA-664
> Project: MINA
> Issue Type: Bug
> Components: Core
> Affects Versions: 2.0.0-M4
> Environment: All?
> Reporter: David Rosenstrauch
> Priority: Minor
>
> The EMPTY_* constants in the IoBuffer class can be made mutable (by using the setAutoExpand(true) method call) which can result in those constant buffers no longer being empty. This can obviously cause a multitude of data errors.
> See this JUnit test case for an example:
> import junit.framework.TestCase;
> import org.apache.mina.core.buffer.IoBuffer;
> public class TestIoBuffer extends TestCase {
> public void testIoBufferAllocate() {
> IoBuffer buf = IoBuffer.allocate(0).setAutoExpand(true);
> buf.putInt(1234);
> buf.flip();
> buf = IoBuffer.allocate(0);
> assertEquals(0, buf.remaining());
> }
> public void testEmptyIoBuffer() {
> IoBuffer buf = IoBuffer.EMPTY_BUFFER.setAutoExpand(true);
> buf.putInt(1234);
> buf.flip();
> buf = IoBuffer.EMPTY_BUFFER;
> assertEquals(0, buf.remaining());
> }
> }
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Resolved: (DIRMINA-664) EMPTY_* IoBuffer constants can be
made mutable and cause data errors
Posted by "Emmanuel Lecharny (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/DIRMINA-664?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]
Emmanuel Lecharny resolved DIRMINA-664.
---------------------------------------
Resolution: Fixed
Fix Version/s: 2.0.0-RC1
Fixed in http://svn.apache.org/viewvc?rev=748525&view=rev
> EMPTY_* IoBuffer constants can be made mutable and cause data errors
> --------------------------------------------------------------------
>
> Key: DIRMINA-664
> URL: https://issues.apache.org/jira/browse/DIRMINA-664
> Project: MINA
> Issue Type: Bug
> Components: Core
> Affects Versions: 2.0.0-M4
> Environment: All?
> Reporter: David Rosenstrauch
> Assignee: Emmanuel Lecharny
> Priority: Minor
> Fix For: 2.0.0-RC1
>
>
> The EMPTY_* constants in the IoBuffer class can be made mutable (by using the setAutoExpand(true) method call) which can result in those constant buffers no longer being empty. This can obviously cause a multitude of data errors.
> See this JUnit test case for an example:
> import junit.framework.TestCase;
> import org.apache.mina.core.buffer.IoBuffer;
> public class TestIoBuffer extends TestCase {
> public void testIoBufferAllocate() {
> IoBuffer buf = IoBuffer.allocate(0).setAutoExpand(true);
> buf.putInt(1234);
> buf.flip();
> buf = IoBuffer.allocate(0);
> assertEquals(0, buf.remaining());
> }
> public void testEmptyIoBuffer() {
> IoBuffer buf = IoBuffer.EMPTY_BUFFER.setAutoExpand(true);
> buf.putInt(1234);
> buf.flip();
> buf = IoBuffer.EMPTY_BUFFER;
> assertEquals(0, buf.remaining());
> }
> }
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Closed: (DIRMINA-664) EMPTY_* IoBuffer constants can be made
mutable and cause data errors
Posted by "Emmanuel Lecharny (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/DIRMINA-664?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]
Emmanuel Lecharny closed DIRMINA-664.
-------------------------------------
> EMPTY_* IoBuffer constants can be made mutable and cause data errors
> --------------------------------------------------------------------
>
> Key: DIRMINA-664
> URL: https://issues.apache.org/jira/browse/DIRMINA-664
> Project: MINA
> Issue Type: Bug
> Components: Core
> Affects Versions: 2.0.0-M4
> Environment: All?
> Reporter: David Rosenstrauch
> Assignee: Emmanuel Lecharny
> Priority: Minor
> Fix For: 2.0.0-M5
>
>
> The EMPTY_* constants in the IoBuffer class can be made mutable (by using the setAutoExpand(true) method call) which can result in those constant buffers no longer being empty. This can obviously cause a multitude of data errors.
> See this JUnit test case for an example:
> import junit.framework.TestCase;
> import org.apache.mina.core.buffer.IoBuffer;
> public class TestIoBuffer extends TestCase {
> public void testIoBufferAllocate() {
> IoBuffer buf = IoBuffer.allocate(0).setAutoExpand(true);
> buf.putInt(1234);
> buf.flip();
> buf = IoBuffer.allocate(0);
> assertEquals(0, buf.remaining());
> }
> public void testEmptyIoBuffer() {
> IoBuffer buf = IoBuffer.EMPTY_BUFFER.setAutoExpand(true);
> buf.putInt(1234);
> buf.flip();
> buf = IoBuffer.EMPTY_BUFFER;
> assertEquals(0, buf.remaining());
> }
> }
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Commented: (DIRMINA-664) EMPTY_* IoBuffer constants can be
made mutable and cause data errors
Posted by "Emmanuel Lecharny (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/DIRMINA-664?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12677199#action_12677199 ]
Emmanuel Lecharny commented on DIRMINA-664:
-------------------------------------------
Hmmm, I'm a bit off rails here.
Basically, you're right : the EMPY_BUFFER won't point on an empty buffer despite the 'static final' qualifier.
After having reviewed the current implementation this afternoon, here is what I think :
- it will be removed in 3.0
- extensible buffer is a bad idea
- using an EMPTY_BUFFER instead of allocating a buffer is not likely to happen often
- In any case, it's not immutable, so it should not be visible to users if we are to use them internally
So I tend to think that, yes, it's a weakness in the class. The object should be declared private, as it's only used internally. Let's do that atm.
> EMPTY_* IoBuffer constants can be made mutable and cause data errors
> --------------------------------------------------------------------
>
> Key: DIRMINA-664
> URL: https://issues.apache.org/jira/browse/DIRMINA-664
> Project: MINA
> Issue Type: Bug
> Components: Core
> Affects Versions: 2.0.0-M4
> Environment: All?
> Reporter: David Rosenstrauch
> Priority: Minor
>
> The EMPTY_* constants in the IoBuffer class can be made mutable (by using the setAutoExpand(true) method call) which can result in those constant buffers no longer being empty. This can obviously cause a multitude of data errors.
> See this JUnit test case for an example:
> import junit.framework.TestCase;
> import org.apache.mina.core.buffer.IoBuffer;
> public class TestIoBuffer extends TestCase {
> public void testIoBufferAllocate() {
> IoBuffer buf = IoBuffer.allocate(0).setAutoExpand(true);
> buf.putInt(1234);
> buf.flip();
> buf = IoBuffer.allocate(0);
> assertEquals(0, buf.remaining());
> }
> public void testEmptyIoBuffer() {
> IoBuffer buf = IoBuffer.EMPTY_BUFFER.setAutoExpand(true);
> buf.putInt(1234);
> buf.flip();
> buf = IoBuffer.EMPTY_BUFFER;
> assertEquals(0, buf.remaining());
> }
> }
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Assigned: (DIRMINA-664) EMPTY_* IoBuffer constants can be
made mutable and cause data errors
Posted by "Emmanuel Lecharny (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/DIRMINA-664?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]
Emmanuel Lecharny reassigned DIRMINA-664:
-----------------------------------------
Assignee: Emmanuel Lecharny
> EMPTY_* IoBuffer constants can be made mutable and cause data errors
> --------------------------------------------------------------------
>
> Key: DIRMINA-664
> URL: https://issues.apache.org/jira/browse/DIRMINA-664
> Project: MINA
> Issue Type: Bug
> Components: Core
> Affects Versions: 2.0.0-M4
> Environment: All?
> Reporter: David Rosenstrauch
> Assignee: Emmanuel Lecharny
> Priority: Minor
> Fix For: 2.0.0-RC1
>
>
> The EMPTY_* constants in the IoBuffer class can be made mutable (by using the setAutoExpand(true) method call) which can result in those constant buffers no longer being empty. This can obviously cause a multitude of data errors.
> See this JUnit test case for an example:
> import junit.framework.TestCase;
> import org.apache.mina.core.buffer.IoBuffer;
> public class TestIoBuffer extends TestCase {
> public void testIoBufferAllocate() {
> IoBuffer buf = IoBuffer.allocate(0).setAutoExpand(true);
> buf.putInt(1234);
> buf.flip();
> buf = IoBuffer.allocate(0);
> assertEquals(0, buf.remaining());
> }
> public void testEmptyIoBuffer() {
> IoBuffer buf = IoBuffer.EMPTY_BUFFER.setAutoExpand(true);
> buf.putInt(1234);
> buf.flip();
> buf = IoBuffer.EMPTY_BUFFER;
> assertEquals(0, buf.remaining());
> }
> }
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Commented: (DIRMINA-664) EMPTY_* IoBuffer constants can be
made mutable and cause data errors
Posted by "Emmanuel Lecharny (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/DIRMINA-664?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12677067#action_12677067 ]
Emmanuel Lecharny commented on DIRMINA-664:
-------------------------------------------
It doesn't matter too much, as an expand() operation will allocate a new ByteBuffer internally. (this is the reason why I think that using expandable buffers is a bad idea...)
> EMPTY_* IoBuffer constants can be made mutable and cause data errors
> --------------------------------------------------------------------
>
> Key: DIRMINA-664
> URL: https://issues.apache.org/jira/browse/DIRMINA-664
> Project: MINA
> Issue Type: Bug
> Components: Core
> Affects Versions: 2.0.0-M4
> Environment: All?
> Reporter: David Rosenstrauch
> Priority: Minor
>
> The EMPTY_* constants in the IoBuffer class can be made mutable (by using the setAutoExpand(true) method call) which can result in those constant buffers no longer being empty. This can obviously cause a multitude of data errors.
> See this JUnit test case for an example:
> import junit.framework.TestCase;
> import org.apache.mina.core.buffer.IoBuffer;
> public class TestIoBuffer extends TestCase {
> public void testIoBufferAllocate() {
> IoBuffer buf = IoBuffer.allocate(0).setAutoExpand(true);
> buf.putInt(1234);
> buf.flip();
> buf = IoBuffer.allocate(0);
> assertEquals(0, buf.remaining());
> }
> public void testEmptyIoBuffer() {
> IoBuffer buf = IoBuffer.EMPTY_BUFFER.setAutoExpand(true);
> buf.putInt(1234);
> buf.flip();
> buf = IoBuffer.EMPTY_BUFFER;
> assertEquals(0, buf.remaining());
> }
> }
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Commented: (DIRMINA-664) EMPTY_* IoBuffer constants can be
made mutable and cause data errors
Posted by "Emmanuel Lecharny (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/DIRMINA-664?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12677357#action_12677357 ]
Emmanuel Lecharny commented on DIRMINA-664:
-------------------------------------------
You are damn right.
Let's remove those faked immutable objects.
> EMPTY_* IoBuffer constants can be made mutable and cause data errors
> --------------------------------------------------------------------
>
> Key: DIRMINA-664
> URL: https://issues.apache.org/jira/browse/DIRMINA-664
> Project: MINA
> Issue Type: Bug
> Components: Core
> Affects Versions: 2.0.0-M4
> Environment: All?
> Reporter: David Rosenstrauch
> Priority: Minor
>
> The EMPTY_* constants in the IoBuffer class can be made mutable (by using the setAutoExpand(true) method call) which can result in those constant buffers no longer being empty. This can obviously cause a multitude of data errors.
> See this JUnit test case for an example:
> import junit.framework.TestCase;
> import org.apache.mina.core.buffer.IoBuffer;
> public class TestIoBuffer extends TestCase {
> public void testIoBufferAllocate() {
> IoBuffer buf = IoBuffer.allocate(0).setAutoExpand(true);
> buf.putInt(1234);
> buf.flip();
> buf = IoBuffer.allocate(0);
> assertEquals(0, buf.remaining());
> }
> public void testEmptyIoBuffer() {
> IoBuffer buf = IoBuffer.EMPTY_BUFFER.setAutoExpand(true);
> buf.putInt(1234);
> buf.flip();
> buf = IoBuffer.EMPTY_BUFFER;
> assertEquals(0, buf.remaining());
> }
> }
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Commented: (DIRMINA-664) EMPTY_* IoBuffer constants can be
made mutable and cause data errors
Posted by "David Rosenstrauch (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/DIRMINA-664?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12677093#action_12677093 ]
David Rosenstrauch commented on DIRMINA-664:
--------------------------------------------
? Not sure I understand. I'm never calling expand() on the buffer, and so I'm getting erroneous data behavior. (You can see this if you run my unit tests. The tests are failing!)
Seems like a big deal to me - especially the part pertaining to IoBuffer.allocate(0).setAutoExpand(true). It caused some bugs in my system whereby I thought I was getting allocated a new buffer, but instead Mina was giving me an existing buffer that already had data in it.
Just my $0.02, but IMO using expandable buffers isn't the problem here, but rather keeping some buffers around as static constants. The assumption there is that you can hold them as static constants since they're immutable, but as you can see that assumption of immutability is not correct. IMO, the fix here is to get rid of the static constants, and have IoBuffer allocate you a new one each time.
> EMPTY_* IoBuffer constants can be made mutable and cause data errors
> --------------------------------------------------------------------
>
> Key: DIRMINA-664
> URL: https://issues.apache.org/jira/browse/DIRMINA-664
> Project: MINA
> Issue Type: Bug
> Components: Core
> Affects Versions: 2.0.0-M4
> Environment: All?
> Reporter: David Rosenstrauch
> Priority: Minor
>
> The EMPTY_* constants in the IoBuffer class can be made mutable (by using the setAutoExpand(true) method call) which can result in those constant buffers no longer being empty. This can obviously cause a multitude of data errors.
> See this JUnit test case for an example:
> import junit.framework.TestCase;
> import org.apache.mina.core.buffer.IoBuffer;
> public class TestIoBuffer extends TestCase {
> public void testIoBufferAllocate() {
> IoBuffer buf = IoBuffer.allocate(0).setAutoExpand(true);
> buf.putInt(1234);
> buf.flip();
> buf = IoBuffer.allocate(0);
> assertEquals(0, buf.remaining());
> }
> public void testEmptyIoBuffer() {
> IoBuffer buf = IoBuffer.EMPTY_BUFFER.setAutoExpand(true);
> buf.putInt(1234);
> buf.flip();
> buf = IoBuffer.EMPTY_BUFFER;
> assertEquals(0, buf.remaining());
> }
> }
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.