Do not serialize JGit commit objects

JGit commit objects are a recursive data structure; they have links to
their parent commits. Serializing a JGit commit will try to recursively
serialize all reachable ancestors as faras they have been loaded. If
that ancestor chain is too long, a StackOverflowError is thrown during
Wicket's page serialization if a page has a reference to sucha JGit
commit.

Fixed by making sure that pages o not contain references to JGit
commits. Use the (existing) wrapper object RepositoryCommit instead.

* RepositoryCommit has a transient reference to the JGit commit and
  reads the commit from the repository upon de-serialization.
* RefModel is a similar case (JGit tags/branches may also have links
  to the commits they point to). Solved a bit differently by making it
  a pure data object by transferring the interesting data from the JGit
  object in the constructor.
* Change DataViews instantiated with RevCommit to use RepositoryCommit
  instead.
* Change inner anonymous DataViews to ensure they do not have a
  synthesized field referencing the "allRefs" map. Such a synthesized
  field would also get serialized, and then serialize JGit commits
  again.

Finally, remove non-transient logger instances in Wicket classes. Those
might lead to NotSerializableException.

These StackOverflowErrors have been reported in several places since
2014:

* https://groups.google.com/forum/#!topic/gitblit/GH1d8WSlR6Q
* https://bugs.chromium.org/p/gerrit/issues/detail?id=3316
* https://groups.google.com/d/msg/repo-discuss/Kcl0JIGNiGk/0DjH4mO8hA8J
* https://groups.google.com/d/msg/repo-discuss/0_P6A3fjTec/2kcpVPIUAQAJ
* gitblit-org#1011
* tomaswolf/gerrit-gitblit-plugin#21

Issue: gitblit-org#1011

(cherry picked from commit 005e8e2)

Author: tomaswolf

src/main/java/com/gitblit/models/RefModel.java

