谷本 心 in せろ部屋 このページをアンテナに追加 RSSフィード

2010-02-04

[]レビューで鍛えるJavaコーディング力 その7(文字コードチェック)

今回は、文字コードのチェック(エンコーディングチェック)を行う処理に関する問題です。

問題

以下のコードの問題を指摘し、修正してください。

ただし、問題は複数あることもあれば、全くないこともあります。

public class StringValidator {
	private static boolean checkCharacterCode(String str, String encoding) {
		if (str == null) {
			return true;
		}

		try {
			byte[] bytes = str.getBytes(encoding);
			return str.equals(new String(bytes, encoding));
		} catch (UnsupportedEncodingException ex) {
			throw new RuntimeException("エンコード名称が正しくありません。", ex);
		}
	}

	public static boolean isWindows31j(String str) {
		return checkCharacterCode(str, "Windows-31j");
	}

	public static boolean isSJIS(String str) {
		return checkCharacterCode(str, "SJIS");
	}

	public static boolean isEUC(String str) {
		return checkCharacterCode(str, "euc-jp");
	}

	public static boolean isUTF8(String str) {
		return checkCharacterCode(str, "UTF-8");
	}
}

「そもそも文字コードチェックなどする必要があるのか?」なんて

疑問があるかも知れませんが、たとえばWebアプリケーションにおいて、

Web画面以外にも、テキストファイルやPDFファイル出力機能などと連携したり

携帯向け情報画面を別途作成するような場合に、ちょくちょく発生する要件ですね。

コラム

文字コードエンコードについては、

私自身、いくつかの案件でハマったり、学んだりしてきたのですが、

いまだに新しい問題を見つけてしまうことがある、一筋縄ではいかない問題だと思います。


っていうか、そもそも分かりづらいですよね?


 ・「文字コード」と「エンコード」の区別がつかない

 ・「ユニコード」の言葉の意味が実はよく分からない

 ・「文字コード」と「エンコード」の間違いにツッコミを入れたら「細かい」って言われてしまった

 ・JavaUnicode変換表と、Unicodeコンソーシアムの変換表が違う事を知らず、ドツボにハマった

 ・文字コードとかよく分からないんだけど、俺のシステムは、ちゃんと動いてるらしい

 ・実は使われるとダメな文字があるんだけど、ユーザが使ってないから、たまたま助かってる


なんて、一つや二つ、思いあたる所がある方も多いでしょう。


今回の問題や、また別エントリなどを通して

文字コードエンコードの問題の対処方法が分かれば・・・と思います。

解答

さて、今回の答えは「問題なし」です。


今回のソースは、決して万能のチェックとは言えない面もありますが、

ほぼ問題なく、多くの案件で利用できる方法だと思います。


詳しい仕組みなどを説明し始めると、かなり長文になってしまうため

文字コードエンコードUnicodeなどについては、また別にエントリにて説明することにして

今回は、簡単な挙動だけ説明しましょう。

StringValidatorの挙動

StringValidatorのisXxxメソッドを呼び出した際に、

きちんと変換できる文字であれば、、、

 1. getBytes(encoding)を行なうと、その文字列のバイト配列を取得できる。

 2. このバイト配列を使ってnew Stringを行うことで、元の文字列が得られる。

 3. 変換前の文字列と、2で得られた変換後の文字列は、一致する。

という挙動になります。


しかし、指定したエンコード変換できない場合(対象の文字が存在しない場合)、、、

 1. getBytesすると、変換出きない文字を、「?」に相当する「0x3F」にしたバイト配列が取得される。

 2. このバイト配列を使ってnew Stringすると、一部の文字列が「?」に化けてしまう。

 3. 変換前の文字列と、2で得られた文字列は、「?」の部分が一致しない。

という挙動になります。


と言ってもイメージしにくいので、分かりやすいよう、Windows-31jの場合の変換を表にまとめました。

元の文字列元の文字列UnicodegetBytes結果new String結果変換後のUnicode判定
\u30420x82 0xA0\u3042TRUE
あお\u3042 \u304A0x82 0xA0 0x82 0xA8あお\u3042 \u304ATRUE
アオ (半角カナ)\uFF71 \uFF750xB1 0xB5アオ\uFF71 \uFF75TRUE
\u24600x87 0x40\u2460TRUE
お〜い\u304A \uFF5E \u30440x82 0xA8 0x81 0x60 0x82 0xA2お〜い\u304A \uFF5E \u3044TRUE
お〜い\u304A \u301C \u30440x82 0xA8 0x3F 0x82 0xA2?\u304A \u003F \u3044FALSE

