Skip to content

Commit 0e45e04

Browse files
ejona86ejona86
authored
Avoid accidental locale-sensitive String.format()
%s is fairly safe (requires a Formattable to use Locale), so %d is the main risk item. Places that really didn't need to use String.format() were converted to plain string concatenation. Logging locations were generally converted to using the log infrastructure's delayed formatting, which is generally locale-sensitive but we're okay with that. That wasn't done in okhttp, however, because Android frequently doesn't use MessageFormat so we'd lose the parameters. Everywhere else was explicitly defined to be Locale.US, to be consistent independent of the default system locale.
1 parent 7568f8c commit 0e45e04

File tree

24 files changed

+89
-50
lines changed

24 files changed

+89
-50
lines changed

‎core/src/main/java/io/grpc/inprocess/InProcessTransport.java‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -66,6 +66,7 @@
6666
import java.util.Collections;
6767
import java.util.IdentityHashMap;
6868
import java.util.List;
69+
import java.util.Locale;
6970
import java.util.Set;
7071
import java.util.concurrent.Executor;
7172
import java.util.concurrent.ScheduledExecutorService;
@@ -240,6 +241,7 @@ public synchronized ClientStream newStream(
240241
// statuscodes.md is updated.
241242
Status status = Status.RESOURCE_EXHAUSTED.withDescription(
242243
String.format(
244+
Locale.US,
243245
"Request metadata larger than %d: %d",
244246
serverMaxInboundMetadataSize,
245247
metadataSize));
@@ -554,6 +556,7 @@ public void writeHeaders(Metadata headers) {
554556
// Status, which may need to be updated if statuscodes.md is updated.
555557
Status failedStatus = Status.RESOURCE_EXHAUSTED.withDescription(
556558
String.format(
559+
Locale.US,
557560
"Response header metadata larger than %d: %d",
558561
clientMaxInboundMetadataSize,
559562
metadataSize));
@@ -593,6 +596,7 @@ public void close(Status status, Metadata trailers) {
593596
// Status, which may need to be updated if statuscodes.md is updated.
594597
status = Status.RESOURCE_EXHAUSTED.withDescription(
595598
String.format(
599+
Locale.US,
596600
"Response header metadata larger than %d: %d",
597601
clientMaxInboundMetadataSize,
598602
metadataSize));

‎core/src/main/java/io/grpc/internal/AbstractClientStream.java‎

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -331,8 +331,7 @@ protected void inboundHeadersReceived(Metadata headers) {
331331
if (compressedStream) {
332332
deframeFailed(
333333
Status.INTERNAL
334-
.withDescription(
335-
String.format("Full stream and gRPC message encoding cannot both be set"))
334+
.withDescription("Full stream and gRPC message encoding cannot both be set")
336335
.asRuntimeException());
337336
return;
338337
}

‎core/src/main/java/io/grpc/internal/ClientCallImpl.java‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -358,12 +358,13 @@ private static void logIfContextNarrowedTimeout(
358358

359359
long effectiveTimeout = max(0, effectiveDeadline.timeRemaining(TimeUnit.NANOSECONDS));
360360
StringBuilder builder = new StringBuilder(String.format(
361+
Locale.US,
361362
"Call timeout set to '%d' ns, due to context deadline.", effectiveTimeout));
362363
if (callDeadline == null) {
363364
builder.append(" Explicit call timeout was not set.");
364365
} else {
365366
long callTimeout = callDeadline.timeRemaining(TimeUnit.NANOSECONDS);
366-
builder.append(String.format(" Explicit call timeout was '%d' ns.", callTimeout));
367+
builder.append(String.format(Locale.US, " Explicit call timeout was '%d' ns.", callTimeout));
367368
}
368369

369370
log.fine(builder.toString());

‎core/src/main/java/io/grpc/internal/DelayedClientCall.java‎

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -98,12 +98,14 @@ private ScheduledFuture<?> scheduleDeadlineIfNeeded(
9898
StringBuilder builder =
9999
new StringBuilder(
100100
String.format(
101+
Locale.US,
101102
"Call timeout set to '%d' ns, due to context deadline.", remainingNanos));
102103
if (deadline == null) {
103104
builder.append(" Explicit call timeout was not set.");
104105
} else {
105106
long callTimeout = deadline.timeRemaining(TimeUnit.NANOSECONDS);
106-
builder.append(String.format(" Explicit call timeout was '%d' ns.", callTimeout));
107+
builder.append(String.format(
108+
Locale.US, " Explicit call timeout was '%d' ns.", callTimeout));
107109
}
108110
logger.fine(builder.toString());
109111
}

‎core/src/main/java/io/grpc/internal/JsonUtil.java‎

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -20,6 +20,7 @@
2020

2121
import java.text.ParseException;
2222
import java.util.List;
23+
import java.util.Locale;
2324
import java.util.Map;
2425
import java.util.concurrent.TimeUnit;
2526
import javax.annotation.Nullable;
@@ -244,7 +245,8 @@ public static Boolean getBoolean(Map<String, ?> obj, String key) {
244245
for (int i = 0; i < rawList.size(); i++) {
245246
if (!(rawList.get(i) instanceof Map)) {
246247
throw new ClassCastException(
247-
String.format("value %s for idx %d in %s is not object", rawList.get(i), i, rawList));
248+
String.format(
249+
Locale.US, "value %s for idx %d in %s is not object", rawList.get(i), i, rawList));
248250
}
249251
}
250252
return (List<Map<String, ?>>) rawList;
@@ -260,6 +262,7 @@ public static List<String> checkStringList(List<?> rawList) {
260262
if (!(rawList.get(i) instanceof String)) {
261263
throw new ClassCastException(
262264
String.format(
265+
Locale.US,
263266
"value '%s' for idx %d in '%s' is not string", rawList.get(i), i, rawList));
264267
}
265268
}

‎core/src/main/java/io/grpc/internal/MessageDeframer.java‎

Lines changed: 5 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -28,6 +28,7 @@
2828
import java.io.FilterInputStream;
2929
import java.io.IOException;
3030
import java.io.InputStream;
31+
import java.util.Locale;
3132
import java.util.zip.DataFormatException;
3233
import javax.annotation.Nullable;
3334
import javax.annotation.concurrent.NotThreadSafe;
@@ -386,7 +387,7 @@ private void processHeader() {
386387
requiredLength = nextFrame.readInt();
387388
if (requiredLength < 0 || requiredLength > maxInboundMessageSize) {
388389
throw Status.RESOURCE_EXHAUSTED.withDescription(
389-
String.format("gRPC message exceeds maximum size %d: %d",
390+
String.format(Locale.US, "gRPC message exceeds maximum size %d: %d",
390391
maxInboundMessageSize, requiredLength))
391392
.asRuntimeException();
392393
}
@@ -516,9 +517,9 @@ private void reportCount() {
516517

517518
private void verifySize() {
518519
if (count > maxMessageSize) {
519-
throw Status.RESOURCE_EXHAUSTED.withDescription(String.format(
520-
"Decompressed gRPC message exceeds maximum size %d",
521-
maxMessageSize)).asRuntimeException();
520+
throw Status.RESOURCE_EXHAUSTED
521+
.withDescription("Decompressed gRPC message exceeds maximum size " + maxMessageSize)
522+
.asRuntimeException();
522523
}
523524
}
524525
}

‎core/src/main/java/io/grpc/internal/MessageFramer.java‎

Lines changed: 7 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -34,6 +34,7 @@
3434
import java.nio.ByteBuffer;
3535
import java.util.ArrayList;
3636
import java.util.List;
37+
import java.util.Locale;
3738
import javax.annotation.Nullable;
3839

3940
/**
@@ -172,7 +173,8 @@ private int writeUncompressed(InputStream message, int messageLength) throws IOE
172173
if (maxOutboundMessageSize >= 0 && written > maxOutboundMessageSize) {
173174
throw Status.RESOURCE_EXHAUSTED
174175
.withDescription(
175-
String.format("message too large %d > %d", written , maxOutboundMessageSize))
176+
String.format(
177+
Locale.US, "message too large %d > %d", written , maxOutboundMessageSize))
176178
.asRuntimeException();
177179
}
178180
writeBufferChain(bufferChain, false);
@@ -192,7 +194,8 @@ private int writeCompressed(InputStream message, int unusedMessageLength) throws
192194
if (maxOutboundMessageSize >= 0 && written > maxOutboundMessageSize) {
193195
throw Status.RESOURCE_EXHAUSTED
194196
.withDescription(
195-
String.format("message too large %d > %d", written , maxOutboundMessageSize))
197+
String.format(
198+
Locale.US, "message too large %d > %d", written , maxOutboundMessageSize))
196199
.asRuntimeException();
197200
}
198201

@@ -215,7 +218,8 @@ private int writeKnownLengthUncompressed(InputStream message, int messageLength)
215218
if (maxOutboundMessageSize >= 0 && messageLength > maxOutboundMessageSize) {
216219
throw Status.RESOURCE_EXHAUSTED
217220
.withDescription(
218-
String.format("message too large %d > %d", messageLength , maxOutboundMessageSize))
221+
String.format(
222+
Locale.US, "message too large %d > %d", messageLength , maxOutboundMessageSize))
219223
.asRuntimeException();
220224
}
221225
headerScratch.clear();

‎core/src/main/java/io/grpc/internal/ProxyDetectorImpl.java‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -133,7 +133,7 @@ public PasswordAuthentication requestPasswordAuthentication(
133133
// let url be null
134134
log.log(
135135
Level.WARNING,
136-
String.format("failed to create URL for Authenticator: %s %s", protocol, host));
136+
"failed to create URL for Authenticator: {0} {1}", new Object[] {protocol, host});
137137
}
138138
// TODO(spencerfang): consider using java.security.AccessController here
139139
return Authenticator.requestPasswordAuthentication(

‎core/src/test/java/io/grpc/internal/MessageDeframerTest.java‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -49,6 +49,7 @@
4949
import java.util.Arrays;
5050
import java.util.Collection;
5151
import java.util.List;
52+
import java.util.Locale;
5253
import java.util.concurrent.TimeUnit;
5354
import java.util.zip.GZIPOutputStream;
5455
import org.junit.Before;
@@ -507,7 +508,7 @@ private static void checkStats(
507508
for (int i = 0; i < count; i++) {
508509
assertEquals("inboundMessage(" + i + ")", tracer.nextInboundEvent());
509510
assertEquals(
510-
String.format("inboundMessageRead(%d, %d, -1)", i, sizes[i * 2]),
511+
String.format(Locale.US, "inboundMessageRead(%d, %d, -1)", i, sizes[i * 2]),
511512
tracer.nextInboundEvent());
512513
expectedWireSize += sizes[i * 2];
513514
expectedUncompressedSize += sizes[i * 2 + 1];

‎core/src/test/java/io/grpc/internal/MessageFramerTest.java‎

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -34,6 +34,7 @@
3434
import java.nio.ByteBuffer;
3535
import java.nio.ByteOrder;
3636
import java.util.Arrays;
37+
import java.util.Locale;
3738
import org.junit.Before;
3839
import org.junit.Rule;
3940
import org.junit.Test;
@@ -382,7 +383,8 @@ private void checkStats(long... sizes) {
382383
for (int i = 0; i < count; i++) {
383384
assertEquals("outboundMessage(" + i + ")", tracer.nextOutboundEvent());
384385
assertEquals(
385-
String.format("outboundMessageSent(%d, %d, %d)", i, sizes[i * 2], sizes[i * 2 + 1]),
386+
String.format(
387+
Locale.US, "outboundMessageSent(%d, %d, %d)", i, sizes[i * 2], sizes[i * 2 + 1]),
386388
tracer.nextOutboundEvent());
387389
expectedWireSize += sizes[i * 2];
388390
expectedUncompressedSize += sizes[i * 2 + 1];

0 commit comments

Comments
 (0)