Group with empty UUID causes NPE during persisted projects deserialisation [383513682]

Fixed

Bug

Status Update

No update yet.

Description

po...@gmail.com

created issue #1

Dec 11, 2024 05:00PM

*** !!!! THIS BUG TRACKER IS FOR GERRIT CODE REVIEW !!!! *** Do not submit bugs for chrome/android and issues with your company's *** Gerrit setup here. Those issues belong in different issue trackers.

What steps will reproduce the problem?

Checkout the All-Projects
Checkout meta/config: git fetch origin refs/meta/config && git checkout FETCH_HEAD
Leave the first column empty instead of a group UUID:

52f0c08a09692cec1a468a459729f863a1771931 Administrators 82f4a7e33c01512f7808e87c327ef783b7a4f509 Service Users <THIS_IS_A_TAB>Fake Group global:Anonymous-Users Anonymous Users global:Project-Owners Project Owners global:Registered-Users Registered Users

commit and push the change: git push origin HEAD:refs/meta/config

What is the expected output? Gerrit should not allow the change since the group ID is not valid.

What do you see instead? Gerrit accepts the commit, and this creates a 500 when accessing projects list.

Not that it is also possible to reproduce the issue:

via the UI, changing the groups via online edit
via a merge, i.e.: creating a change and merging it (that means validation is not "postponed" to the merge step)

Example of stack trace when browsing the repos from the UI:

[2024-12-11T17:49:57.987+01:00] [HTTP-76] ERROR com.google.gerrit.pgm.http.jetty.HiddenErrorHandler : Error in GET /admin/repos
com.google.gerrit.exceptions.StorageException: project state of project All-Projects not available
	at com.google.gerrit.server.project.ProjectCacheImpl.get(ProjectCacheImpl.java:226)
	at com.google.gerrit.server.project.ProjectCacheImpl.getAllProjects(ProjectCacheImpl.java:208)
	at com.google.gerrit.server.permissions.DefaultPermissionBackend.capabilities(DefaultPermissionBackend.java:70)
	at com.google.gerrit.server.permissions.DefaultPermissionBackend$WithUserImpl.has(DefaultPermissionBackend.java:259)
	at com.google.gerrit.server.permissions.DefaultPermissionBackend$WithUserImpl.can(DefaultPermissionBackend.java:205)
	at com.google.gerrit.server.permissions.DefaultPermissionBackend$WithUserImpl.can(DefaultPermissionBackend.java:168)
	at com.google.gerrit.server.permissions.DefaultPermissionBackend$WithUserImpl.test(DefaultPermissionBackend.java:154)
	at com.google.gerrit.server.permissions.PermissionBackend$WithUser.test(PermissionBackend.java:220)
	at com.google.gerrit.server.account.InternalAccountDirectory.fillAccountInfo(InternalAccountDirectory.java:103)
	at com.google.gerrit.server.account.AccountLoader.fill(AccountLoader.java:93)
	at com.google.gerrit.server.api.accounts.AccountApiImpl.get(AccountApiImpl.java:230)
	at com.google.gerrit.httpd.raw.IndexHtmlUtil.dynamicTemplateData(IndexHtmlUtil.java:123)
	at com.google.gerrit.httpd.raw.IndexHtmlUtil.templateData(IndexHtmlUtil.java:72)
	at com.google.gerrit.httpd.raw.IndexServlet.doGet(IndexServlet.java:81)
	at javax.servlet.http.HttpServlet.service(HttpServlet.java:687)
	at javax.servlet.http.HttpServlet.service(HttpServlet.java:790)
	at com.google.gerrit.httpd.raw.StaticModule$PolyGerritFilter.doFilter(StaticModule.java:394)
	at com.google.gerrit.httpd.XsrfCookieFilter.doFilter(XsrfCookieFilter.java:59)
	at com.google.gerrit.httpd.GetUserFilter.doFilter(GetUserFilter.java:92)
	at com.google.gerrit.httpd.RunAsFilter.doFilter(RunAsFilter.java:120)
	at com.google.gerrit.httpd.SetThreadNameFilter.doFilter(SetThreadNameFilter.java:62)
	at com.google.gerrit.httpd.AllRequestFilter$FilterProxy$1.doFilter(AllRequestFilter.java:139)
	at com.google.gerrit.httpd.AllowRenderInFrameFilter.doFilter(AllowRenderInFrameFilter.java:56)
	at com.google.gerrit.httpd.AllRequestFilter$FilterProxy$1.doFilter(AllRequestFilter.java:135)
	at com.google.gerrit.httpd.AllRequestFilter$FilterProxy.doFilter(AllRequestFilter.java:141)
	at com.google.gerrit.httpd.RequestCleanupFilter.doFilter(RequestCleanupFilter.java:60)
	at com.google.gerrit.httpd.RequestMetricsFilter.doFilter(RequestMetricsFilter.java:92)
	at com.google.gerrit.httpd.RequestContextFilter.doFilter(RequestContextFilter.java:64)
	at com.google.inject.servlet.ManagedFilterPipeline.dispatch(ManagedFilterPipeline.java:121)
	at com.google.inject.servlet.GuiceFilter.doFilter(GuiceFilter.java:133)
	at org.eclipse.jetty.servlet.FilterHolder.doFilter(FilterHolder.java:193)
	at org.eclipse.jetty.servlet.ServletHandler$Chain.doFilter(ServletHandler.java:1626)
	at org.eclipse.jetty.servlet.ServletHandler.doHandle(ServletHandler.java:552)
	at org.eclipse.jetty.server.handler.ScopedHandler.nextHandle(ScopedHandler.java:233)
	at org.eclipse.jetty.server.session.SessionHandler.doHandle(SessionHandler.java:1624)
	at org.eclipse.jetty.server.handler.ScopedHandler.nextHandle(ScopedHandler.java:233)
	at org.eclipse.jetty.server.handler.ContextHandler.doHandle(ContextHandler.java:1440)
	at org.eclipse.jetty.server.handler.ScopedHandler.nextScope(ScopedHandler.java:188)
	at org.eclipse.jetty.servlet.ServletHandler.doScope(ServletHandler.java:505)
	at org.eclipse.jetty.server.session.SessionHandler.doScope(SessionHandler.java:1594)
	at org.eclipse.jetty.server.handler.ScopedHandler.nextScope(ScopedHandler.java:186)
	at org.eclipse.jetty.server.handler.ContextHandler.doScope(ContextHandler.java:1355)
	at org.eclipse.jetty.server.handler.ScopedHandler.handle(ScopedHandler.java:141)
	at org.eclipse.jetty.server.handler.RequestLogHandler.handle(RequestLogHandler.java:54)
	at org.eclipse.jetty.server.handler.HandlerWrapper.handle(HandlerWrapper.java:127)
	at org.eclipse.jetty.server.Server.handle(Server.java:516)
	at org.eclipse.jetty.server.HttpChannel.lambda$handle$1(HttpChannel.java:487)
	at org.eclipse.jetty.server.HttpChannel.dispatch(HttpChannel.java:732)
	at org.eclipse.jetty.server.HttpChannel.handle(HttpChannel.java:479)
	at org.eclipse.jetty.server.HttpConnection.onFillable(HttpConnection.java:277)
	at org.eclipse.jetty.io.AbstractConnection$ReadCallback.succeeded(AbstractConnection.java:311)
	at org.eclipse.jetty.io.FillInterest.fillable(FillInterest.java:105)
	at org.eclipse.jetty.io.ChannelEndPoint$1.run(ChannelEndPoint.java:104)
	at org.eclipse.jetty.util.thread.strategy.EatWhatYouKill.runTask(EatWhatYouKill.java:338)
	at org.eclipse.jetty.util.thread.strategy.EatWhatYouKill.doProduce(EatWhatYouKill.java:315)
	at org.eclipse.jetty.util.thread.strategy.EatWhatYouKill.tryProduce(EatWhatYouKill.java:173)
	at org.eclipse.jetty.util.thread.strategy.EatWhatYouKill.run(EatWhatYouKill.java:131)
	at org.eclipse.jetty.util.thread.ReservedThreadExecutor$ReservedThread.run(ReservedThreadExecutor.java:409)
	at org.eclipse.jetty.util.thread.QueuedThreadPool.runJob(QueuedThreadPool.java:883)
	at org.eclipse.jetty.util.thread.QueuedThreadPool$Runner.run(QueuedThreadPool.java:1034)
	at java.base/java.lang.Thread.run(Thread.java:840)