この表から分かるように、

Windows-31jで扱える文字は、機種固有文字含めて「TRUE」となりますが、

「〜」いわゆる「気持ち悪いニョロ」などは、

Windows-31jには対応する文字がないため、「FALSE」となるのです。


つまり「元の文字列が、Windows-31jであるかどうか」という判定が

きちんとできている、ということになります。

ラウンドトリップできない文字はどうか?

ところで、世の中には「Windows-31j → UnicodeWindows-31j」の変換の過程で

勝手に文字コードが変わってしまう文字があります。

http://support.microsoft.com/default.aspx?scid=kb;ja;JP170559


さらに「UnicodeWindows-31j → Unicode」の変換の過程で

勝手に文字そのものが変わってしまう文字もあるのです。

http://www.bugbearr.jp/?Java/%E6%96%87%E5%AD%97%E3%82%B3%E3%83%BC%E3%83%89

このページの「Javaでのラウンドトリップ問題」に、その文字の一覧があります。


こうした現象がなぜ起きるのかは、今回は割愛しますが

対応としては、

Windows-31j → UnicodeWindows-31」のラウンドトリップ問題は無視してよく、

UnicodeWindows-31j → Unicode」のラウンドトリップ問題には、注意を払う必要があります。


では、ラウンドトリップ問題が起きる文字列

どのように変換されるのか、また表にまとめてみましょう。

元の文字列元の文字列UnicodegetBytes結果new String結果変換後のUnicode判定
\u22520x81 0xE0\u2252TRUE
\u221a0x81 0xE3\u221aTRUE
«\u00AB0x81 0xE1\u226AFALSE
¢\u00A20x81 0x91¢\uFFE0FALSE

注意を払うべきと書いた「UnicodeWindows-31j → Unicode」については、エラーとしています。

多少悲観的な方法ではありますが、エラーとしておくのが無難でしょう。


エラーとする以外には、問題が起きる文字列

「事前にWindows-31jの文字列に変換する」という対策がありますが、ここでは割愛します。

まとめ

それでは、今回の問題のまとめです。


今回の問題は、文字コードについて少し詳しい方でないと、分かりづらかったかも知れません。

いずれ改めて、文字コードエンコードなどについて説明したエントリを作成する予定ですので

そちらを併せて確認してください。

2010-02-02

[]レビューで鍛えるJavaコーディング力 その6(コレクション)

今回はコレクションを用いたキャッシュに関する問題です。

問題

以下のコードの問題を指摘し、修正してください。

ただし、問題は複数あることもあれば、全くないこともあります。

import java.util.List;

public class IdService {
	/** IDキャッシュ */
	private List<Integer> cache;

	/** リモートシステムへのアクセッサ */
	private RemoteAccessor accessor;

	public IdService() {
		// accessorの初期化は割愛。
		reloadCache();
	}

	/**
	 * IDが存在するかどうかを判定します。
	 * @param id ID
	 * @return IDが存在する場合はtrue、そうでない場合はfalseを返します。
	 */
	public boolean exists(Integer id) {
		return cache.contains(id);
	}

	/**
	 * キャッシュの更新を行います。
	 */
	public void reloadCache() {
		cache = accessor.getIdList();
	}
}

リモートシステムへのアクセス(RemoteAccessorの利用)をできるだけ避けるため、

取得した情報をキャッシュすることが、この処理の目的です。


現実的には、ちょっと仕様(要件)がおかしい感じもしますが、

問題を簡単にするために、敢えてこんな内容にしています。

あくまで目的を達成することを、今回の主眼としてください。

コラム

前回の問題について、id:backpaper0さんからはてブでコメントを頂きました。

getTemplateLines は配列のコピーを返さなくて良いのかな?(変更される恐れがあるので)


これはおっしゃる通りですね。

public String[] getTemplateLines() {
	return this.templateLines;
}

の箇所を、以下のように修正すべきです。

public String[] getTemplateLines() {
	String[] copy = new String[templateLines.length];
	System.arraycopy(templateLines, 0, copy, 0, templateLines.length);
	return copy;
}

あるいは、性能を気にしないなら(あまりオススメできませんが、読みやすくするために)

public String[] getTemplateLines() {
	return StringUtils.splitByWholeSeparatorPreserveAllTokens(this.template, "\r\n");
}

