Hoy en día todos conocen los servicios en la nube. Muchas empresas han descifrado este segmento del mercado y han creado sus propios servicios en la nube con diversos fines. Recientemente, nuestro equipo también ha estado interesado en estos servicios en términos de integrar el analizador de código PVS-Studio en ellos. Lo más probable es que nuestros lectores habituales ya hayan adivinado qué tipo de proyecto revisaremos esta vez. La elección recayó en el código de los servicios en la nube de Huawei.
Introduccion
Si está siguiendo las publicaciones del equipo PVS-Studio, probablemente haya notado que últimamente hemos estado profundizando en las tecnologías de la nube. Ya hemos publicado varios artículos sobre este tema:
Justo cuando estaba buscando un proyecto inusual para el próximo artículo, recibí un correo electrónico con una oferta de trabajo de
Huawei . Después de recopilar información sobre esta compañía, resultó que tenían sus propios servicios en la nube, pero lo principal es que el código fuente de estos servicios está disponible en GitHub. Esta fue la razón principal para elegir esta compañía para este artículo. Como dijo un sabio chino: "Los accidentes no son accidentales".
Déjame darte algunos detalles sobre nuestro analizador. PVS-Studio es un analizador estático para la detección de errores en el código fuente de los programas, escrito en C, C ++, C # y Java. El analizador funciona en Windows, Linux y macOS. Además de los complementos para entornos de desarrollo clásicos, como Visual Studio o IntelliJ IDEA, el analizador tiene la capacidad de integrarse en SonarQube y Jenkins:
Analisis de proyecto
Cuando estaba investigando el artículo, descubrí que Huawei tenía un
centro de desarrolladores con información disponible, manuales y fuentes de sus servicios en la nube. Se utilizó una amplia variedad de lenguajes de programación para crear estos servicios, pero los lenguajes más populares fueron Go, Java y Python.
Como me especializo en Java, los proyectos han sido seleccionados de acuerdo con mis conocimientos y habilidades. Puede obtener fuentes de proyectos analizadas en el artículo en un repositorio de GitHub
huaweicloud .
Para analizar proyectos, solo necesitaba algunas cosas que hacer:
- Obtenga proyectos del repositorio;
- Utilice las instrucciones de inicio del analizador Java y ejecute el análisis en cada proyecto.
Después de analizar los proyectos, seleccionamos solo tres de ellos, a los que nos gustaría prestarles atención. Es por el hecho de que el tamaño del resto de los proyectos de Java resultó ser demasiado pequeño.
Resultados del análisis del proyecto (número de advertencias y número de archivos):
Hubo pocas advertencias, lo que nos informa sobre la alta calidad del código, sobre todo porque no todas las advertencias apuntan a errores reales. Esto se debe al hecho de que el analizador a veces carece de información para distinguir el código correcto del erróneo. Por esta razón, ajustamos los diagnósticos del analizador día a día recurriendo a la información de los usuarios. Le invitamos a ver el artículo "
La forma en que los analizadores estáticos luchan contra los falsos positivos y por qué lo hacen ".
Al analizar el proyecto, elegí solo las advertencias más populares, de las que hablaré en este artículo.
Orden de inicialización de campos
El ciclo de inicialización de la clase
V6050 está presente. La inicialización de 'INSTANCE' aparece antes de la inicialización de 'LOG'. UntrustedSSL.java (32), UntrustedSSL.java (59), UntrustedSSL.java (33)
public class UntrustedSSL { private static final UntrustedSSL INSTANCE = new UntrustedSSL(); private static final Logger LOG = LoggerFactory.getLogger(UntrustedSSL.class); .... private UntrustedSSL() { try { .... } catch (Throwable t) { LOG.error(t.getMessage(), t);
Si hay alguna excepción en el constructor de la clase
UntrustedSSL , la información sobre esta excepción se registra en el bloque
catch utilizando el registrador
LOG . Sin embargo, debido al orden de inicialización de los campos estáticos, en el momento de inicializar el campo
INSTANCE , el
LOG aún no se ha inicializado. Por lo tanto, si registra información sobre la excepción en el constructor, dará como resultado
NullPointerException . Esta excepción es la razón de otra excepción,
ExceptionInInitializerError , que se genera si hubo una excepción cuando se inicializó el campo estático. Lo que necesita para resolver este problema es colocar la inicialización de
LOG antes de la inicialización de
INSTANCE .Error tipográfico discreto
V6005 La variable 'this.metricSchema' se asigna a sí misma. OpenTSDBSchema.java (72)
public class OpenTSDBSchema { @JsonProperty("metric") private List<SchemaField> metricSchema; .... public void setMetricsSchema(List<SchemaField> metricsSchema) { this.metricSchema = metricSchema;
Ambos métodos establecen el campo
metricSchema , pero los nombres de los métodos difieren según el símbolo 's'. El programador nombró los argumentos de estos métodos de acuerdo con el nombre del método. Como resultado, en la línea que señala el analizador, el campo metricSchema se asigna a sí mismo y no se utiliza el argumento del método metricsSchema.
V6005 La variable 'suspender' se asigna a sí misma. SuspendTransferTaskRequest.java (77)
public class SuspendTransferTaskRequest { .... private boolean suspend; .... public void setSuspend(boolean suspend) { suspend = suspend; } .... }
Aquí hay un error trivial relacionado con el descuido, debido a que el argumento de
suspensión se asigna a sí mismo. Como resultado, al campo de
suspensión no se le asignará el valor del argumento obtenido como está implícito. La versión correcta:
public void setSuspend(boolean suspend) { this.suspend = suspend; }
Condiciones predeterminadas
Como suele suceder, la regla
V6007 se rompe en términos de cantidad de advertencias.
V6007 La expresión 'firewallPolicyId == null' siempre es falsa. FirewallPolicyServiceImpl.java (125)
public FirewallPolicy removeFirewallRuleFromPolicy(String firewallPolicyId, String firewallRuleId) { checkNotNull(firewallPolicyId); checkNotNull(firewallRuleId); checkState(!(firewallPolicyId == null && firewallRuleId == null), "Either a Firewall Policy or Firewall Rule identifier must be set"); .... }
En este método, el argumento
checkNotNull verifica que los argumentos sean
nulos :
@CanIgnoreReturnValue public static <T> T checkNotNull(T reference) { if (reference == null) { throw new NullPointerException(); } else { return reference; } }
Después de verificar el argumento mediante el método
checkNotNull , puede estar 100% seguro de que el argumento pasado a este método no es igual a
nulo . Dado que ambos
métodos del método
removeFirewallRuleFromPolicy son verificados por el método
checkNotNull , su verificación adicional de
nulo no tiene sentido. Sin embargo, la expresión, donde los argumentos
firewallPolicyId y
firewallRuleId se vuelven a verificar para obtener un
valor nulo , se pasa como primer argumento al método
checkState .
También se emite una advertencia similar para
firewallRuleId :
- V6007 La expresión 'firewallRuleId == null' siempre es falsa. FirewallPolicyServiceImpl.java (125)
V6007 La expresión 'filteringParams! = Null' siempre es verdadera. NetworkPolicyServiceImpl.java (60)
private Invocation<NetworkServicePolicies> buildInvocation(Map<String, String> filteringParams) { .... if (filteringParams == null) { return servicePoliciesInvocation; } if (filteringParams != null) {
En este método, si el argumento
filteringParams es
nulo , el método devuelve un valor. Es por eso que la verificación a la que apunta el analizador siempre será verdadera, lo que, a su vez, significa que esta verificación no tiene sentido.
13 clases más son similares:
- V6007 La expresión 'filteringParams! = Null' siempre es verdadera. PolicyRuleServiceImpl.java (58)
- V6007 La expresión 'filteringParams! = Null' siempre es verdadera. GroupServiceImpl.java (58)
- V6007 La expresión 'filteringParams! = Null' siempre es verdadera. ExternalSegmentServiceImpl.java (57)
- V6007 La expresión 'filteringParams! = Null' siempre es verdadera. L3policyServiceImpl.java (57)
- V6007 La expresión 'filteringParams! = Null' siempre es verdadera. PolicyRuleSetServiceImpl.java (58)
- y así sucesivamente ...
Referencia nula
V6008 Desreferencia potencial nula de 'm.blockDeviceMapping'. NovaServerCreate.java (390)
@Override public ServerCreateBuilder blockDevice(BlockDeviceMappingCreate blockDevice) { if (blockDevice != null && m.blockDeviceMapping == null) { m.blockDeviceMapping = Lists.newArrayList(); } m.blockDeviceMapping.add(blockDevice);
En este método, la inicialización del campo de referencia
m.blockDeviceMapping no ocurrirá si el argumento
blockDevice es
nulo . Este campo se inicializa solo en este método, por lo que al llamar al método
add desde el campo
m.blockDeviceMapping , se producirá una
NullPointerException .
V6008 Desreferencia potencial nula de 'FileId.get (ruta)' en la función '<init>'. TrackedFile.java (140), TrackedFile.java (115)
public TrackedFile(FileFlow<?> flow, Path path) throws IOException { this(flow, path, FileId.get(path), ....); }
El constructor de la clase
TrackedFile recibe el resultado del
método estático
FileId.get (ruta) como un tercer argumento. Pero este método puede devolver
nulo :
public static FileId get(Path file) throws IOException { if (!Files.exists(file)) { return null; } .... }
En el constructor, llamado a través de
esto , el argumento
id no cambia hasta su primer uso:
public TrackedFile(...., ...., FileId id, ....) throws IOException { .... FileId newId = FileId.get(path); if (!id.equals(newId)) { .... } }
Como podemos ver, si se pasa
nulo como el tercer argumento del método, se producirá una excepción.
Aquí hay otro caso similar:
- V6008 Desreferencia potencial nula de 'buffer'. PublishingQueue.java (518)
V6008 Desreferencia potencial nula de 'dataTmpFile'. CacheManager.java (91)
@Override public void putToCache(PutRecordsRequest putRecordsRequest) { .... if (dataTmpFile == null || !dataTmpFile.exists()) { try { dataTmpFile.createNewFile();
NPE nuevamente. Una serie de comprobaciones en el operador condicional permite al objeto cero
dataTmpFile para una mayor desreferencia. Creo que hay dos errores tipográficos aquí y el cheque debería verse así:
if (dataTmpFile != null && !dataTmpFile.exists())
Subcadenas y números negativos
V6009 La función 'subcadena' podría recibir el valor '-1' mientras se espera un valor no negativo. Examine el argumento: 2. RemoveVersionProjectIdFromURL.java (37)
@Override public String apply(String url) { String urlRmovePojectId = url.substring(0, url.lastIndexOf("/")); return urlRmovePojectId.substring(0, urlRmovePojectId.lastIndexOf("/")); }
La implicación es que este método obtiene una URL como una cadena, que no está validada de ninguna manera. Más tarde, esta cadena se corta varias veces utilizando el método
lastIndexOf . Si el método
lastIndexOf no encuentra una coincidencia en la cadena, devolverá -1. Esto conducirá a
StringIndexOutOfBoundsException , ya que los argumentos del método de
subcadena tienen que ser números no negativos. Para la operación correcta del método, uno tiene que agregar una validación de argumento de entrada o verificar que los resultados del método
lastIndexOf son números no negativos.
Aquí hay algunos otros fragmentos con una forma similar:
- V6009 La función 'subcadena' podría recibir el valor '-1' mientras se espera un valor no negativo. Examine el argumento: 2. RemoveProjectIdFromURL.java (37)
- V6009 La función 'subcadena' podría recibir el valor '-1' mientras se espera un valor no negativo. Examine el argumento: 2. RemoveVersionProjectIdFromURL.java (38)
Resultado olvidado
V6010 Se requiere utilizar el valor de retorno de la función 'concat'. AKSK.java (278)
public static String buildCanonicalHost(URL url) { String host = url.getHost(); int port = url.getPort(); if (port > -1) { host.concat(":" + Integer.toString(port)); } return host; }
Al escribir este código, su autor no tuvo en cuenta que una llamada del método
concat no cambiará la cadena de
host debido a la inmutabilidad de los objetos de tipo
String . Para la operación correcta del método, el resultado del método
concat debe asignarse a la variable del
host en el bloque
if . La versión correcta:
if (port > -1) { host = host.concat(":" + Integer.toString(port)); }
Variables no utilizadas
V6021 La variable 'url' no se usa. TriggerV2Service.java (95)
public ActionResponse deleteAllTriggersForFunction(String functionUrn) { checkArgument(!Strings.isNullOrEmpty(functionUrn), ....); String url = ClientConstants.FGS_TRIGGERS_V2 + ClientConstants.URI_SEP + functionUrn; return deleteWithResponse(uri(triggersUrlFmt, functionUrn)).execute(); }
En este método, la variable
url no se usa después de su inicialización. Lo más probable es que la variable
url se pase al método
uri como un segundo argumento en lugar de
functionUrn , ya que la variable
functionUrn participa en la inicialización de la variable
url .
Argumento no utilizado el constructor
V6022 El parámetro 'returnType' no se usa dentro del cuerpo del constructor. HttpRequest.java (68)
public class HttpReQuest<R> { .... Class<R> returnType; .... public HttpRequest(...., Class<R> returnType)
En este constructor, el programador olvidó usar el argumento
returnType y asignó su valor al campo
returnType . Es por eso que al llamar al método
getReturnType desde el objeto, creado por este constructor, se devolverá
nulo de forma predeterminada. Pero lo más probable es que el programador pretendiera obtener el objeto, previamente pasado al constructor.
La misma funcionalidad
V6032 Es extraño que el cuerpo del método 'enable' sea completamente equivalente al cuerpo de otro método 'enable'. ServiceAction.java (32), ServiceAction.java (36)
public class ServiceAction implements ModelEntity { private String binary; private String host; private ServiceAction(String binary, String host) { this.binary = binary; this.host = host; } public static ServiceAction enable(String binary, String host) {
Tener dos métodos idénticos no es un error, pero el hecho de que dos métodos realicen la misma acción es al menos extraño. Mirando los nombres de los métodos anteriores, podemos suponer que deben realizar las acciones opuestas. De hecho, ambos métodos hacen lo mismo: crear y devolver el objeto
ServiceAction . Lo más probable es que el método de
inhabilitación se haya creado copiando el código del método de
habilitación , pero el cuerpo del método sigue siendo el mismo.
Olvidé comprobar lo principal
V6060 La referencia de 'params' se utilizó antes de que se verificara contra nulo. DomainService.java (49), DomainService.java (46)
public Domains list(Map<String, String> params) { Preconditions.checkNotNull(params.get("page_size"), ....); Preconditions.checkNotNull(params.get("page_number"), ....); Invocation<Domains> domainInvocation = get(Domains.class, uri("/domains")); if (params != null) {
En este método, el autor decidió verificar el contenido de una estructura del tipo
Mapa para
nulo . Para hacer esto, el método
get se llama dos veces desde el argumento
params . El resultado del método
get se pasa al método
checkNotNull . ¡Todo parece lógico, pero no es así! El argumento
params se verifica para
nulo en
if . Después de esto, se espera que el argumento de entrada sea
nulo , pero antes de esta comprobación, el método
get ya se ha llamado dos veces desde
params. Si se pasa
nulo como argumento a este método, la primera vez que llame al método
get , se generará una excepción.
Una situación similar ocurre en otros tres lugares:
- V6060 La referencia de 'params' se utilizó antes de que se verificara contra nulo. DomainService.java (389), DomainService.java (387)
- V6060 La referencia de 'params' se utilizó antes de que se verificara contra nulo. DomainService.java (372), DomainService.java (369)
- V6060 La referencia de 'params' se utilizó antes de que se verificara contra nulo. DomainService.java (353), DomainService.java (350)
Conclusión
Las grandes empresas de hoy no pueden prescindir del uso de servicios en la nube. Una gran cantidad de personas utilizan estos servicios. Desde este punto de vista, incluso un pequeño error en un servicio puede generar problemas para muchas personas, así como pérdidas adicionales, acumuladas por una empresa para remediar las consecuencias adversas de este error. Las fallas humanas siempre deben tenerse en cuenta, especialmente porque tarde o temprano todos cometen errores, como se describe en este artículo. Este hecho confirma el uso de todas las herramientas posibles para mejorar la calidad del código.
PVS-Studio definitivamente informará a la compañía Huawei sobre los resultados de verificar sus servicios en la nube para que los desarrolladores de Huawei puedan detenerse en ellos, porque el uso único del análisis de código estático cubierto por estos artículos (
1 ,
2 ) no puede demostrar completamente Todas sus ventajas. Puede descargar el analizador PVS-Studio
aquí .