You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@drill.apache.org by GitBox <gi...@apache.org> on 2022/08/24 07:04:33 UTC
[GitHub] [drill] jnturton opened a new pull request, #2632: DRILL-8283: Add a configurable recursive file listing size limit
jnturton opened a new pull request, #2632:
URL: https://github.com/apache/drill/pull/2632
# [DRILL-8283](https://issues.apache.org/jira/browse/DRILL-8283): Add a configurable recursive file listing size limit
## Description
Currently a malicious or merely unwitting user can crash their Drill foreman by sending
```
select * from dfs.huge_workspace limit 10
```
causing the query planner to recurse over every file in huge_workspace and culminating in
```
2022-08-09 15:13:22,251 [1d0da29f-e50c-fd51-43d9-8a5086d52c4e:foreman] ERROR o.a.drill.common.CatastrophicFailure - Catastrophic Failure Occurred, exiting. Information message: Unable to handle out of memory condition in Foreman.java.lang.OutOfMemoryError: null
```
if there are enough files in huge_workspace. A SHOW FILES command can produce the same effect. This issue proposes a new BOOT option named drill.exec.storage.file.recursive_listing_max_size with a default value of, say 10 000. If a file listing task exceeds this limit then the initiating operation is terminated with a UserException preventing runaway resource usage.
## Documentation
New entry on https://drill.apache.org/docs/start-up-options/
## Testing
FileSystemUtilTest#testRecursiveListingMaxSize
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@drill.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [drill] pjfanning commented on a diff in pull request #2632: DRILL-8283: Add a configurable recursive file listing size limit
Posted by GitBox <gi...@apache.org>.
pjfanning commented on code in PR #2632:
URL: https://github.com/apache/drill/pull/2632#discussion_r954255629
##########
exec/java-exec/src/main/resources/drill-module.conf:
##########
@@ -115,7 +115,8 @@ drill.exec: {
text: {
buffer.size: 262144,
batch.size: 4000
- }
+ },
+ recursive_listing_max_size: 10000
Review Comment:
My 2 cents is that limits ideally should be set by default to a sensible level. For Drill 2.0.0, enforcing that some sort of limit is set would be something that I'd support. For Drill 1.x, it would not be a good idea to enforce limits by default but supporting them optionally would be useful (to avoid introducing changes that might force users to tune configs in a minor release).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@drill.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [drill] vvysotskyi commented on a diff in pull request #2632: DRILL-8283: Add a configurable recursive file listing size limit
Posted by GitBox <gi...@apache.org>.
vvysotskyi commented on code in PR #2632:
URL: https://github.com/apache/drill/pull/2632#discussion_r954278444
##########
exec/java-exec/src/main/resources/drill-module.conf:
##########
@@ -115,7 +115,8 @@ drill.exec: {
text: {
buffer.size: 262144,
batch.size: 4000
- }
+ },
+ recursive_listing_max_size: 10000
Review Comment:
Yes, the default value should be adjusted. For the big data world, thousands of files are quite a small amount. For non-parquet files FileStatus is small, so it shouldn't cause large pressure on memory. For parquet files, it would be good to provide the functionality to disable reading metadata for planning and use it only during execution to avoid issues with huge files amount.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@drill.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [drill] vvysotskyi commented on a diff in pull request #2632: DRILL-8283: Add a configurable recursive file listing size limit
Posted by GitBox <gi...@apache.org>.
vvysotskyi commented on code in PR #2632:
URL: https://github.com/apache/drill/pull/2632#discussion_r954723117
##########
exec/java-exec/src/main/java/org/apache/drill/exec/util/FileSystemUtil.java:
##########
@@ -302,12 +332,32 @@ protected List<FileStatus> compute() {
List<RecursiveListing> tasks = new ArrayList<>();
try {
- for (FileStatus status : fs.listStatus(path, filter)) {
+ FileStatus[] dirFs = fs.listStatus(path, filter);
+ if (recursiveListingMaxSize > 0 && fileCounter.addAndGet(dirFs.length) > recursiveListingMaxSize) {
+ throw UserException
Review Comment:
This code is executed within fork join pool, and if error suppression flag is enabled, it will call task.fork(). Can we somehow stop executing all tasks for the case when count is reached?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@drill.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [drill] vvysotskyi commented on a diff in pull request #2632: DRILL-8283: Add a configurable recursive file listing size limit
Posted by GitBox <gi...@apache.org>.
vvysotskyi commented on code in PR #2632:
URL: https://github.com/apache/drill/pull/2632#discussion_r954203172
##########
exec/java-exec/src/main/resources/drill-module.conf:
##########
@@ -115,7 +115,8 @@ drill.exec: {
text: {
buffer.size: 262144,
batch.size: 4000
- }
+ },
+ recursive_listing_max_size: 10000
Review Comment:
Could you please make this limit optional?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@drill.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [drill] jnturton commented on a diff in pull request #2632: DRILL-8283: Add a configurable recursive file listing size limit
Posted by GitBox <gi...@apache.org>.
jnturton commented on code in PR #2632:
URL: https://github.com/apache/drill/pull/2632#discussion_r953428323
##########
exec/java-exec/src/main/java/org/apache/drill/exec/util/FileSystemUtil.java:
##########
@@ -42,6 +47,12 @@ public class FileSystemUtil {
private static final org.slf4j.Logger logger = org.slf4j.LoggerFactory.getLogger(FileSystemUtil.class);
+ private static int recursiveListingMaxSize;
+
+ static {
+ recursiveListingMaxSize = DrillConfig.create().getInt(ExecConstants.RECURSIVE_FILE_LISTING_MAX_SIZE);
+ }
Review Comment:
This route to the config option felt pretty weird, I don't if there's a better way?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@drill.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [drill] jnturton merged pull request #2632: DRILL-8283: Add a configurable recursive file listing size limit
Posted by GitBox <gi...@apache.org>.
jnturton merged PR #2632:
URL: https://github.com/apache/drill/pull/2632
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@drill.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [drill] jnturton commented on a diff in pull request #2632: DRILL-8283: Add a configurable recursive file listing size limit
Posted by GitBox <gi...@apache.org>.
jnturton commented on code in PR #2632:
URL: https://github.com/apache/drill/pull/2632#discussion_r954914617
##########
exec/java-exec/src/main/java/org/apache/drill/exec/util/FileSystemUtil.java:
##########
@@ -42,6 +47,12 @@ public class FileSystemUtil {
private static final org.slf4j.Logger logger = org.slf4j.LoggerFactory.getLogger(FileSystemUtil.class);
+ private static int recursiveListingMaxSize;
+
+ static {
+ recursiveListingMaxSize = DrillConfig.create().getInt(ExecConstants.RECURSIVE_FILE_LISTING_MAX_SIZE);
+ }
Review Comment:
That it might be a heavyweight duplication of work was bothering me enough that I went and timed it. It takes about ~100ms when I start embedded Drill locally. That's just enough to make me wonder if it's worth trying to redesign this stuff so that it loads from an existing instance of DrillConfig instead of constructing its own.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@drill.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [drill] jnturton commented on a diff in pull request #2632: DRILL-8283: Add a configurable recursive file listing size limit
Posted by GitBox <gi...@apache.org>.
jnturton commented on code in PR #2632:
URL: https://github.com/apache/drill/pull/2632#discussion_r953428323
##########
exec/java-exec/src/main/java/org/apache/drill/exec/util/FileSystemUtil.java:
##########
@@ -42,6 +47,12 @@ public class FileSystemUtil {
private static final org.slf4j.Logger logger = org.slf4j.LoggerFactory.getLogger(FileSystemUtil.class);
+ private static int recursiveListingMaxSize;
+
+ static {
+ recursiveListingMaxSize = DrillConfig.create().getInt(ExecConstants.RECURSIVE_FILE_LISTING_MAX_SIZE);
+ }
Review Comment:
This route to the config option felt pretty weird, I don't know if there's a better way?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@drill.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [drill] vvysotskyi commented on a diff in pull request #2632: DRILL-8283: Add a configurable recursive file listing size limit
Posted by GitBox <gi...@apache.org>.
vvysotskyi commented on code in PR #2632:
URL: https://github.com/apache/drill/pull/2632#discussion_r954723117
##########
exec/java-exec/src/main/java/org/apache/drill/exec/util/FileSystemUtil.java:
##########
@@ -302,12 +332,32 @@ protected List<FileStatus> compute() {
List<RecursiveListing> tasks = new ArrayList<>();
try {
- for (FileStatus status : fs.listStatus(path, filter)) {
+ FileStatus[] dirFs = fs.listStatus(path, filter);
+ if (recursiveListingMaxSize > 0 && fileCounter.addAndGet(dirFs.length) > recursiveListingMaxSize) {
+ throw UserException
Review Comment:
This code is executed within fork join pool. Can we somehow stop executing all tasks for the case when count is reached?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@drill.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [drill] jnturton commented on a diff in pull request #2632: DRILL-8283: Add a configurable recursive file listing size limit
Posted by GitBox <gi...@apache.org>.
jnturton commented on code in PR #2632:
URL: https://github.com/apache/drill/pull/2632#discussion_r954644543
##########
exec/java-exec/src/main/resources/drill-module.conf:
##########
@@ -115,7 +115,8 @@ drill.exec: {
text: {
buffer.size: 262144,
batch.size: 4000
- }
+ },
+ recursive_listing_max_size: 10000
Review Comment:
A limit of 0 (or less) now means no limit and the default, for this PR, is now 0.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@drill.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [drill] cgivre commented on a diff in pull request #2632: DRILL-8283: Add a configurable recursive file listing size limit
Posted by GitBox <gi...@apache.org>.
cgivre commented on code in PR #2632:
URL: https://github.com/apache/drill/pull/2632#discussion_r953900054
##########
exec/java-exec/src/main/java/org/apache/drill/exec/util/FileSystemUtil.java:
##########
@@ -42,6 +47,12 @@ public class FileSystemUtil {
private static final org.slf4j.Logger logger = org.slf4j.LoggerFactory.getLogger(FileSystemUtil.class);
+ private static int recursiveListingMaxSize;
+
+ static {
+ recursiveListingMaxSize = DrillConfig.create().getInt(ExecConstants.RECURSIVE_FILE_LISTING_MAX_SIZE);
+ }
Review Comment:
This looks wonky but correct.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@drill.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [drill] jnturton commented on pull request #2632: DRILL-8283: Add a configurable recursive file listing size limit
Posted by GitBox <gi...@apache.org>.
jnturton commented on PR #2632:
URL: https://github.com/apache/drill/pull/2632#issuecomment-1234106371
I just went through an exercise to replace the `boolean recursive` parameter with a new `RecursionOpts recurOpts` that allows the specification of the max listing size, but that change rippled and in some cases callers _also_ don't have access to the Drill config. I now think the only reasonable way for this limit to reach the file listing utility classes is by it becoming an env var.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@drill.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [drill] jnturton commented on a diff in pull request #2632: DRILL-8283: Add a configurable recursive file listing size limit
Posted by GitBox <gi...@apache.org>.
jnturton commented on code in PR #2632:
URL: https://github.com/apache/drill/pull/2632#discussion_r953427756
##########
exec/java-exec/src/main/java/org/apache/drill/exec/util/FileSystemUtil.java:
##########
@@ -42,6 +47,12 @@ public class FileSystemUtil {
private static final org.slf4j.Logger logger = org.slf4j.LoggerFactory.getLogger(FileSystemUtil.class);
+ private static int recursiveListingMaxSize;
+
+ static {
+ recursiveListingMaxSize = DrillConfig.create().getInt(ExecConstants.RECURSIVE_FILE_LISTING_MAX_SIZE);
Review Comment:
This route to the config option felt pretty weird, I don't if there's a better way?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@drill.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [drill] jnturton commented on a diff in pull request #2632: DRILL-8283: Add a configurable recursive file listing size limit
Posted by GitBox <gi...@apache.org>.
jnturton commented on code in PR #2632:
URL: https://github.com/apache/drill/pull/2632#discussion_r954781871
##########
exec/java-exec/src/main/java/org/apache/drill/exec/util/FileSystemUtil.java:
##########
@@ -302,12 +332,32 @@ protected List<FileStatus> compute() {
List<RecursiveListing> tasks = new ArrayList<>();
try {
- for (FileStatus status : fs.listStatus(path, filter)) {
+ FileStatus[] dirFs = fs.listStatus(path, filter);
+ if (recursiveListingMaxSize > 0 && fileCounter.addAndGet(dirFs.length) > recursiveListingMaxSize) {
+ throw UserException
Review Comment:
@vvysotskyi I've added an attempt to do that now.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@drill.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [drill] jnturton commented on a diff in pull request #2632: DRILL-8283: Add a configurable recursive file listing size limit
Posted by GitBox <gi...@apache.org>.
jnturton commented on code in PR #2632:
URL: https://github.com/apache/drill/pull/2632#discussion_r960531148
##########
exec/java-exec/src/main/java/org/apache/drill/exec/util/FileSystemUtil.java:
##########
@@ -42,6 +47,12 @@ public class FileSystemUtil {
private static final org.slf4j.Logger logger = org.slf4j.LoggerFactory.getLogger(FileSystemUtil.class);
+ private static int recursiveListingMaxSize;
+
+ static {
+ recursiveListingMaxSize = DrillConfig.create().getInt(ExecConstants.RECURSIVE_FILE_LISTING_MAX_SIZE);
+ }
Review Comment:
It's gone now.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@drill.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org