としても良いでしょうか。


問題が不完全な事もちょくちょくありますので

ぜひコメントやトラックバックブクマコメント、スターなどで突っ込んでください m(_ _)m

解答

今回の問題点は、ここです。

public boolean exists(Integer id) {
	return cache.contains(id);
}

ArrayListのcontainsメソッドは、ただの線形検索です。

素数が10や100程度なら1ms以内に終わるでしょうけど、

10000程度にもなってくると、無視出来ない処理時間になってきます。


そもそも「検索(存在判定)」が目的なのですから、ArrayListは不向きです。

ここではSet、実装としてはHashSetなどを使えば良いでしょう。

private Set<Integer> cache;

public void reloadCache() {
	cache = new HashSet<Integer>(accessor.getIdList());
}

また、Collections#binarySerachによる二分探索を使う事も可能です。

その場合、リストのソートをしておくことが前提となるため、以下のような修正になります。

public boolean exists(Integer id) {
	int result = Collections.binarySearch(cache, id);
	return result > -1;
}

public void reloadCache() {
	List<Integer> list = accessor.getIdList();
	Collections.sort(list);
	cache = list;
}

ちなみに私のCore2Duoマシンでは、以下のような処理時間の差が出ました。


10,000要素の線形検索を10,000回繰り返した場合(=全要素を1回ずつ検索した場合)

 ・HashSet : 0ms

 ・ArrayList : 485ms

 ・ArrayList(binarySearch) : 16ms


100,000要素の線形検索を100,000回繰り返した場合(=全要素を1回ずつ検索した場合)

 ・HashSet : 15ms

 ・ArrayList : 47,781ms

 ・ArrayList(binarySearch) : 47ms


基本的に「List#containsは不吉な匂い」と考えて、差し支えないでしょう。

補足

今回の内容について、スレッドセーフ性について疑問を持った人もいるかも知れません。

そのような方は、恐らく以下の2つについて考えたのではないでしょうか。


 1. Listのgetとaddが同時に行われて、問題が発生しないか?

 2. reloadCacheが連続して呼び出されたら、問題が発生しないか?


観点としては大切なのですが、

今回は、いずれの問題も起きることはありません。


処理順序を追って行くと分かるのですが、

cache変数はreloadCacheメソッドの中で、インスタンスを置き換えているため

(また、そのインスタンスに対する処理は、reloadCache内では行わないため)

複数のスレッドからcacheに対して「get」を行うことはあっても、

同時に「get」と「add」することはないので、問題は発生しないのです。


逆に言えば、reloadCacheメソッドの中で、

cacheに対してインスタンスの置き換え以外の処理を行なってしまえば、

問題が起きる可能性があります。


たとえば、解答の中でソートを行う処理がありましたが、

public void reloadCache() {
	List<Integer> list = accessor.getIdList();
	Collections.sort(list);
	cache = list;
}

もしこれを、以下のようにしてしまうと、問題が発生してしまいます。

public void reloadCache() {
	cache = accessor.getIdList();
	Collections.sort(cache);
}

ソートが終わる前に、cacheに対してbinarySearchされてしまえば、きちんと検索ができませんし

ソート中にインスタンスが入れ替わってしまうと、ソート結果がどうなるか分かりません。


このように、キャッシュの更新などを行う場合には、

最後にまとめてインスタンスを置き換えることで

synchronizedせずにスレッドセーフ化することもできるのです。


若干、ソースからでは伝わりにくい、、、かも知れませんけどね。

まとめ


というかそもそも、きちんとしたキャッシュ機構を作りたいなら

ローカルにDB(あるいはインメモリDB)を置くか、

サードパーティ製のキャッシュライブラリの利用を検討すべきなんでしょうけどね。


と言いつつ、実は私もキャッシュライブラリについては詳しくないので、

「これが鉄板でしょう」というものがあれば、ぜひ教えてください m(_ _)m

2010-01-24

[]レビューで鍛えるJavaコーディング力 その5(文字列操作)

今回は、Apache Commons Langを用いた問題です。

問題

以下のコードの問題を指摘し、修正してください。

ただし、問題は複数あることもあれば、全くないこともあります。

import org.apache.commons.lang.StringUtils;

public class TemplateReader {
	private String template;
	private String[] templateLines;

	public TemplateReader(String resourceName) {
		this.template = readTemplate(resourceName);
		this.templateLines = StringUtils.split(this.template, "\r\n");
	}

