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.

Sign up for GitHub

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

CheckCharset on PostData to prevent hanging or crashing #564

Open
wants to merge 6 commits into
base: main
Choose a base branch
from
Open

CheckCharset on PostData to prevent hanging or crashing #564

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
d59b3a4
check characterset
thebehera Sep 15, 2017
f78e544
no message
thebehera Sep 15, 2017
561f7f7
update error messages
thebehera Sep 15, 2017
d603cf3
NPE check
thebehera Sep 18, 2017
fa44a90
switch to SQLCipher
ArikYa Oct 16, 2017
43d07c5
Merge pull request #1 from ArikYa/check-charset
thebehera Oct 16, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion build.gradle
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ buildscript {
jcenter()
}
dependencies {
classpath 'com.android.tools.build:gradle:2.3.0'
classpath 'com.android.tools.build:gradle:2.3.3'
}
}

Expand Down
1 change: 1 addition & 0 deletions stetho/build.gradle
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ dependencies {
compile 'com.google.code.findbugs:jsr305:2.0.1'

compile 'com.android.support:appcompat-v7:23.0.1' // optional
compile 'net.zetetic:android-database-sqlcipher:3.5.7@aar'
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You appear to have a number of unrelated changes here that should be removed. Also an aside, you don't need this SQLCipher thing directly in Stetho as you can just install your own DatabaseDriver2 implementation now (see SqliteDatabaseDriver and work from there as needed).


testCompile 'junit:junit:4.12'
testCompile('org.robolectric:robolectric:2.4') {
Expand Down
4 changes: 3 additions & 1 deletion stetho/src/main/java/com/facebook/stetho/Stetho.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -82,6 +82,7 @@
* the {@code stetho-sample} for more information.
*/
public class Stetho {
public static String password = "";
private Stetho() {
}

Expand All @@ -101,7 +102,8 @@ public static InitializerBuilder newInitializerBuilder(Context context) {
* first socket connection is received, allowing this to be safely used for debug builds on
* even low-end hardware without noticeably affecting performance.
*/
public static void initializeWithDefaults(final Context context) {
public static void initializeWithDefaults(final Context context, String password) {
Stetho.password = password;
initialize(new Initializer(context) {
@Override
protected Iterable<DumperPlugin> getDumperPlugins() {
Expand Down
3 changes: 2 additions & 1 deletion stetho/src/main/java/com/facebook/stetho/inspector/database/DatabaseConnectionProvider.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,8 @@

package com.facebook.stetho.inspector.database;

import android.database.sqlite.SQLiteDatabase;
//import android.database.sqlite.SQLiteDatabase;
import net.sqlcipher.database.SQLiteDatabase;
import android.database.sqlite.SQLiteException;

import java.io.File;
Expand Down
7 changes: 5 additions & 2 deletions ...c/main/java/com/facebook/stetho/inspector/database/DefaultDatabaseConnectionProvider.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,13 +9,16 @@

package com.facebook.stetho.inspector.database;

import android.database.sqlite.SQLiteDatabase;
//import android.database.sqlite.SQLiteDatabase;
import net.sqlcipher.database.SQLiteDatabase;
import android.database.sqlite.SQLiteException;

import com.facebook.stetho.inspector.database.SQLiteDatabaseCompat.SQLiteOpenOptions;

import java.io.File;

import static com.facebook.stetho.Stetho.password;

/**
* Opens the requested database using
* {@link SQLiteDatabase#openDatabase(String, SQLiteDatabase.CursorFactory, int)} directly.
Expand Down Expand Up @@ -62,7 +65,7 @@ protected SQLiteDatabase performOpen(File databaseFile, @SQLiteOpenOptions int o
flags |= compatInstance.provideOpenFlags(options);

SQLiteDatabase db = SQLiteDatabase.openDatabase(
databaseFile.getAbsolutePath(),
databaseFile.getAbsolutePath(), password,
null /* cursorFactory */,
flags);
compatInstance.enableFeatures(options, db);
Expand Down
18 changes: 10 additions & 8 deletions stetho/src/main/java/com/facebook/stetho/inspector/database/SQLiteDatabaseCompat.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,8 @@
package com.facebook.stetho.inspector.database;

import android.annotation.TargetApi;
import android.database.sqlite.SQLiteDatabase;
//import android.database.sqlite.SQLiteDatabase;
import net.sqlcipher.database.SQLiteDatabase;
import android.os.Build;
import android.support.annotation.IntDef;

Expand All @@ -31,9 +32,10 @@ public abstract class SQLiteDatabaseCompat {
private static final SQLiteDatabaseCompat sInstance;

static {
if (Build.VERSION.SDK_INT >= Build.VERSION_CODES.JELLY_BEAN) {
sInstance = new JellyBeanAndBeyondImpl();
} else if (Build.VERSION.SDK_INT >= Build.VERSION_CODES.HONEYCOMB) {
// if (Build.VERSION.SDK_INT >= Build.VERSION_CODES.JELLY_BEAN) {
// sInstance = new JellyBeanAndBeyondImpl();
// } else
if (Build.VERSION.SDK_INT >= Build.VERSION_CODES.HONEYCOMB) {
sInstance = new HoneycombImpl();
} else {
sInstance = new NoopImpl();
Expand All @@ -47,7 +49,7 @@ public static SQLiteDatabaseCompat getInstance() {
public abstract int provideOpenFlags(@SQLiteOpenOptions int openOptions);
public abstract void enableFeatures(@SQLiteOpenOptions int openOptions, SQLiteDatabase db);

@TargetApi(Build.VERSION_CODES.JELLY_BEAN)
/*@TargetApi(Build.VERSION_CODES.JELLY_BEAN)
private static class JellyBeanAndBeyondImpl extends SQLiteDatabaseCompat {
@Override
public int provideOpenFlags(@SQLiteOpenOptions int openOptions) {
Expand All @@ -64,9 +66,9 @@ public void enableFeatures(@SQLiteOpenOptions int openOptions, SQLiteDatabase db
db.setForeignKeyConstraintsEnabled(true);
}
}
}
}*/

@TargetApi(Build.VERSION_CODES.HONEYCOMB)
// @TargetApi(Build.VERSION_CODES.HONEYCOMB)
private static class HoneycombImpl extends SQLiteDatabaseCompat {
@Override
public int provideOpenFlags(@SQLiteOpenOptions int openOptions) {
Expand All @@ -76,7 +78,7 @@ public int provideOpenFlags(@SQLiteOpenOptions int openOptions) {
@Override
public void enableFeatures(@SQLiteOpenOptions int openOptions, SQLiteDatabase db) {
if ((openOptions & ENABLE_WRITE_AHEAD_LOGGING) != 0) {
db.enableWriteAheadLogging();
db.execSQL("PRAGMA journal_mode=WAL;");
}

if ((openOptions & ENABLE_FOREIGN_KEY_CONSTRAINTS) != 0) {
Expand Down
10 changes: 7 additions & 3 deletions stetho/src/main/java/com/facebook/stetho/inspector/database/SqliteDatabaseDriver.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,9 +12,12 @@
import android.annotation.TargetApi;
import android.content.Context;
import android.database.Cursor;
import android.database.sqlite.SQLiteDatabase;
import android.database.sqlite.SQLiteException;
import android.database.sqlite.SQLiteStatement;
//import android.database.sqlite.SQLiteDatabase;
import net.sqlcipher.database.SQLiteDatabase;
import net.sqlcipher.database.SQLiteException;
import net.sqlcipher.database.SQLiteStatement;
//import android.database.sqlite.SQLiteException;
//import android.database.sqlite.SQLiteStatement;

import com.facebook.stetho.common.Util;
import com.facebook.stetho.inspector.protocol.module.Database;
Expand Down Expand Up @@ -57,6 +60,7 @@ public SqliteDatabaseDriver(Context context) {
context,
new DefaultDatabaseFilesProvider(context),
new DefaultDatabaseConnectionProvider());
SQLiteDatabase.loadLibs(context);
}

/**
Expand Down
43 changes: 35 additions & 8 deletions stetho/src/main/java/com/facebook/stetho/inspector/network/NetworkEventReporterImpl.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,22 +10,31 @@
package com.facebook.stetho.inspector.network;

import android.os.SystemClock;

import com.facebook.stetho.common.Utf8Charset;
import com.facebook.stetho.inspector.console.CLog;
import com.facebook.stetho.inspector.protocol.module.Console;
import com.facebook.stetho.inspector.protocol.module.Network;
import com.facebook.stetho.inspector.protocol.module.Page;

import org.json.JSONException;
import org.json.JSONObject;

import javax.annotation.Nonnull;
import javax.annotation.Nullable;
import java.io.IOException;
import java.io.InputStream;
import java.io.OutputStream;
import java.nio.ByteBuffer;
import java.nio.CharBuffer;
import java.nio.charset.CharacterCodingException;
import java.nio.charset.Charset;
import java.nio.charset.CharsetDecoder;
import java.nio.charset.CodingErrorAction;
import java.util.ArrayList;
import java.util.concurrent.atomic.AtomicInteger;

import javax.annotation.Nonnull;
import javax.annotation.Nullable;

/**
* Implementation of {@link NetworkEventReporter} which allows callers to inform the Stetho
* system of network traffic. Callers can safely eagerly access this class and store a
Expand All @@ -39,6 +48,8 @@ public class NetworkEventReporterImpl implements NetworkEventReporter {

private static NetworkEventReporter sInstance;

private static final CharsetDecoder decoder = Charset.forName(Utf8Charset.NAME).newDecoder();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Make this a non-static member variable and initialize it in the constructor. No need to muck up the get() method below.


private NetworkEventReporterImpl() {
}

Expand All @@ -49,6 +60,8 @@ private NetworkEventReporterImpl() {
public static synchronized NetworkEventReporter get() {
if (sInstance == null) {
sInstance = new NetworkEventReporterImpl();
decoder.onUnmappableCharacter(CodingErrorAction.REPORT);
decoder.onMalformedInput(CodingErrorAction.REPORT);
}
return sInstance;
}
Expand Down Expand Up @@ -115,15 +128,29 @@ private static String readBodyAsString(
InspectorRequest request) {
try {
byte[] body = request.body();
if (body != null) {
return new String(body, Utf8Charset.INSTANCE);
if (body == null || body.length == 0) {
return "";
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It used to return null if body was null before. Should it still?

}
try {
CharBuffer charBuffer = decoder.decode(ByteBuffer.wrap(body));
return charBuffer.toString();
} catch (CharacterCodingException e) {
String logMessage = "Charset in POST/PUT is not UTF-8. Data (length:"+ body.length
+") cannot be represented as a string. ";
CLog.writeToConsole(
peerManager,
Console.MessageLevel.WARNING,
Console.MessageSource.NETWORK,
logMessage + e);
return logMessage;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, I like returning the log message here, but it suggests actually that we'd be better off merging the error cases below:

try {
 // decode...
} catch (IOException | OutOfMemoryError e) {
  String error = determineErrorMessage(e);
  CLog.writeToConsole("Could not reproduce POST/PUT body: " + error);
  return error;
}

Then determineErrorMessage could peak and see if it's a CharsetCodingException and craft your better error message above.

}
} catch (IOException | OutOfMemoryError e) {
CLog.writeToConsole(
peerManager,
Console.MessageLevel.WARNING,
Console.MessageSource.NETWORK,
"Could not reproduce POST body: " + e);
peerManager,
Console.MessageLevel.WARNING,
Console.MessageSource.NETWORK,
"Could not reproduce POST/PUT body: " + e);

}
return null;
}
Expand Down