Fix missing format strings in calls to DruidException.build #15056

pranavbhole · 2023-09-29T03:45:30Z

Description

PR fix exception eating duck and helps to surface the original exception,

String.format(Locale.ENGLISH, null, 1); or String.format(Locale.ENGLISH, null);

throws NPE and I safeguarding the nonStrictFormat function with null check.

Exception in thread "main" java.lang.NullPointerException
	at java.util.regex.Matcher.getTextLength(Matcher.java:1283)
	at java.util.regex.Matcher.reset(Matcher.java:309)
	at java.util.regex.Matcher.<init>(Matcher.java:229)
	at java.util.regex.Pattern.matcher(Pattern.java:1093)
	at java.util.Formatter.parse(Formatter.java:2547)
	at java.util.Formatter.format(Formatter.java:2501)
	at java.util.Formatter.format(Formatter.java:2455)
	at java.lang.String.format(String.java:2981)

Fixed the bug ...

Renamed the class ...

Added a forbidden-apis entry ...

This PR has:

kfaraz · 2023-09-29T03:57:54Z

processing/src/main/java/org/apache/druid/java/util/common/StringUtils.java

@@ -373,16 +373,24 @@ public static String nonStrictFormat(String message, Object... formatArgs)
    if (formatArgs == null || formatArgs.length == 0) {
      return message;
    }
+    if (message == null) {


Instead of these changes, it would be simpler to just do a Preconditions.checkNotNull(message) at the start of this method.

Preconditions check will throw an exception again. Some of the exceptions are not having message at all and we are calling nonStrictFormat while making DruidException, esp exception from third party library. As the name says, expectation from nonStrictFormat in this case is that it should just give back some non null string. Best case effort is to append the args to string and give it back.

If we add precondition then we need to safeguard the invoker lines with not null.

Based on comments, I found the instances where we could potentially pass null as formatString. I fixed all of them I could find in this PR. I am still doubtful about adding precondition check to nonStrictFormat as it is used at tons of places. I removed the change here and fixing the invokers.

gianm · 2023-09-29T04:10:20Z

Anything that is calling StringUtils.nonStrictFormat with a null format string is a bug. In the case of #15055, the bug is that QueryExceptionCompat#makeException calls druidException.build(exception, exception.getMessage()). The build method expects a format string in the second parameter. The call should instead be .build(exception, "%s", exception.getMessage()).

IMO, better to fix the above bug than to work around it in StringUtils#nonStrictFormat.

Btw, looking at other calls to DruidException#build I see a bunch of places that have the same problem, where they pass a regular string instead of a format string. Many of them should be changed from build(e, s) to build(e, "%s", s). We should fix these too. Even if s is not null, it is problematic if it contains % characters that will be interpreted in special ways by the formatter.

kgyrtkirk

not sure how null could have been passed as message ; but maybe it would be good to mark that arg as @NonNull in case something could process and uncover its source in the future

or possibly enable:
https://errorprone.info/api/latest/com/google/errorprone/annotations/FormatString.html
so that -DstrictCompile could possibly catch formatting issues
not sure if it that would have worked in this case...

processing/src/test/java/org/apache/druid/java/util/common/StringUtilsTest.java

pranavbhole · 2023-09-29T05:40:10Z

Anything that is calling StringUtils.nonStrictFormat with a null format string is a bug. In the case of #15055, the bug is that QueryExceptionCompat#makeException calls druidException.build(exception, exception.getMessage()). The build method expects a format string in the second parameter. The call should instead be .build(exception, "%s", exception.getMessage()).

IMO, better to fix the above bug than to work around it in StringUtils#nonStrictFormat.

Btw, looking at other calls to DruidException#build I see a bunch of places that have the same problem, where they pass a regular string instead of a format string. Many of them should be changed from build(e, s) to build(e, "%s", s). We should fix these too. Even if s is not null, it is problematic if it contains % characters that will be interpreted in special ways by the formatter.

Thank you, I was able to find the potential null at multiple places, fixed all of them I could find. Added test to StringUtilsTest to ensure formatting with string containing % char.

kgyrtkirk · 2023-09-29T10:44:21Z

I've played around a bit with adding @FormatMethod/@FormatString annotations; it uncovers a few more real and theoretical issues - but it will take some time to address all of them in the whole project :)

https://github.com/kgyrtkirk/druid/tree/formatmethod-formatstring

pranavbhole · 2023-09-29T14:31:49Z

I've played around a bit with adding @FormatMethod/@FormatString annotations; it uncovers a few more real and theoretical issues - but it will take some time to address all of them in the whole project :)

https://github.com/kgyrtkirk/druid/tree/formatmethod-formatstring

great, it would be helpful to open another followup PR after this with annotation.

pranavbhole · 2023-09-29T14:36:42Z

@gianm @kfaraz
Addressed comments, please review again.
Thank you

somu-imply

Nice find ! LGTM

gianm · 2023-09-29T20:23:22Z

I've played around a bit with adding @FormatMethod/@FormatString annotations; it uncovers a few more real and theoretical issues - but it will take some time to address all of them in the whole project :)

Neat, nice idea!

gianm

Changes LGTM, although the title of the PR is no longer correct. Please update it to something like "Fix missing format strings in calls to DruidException.build"

clintropolis · 2023-09-29T21:33:36Z

processing/src/test/java/org/apache/druid/java/util/common/StringUtilsTest.java

@@ -386,4 +386,19 @@ public void testJavaStringCompare()
      }
    }
  }