	/**
	 * テンプレートの本文全体を取得します。
	 * @return テンプレートの本文全体
	 */
	public String getTemplate() {
		return this.template;
	}

	/**
	 * テンプレートを改行で分割した配列を取得します。<br>
	 * 空行も空文字列として、配列に含めます。
	 * @return テンプレートを改行で分割した配列
	 */
	public String[] getTemplateLines() {
		return this.templateLines;
	}

	/**
	 * テンプレートのリソースを読み込みます。<br>
	 * テンプレートが読み込めない場合には、空の文字列を返します。
	 * @param resourceName リソース名
	 * @return リソース名に対応したテキストファイルの本文
	 */
	protected String readTemplate(String resourceName) {
		String result = null;
		// 省略
		return result;
	}

ファイル(リソース)読み込みを行い、その後に文字列操作を行なっています。

コラム

今週の1/27(水)に、関西Javaエンジニアの会(関ジャバ)の勉強会を開催します。


今回は「T2フレームワーク」「Griffon」「GWT vs XWT」など、

まだちょっと手を出してない人が多そうな(隙間の?)ネタが中心になっているので

興味はあるけど、実物を見たことがない、という人にもオススメです。


http://kokucheese.com/event/index/1199/

まだ少しだけ残席があるので、ぜひ今からでも応募してくださいね!


また、関ジャバでは発表者も募集しています。

Javaに直接関係ないネタでも構いませんので、とにかく「話したい!」という想いのある人は、

メールでもコメントでも、Twitterでも何でも構わないので、ぜひ、声を掛けてください!

解答

今回は、ライブラリの利用方法に誤りがあります。

this.templateLines = StringUtils.split(this.template, "\r\n");

splitに正規表現を使いたくない場合によく使うStringUtilsですが、

APIをきちんと確認しないと、思わぬ挙動に悩まされることがあります。


StringUtils#splitは、以下ような挙動になるのです。

  • 空要素はスキップする(連続したセパレータは、一つのセパレータとみなす)
  • セパレータに指定された文字列の一文字ずつで分割する

今回の例では、テキストファイルを読み込み、空行も空文字列として扱うのですから

空要素をスキップされては困ります。

そのため、空要素をスキップしない「splitPreserveAllTokens」を利用する必要があります。

this.templateLines = StringUtils.splitPreserveAllTokens(this.template, "\r\n");

しかし、これでもまだ問題があります。

StringUtils#splitは、String#splitと違い、引数に指定されたセパレータを

それぞれ1文字ずつに分割し、それぞれをセパレータとして扱います。


要するに、第二引数の「\r\n」は「\r」と「\n」に分解され

「\r」で分割し、「\n」でも分割されてしまうわけです。

そうすると、たとえば

StringUtils.splitPreserveAllTokens("abc\r\ndef", "\r\n");

この結果は {"abc", "def"} ではなく {"abc", "", "def"} となるのです。


引数に指定したセパレータ文字列を、まとめて1つのセパレータとして扱いたい場合は、

「splitByWholeSeparatorPreserveAllTokens」メソッドを利用します。

this.templateLines = StringUtils.splitByWholeSeparatorPreserveAllTokens(this.template, "\r\n");

これで、期待通りの挙動になるはずです。

補足

上の解答の対処で、少しだけ気になる所があります。

それは末尾の改行に関して、です。


テキストファイルを作成する際、末尾に改行を入れたり入れなかったりすることがありますが、

仮に末尾に改行が入っていた場合、その次の行は空行とみなすべきでしょうか、

それとも無視すべきでしょうか?


たとえば同じApache CommonsのFilesUtils#readLinesなどでは、

末尾の改行コードは無視するような挙動となっています。

(末尾に改行コードが連続していれば、そこまでの空行はきちんと保持されます)


無視するか、無視しないか、どちらが正解かは仕様次第でしょうけど

現実的には、末尾の改行は無視する方が妥当だと思います。


末尾の改行を削除するには、StringUtils#chompメソッドを使えば良いでしょう。

「\r\n」「\r」「\n」のいずれでも、末尾の改行を一つだけ削除してくれます。

まとめ

それでは、今回の問題をおさらいしておきましょう。