Caused by: java.util.concurrent.ExecutionException: java.util.concurrent.ExecutionException: java.lang.NullPointerException: null key in entry: null=Group[Fake Group / null]
	at com.github.benmanes.caffeine.guava.CaffeinatedGuavaLoadingCache.get(CaffeinatedGuavaLoadingCache.java:63)
	at com.google.gerrit.server.project.ProjectCacheImpl.get(ProjectCacheImpl.java:223)
	... 60 more
Caused by: java.util.concurrent.ExecutionException: java.lang.NullPointerException: null key in entry: null=Group[Fake Group / null]
	at com.google.gerrit.server.cache.mem.PassthroughLoadingCache.get(PassthroughLoadingCache.java:90)
	at com.google.gerrit.server.cache.h2.H2CacheImpl.get(H2CacheImpl.java:134)
	at com.google.gerrit.server.project.ProjectCacheImpl$InMemoryLoader.load(ProjectCacheImpl.java:415)
	at com.google.gerrit.server.project.ProjectCacheImpl$InMemoryLoader.load(ProjectCacheImpl.java:356)
	at com.github.benmanes.caffeine.guava.CaffeinatedGuavaLoadingCache$SingleLoader.load(CaffeinatedGuavaLoadingCache.java:136)
	at com.github.benmanes.caffeine.cache.LocalLoadingCache.lambda$newMappingFunction$2(LocalLoadingCache.java:141)
	at com.github.benmanes.caffeine.cache.LocalCache.lambda$statsAware$0(LocalCache.java:139)
	at com.github.benmanes.caffeine.cache.BoundedLocalCache.lambda$doComputeIfAbsent$14(BoundedLocalCache.java:2413)
	at java.base/java.util.concurrent.ConcurrentHashMap.compute(ConcurrentHashMap.java:1916)
	at com.github.benmanes.caffeine.cache.BoundedLocalCache.doComputeIfAbsent(BoundedLocalCache.java:2411)
	at com.github.benmanes.caffeine.cache.BoundedLocalCache.computeIfAbsent(BoundedLocalCache.java:2394)
	at com.github.benmanes.caffeine.cache.LocalCache.computeIfAbsent(LocalCache.java:108)
	at com.github.benmanes.caffeine.cache.LocalLoadingCache.get(LocalLoadingCache.java:54)
	at com.github.benmanes.caffeine.guava.CaffeinatedGuavaLoadingCache.get(CaffeinatedGuavaLoadingCache.java:59)
	... 61 more
