Limit the number of files per change

Changes with >>10k of files have in the past caused breakage. The default limit is 50k for now, though we are still in discussion with Android and they might be OK with 10k. Change-Id: I55d1584a4154a3d64ba4b7eb3cbd0bb8e2c0746b
2019-12-19 11:02:30 +01:00
parent b5ef9bf951
commit 56c74d7d0d
5 changed files with 126 additions and 33 deletions
--- a/java/com/google/gerrit/server/git/validators/CommitValidators.java
+++ b/java/com/google/gerrit/server/git/validators/CommitValidators.java
@@ -44,6 +44,11 @@ import com.google.gerrit.server.events.CommitReceivedEvent;
 import com.google.gerrit.server.git.GitRepositoryManager;
 import com.google.gerrit.server.git.ValidationError;
 import com.google.gerrit.server.git.validators.ValidationMessage.Type;
+import com.google.gerrit.server.patch.DiffSummary;
+import com.google.gerrit.server.patch.DiffSummaryKey;
+import com.google.gerrit.server.patch.PatchListCache;
+import com.google.gerrit.server.patch.PatchListKey;
+import com.google.gerrit.server.patch.PatchListNotAvailableException;
 import com.google.gerrit.server.permissions.PermissionBackend;
 import com.google.gerrit.server.permissions.PermissionBackendException;
 import com.google.gerrit.server.permissions.RefPermission;