  • StringUtils#splitは、第二引数を分解して1文字ずつのセパレータとして扱う
  • StringUtils#splitは、空の要素をスキップする
  • 末尾の改行を削除する場合は、StringUtils#chompを使うと良い

2010-01-15

[]レビューで鍛えるJavaコーディング力 その4(簡易テンプレートエンジン)

前回の問題は、ちょっと内容も複雑で、取り組みづらかったかも知れませんね。

せめてフレームワークを使わず、HttpServletRequest#getSessionを使って

処理するような内容にすれば良かったか、と投稿した後になってから思ったわけですが。


今回は、その反省、、、というわけでもないですが、

Javaの標準APIのみを使った問題にしました。

問題

以下のコードの問題を指摘し、修正してください。

ただし、問題は複数あることもあれば、全くないこともあります。

/**
 * テンプレートを利用するメッセージ構築クラスです。
 */
public class MessageBuilder {
	/** メッセージのテンプレート。 */
	private String template;

	/**
	 * リソースを読み込み、テンプレートとして利用します。
	 * @param resourceName リソース名
	 */
	public MessageBuilder(String resourceName) {
		template = readTemplate(resourceName);
	}

	/**
	 * テンプレートにマップの値を適用して、メッセージを作成します。
	 * <p>
	 * テンプレート中に埋め込まれた ${xxx} 形式の変数を、<br>
	 * 対応するマップの値に置き換えます。<br>
	 * 対応するキーがマップにない場合には、置き換えは行いません。<br>
	 * <br>
	 * マップのキーは半角英数字のみ利用可能とし、値は任意の文字列を利用可能とします。
	 * 
	 * @param map 適用する値のマップ
	 * @return メッセージ
	 */
	public String buildMessage(Map<String, String> map) {
		// 引数チェックは必要だけど、省略。

		String result = template;

		for (Entry<String, String> entry : map.entrySet()) {
			String key = entry.getKey();
			String value = entry.getValue();
			result.replaceAll("${" + key + "}", value);
		}

		return result;
	}

	/**
	 * テンプレートをリソースより読み込みます。
	 * @param resourceName リソース名
	 * @return テンプレート文字列
	 */
	private String readTemplate(String resourceName) {
		String content = null;
		InputStream is = MessageBuilder.class.getResourceAsStream(resourceName);
		// 読み込み処理は省略。
		return content;
	}
}


/**
 * メッセージ構築クラスを利用する、サービスクラスの例です。
 */
public class MailService {
	private MessageBuilder mailBuilder;

	public MailService() {
		mailBuilder = new MessageBuilder("/mailTemplate.txt");
	}

	public void doSomeService() {
		Map<String, String> map = new HashMap<String, String>();

		// ... 検索や、mapへの値代入処理など。

		mailBuilder.buildMessage(map);
	}
}

今回は、簡易テンプレートエンジンのような、メッセージ構築クラスです。


そもそもテンプレートをコンパイルせず、replaceAllを使っている辺りに

性能面での懸念があるかも知れませんが、

今回は性能については検討対象外としてください。

コラム

前回話題にしたセッションの問題は、

正直なところ、かなり多くのシステムに潜んでいる、セキュリティ問題だと思います。


ただ

 ・攻撃手法が明確にならない(共通的な攻撃手法がない)

