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

Handle long overflow when adding paths' totals #23293

Merged
merged 1 commit into from Feb 22, 2017

Conversation

Projects
None yet
2 participants
@dakrone
Member

dakrone commented Feb 21, 2017

From #23093, we fixed the issue where a filesystem can be so large that it
overflows and returns a negative number. However, there is another issue when
adding a path as a sub-path to another FsInfo.Path object, when adding the
totals the values can still overflow.

This adds the same safety to return Long.MAX_VALUE instead of the negative
number, as well as a test exercising the logic.

@jasontedor

I left a comment.

core/src/main/java/org/elasticsearch/monitor/fs/FsProbe.java Outdated
* Take a large value intended to be positive, and if it has overflowed,
* return {@code Long.MAX_VALUE} instead of a negative number.
*/
public static long adjustForHugeFilesystems(long bytes) {

This comment has been minimized.

@jasontedor

jasontedor Feb 21, 2017

Member

Does it have to be public, can it be package-private?

This comment has been minimized.

@dakrone

dakrone Feb 21, 2017

Member

Sure, I'll make it package-private, I just figured it's static, so there's no harm in it being public

Updated in 52ee78f191e1515570ae9e16d611920f336562c0

This comment has been minimized.

@jasontedor

jasontedor Feb 21, 2017

Member

It becomes part of the public API.

@dakrone

This comment has been minimized.

Member

dakrone commented Feb 21, 2017

retest this please

1 similar comment
@dakrone

This comment has been minimized.

Member

dakrone commented Feb 21, 2017

retest this please

@dakrone

This comment has been minimized.

Member

dakrone commented Feb 22, 2017

retest this please /sigh

@jasontedor

I left some more comments.

core/src/test/java/org/elasticsearch/monitor/fs/FsProbeTests.java Outdated
FsInfo stats = probe.stats(null, null);
assertNotNull(stats);
FsInfo.Path total = stats.getTotal();

This comment has been minimized.

@jasontedor

jasontedor Feb 22, 2017

Member

Can you call this something other than total? It's really hard to read things like total.total, total.total, total.total + total.total.

core/src/test/java/org/elasticsearch/monitor/fs/FsProbeTests.java Outdated
assertThat(total.total, greaterThan(0L));
// While not overflowing, keep adding
while ((total.total + total.total) > 0) {

This comment has been minimized.

@jasontedor

jasontedor Feb 22, 2017

Member

If total.total is pathological (like one byte), this will take forever. Can you just add invocations of randomNonNegativeLongs()?

@dakrone

This comment has been minimized.

Member

dakrone commented Feb 22, 2017

I pushed 11a33e48e08154ec5e92e2ea62d44451bd2dbb10 to address the comments

@jasontedor

This comment has been minimized.

Member

jasontedor commented Feb 22, 2017

I think there is a miscommunication; it's my fault. I mean like this:

diff --git a/core/src/test/java/org/elasticsearch/monitor/fs/FsProbeTests.java b/core/src/test/java/org/elasticsearch/monitor/fs/FsProbeTests.java
index 7cb0065628..6777d438a1 100644
--- a/core/src/test/java/org/elasticsearch/monitor/fs/FsProbeTests.java
+++ b/core/src/test/java/org/elasticsearch/monitor/fs/FsProbeTests.java
@@ -93,19 +93,21 @@ public class FsProbeTests extends ESTestCase {
     }
 
     public void testFsInfoOverflow() throws Exception {
-        FsInfo.Path pathStats = new FsInfo.Path("/foo/bar", null,
-                randomNonNegativeLong(), randomNonNegativeLong(), randomNonNegativeLong());
+        FsInfo.Path pathStats =
+                new FsInfo.Path("/foo/bar", null, randomNonNegativeLong(), randomNonNegativeLong(), randomNonNegativeLong());
 
         // While not overflowing, keep adding
-        while ((pathStats.total + pathStats.total) > 0) {
+        FsInfo.Path add = new FsInfo.Path("/baz/qux", null, randomNonNegativeLong(), randomNonNegativeLong(), randomNonNegativeLong());
+        while ((pathStats.total + add.total) > 0) {
             // Add itself as a path, to increase the total bytes until it overflows
-            logger.info("--> adding {} bytes to {}, will be: {}", pathStats.total, pathStats.total, pathStats.total + pathStats.total);
-            pathStats.add(pathStats);
+            logger.info("--> adding {} bytes to {}, will be: {}", add.total, pathStats.total, pathStats.total + add.total);
+            pathStats.add(add);
+            add = new FsInfo.Path("/baz/qux", null, randomNonNegativeLong(), randomNonNegativeLong(), randomNonNegativeLong());
         }
 
-        logger.info("--> adding {} bytes to {}, will be: {}", pathStats.total, pathStats.total, pathStats.total + pathStats.total);
-        assertThat(pathStats.total + pathStats.total, lessThan(0L));
-        pathStats.add(pathStats);
+        logger.info("--> adding {} bytes to {}, will be: {}", add.total, pathStats.total, pathStats.total + add.total);
+        assertThat(pathStats.total + add.total, lessThan(0L));
+        pathStats.add(add);
 
         // Even after overflowing, it should not be negative
         assertThat(pathStats.total, greaterThan(0L));
@dakrone

This comment has been minimized.

Member

dakrone commented Feb 22, 2017

Alright, I changed it in 77128b6170a7f81662f8e49b50c9f215d7b2f484

@jasontedor

LGTM.

Handle long overflow when adding paths' totals
From #23093, we fixed the issue where a filesystem can be so large that it
overflows and returns a negative number. However, there is another issue when
adding a path as a sub-path to another `FsInfo.Path` object, when adding the
totals the values can still overflow.

This adds the same safety to return `Long.MAX_VALUE` instead of the negative
number, as well as a test exercising the logic.

@dakrone dakrone merged commit 77d6412 into elastic:master Feb 22, 2017

1 of 2 checks passed

elasticsearch-ci Build started sha1 is merged.
Details
CLA Commit author has signed the CLA
Details
@jasontedor

This comment has been minimized.

Member

jasontedor commented Mar 19, 2017

I backported this to 5.3 and 5.x.

@dakrone dakrone deleted the dakrone:handle-overflow-when-adding-paths branch May 24, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment