En este siglo, todos han oído hablar de los servicios en la nube. Muchas empresas han dominado este segmento del mercado y creado sus servicios en la nube en varias direcciones. Nuestro equipo también ha estado interesado recientemente en estos servicios en términos de integrar el analizador de código PVS-Studio con ellos. Creemos que nuestros lectores habituales ya adivinan qué tipo de proyecto revisaremos esta vez. La elección recayó en el código de servicios en la nube de Huawei.
Introduccion
Si sigue al equipo de PVS-Studio, probablemente haya notado que recientemente hemos estado muy interesados en las tecnologías en la nube. Ya hemos publicado varios artículos relacionados con este tema:
Y así, cuando estaba recogiendo otro proyecto interesante para el próximo artículo, recibí una oferta de trabajo de
Huawei por correo. Al recopilar información sobre esta empresa, resultó que tienen sus propios servicios en la nube, pero lo principal es que el código fuente de estos servicios está disponible gratuitamente 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".
Ahora un poco sobre nuestro analizador. PVS-Studio es un analizador de código estático que identifica errores y vulnerabilidades en el código fuente de programas escritos en C, C ++, C # y Java. El analizador funciona en las plataformas 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 con SonarQube y Jenkins:
Analisis de proyecto
En el proceso de búsqueda de información para el artículo, descubrí que Huawei tiene un
centro de desarrolladores donde puede obtener información, guías y el código fuente de sus servicios en la nube. Se utilizaron varios lenguajes de programación para crear estos servicios, pero los lenguajes como Go, Java y Python resultaron ser los más populares.
Como me especializo en Java, los proyectos fueron elegidos en consecuencia. Las fuentes de los proyectos analizados en el artículo se pueden obtener en el repositorio de GitHub
huaweicloud .
Para analizar los proyectos, solo necesitaba realizar algunas acciones:
- Obtenga proyectos del repositorio;
- Utilice las instrucciones para iniciar el analizador Java e iniciar el análisis en cada uno de los proyectos.
Después de analizar los proyectos, logramos destacar solo tres a los que me gustaría prestar atención. Esto se debe al hecho de que el tamaño de los proyectos restantes de Java era demasiado pequeño.
Resultados del análisis del proyecto (número de advertencias y número de archivos):
Hay pocas advertencias, por lo que, en general, podemos decir sobre la buena calidad del código. Además, no todas las operaciones son errores válidos. Esto se debe al hecho de que el analizador a veces no tiene suficiente información para distinguir el código correcto del incorrecto. Por lo tanto, los diagnósticos del analizador se mejoran todos los días con la ayuda de la información recibida de los usuarios. Consulte también el artículo "
Cómo y por qué los analizadores estáticos luchan contra los falsos positivos ".
En el proceso de análisis de proyectos, seleccioné las advertencias más interesantes, que analizaré en este artículo.
Orden de inicialización de campo
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 se produce 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, el
LOG al momento de inicializar el campo
INSTANCE aún no se ha inicializado. Por lo tanto, al registrar información de excepción en el constructor, se generará una
NullPointerException . Esta excepción es la causa de otra
excepción ExceptionInInitializerError que se produce si se produce una excepción al inicializar el campo estático. Todo lo que se necesita para resolver el problema descrito es colocar la inicialización de
LOG antes de la inicialización de
INSTANCE .
Error tipográfico invisible
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 carácter 's'. El programador nombró los argumentos de estos métodos de acuerdo con el nombre del método. Como resultado, en la línea señalada por 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 banal relacionado con el descuido, debido a que ocurre la asignación del argumento
suspendido a sí mismo. Como resultado, el valor del argumento llegado no se asignará al campo de
suspensión , como está implícito. Correctamente:
public void setSuspend(boolean suspend) { this.suspend = suspend; }
Condiciones predeterminadas
Como suele suceder, la regla
V6007 se rompe en términos de la 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 método
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 con el método
checkNotNull , puede estar 100% seguro de que el argumento pasado a este método no es
nulo . Dado que el método
checkNotNull verifica ambos argumentos del método
removeFirewallRuleFromPolicy , no tiene sentido volver a verificar los argumentos para
valores nulos . Sin embargo, se pasa una expresión al método
checkState como primer argumento, donde los argumentos
firewallPolicyId y
firewallRuleId se vuelven a verificar para que sean
nulos .
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 sale y devuelve un valor. Por lo tanto, la prueba señalada por el analizador siempre será verdadera, lo que significa que esta prueba no tiene sentido.
Similar ocurre en 13 clases:
- 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 cuando se llama al método
add , el campo
m.blockDeviceMapping arrojará 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 resultado del método estático
FileId.get (ruta) se pasa al constructor de la clase
TrackedFile como 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 que se usa por primera vez:
public TrackedFile(...., ...., FileId id, ....) throws IOException { .... FileId newId = FileId.get(path); if (!id.equals(newId)) { .... } }
Por lo tanto, si se pasa
nulo al método como tercer argumento, se producirá una excepción.
Una situación similar ocurre en otro caso:
- 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();
Y de nuevo NPE. Una serie de comprobaciones en la declaración condicional permiten que el objeto
dataTmpFile sea nulo para una mayor desreferenciación. Creo que hay dos errores tipográficos aquí, y la verificación debería ser 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("/")); }
Se entiende que la URL se pasa a este método como una cadena, que no está validada de ninguna manera. Después de que esta cadena se recorta varias veces utilizando el método
lastIndexOf . Si
lastIndexOf no encuentra una coincidencia en la cadena, devolverá -1. Esto arrojará una
StringIndexOutOfBoundsException , ya que los argumentos del método de
subcadena deben ser números no negativos. Para que el método funcione correctamente, debe agregar la validación del argumento de entrada o verificar que los resultados de los métodos
lastIndexOf no sean números negativos.
Aquí hay otros lugares donde esto sucede:
- 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, no se tuvo en cuenta que llamar al método
concat no cambiará la cadena de
host debido a la inmutabilidad de los objetos de tipo
String . Para que el método funcione correctamente, debe asignar el resultado del método
concat a la variable del
lenguaje principal en el bloque
if . Correctamente:
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 la inicialización. Lo más probable es que la variable
url se pase al método
uri como segundo argumento en lugar de
functionUrn , ya que la variable
functionUrn está involucrada en la inicialización de la variable
url .
Argumento no utilizado en 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, olvidaron usar el argumento
returnType y asignar su valor al campo
returnType . Por lo tanto, cuando se llama al método
getReturnType , el objeto creado por este constructor devolverá el valor predeterminado de
nulo , aunque lo más probable es que tuviera la intención de obtener el objeto que se pasó previamente 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) {
La presencia de 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 realizan las acciones opuestas. De hecho, ambos métodos hacen lo mismo: crean y devuelven un objeto
ServiceAction . Lo más probable es que el método de
deshabilitación se haya creado copiando el código del método de
habilitación , pero se olvidó de cambiar el cuerpo del método.
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, decidimos verificar el contenido de una estructura de tipo
Map para la desigualdad
nula . Para hacer esto, el argumento
params llama al método
get dos veces, cuyo resultado se pasa al método
checkNotNull . Todo parece lógico, ¡pero no importa cómo! En
if , el argumento
params se verifica para
nulo . Después de lo cual se puede suponer que el argumento de entrada puede ser
nulo , pero antes de esta comprobación, el método
get ya se llamaba en los
parámetros dos veces. Si se pasa
nulo como argumento a este método, entonces la primera vez
que se llama al método
get , se generará una excepción.
Una situación similar ocurre en tres lugares más:
- 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 modernas en estos días simplemente no pueden prescindir del uso de servicios en la nube. Una gran cantidad de personas utiliza estos servicios, por lo que incluso el más mínimo error en el servicio puede generar problemas para muchas personas, así como costos adicionales para que la empresa corrija las consecuencias de este error. En el desarrollo, siempre es necesario tener en cuenta el factor humano, ya que cualquier persona tarde o temprano comete errores, y este artículo es un ejemplo de esto. Es por eso que debe usar todas las herramientas posibles para mejorar la calidad del código.
PVS-Studio ciertamente informará a Huawei sobre los resultados de verificar sus servicios en la nube para que los desarrolladores de esta compañía puedan estudiarlos con más detalle.
El uso único del análisis de código estático demostrado en el artículo no puede mostrar todas sus ventajas. Puede encontrar información más detallada sobre cómo usar el análisis estático correctamente
aquí y
aquí . Puede descargar el analizador PVS-Studio
aquí .
Si desea compartir este artículo con una audiencia de habla inglesa, utilice el enlace a la traducción: Valery Komarov.
Huawei Cloud: está nublado en PVS-Studio hoy .