 ・攻撃可能だとしても、タイミングがシビアである

などの理由から、

あまり注目が集まっていないのかな、と思います。


逆に注目が集まってしまうと、

ステートフルなフレームワーク系とかも

軒並み危ないんじゃないのかな、と思うんですが。


では、そろそろ解答です。

解答1

まずは、脊髄反射で気づいて頂けたでしょうか、

FindBugsに怒られるレベルの間違いがあります。

result.replaceAll("${" + key + "}", value);

Stringはイミュータブルであり、replaceなどを呼び出しても

元の値は変わらないため、戻り値を利用する必要があります。

result = result.replaceAll("${" + key + "}", value);

はい、これでOK。


、、、って、もちろん、これだけでは終わりません。

解答2

String#replaceAllは、正規表現による置換を行なうメソッドです。

そのため、${key} という文字列正規表現として解釈されるのですが、

正しい正規表現の書式となっていないため、実行時例外が発生してしまいます。


文字列として扱いたいのであれば、きちんとエスケープする必要があります。

result = result.replaceAll("\\$\\{" + key + "\\}", value);

replaceAll以外にも、splitやmatchesなど、

Stringのメソッドでは正規表現を利用するものが多いため、

きちんとJavadocで確認しておく必要があります。


大抵は単体試験で検出できるのですが、たまにすり抜けるものがあるので、要注意です。

解答3

さて、問題はまだあります。

replaceAllが正規表現により置換だと言いましたが、

これは第一引数だけでなく、第二引数にも影響します。


replaceAllのJavadocには、以下のように記述されています。

指定された正規表現に一致する、この文字列の各部分文字列に対し、指定された置換を実行します。

このフォームのメソッド呼び出し str.replaceAll(regex, repl) では、次の式と正確に同じ結果が得られます。

Pattern.compile(regex).matcher(str).replaceAll(repl)

ということなので、一番最後のMatcher#replaceAllのJavadocも確認してみましょう。

置換文字列内でバックスラッシュ (\) とドル記号 ($) を使用すると、それをリテラル置換文字列として処理した場合とは結果が異なる場合があります。ドル記号は、先に説明したとおり、先方参照された部分シーケンスへの参照として処理される場合があり、バックスラッシュは置換文字列内のリテラル文字をエスケープするのに使用されます。

なんか小難しく書いていますが、

要するに、\ や $ を、あまり意識せずに第二引数に含んでしまっていると、

期待通りの置換が出来なかったり、例外が発生したりすることがあるのです。


今回、MessageBuilder#buildMessageを呼び出す人は、

たとえば "$1,000,000の夜景!!" というメッセージを埋め込みたい時に

わざわざ "\\$1,000,000の夜景!!" なんてエスケープしないでしょうから、

きちんと事前にエスケープしてあげるべきです。


実は、このエスケープのためだけに存在するメソッドが

Javaの標準APIで用意されているので、これを使いましょう。

result = result.replaceAll("\\$\\{" + key + "\\}", Matcher.quoteReplacement(value));

、、、随分と見通しが悪くなってきましたね。


というか、そもそもやりたいことは文字列の置換であって、正規表現は必要ありません。

ただの文字列置換メソッドがJava標準APIにあれば良いんですが、残念ながら存在しないので

Jakarta Commons LangのStringUtilsを使うと良いでしょう。

これを使えば、以下のように書き直せます。

result = StringUtils.replace(result, "${" + key + "}", value);

これで、随分と見やすくなりました。


要するに、普通の文字列操作を行ないたいなら、

StringUtilsを使っとけ、ってことですね。

まとめ

それでは、今回の問題をおさらいしておきましょう。

