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

ARTEMIS-2450 page-size-bytes should not be greater than Integer.MAX_VALUE #2791

Closed
wants to merge 1 commit into from

Conversation

@wy96f
Copy link
Contributor

commented Aug 9, 2019

No description provided.

@@ -505,6 +505,9 @@ public long getPageSizeBytes() {
}

public AddressSettings setPageSizeBytes(final long pageSize) {
if (pageSize > Integer.MAX_VALUE) {
throw new IllegalArgumentException("pageSize must be < " + Integer.MAX_VALUE);

This comment has been minimized.

Copy link
@michaelandrepearce

michaelandrepearce Aug 15, 2019

Contributor

Why not change long to int, this would then be cleaner from anyone trying to use the method

This comment has been minimized.

Copy link
@wy96f

wy96f Aug 16, 2019

Author Contributor

The AddressSettings maybe persisted in disk, and if we change long to int, we would incorrectly decode it. Does it make sense?

This comment has been minimized.

Copy link
@michaelandrepearce

michaelandrepearce Aug 16, 2019

Contributor

Not really. If you now decode something thats larger than int it will blow up with the exception youve added. No different. Except we are actually making the method take the correct primitive number

This comment has been minimized.

Copy link
@michaelandrepearce

michaelandrepearce Aug 16, 2019

Contributor

Also should update the schema to int so then when people do parse configs via xsd validators it would fail

@@ -257,6 +257,26 @@ public void testParsingDefaultServerConfigWithENCMaskedPwd() throws Exception {
assertEquals("helloworld", bconfig.getPassword());
}

@Test
public void testParsingOverflowPageSize() throws Exception {
FileConfigurationParser parser = new FileConfigurationParser();

This comment has been minimized.

Copy link
@michaelandrepearce

michaelandrepearce Aug 15, 2019

Contributor

This test, test the new change, but doesnt test for what we are protecting from. E.g whats the underlying issue being fixed

This comment has been minimized.

Copy link
@michaelandrepearce

michaelandrepearce Aug 15, 2019

Contributor

E.g. other sizes are quite happily long currently also. Why is this special

This comment has been minimized.

Copy link
@wy96f

wy96f Aug 16, 2019

Author Contributor

E.g. other sizes are quite happily long currently also. Why is this special

I see in Page::write()/Page::read() all operations are limited to int range, such as PagingStoreImpl::currentPageSize, Page::size, and local variables like fileSize, processedBytes in Page::readFromSequentialFile(), etc.

This test, test the new change, but doesnt test for what we are protecting from. E.g whats the underlying issue being fixed

I'm not sure what you mean. We're protecting page-size-bytes from being greater than Integer.MAX_VALUE(2147483647). So if we set page-size-bytes to 2147483648, the broker is expected to fail fast to throw exception , correct?

This comment has been minimized.

Copy link
@michaelandrepearce

michaelandrepearce Aug 16, 2019

Contributor

Point more so was i assume the same issue may lay in other persistence such as journal etc. As alot of it started from a common base.

@jbertram

This comment has been minimized.

Copy link
Contributor

commented Aug 16, 2019

@wy96f, can you rebase and push -f to trigger a rebuild of the PR by Travis? An issue was recently fixed that was preventing the PR build from completing properly. Thanks!

@wy96f

This comment has been minimized.

Copy link
Contributor Author

commented Aug 17, 2019

@jbertram will re-work this based on comments. After that will rebase and push :)

@wy96f wy96f force-pushed the wy96f:page_size branch from 1b2590b to a45f85d Aug 18, 2019

@wy96f wy96f force-pushed the wy96f:page_size branch from a45f85d to eae63b1 Aug 19, 2019

@wy96f

This comment has been minimized.

Copy link
Contributor Author

commented Aug 19, 2019

pushed to handle "page-size-bytes" the same way as "journal-file-size"

@@ -122,6 +122,30 @@ public void validate(final String name, final Object value) {
}
};

public static final Validator POSITIVE_INT = new Validator() {

This comment has been minimized.

Copy link
@michaelpearce-gain

michaelpearce-gain Aug 19, 2019

This isnt needed, simply change the xsd defination to int and it would fail

This comment has been minimized.

Copy link
@michaelpearce-gain

michaelpearce-gain Aug 19, 2019

ignore i spotted, why finally, its because GB etc can be used in the string type, and then converted as such cannot limit this in xsd.

}
};

public static final Validator MINUS_ONE_OR_POSITIVE_INT = new Validator() {

This comment has been minimized.

Copy link
@michaelpearce-gain

michaelpearce-gain Aug 19, 2019

again not needed, if the xsd is corrected to correct type, as it would ensure type size bound. also ensruing xsd is correct, allows end users to use the xsd to pre validate.

This comment has been minimized.

Copy link
@michaelpearce-gain

michaelpearce-gain Aug 19, 2019

ignore i spotted, why finally, its because GB etc can be used in the string type, and then converted as such cannot limit this in xsd.

@@ -1756,17 +1758,17 @@ private void parseClusterConnectionConfiguration(final Element e, final Configur

double retryIntervalMultiplier = getDouble(e, "retry-interval-multiplier", ActiveMQDefaultConfiguration.getDefaultClusterRetryIntervalMultiplier(), Validators.GT_ZERO);

int minLargeMessageSize = getTextBytesAsIntBytes(e, "min-large-message-size", ActiveMQClient.DEFAULT_MIN_LARGE_MESSAGE_SIZE, Validators.GT_ZERO);
int minLargeMessageSize = getTextBytesAsIntBytes(e, "min-large-message-size", ActiveMQClient.DEFAULT_MIN_LARGE_MESSAGE_SIZE, Validators.POSITIVE_INT);

This comment has been minimized.

Copy link
@michaelpearce-gain

This comment has been minimized.

Copy link
@michaelpearce-gain

michaelpearce-gain Aug 19, 2019

ignore i was wrong, i see the issue.

@@ -597,11 +597,11 @@ public void parseMainConfig(final Element e, final Configuration config) throws

config.setJournalSyncNonTransactional(getBoolean(e, "journal-sync-non-transactional", config.isJournalSyncNonTransactional()));

config.setJournalFileSize(getTextBytesAsIntBytes(e, "journal-file-size", config.getJournalFileSize(), Validators.GT_ZERO));
config.setJournalFileSize(getTextBytesAsIntBytes(e, "journal-file-size", config.getJournalFileSize(), Validators.POSITIVE_INT));

This comment has been minimized.

Copy link
@michaelpearce-gain

michaelpearce-gain Aug 19, 2019

change was not needed here, int range should all be validated by xsd type.

This comment has been minimized.

Copy link
@michaelpearce-gain

michaelpearce-gain Aug 19, 2019

ignore i spotted why you need.

@michaelpearce-gain
Copy link

left a comment

The new int validator isnt needed, that should be reverted, the fact the type should be int, should be corrected in the xsd, and this will be picked up and validated at point of schema validation.

@michaelpearce-gain
Copy link

left a comment

ignore i spotted, why finally, its because GB etc can be used in the string type, and then converted as such cannot limit this in xsd.

@asfgit asfgit closed this in b9dda34 Aug 19, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.