@@ -76,7 +81,8 @@ import org.eclipse.jgit.revwalk.RevWalk;
 import org.eclipse.jgit.util.SystemReader;

 /**
- * Represents a list of CommitValidationListeners to run for a push to one branch of one project.
+ * Represents a list of {@link CommitValidationListener}s to run for a push to one branch of one
+ * project.
 */
 public class CommitValidators {
  private static final FluentLogger logger = FluentLogger.forEnclosingClass();
@@ -94,15 +100,16 @@ public class CommitValidators {
    private final AllProjectsName allProjects;
    private final ExternalIdsConsistencyChecker externalIdsConsistencyChecker;
    private final AccountValidator accountValidator;
-    private final String installCommitMsgHookCommand;
    private final ProjectCache projectCache;
    private final ProjectConfig.Factory projectConfigFactory;
+    private final PatchListCache patchListCache;
+    private final Config config;

    @Inject
    Factory(
        @GerritPersonIdent PersonIdent gerritIdent,
        DynamicItem<UrlFormatter> urlFormatter,
-        @GerritServerConfig Config cfg,
+        @GerritServerConfig Config config,
        PluginSetContext<CommitValidationListener> pluginValidators,
        GitRepositoryManager repoManager,
        AllUsersName allUsers,
@@ -110,19 +117,20 @@ public class CommitValidators {
        ExternalIdsConsistencyChecker externalIdsConsistencyChecker,
        AccountValidator accountValidator,
        ProjectCache projectCache,
-        ProjectConfig.Factory projectConfigFactory) {
+        ProjectConfig.Factory projectConfigFactory,
+        PatchListCache patchListCache) {
      this.gerritIdent = gerritIdent;
      this.urlFormatter = urlFormatter;
+      this.config = config;
      this.pluginValidators = pluginValidators;
      this.repoManager = repoManager;
      this.allUsers = allUsers;
      this.allProjects = allProjects;
      this.externalIdsConsistencyChecker = externalIdsConsistencyChecker;
      this.accountValidator = accountValidator;
-      this.installCommitMsgHookCommand =
-          cfg != null ? cfg.getString("gerrit", null, "installCommitMsgHookCommand") : null;
      this.projectCache = projectCache;
      this.projectConfigFactory = projectConfigFactory;
+      this.patchListCache = patchListCache;
    }

    public CommitValidators forReceiveCommits(
@@ -146,12 +154,7 @@ public class CommitValidators {
              new CommitterUploaderValidator(user, perm, urlFormatter.get()),
              new SignedOffByValidator(user, perm, projectState),
              new ChangeIdValidator(
-                  projectState,
-                  user,
-                  urlFormatter.get(),
-                  installCommitMsgHookCommand,
-                  sshInfo,
-                  change),
+                  projectState, user, urlFormatter.get(), config, sshInfo, change),
              new ConfigValidator(projectConfigFactory, branch, user, rw, allUsers, allProjects),
              new BannedCommitsValidator(rejectCommits),
              new PluginCommitValidationListener(pluginValidators, skipValidation),
@@ -176,14 +179,10 @@ public class CommitValidators {
              new ProjectStateValidationListener(projectState),
              new AmendedGerritMergeCommitValidationListener(perm, gerritIdent),
              new AuthorUploaderValidator(user, perm, urlFormatter.get()),
+              new FileCountValidator(patchListCache, config),
              new SignedOffByValidator(user, perm, projectCache.checkedGet(branch.project())),
              new ChangeIdValidator(
-                  projectState,
-                  user,
-                  urlFormatter.get(),
-                  installCommitMsgHookCommand,
-                  sshInfo,
-                  change),
+                  projectState, user, urlFormatter.get(), config, sshInfo, change),
              new ConfigValidator(projectConfigFactory, branch, user, rw, allUsers, allProjects),
              new PluginCommitValidationListener(pluginValidators),
              new ExternalIdUpdateListener(allUsers, externalIdsConsistencyChecker),
@@ -268,14 +267,14 @@ public class CommitValidators {
        ProjectState projectState,
        IdentifiedUser user,
        UrlFormatter urlFormatter,
-        String installCommitMsgHookCommand,
+        Config config,
        SshInfo sshInfo,
        Change change) {
      this.projectState = projectState;
-      this.urlFormatter = urlFormatter;
-      this.installCommitMsgHookCommand = installCommitMsgHookCommand;
-      this.sshInfo = sshInfo;
      this.user = user;
+      this.urlFormatter = urlFormatter;
+      installCommitMsgHookCommand = config.getString("gerrit", null, "installCommitMsgHookCommand");
+      this.sshInfo = sshInfo;
      this.change = change;
    }

@@ -387,6 +386,40 @@ public class CommitValidators {
    }
  }

+  /** Limits the number of files per change. */
+  private static class FileCountValidator implements CommitValidationListener {
+
+    private final PatchListCache patchListCache;
+    private final int maxFileCount;
+
+    FileCountValidator(PatchListCache patchListCache, Config config) {
+      this.patchListCache = patchListCache;
+      maxFileCount = config.getInt("change", null, "maxFiles", 50_000);
+    }
+
+    @Override
+    public List<CommitValidationMessage> onCommitReceived(CommitReceivedEvent receiveEvent)
+        throws CommitValidationException {
+      PatchListKey patchListKey =
+          PatchListKey.againstBase(
+              receiveEvent.commit.getId(), receiveEvent.commit.getParentCount());
+      DiffSummaryKey diffSummaryKey = DiffSummaryKey.fromPatchListKey(patchListKey);
+      try {
+        DiffSummary diffSummary =
+            patchListCache.getDiffSummary(diffSummaryKey, receiveEvent.project.getNameKey());
+        if (diffSummary.getPaths().size() > maxFileCount) {
+          throw new CommitValidationException(
+              String.format(
+                  "Exceeding maximum number of files per change (%d > %d)",
+                  diffSummary.getPaths().size(), maxFileCount));
+        }
+      } catch (PatchListNotAvailableException e) {
+        logger.atWarning().withCause(e).log("Failed to validate file count");
+      }
+      return Collections.emptyList();
+    }
+  }
+
  /** If this is the special project configuration branch, validate the config. */
  public static class ConfigValidator implements CommitValidationListener {
    private final ProjectConfig.Factory projectConfigFactory;
--- a/java/com/google/gerrit/server/patch/PatchListCacheImpl.java
+++ b/java/com/google/gerrit/server/patch/PatchListCacheImpl.java
@@ -138,11 +138,10 @@ public class PatchListCacheImpl implements PatchListCache {
      throws PatchListNotAvailableException {
    Project.NameKey project = change.getProject();
    ObjectId b = patchSet.commitId();
-    Whitespace ws = Whitespace.IGNORE_NONE;
    if (parentNum != null) {
-      return get(PatchListKey.againstParentNum(parentNum, b, ws), project);
+      return get(PatchListKey.againstParentNum(parentNum, b, Whitespace.IGNORE_NONE), project);
    }
-    return get(PatchListKey.againstDefaultBase(b, ws), project);
+    return get(PatchListKey.againstDefaultBase(b, Whitespace.IGNORE_NONE), project);
  }

  @Override
--- a/java/com/google/gerrit/server/patch/PatchListKey.java
+++ b/java/com/google/gerrit/server/patch/PatchListKey.java
@@ -59,6 +59,12 @@ public class PatchListKey implements Serializable {
    return new PatchListKey(otherCommitId, newId, whitespace);
  }

+  public static PatchListKey againstBase(ObjectId id, int parentCount) {
+    return parentCount > 1
+        ? PatchListKey.againstParentNum(1, id, Whitespace.IGNORE_NONE)
+        : PatchListKey.againstDefaultBase(id, Whitespace.IGNORE_NONE);
+  }
+
  /**
   * Old patch-set ID
   *
--- a/java/com/google/gerrit/server/query/change/ChangeData.java
+++ b/java/com/google/gerrit/server/query/change/ChangeData.java
@@ -45,7 +45,6 @@ import com.google.gerrit.entities.Project;
 import com.google.gerrit.entities.RefNames;
 import com.google.gerrit.entities.RobotComment;
 import com.google.gerrit.exceptions.StorageException;
-import com.google.gerrit.extensions.client.DiffPreferencesInfo.Whitespace;
 import com.google.gerrit.extensions.restapi.BadRequestException;
 import com.google.gerrit.extensions.restapi.ResourceConflictException;
 import com.google.gerrit.server.ApprovalsUtil;
@@ -396,12 +395,7 @@ public class ChangeData {
        return Optional.empty();
      }

-      ObjectId id = ps.commitId();
-      Whitespace ws = Whitespace.IGNORE_NONE;
-      PatchListKey pk =
-          parentCount > 1
-              ? PatchListKey.againstParentNum(1, id, ws)
-              : PatchListKey.againstDefaultBase(id, ws);
+      PatchListKey pk = PatchListKey.againstBase(ps.commitId(), parentCount);
      DiffSummaryKey key = DiffSummaryKey.fromPatchListKey(pk);
      try {
        diffSummary = Optional.of(patchListCache.getDiffSummary(key, c.getProject()));
--- a/javatests/com/google/gerrit/server/update/BatchUpdateTest.java
+++ b/javatests/com/google/gerrit/server/update/BatchUpdateTest.java
@@ -19,6 +19,7 @@ import static com.google.common.base.Preconditions.checkState;
 import static com.google.common.truth.Truth.assertThat;
 import static com.google.gerrit.testing.GerritJUnit.assertThrows;

+import com.google.common.cache.Cache;
 import com.google.common.collect.ImmutableList;
 import com.google.gerrit.common.Nullable;
 import com.google.gerrit.common.data.SubmitRecord;
@@ -35,10 +36,13 @@ import com.google.gerrit.server.git.GitRepositoryManager;
 import com.google.gerrit.server.notedb.ChangeNotes;
 import com.google.gerrit.server.notedb.ChangeUpdate;
 import com.google.gerrit.server.notedb.Sequences;
+import com.google.gerrit.server.patch.DiffSummary;
+import com.google.gerrit.server.patch.DiffSummaryKey;
 import com.google.gerrit.server.util.time.TimeUtil;
 import com.google.gerrit.testing.InMemoryTestEnvironment;
 import com.google.inject.Inject;
 import com.google.inject.Provider;
+import com.google.inject.name.Named;
 import org.eclipse.jgit.junit.TestRepository;
 import org.eclipse.jgit.lib.Config;
 import org.eclipse.jgit.lib.ObjectId;
@@ -57,8 +61,9 @@ public class BatchUpdateTest {
      new InMemoryTestEnvironment(
          () -> {
            Config cfg = new Config();
-            cfg.setInt("change", null, "maxUpdates", MAX_UPDATES);
+            cfg.setInt("change", null, "maxFiles", 2);
            cfg.setInt("change", null, "maxPatchSets", MAX_PATCH_SETS);
+            cfg.setInt("change", null, "maxUpdates", MAX_UPDATES);
            return cfg;
          });

@@ -70,6 +75,9 @@ public class BatchUpdateTest {
  @Inject private Provider<CurrentUser> user;
  @Inject private Sequences sequences;

+  @Inject
+  private @Named("diff_summary") Cache<DiffSummaryKey, DiffSummary> diffSummaryCache;
+
  private Project.NameKey project;
  private TestRepository<Repository> repo;

@@ -243,6 +251,59 @@ public class BatchUpdateTest {
    assertThat(getMetaId(changeId)).isEqualTo(oldMetaId);
  }

+  @Test
+  public void limitFileCount_exceed() throws Exception {
+    Change.Id changeId = createChangeWithUpdates(1);
+    ChangeNotes notes = changeNotesFactory.create(project, changeId);
+
+    try (BatchUpdate bu = batchUpdateFactory.create(project, user.get(), TimeUtil.nowTs())) {
+      ObjectId commitId =
+          repo.amend(notes.getCurrentPatchSet().commitId())
+              .add("bar.txt", "bar")
+              .add("baz.txt", "baz")
+              .add("boom.txt", "boom")
+              .message("blah")
+              .create();
+      bu.addOp(
+          changeId,
+          patchSetInserterFactory
+              .create(notes, PatchSet.id(changeId, 2), commitId)
+              .setMessage("blah"));
+      ResourceConflictException thrown = assertThrows(ResourceConflictException.class, bu::execute);
+      assertThat(thrown)
+          .hasMessageThat()
+          .contains("Exceeding maximum number of files per change (3 > 2)");
+    }
+  }
+
+  @Test
+  public void limitFileCount_cacheKeyMatches() throws Exception {
+    Change.Id changeId = createChangeWithUpdates(1);
+    ChangeNotes notes = changeNotesFactory.create(project, changeId);
+
+    int cacheSizeBefore = diffSummaryCache.asMap().size();
+
+    // We don't want to depend on the test helper used above so we perform an explicit commit here.
+    try (BatchUpdate bu = batchUpdateFactory.create(project, user.get(), TimeUtil.nowTs())) {
+      ObjectId commitId =
+          repo.amend(notes.getCurrentPatchSet().commitId())
+              .add("bar.txt", "bar")
+              .add("baz.txt", "baz")
+              .message("blah")
+              .create();
+      bu.addOp(
+          changeId,
+          patchSetInserterFactory
+              .create(notes, PatchSet.id(changeId, 3), commitId)
+              .setMessage("blah"));
+      bu.execute();
+    }
+
+    // Assert that we only performed the diff computation once. This would e.g. catch
+    // bugs/deviations in the computation of the cache key.
+    assertThat(diffSummaryCache.asMap()).hasSize(cacheSizeBefore + 1);
+  }
+
  private Change.Id createChangeWithUpdates(int totalUpdates) throws Exception {
    checkArgument(totalUpdates > 0);
    checkArgument(totalUpdates <= MAX_UPDATES);