github-actions[bot] commented on code in PR #63681: URL: https://github.com/apache/doris/pull/63681#discussion_r3308082060
########## fe/fe-core/src/main/java/org/apache/doris/catalog/JdbcDriverLoader.java: ########## @@ -0,0 +1,139 @@ +// Licensed to the Apache Software Foundation (ASF) under one +// or more contributor license agreements. See the NOTICE file +// distributed with this work for additional information +// regarding copyright ownership. The ASF licenses this file +// to you under the Apache License, Version 2.0 (the +// "License"); you may not use this file except in compliance +// with the License. You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, +// software distributed under the License is distributed on an +// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +// KIND, either express or implied. See the License for the +// specific language governing permissions and limitations +// under the License. + +package org.apache.doris.catalog; + +import org.apache.doris.common.DdlException; + +import org.apache.commons.lang3.StringUtils; +import org.apache.logging.log4j.LogManager; +import org.apache.logging.log4j.Logger; + +import java.net.MalformedURLException; +import java.net.URL; +import java.net.URLClassLoader; +import java.sql.Connection; +import java.sql.Driver; +import java.sql.DriverManager; +import java.sql.DriverPropertyInfo; +import java.sql.SQLException; +import java.sql.SQLFeatureNotSupportedException; +import java.util.Map; +import java.util.Properties; +import java.util.Set; +import java.util.concurrent.ConcurrentHashMap; + +public class JdbcDriverLoader { + private static final Logger LOG = LogManager.getLogger(JdbcDriverLoader.class); + private static final Map<URL, ClassLoader> DRIVER_CLASS_LOADER_CACHE = new ConcurrentHashMap<>(); + private static final Set<String> REGISTERED_DRIVER_KEYS = ConcurrentHashMap.newKeySet(); + + private JdbcDriverLoader() { + } + + public static String validateDriverChecksum(String driverUrl, String expectedChecksum) throws DdlException { + String actualChecksum = JdbcResource.computeObjectChecksum(driverUrl); + if (StringUtils.isNotBlank(expectedChecksum) && !expectedChecksum.equals(actualChecksum)) { + throw new DdlException("The provided checksum (" + expectedChecksum + + ") does not match the computed checksum (" + actualChecksum + + ") for the driver_url."); + } + return actualChecksum; + } + + public static String registerDriver(String driverUrl, String driverClassName, String expectedChecksum, + ClassLoader parentClassLoader) { + if (StringUtils.isBlank(driverClassName)) { + throw new IllegalArgumentException("JDBC driver class is required when driver url is specified."); + } + + try { + String fullDriverUrl = JdbcResource.getFullDriverUrl(driverUrl); + try { + validateDriverChecksum(fullDriverUrl, expectedChecksum); + } catch (DdlException e) { + throw new IllegalArgumentException(e.getMessage(), e); + } + URL url = new URL(fullDriverUrl); + String driverKey = fullDriverUrl + "#" + driverClassName; + if (!REGISTERED_DRIVER_KEYS.add(driverKey)) { + LOG.info("JDBC driver already registered: {} from {}", driverClassName, fullDriverUrl); Review Comment: This cache key does not include the checksum/content identity, so validating the new bytes is not enough to ensure those bytes are the driver that will execute. A concrete failure path is: create/use a Paimon JDBC catalog with `driver_url=file:///x/driver.jar`, class `C`, checksum A; FE registers and caches that classloader/driver. Replace `/x/driver.jar` with different bytes and update the catalog checksum to B. `registerDriver()` computes and accepts B, but `REGISTERED_DRIVER_KEYS.add(fullDriverUrl + "#" + driverClassName)` returns false and this method returns the old already-registered driver. The checksum then no longer binds to the code used by DriverManager. Please include the checksum/content identity in the registration and classloader cache key, or reject/reload same-URL content changes explicitly. ########## fe/fe-core/src/main/java/org/apache/doris/datasource/paimon/PaimonExternalCatalog.java: ########## @@ -169,6 +173,45 @@ public void checkProperties() throws DdlException { catalogProperty.checkMetaStoreAndStorageProperties(AbstractPaimonProperties.class); } + @Override + public void checkWhenCreating() throws DdlException { + validateJdbcDriverChecksumIfNeeded(); Review Comment: Putting checksum validation/defaulting only in `checkWhenCreating()` misses `ALTER CATALOG`. The alter path calls `tryModifyCatalogProps()` and then `checkProperties()`, but not `checkWhenCreating()`, so a user can alter an existing Paimon JDBC catalog to set `paimon.jdbc.driver_url`/`paimon.jdbc.driver_checksum` with a bad checksum and the property is accepted and logged; missing checksums are also not defaulted/persisted on alter. Please run the same validation/defaulting from the property-check path used by alter, and add an alter-path test. ########## fe/be-java-extensions/java-common/src/main/java/org/apache/doris/common/jdbc/JdbcDriverUtils.java: ########## @@ -31,13 +39,26 @@ import java.util.concurrent.ConcurrentHashMap; public final class JdbcDriverUtils { + private static final int HTTP_TIMEOUT_MS = 10000; private static final ConcurrentHashMap<URL, ClassLoader> DRIVER_CLASS_LOADER_CACHE = new ConcurrentHashMap<>(); private static final Set<String> REGISTERED_DRIVER_KEYS = ConcurrentHashMap.newKeySet(); private JdbcDriverUtils() { } public static void registerDriver(String driverUrl, String driverClassName, ClassLoader classLoader) { + registerDriver(driverUrl, driverClassName, "", classLoader); + } + + public static void registerDriver(String driverUrl, String driverClassName, String expectedChecksum, + ClassLoader classLoader) { + if (StringUtils.isBlank(driverClassName)) { + throw new IllegalArgumentException("JDBC driver class is required when driver url is specified."); + } + if (StringUtils.isNotBlank(expectedChecksum)) { + validateDriverChecksum(driverUrl, expectedChecksum); + } + try { URL url = new URL(driverUrl); Review Comment: The BE-side registration has the same same-URL replacement problem as the FE loader: checksum validation happens before registration, but the process-wide `REGISTERED_DRIVER_KEYS` and `DRIVER_CLASS_LOADER_CACHE` are still keyed only by URL/classloader URL. If the JAR at the same URL is replaced and FE sends the new checksum, BE accepts the checksum and then reuses the old registered driver/classloader. Please make the checksum/content identity part of the cache/registration identity or reject this case so the validated bytes are the bytes that execute. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