+
+  @Test(expected = NullPointerException.class)


nit: maybe use Assert.assertThrows instead of expected exceptions

…5056) * Fix the NPE bug in nonStrictFormat * using non null format string * using Assert.assertThrows

Fix the NPE bug in nonStrictFormat

abec4a7

pranavbhole marked this pull request as ready for review September 29, 2023 03:46

kfaraz reviewed Sep 29, 2023

View reviewed changes

kgyrtkirk approved these changes Sep 29, 2023

View reviewed changes

github-advanced-security bot found potential problems Sep 29, 2023

View reviewed changes

processing/src/test/java/org/apache/druid/java/util/common/StringUtilsTest.java Fixed Show fixed Hide fixed

github-actions bot added Area - Batch Ingestion Area - Querying Area - MSQ For multi stage queries - https://github.com/apache/druid/issues/12262 labels Sep 29, 2023

using non null format string

45739b4

pranavbhole force-pushed the nonStrictFormatFix branch from 2813a5e to 45739b4 Compare September 29, 2023 05:19

somu-imply approved these changes Sep 29, 2023

View reviewed changes

gianm reviewed Sep 29, 2023

View reviewed changes

pranavbhole changed the title ~~Fix the NPE bug in nonStrictFormat~~ Fix missing format strings in calls to DruidException.build Sep 29, 2023

clintropolis approved these changes Sep 29, 2023

View reviewed changes

using Assert.assertThrows

f4bd272

gianm approved these changes Sep 30, 2023

View reviewed changes

gianm merged commit 07c28f1 into apache:master Sep 30, 2023
74 checks passed

pranavbhole deleted the nonStrictFormatFix branch September 30, 2023 00:11

LakshSingla added this to the 28.0 milestone Oct 12, 2023

ektravel pushed a commit to ektravel/druid that referenced this pull request Oct 16, 2023

Fix missing format strings in calls to DruidException.build (apache#1…

6868054

…5056) * Fix the NPE bug in nonStrictFormat * using non null format string * using Assert.assertThrows

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix missing format strings in calls to DruidException.build #15056

Fix missing format strings in calls to DruidException.build #15056

pranavbhole commented Sep 29, 2023 •

edited

Loading

kfaraz Sep 29, 2023 •

edited

Loading

pranavbhole Sep 29, 2023

pranavbhole Sep 29, 2023

gianm commented Sep 29, 2023 •

edited

Loading

kgyrtkirk left a comment

pranavbhole commented Sep 29, 2023

kgyrtkirk commented Sep 29, 2023

pranavbhole commented Sep 29, 2023

pranavbhole commented Sep 29, 2023

somu-imply left a comment

gianm commented Sep 29, 2023

gianm left a comment

clintropolis Sep 29, 2023

pranavbhole Sep 29, 2023

Fix missing format strings in calls to DruidException.build #15056

Fix missing format strings in calls to DruidException.build #15056

Conversation

pranavbhole commented Sep 29, 2023 • edited Loading

Description

Fixed the bug ...

Renamed the class ...

Added a forbidden-apis entry ...

kfaraz Sep 29, 2023 • edited Loading

Choose a reason for hiding this comment

pranavbhole Sep 29, 2023

Choose a reason for hiding this comment

pranavbhole Sep 29, 2023

Choose a reason for hiding this comment

gianm commented Sep 29, 2023 • edited Loading

kgyrtkirk left a comment

Choose a reason for hiding this comment

pranavbhole commented Sep 29, 2023

kgyrtkirk commented Sep 29, 2023

pranavbhole commented Sep 29, 2023

pranavbhole commented Sep 29, 2023

somu-imply left a comment

Choose a reason for hiding this comment

gianm commented Sep 29, 2023

gianm left a comment

Choose a reason for hiding this comment

clintropolis Sep 29, 2023

Choose a reason for hiding this comment

pranavbhole Sep 29, 2023

Choose a reason for hiding this comment

pranavbhole commented Sep 29, 2023 •

edited

Loading

kfaraz Sep 29, 2023 •

edited

Loading

gianm commented Sep 29, 2023 •

edited

Loading