  • String#replaceAllの第一引数は、正規表現として解釈される。
  • String#replaceAllの第二引数も、エスケープが必要。
  • 正規表現を使わないなら、Jakarta Commons LangのStringUtilsを使うと良い。

Guavaが出てきたら、きっとGuavaを使った方が良いと思うんですけどね!

2010-01-11

[]レビューで鍛えるJavaコーディング力 その3(Webアプリケーション

今回は、DIフレームワークを用いたWebアプリケーション

ソースコードに関する問題です。


特定のフレームワークではなく、あくまで一般論の問題ですので、

少なくとも、Webアプリケーションを開発された事がある方なら、

取り組むことができると思います。

問題

以下にWebアプリケーションのソースコードを示します。

コメントから動作を読み取りながらソースコードを読み、

問題を指摘し、修正してください。

ただし、問題は複数あることもあれば、全くないこともあります。

public class EmpRegisterAction {
	/** 従業員エンティティ(セッションで管理されているインスタンスがバインドされる) */
	public Emp emp;

	/** 従業員サービス(シングルトンインスタンスがバインドされる) */
	public EmpService empService;

	/**
	 * 従業員を登録します(/emp/registerというURLにマッピングされる)
	 * @return 遷移先のパス
	 */
	public String doRegister() {
		validate(emp);
		empService.register(emp);
		return "/emp/complete.jsp";
	}

	/**
	 * 値の検証を行います。
	 */
	protected void validate(Emp inputEmp) {
		// 例外処理や、メッセージ部分は、分かりやすさのためにベタ書きしているため指摘対象外。

		if (inputEmp == null) {
			throw new SystemException("不正なリクエストです");
		}

		if (ValidatorUtils.checkRequired(inputEmp.getName()) == false) {
			throw new ValidatorException("名前は必ず入力してください。");
		}

		// 他のチェックは割愛。
	}
}

public class EmpService {
	/** 従業員テーブルアクセッサ(シングルトンインスタンスがバインドされる) */
	public EmpDao empDao;

	/**
	 * 従業員を登録します。
	 * @param emp 従業員エンティティ
	 */
	public void register(Emp emp) {
		empDao.insert(emp);
	}
}

何となく、どこかにあるサンプルのような内容ですね。

コラム

解答が見えてしまわないようにするために、コラムを挟みます。


前回の問題でgetBytesやnew Stringを出しましたが、

第二引数エンコードを指定すると、UnsupportedEncodingExceptionがスローされて

鬱陶しいことこの上ないですよね。


その対策なのか何なのか、Java6からはgetBytesやnew Stringの引数

「Charset」という抽象クラスを渡せるようになりました。

このCharsetを使ったメソッドでは、UnsupportedEncodingExceptionがスローされません。


(2010/01/12修正)

Charsetの実装クラスは、Charset.forName()で取得するのですが

このforNameメソッドでスローされるのは、UnsupportedCharsetExceptionなどの

RuntimeExceptionになっており、わざわざキャッチする必要がありません。

Java6以降なら、こちらを使えば良いでしょうね。


また、Google製のJavaライブラリ「Guava」にも

いくつかのCharsetが定数として定義されています。

http://guava-libraries.googlecode.com/svn/trunk/javadoc/com/google/common/base/Charsets.html

あくまで Charset.forName("UTF-8") を定数として保持しているだけですが。


Guavaには、Generics対応の文字列操作のライブラリ

コレクションライブラリ(Google Collections Libraryのスーパーセット)など

たくさんのコアライブラリが用意されているようなので、

Java1.4までの案件ではJakarta Commons

Java5以降の案件ではGuava、という使い分けになっていきそうですね。


まだ正式版がリリースされていませんが(いつ出るのかな)

PDFやAPIドキュメントなどが公開されているので、ぜひ見てみてください。


では、そろそろ解答です。

解答

今回は「Webアプリケーション」というところが最大のカギで、

問題は、以下の箇所にあります。

validate(emp);
empService.register(emp);

バリデートをした後に、registerしているだけなので

一見、問題があるようには思えませんが、ここに落とし穴があります。


empは「セッションから取得するオブジェクト」ですから、

同一ユーザーが、複数リクエストを発行した際には、

このオブジェクトが共有されます。


そのため、かなりシビアなタイミングとは言え、

以下のような「バリデーションを無効化する」攻撃が可能になってしまうのです。


1. (ユーザーが)正常なリクエストを送信

2. 1のリクエストを処理するためのスレッド「Thread-1」が生成される。

3. Thread-1 : (フレームワークが)リクエストの内容をセッションのempオブジェクトに反映する。

4. Thread-1 : validateメソッドを実行し、empオブジェクトのチェックが完了する。

5. (ユーザーが)悪意を持ったリクエストを送信

6. 5のリクエストを処理するためのスレッド「Thread-2」が生成される。

7. Thread-2 : (フレームワークが)リクエストの内容をセッションのempオブジェクトに反映する。

8. Thread-1 : registerメソッドを実行する。7の値が設定されたempが登録される。

9. Thread-2 : validateメソッドを実行し、チェックエラーが発生する。


、、、少しわかりづらいですが、理解してもらえたでしょうか。


複数リクエストを発行した際に、それぞれのリクエストを処理するためのスレッドが立ち上がるものの

セッションオブジェクトである「emp」は、それぞれのスレッドで共有されているために

後から来たリクエストに、値を上書きされてしまうのです。


後から来たリクエスト(Thread-2)自体は、validateによってエラーとなるのですが

先に動いていたリクエスト(Thread-1)は、そのまま処理を継続するため、

不正な値を持ったempオブジェクトを、8で利用してしまうのです。


要するに、セッションオブジェクトに対してバリデーションを行なっても意味がない、

無効化される危険性がある、ということです。


この対策は、複数あります。

対策1:ディープコピーする(おすすめ度:低)

セッションオブジェクトをそのまま利用せず、

ディープコピーを作成してから、利用するという方法です。

Emp newEmp = new Emp();
PropertyUtils.copyProperties(newEmp, emp);
// もしくは
// Emp newEmp = (Emp)BeanUtils.cloneBean(emp);
validate(newEmp);
empService.register(newEmp);

このように、新しいオブジェクトを使って処理をすれば、値を書き換えられる心配はありません。


ただし、場当たり的な対応であり、うっかりこの処理の記述を忘れてしまっても

単体テストでは検出できないため、あまりオススメできる方法ではありません。


リリース前後、時間も予算もない場合に、

場当たり的でも良いから対応したい場合のみに使う手段、でしょうか。

対策2:画面入力オブジェクトと、サービス用オブジェクトを分ける(おすすめ度:高)

一番の王道は、この方法でしょうか。

対策1では「画面からの入力オブジェクト」と「サービス用オブジェクト」に

同じものを使っていたために、ディープコピーをし忘れても気づかない、という問題がありました。


であれば、「画面からの入力オブジェクト」と「サービス用オブジェクト」を異なったものにして、

Actionで変換するようなアーキテクチャにすれば、変換忘れを防げるはずです。

/** 従業員エンティティ(セッションで管理されているインスタンスがバインドされる) */
public EmpForm empForm;

/**
 * 従業員を登録します(/emp/registerというURLにマッピングされる)
 * @return 遷移先のパス
 */
public String doRegister() {
	Emp emp = convert(EmpForm);
	validate(emp);
	empService.register(emp);
	return "/emp/complete.jsp";
}

画面からの入力はEmpForm、サービスで使うのはEmpとして分けています。


サービスをきちんと作っている限りは、

変換処理を忘れてempService.register(empForm)としても

コンパイルエラーが発生するので、すぐに問題に気づきます。


というか、そもそも「画面からの入力オブジェクト」と「サービス用オブジェクト」は別物なので、

似ているからと言って、共通化すべきではありません。

最初から、こうすべきなのです。

対策3:セッションで同期化する(おすすめ度:高)

対策2のようなアーキテクチャにせず、プロジェクトを進めてしまった場合の対策として、

ServletFilterを使った方法があります。


ServletFilterでセッションを同期化し、1ユーザー(1セッション)で

同時に1つのリクエストしか送信できないようにするのです。

@Override
public void doFilter(ServletRequest request, ServletResponse response,
        FilterChain chain) throws IOException, ServletException {
	HttpSession session = ((HttpServletRequest) request).getSession(true);
	synchronized (session) {
		chain.doFilter(request, response);
	}
}

このように、セッションオブジェクトでsynchronizedすることで、

同一セッションで複数リクエストが来た場合に、ここで処理を待たせることができます。


多少はパフォーマンスに影響するかも知れませんが、

Ajaxなど、で「同時に複数リクエストを処理したい」場合を除き、

この方法で問題ないでしょう。


(2010/01/14追記)

コメントで指摘を頂きましたが、APサーバによっては

リクエストごとにセッションオブジェクトが変わるものがあるそうです。

言われてみれば、そういうAPサーバでなかったとしても、

シリアライズ→デシリアライズされたりすると、インスタンスは変わりますよね。


と言うことで、そういうAPサーバも考慮に入れると、以下のようなコードになります。

@Override
public void doFilter(ServletRequest request, ServletResponse response,
        FilterChain chain) throws IOException, ServletException {
	HttpSession session = ((HttpServletRequest) request).getSession(true);
	synchronized (session.<b>getId().intern()</b>) {
		chain.doFilter(request, response);
	}
}

これでOK。

対策4:そもそもセッションとか使わない(おすすめ度:中)

小難しいことを考えなくて済むよう、そもそもセッションにオブジェクトを格納せず

すべてリクエスト処理だけで済ませる、という方法です。


安全で、思わぬ落とし穴もあまりないので、

私は少人数で開発する場合には、よくこの方法を使います。

セッションではなく、hiddenを中心に使う方法です。


ただ、Wicketのようなステートフルなフレームワークは当然使えないですし、

世の中のサンプルも、セッションを使うことを前提にしているものが多いので、

開発者の教育も含めた、開発コストへの影響が多少あるでしょう。

まとめ

それでは、今回の問題をおさらいしておきましょう。

  • 複数リクエストが同時に来た場合の、セッションオブジェクトの値の書き換えに注意。
  • 対策としては、画面入力用オブジェクトと、サービス用オブジェクトを分けて変換するのが良い。
  • 次善策は、SevletFilterにて、セッションで同期化すること。
  • リクエストのみのステートレスな構成にできれば、落とし穴にはまりにくい。

世の中のサンプルが、全部「対策2」の記述になってくれていて、

さらに、同時リクエストによるセッションの上書きについて書いていてくれれば

こんな問題にハマることはないと思うのですが。


そういう意味では

「なんたらスーパーサンプル」とかっていう本を出してる会社に期待、でしょうか(笑