Conversation
…tors Null dereferences: ADLdapUtil swallowed the NamingException of a GUID read and then indexed the null array - it throws a ConnectorException now; ObjectClassRunner iterated a null set when a test requires no operation; the CREST request converter had no case for RequestType.API and went on to use a null request; guards that came after the dereference (SchemaParser, AttributeTypeUtil, ObjectPool, MultiOpTests, TstAbstractConnector, CSVFileConnector.generateSyncDelta) are ordered or made explicit. Resources: the DocBook resources manifest writer was never closed, and IOUtil's reader and writer helpers close the outermost stream. Concurrency: OperationalContext publishes its Configuration only after the change callback is registered; ScriptedConfiguration's engine getter is synchronised for the whole initialisation because the customizer script re-enters it, so double-checked locking cannot be made safe by reordering; getCause() overrides and CompletionListener.start() are synchronised like the methods they override. Housekeeping: message listener queues that shadowed a superclass field are messageListeners; SQLParam.equals uses Objects.equals; boxed locals that never hold null are primitives; three always-true loop conditions are gone.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes 35 of the 37 open CodeQL alerts of severity
warningwithout a security rating. Not in here:java/constant-comparison#1541 (ObjectPool, already fixed in #126) andjava/unsafe-get-resource#1741 (ScriptedConfiguration.getClass().getResource(...): the default customizer path is built fromgetClass().getPackage()on purpose, so that a subclass ships its ownCustomizerScript.groovynext to itself — to be dismissed as intended).Real defects
ADLdapUtil(dereferenced-value-may-be-null#1567 #1568): both GUID conversions swallowed theNamingExceptionofattr.get()and then indexed the still-null array — the caller saw aNullPointerExceptionwith no clue about the LDAP error. They throw aConnectorExceptioncarrying it now.AbstractRemoteConnection.convert(missing-case-in-switch#1538, #1562): no case forRequestType.API, so the method went on to callsetHeaderon a null request.defaultthrowsNotSupportedException.ObjectClassRunner.isObjectClassSupported(#1563): a contract test with no required operation iterated a null set; it now falls back to every object class of the schema.DocBookResourceMojo(output-resource-leak#1559): theFileWriterof the remote-resources manifest was never closed, i.e. not even reliably flushed. try-with-resources.OperationalContext.getConfiguration(unsafe-double-checked-locking-init-order#1553): the configuration bean was stored in the volatile field before its change callback was registered, so another thread could use it without the callback; it is built completely and published last.ScriptedConfiguration.getGroovyScriptEngine(#1552): the same pattern, but it can not be fixed by reordering —initializeCustomizer()→getCustomizerClass()→getGroovyScriptEngine()re-enters the getter, and the customizer script getsthis. The getter issynchronizedfor the whole initialisation instead of double-checked, so other threads wait for the customizer rather than see the engine before it ran. Uncontended monitor, not a hot path.Guards after the dereference / redundant guards
SchemaParser(type.getName()beforeif (type != null)— now acontinueguard at the top, the block dedented; review with-w),AttributeTypeUtil(attrInfo.getType()beforeattrInfo != null),ObjectPool.borrowObject(if (null != rv)afterrv.getPooledObject();borrowObjectNoTestnever returns null),MultiOpTests(coBeforeTeststarts as an empty map),TstAbstractConnector(null-safe paged-results cookie),CSVFileConnector.generateSyncDelta(explicitIllegalArgumentExceptionwhen both objects are null).Housekeeping
field-masks-super-field#1739 #1740: theOperationMessageListenerqueues ofICFWebSocketandOpenICFWebSocketshadowed thelistenersfields of their superclasses; renamed tomessageListeners(theWebSocketListener...constructor parameter is untouched).input-resource-leak#1557 /output-resource-leak#1558:IOUtil.getResourceAsStringcloses the reader (which closes the stream),IOUtil.writeFileUTF8uses try-with-resources withStandardCharsets.UTF_8.non-sync-override#1548 #1547 #1783:getCause()overrides andCompletionListener.start()aresynchronizedlike the methods they override.reference-equality-on-strings#1539:SQLParam.equalsviaObjects.equals.non-null-boxed-variable×9:Boolean/Integerlocals that never hold null are primitives (getColumnTypenever returns null —Types.NULLfallback).constant-comparison#1540 #1542 #1543: always-true conditions removed.Tests
No new tests: the changes are dead conditions, types, monitors and ordering with no behaviour a test can observe, plus null paths unreachable with the current callers (the
ADLdapUtilone needs an LDAPAttributewhoseget()throws; can be added with a stub if wanted).Local runs of all 13 touched modules, all green: framework 186, dbcommon 86, internal 469, contract 43, server 29, grizzly 34, csvfile 78, databasetable 43, groovy 125, ldap 159 (embedded OpenDJ), xml 81.
CSVFileConnector's header gets the same line #131 adds, so the two merge cleanly.