Caused by: java.lang.NullPointerException: null key in entry: null=Group[Fake Group / null]
	at com.google.common.collect.CollectPreconditions.checkEntryNotNull(CollectPreconditions.java:31)
	at com.google.common.collect.ImmutableMapEntry.<init>(ImmutableMapEntry.java:54)
	at com.google.common.collect.ImmutableMap.entryOf(ImmutableMap.java:345)
	at com.google.common.collect.ImmutableMap$Builder.put(ImmutableMap.java:454)
	at com.google.gerrit.entities.CachedProjectConfig$Builder.addGroup(CachedProjectConfig.java:154)
	at java.base/java.util.stream.ForEachOps$ForEachOp$OfRef.accept(ForEachOps.java:183)
	at java.base/java.util.stream.ReferencePipeline$3$1.accept(ReferencePipeline.java:197)
	at java.base/java.util.ArrayList$ArrayListSpliterator.forEachRemaining(ArrayList.java:1625)
	at java.base/java.util.stream.AbstractPipeline.copyInto(AbstractPipeline.java:509)
	at java.base/java.util.stream.AbstractPipeline.wrapAndCopyInto(AbstractPipeline.java:499)
	at java.base/java.util.stream.ForEachOps$ForEachOp.evaluateSequential(ForEachOps.java:150)
	at java.base/java.util.stream.ForEachOps$ForEachOp$OfRef.evaluateSequential(ForEachOps.java:173)
	at java.base/java.util.stream.AbstractPipeline.evaluate(AbstractPipeline.java:234)
	at java.base/java.util.stream.ReferencePipeline.forEach(ReferencePipeline.java:596)
	at com.google.gerrit.server.cache.serialize.entities.CachedProjectConfigSerializer.deserialize(CachedProjectConfigSerializer.java:72)
	at com.google.gerrit.server.project.ProjectCacheImpl$PersistedProjectConfigSerializer.deserialize(ProjectCacheImpl.java:495)
	at com.google.gerrit.server.project.ProjectCacheImpl$PersistedProjectConfigSerializer.deserialize(ProjectCacheImpl.java:485)
	at com.google.gerrit.server.cache.h2.H2CacheImpl$SqlStore.getIfPresent(H2CacheImpl.java:503)
	at com.google.gerrit.server.cache.h2.H2CacheImpl$Loader.load(H2CacheImpl.java:269)
	at com.google.gerrit.server.cache.h2.H2CacheImpl$Loader.load(H2CacheImpl.java:252)
	at com.google.gerrit.server.cache.mem.PassthroughLoadingCache.get(PassthroughLoadingCache.java:88)
	... 74 more

Comments

lu...@gmail.com <lu...@gmail.com> #2Dec 12, 2024 12:23AM

The validation is there, but the issue is that a formally valid groups file may actually contain an empty group UUID, which was supported and well tolerated for many releases.

The introduction of the persistent project cache with Change 275580 forgot to manage the empty field which isn't well supported by Protobuf that silently converts "" into null.

When the groups is updated, the change is accepted because the data is valid and the project cache is fully loaded as expected. However, once the in-memory cache expires, the attempt to reload it using protobuf would fail because of NPE.

lu...@gmail.com <lu...@gmail.com> #3Dec 12, 2024 01:16AM

Fix proposed with https://gerrit-review.googlesource.com/c/gerrit/+/445604

ap...@google.com <ap...@google.com> #4Dec 12, 2024 09:58PM

Project: gerrit
Branch: stable-3.10
Author: Luca Milanesio <luca.milanesio@gmail.com>
Link: https://gerrit-review.googlesource.com/445604

Fix NPE when de-serializing ProjectConfig with empty Group UUID

Expand for full commit details

Fix NPE when de-serializing ProjectConfig with empty Group UUID 
 
Protobuf is well-known for not managing correctly empty fields 
values, therefore the custom serializer/deserializer has to take 
into consideration and manage nulls coming from deserialization. 
 
Handle the null UUID by adding a GroupReference with an empty 
UUID string value, consistently with what was happening in Gerrit v3.2 
or earlier. 
 
Bug: Issue 383513682 
Release-Notes: Fix Protobuf deserialization of project config with empty Group UUIDs 
Forward-Compatible: checked 
Change-Id: I935539c22675027b7fd9d480756ef858d4a874d3

Files:

M java/com/google/gerrit/entities/AccountGroup.java
M java/com/google/gerrit/entities/CachedProjectConfig.java
M javatests/com/google/gerrit/server/cache/serialize/entities/CachedProjectConfigSerializerTest.java
M javatests/com/google/gerrit/server/cache/serialize/entities/GroupReferenceSerializerTest.java

Hash: e8c4c03da5ccbe7a9e2a21bb2ef8144a2ad3637a
Date: Thu Dec 12 00:28:29 2024

sy...@gmail.com <sy...@gmail.com> Jan 13, 2025 04:03PM

Marked as fixed, reassigned to sy...@gmail.com.

Issue 383513682

Description

Issue summary

Comments

lu...@gmail.com <lu...@gmail.com> #2Dec 12, 2024 12:23AM

lu...@gmail.com <lu...@gmail.com> #3Dec 12, 2024 01:16AM

ap...@google.com <ap...@google.com> #4Dec 12, 2024 09:58PM

sy...@gmail.com <sy...@gmail.com> Jan 13, 2025 04:03PM

Add comment

Issue metadata