@@ -36,18 +36,42 @@
  */
 public class RefModel implements Serializable, Comparable<RefModel> {
 
-	private static final long serialVersionUID = 1L;
+	private static final long serialVersionUID = 876822269940583606L;
+
 	public final String displayName;
-	public final RevObject referencedObject;
-	public transient Ref reference;
+
+	private final Date date;
+	private final String name;
+	private final int type;
+	private final String id;
+	private final String referencedId;
+	private final boolean annotated;
+	private final PersonIdent person;
+	private final String shortMessage;
+	private final String fullMessage;
+
+	private transient ObjectId objectId;
+	private transient ObjectId referencedObjectId;
+
+	public transient Ref reference; // Used in too many places.
 
 	public RefModel(String displayName, Ref ref, RevObject refObject) {
-		this.displayName = displayName;
 		this.reference = ref;
-		this.referencedObject = refObject;
+		this.displayName = displayName;
+		this.date = internalGetDate(refObject);
+		this.name = (ref != null) ? ref.getName() : displayName;
+		this.type = internalGetReferencedObjectType(refObject);
+		this.objectId = internalGetObjectId(reference);
+		this.id = this.objectId.getName();
+		this.referencedObjectId = internalGetReferencedObjectId(refObject);
+		this.referencedId = this.referencedObjectId.getName();
+		this.annotated = internalIsAnnotatedTag(ref, refObject);
+		this.person = internalGetAuthorIdent(refObject);
+		this.shortMessage = internalGetShortMessage(refObject);
+		this.fullMessage = internalGetFullMessage(refObject);
 	}
 
-	public Date getDate() {
+	private Date internalGetDate(RevObject referencedObject) {
 		Date date = new Date(0);
 		if (referencedObject != null) {
 			if (referencedObject instanceof RevTag) {
@@ -64,29 +88,41 @@ public Date getDate() {
 		return date;
 	}
 
+	public Date getDate() {
+		return date;
+	}
+
 	public String getName() {
-		if (reference == null) {
-			return displayName;
-		}
-		return reference.getName();
+		return name;
 	}
 
-	public int getReferencedObjectType() {
+	private int internalGetReferencedObjectType(RevObject referencedObject) {
 		int type = referencedObject.getType();
 		if (referencedObject instanceof RevTag) {
 			type = ((RevTag) referencedObject).getObject().getType();
 		}
 		return type;
 	}
 
-	public ObjectId getReferencedObjectId() {
+	public int getReferencedObjectType() {
+		return type;
+	}
+
+	private ObjectId internalGetReferencedObjectId(RevObject referencedObject) {
 		if (referencedObject instanceof RevTag) {
 			return ((RevTag) referencedObject).getObject().getId();
 		}
 		return referencedObject.getId();
 	}
 
-	public String getShortMessage() {
+	public ObjectId getReferencedObjectId() {
+		if (referencedObjectId == null) {
+			referencedObjectId = ObjectId.fromString(referencedId);
+		}
+		return referencedObjectId;
+	}
+
+	private String internalGetShortMessage(RevObject referencedObject) {
 		String message = "";
 		if (referencedObject instanceof RevTag) {
 			message = ((RevTag) referencedObject).getShortMessage();
@@ -96,7 +132,11 @@ public String getShortMessage() {
 		return message;
 	}
 
-	public String getFullMessage() {
+	public String getShortMessage() {
+		return shortMessage;
+	}
+
+	private String internalGetFullMessage(RevObject referencedObject) {
 		String message = "";
 		if (referencedObject instanceof RevTag) {
 			message = ((RevTag) referencedObject).getFullMessage();
@@ -106,7 +146,11 @@ public String getFullMessage() {
 		return message;
 	}
 
-	public PersonIdent getAuthorIdent() {
+	public String getFullMessage() {
+		return fullMessage;
+	}
+
+	private PersonIdent internalGetAuthorIdent(RevObject referencedObject) {
 		if (referencedObject instanceof RevTag) {
 			return ((RevTag) referencedObject).getTaggerIdent();
 		} else if (referencedObject instanceof RevCommit) {
@@ -115,17 +159,32 @@ public PersonIdent getAuthorIdent() {
 		return null;
 	}
 
-	public ObjectId getObjectId() {
+	public PersonIdent getAuthorIdent() {
+		return person;
+	}
+
+	private ObjectId internalGetObjectId(Ref reference) {
 		return reference.getObjectId();
 	}
 
-	public boolean isAnnotatedTag() {
+	public ObjectId getObjectId() {
+		if (objectId == null) {
+			objectId = ObjectId.fromString(id);
+		}
+		return objectId;
+	}
+
+	private boolean internalIsAnnotatedTag(Ref reference, RevObject referencedObject) {
 		if (referencedObject instanceof RevTag) {
 			return !getReferencedObjectId().equals(getObjectId());
 		}
 		return reference.getPeeledObjectId() != null;
 	}
 
+	public boolean isAnnotatedTag() {
+		return annotated;
+	}
+
 	@Override
 	public int hashCode() {
 		return getReferencedObjectId().hashCode() + getName().hashCode();

src/main/java/com/gitblit/models/RepositoryCommit.java

@@ -15,37 +15,45 @@
  */
 package com.gitblit.models;
 
+import java.io.IOException;
+import java.io.ObjectInputStream;
 import java.io.Serializable;
 import java.text.MessageFormat;
 import java.util.Date;
 import java.util.List;
 
 import org.eclipse.jgit.lib.ObjectId;
 import org.eclipse.jgit.lib.PersonIdent;
+import org.eclipse.jgit.lib.Repository;
 import org.eclipse.jgit.revwalk.RevCommit;
+import org.eclipse.jgit.revwalk.RevWalk;
+
+import com.gitblit.wicket.GitBlitWebApp;
 
 /**
- * Model class to represent a RevCommit, it's source repository, and the branch.
- * This class is used by the activity page.
+ * Model class to represent a RevCommit, it's source repository, and the branch. This class is used by the activity page.
  *
  * @author James Moger
  */
 public class RepositoryCommit implements Serializable, Comparable<RepositoryCommit> {
 
-	private static final long serialVersionUID = 1L;
+	private static final long serialVersionUID = -2214911650485772022L;
 
-	public final String repository;
+	public String repository;
 
-	public final String branch;
+	public String branch;
 
-	private final RevCommit commit;
+	private final String commitId;
 
 	private List<RefModel> refs;
 
+	private transient RevCommit commit;
+
 	public RepositoryCommit(String repository, String branch, RevCommit commit) {
 		this.repository = repository;
 		this.branch = branch;
 		this.commit = commit;
+		this.commitId = commit.getName();
 	}
 
 	public void setRefs(List<RefModel> refs) {
@@ -80,7 +88,7 @@ public int getParentCount() {
 		return commit.getParentCount();
 	}
 
-	public RevCommit [] getParents() {
+	public RevCommit[] getParents() {
 		return commit.getParents();
 	}
 
@@ -92,10 +100,14 @@ public PersonIdent getCommitterIdent() {
 		return commit.getCommitterIdent();
 	}
 
+	public RevCommit getCommit() {
+		return commit;
+	}
+
 	@Override
 	public boolean equals(Object o) {
 		if (o instanceof RepositoryCommit) {
-			RepositoryCommit commit = (RepositoryCommit) o;
+			final RepositoryCommit commit = (RepositoryCommit) o;
 			return repository.equals(commit.repository) && getName().equals(commit.getName());
 		}
 		return false;
@@ -123,8 +135,23 @@ public RepositoryCommit clone(String withRef) {
 
 	@Override
 	public String toString() {
-		return MessageFormat.format("{0} {1} {2,date,yyyy-MM-dd HH:mm} {3} {4}",
-				getShortName(), branch, getCommitterIdent().getWhen(), getAuthorIdent().getName(),
-				getShortMessage());
+		return MessageFormat.format("{0} {1} {2,date,yyyy-MM-dd HH:mm} {3} {4}", getShortName(), branch, getCommitterIdent().getWhen(),
+				getAuthorIdent().getName(), getShortMessage());
+	}
+
+	// Serialization: restore the JGit RevCommit on reading
+
+	private void readObject(ObjectInputStream input) throws IOException, ClassNotFoundException {
+		// Read in fields and any hidden stuff
+		input.defaultReadObject();
+		// Go find the commit again.
+		final Repository repo = GitBlitWebApp.get().repositories().getRepository(repository);
+		if (repo == null) {
+			throw new IOException("Cannot find repositoy " + repository);
+		}
+		try (RevWalk walk = new RevWalk(repo)) {
+			commit = walk.parseCommit(repo.resolve(commitId));
+		}
 	}
+
 }

src/main/java/com/gitblit/wicket/SessionlessForm.java

@@ -58,7 +58,7 @@ public class SessionlessForm<T> extends StatelessForm<T> {
 
 	protected final PageParameters pageParameters;
 
-	private final Logger log = LoggerFactory.getLogger(SessionlessForm.class);
+	private transient Logger logger;
 
 	/**
 	 * Sessionless forms must have a bookmarkable page class.  A bookmarkable
@@ -118,7 +118,10 @@ protected void onComponentTagBody(final MarkupStream markupStream, final Compone
 				if (c != null) {
 					// this form has a field id which matches the
 					// parameter name, skip embedding a hidden value
-					log.warn(MessageFormat.format("Skipping page parameter \"{0}\" from sessionless form hidden fields because it collides with a form child wicket:id", key));
+					logger().warn(
+							MessageFormat
+									.format("Skipping page parameter \"{0}\" from sessionless form hidden fields because it collides with a form child wicket:id",
+											key));
 					continue;
 				}
 				String value = pageParameters.getString(key);
@@ -156,4 +159,11 @@ protected String getAbsoluteUrl(Class<? extends BasePage> pageClass, PageParamet
 		String absoluteUrl = RequestUtils.toAbsolutePath(relativeUrl);
 		return absoluteUrl;
 	}
+
+	private Logger logger() {
+		if (logger == null) {
+			logger = LoggerFactory.getLogger(SessionlessForm.class);
+		}
+		return logger;
+	}
 }

src/main/java/com/gitblit/wicket/pages/BlamePage.java

@@ -108,7 +108,7 @@ public BlamePage(PageParameters params) {
 		if (pathModel == null) {
 			final String notFound = MessageFormat.format("Blame page failed to find {0} in {1} @ {2}",
 					blobPath, repositoryName, objectId);
-			logger.error(notFound);
+			logger().error(notFound);
 			add(new Label("annotation").setVisible(false));
 			add(new Label("missingBlob", missingBlob(blobPath, commit)).setEscapeModelStrings(false));
 			return;

src/main/java/com/gitblit/wicket/pages/EditFilePage.java

@@ -123,7 +123,7 @@ protected void onSubmit() {
 					try {			
 						ObjectId docAtLoad = getRepository().resolve(commitIdAtLoad.getObject());
 
-						logger.trace("Commiting Edit File page: " + commitIdAtLoad.getObject());
+						logger().trace("Commiting Edit File page: " + commitIdAtLoad.getObject());
 
 						DirCache index = DirCache.newInCore();
 						DirCacheBuilder builder = index.builder();

src/main/java/com/gitblit/wicket/pages/MetricsPage.java

@@ -94,7 +94,7 @@ private void createLineChart(Charts charts, String id, List<Metric> metrics) {
 				try {
 					date = df.parse(metric.name);
 				} catch (ParseException e) {
-					logger.error("Unable to parse date: " + metric.name);
+					logger().error("Unable to parse date: " + metric.name);
 					return;
 				}
 				chart.addValue(date, (int)metric.count);