Changeset 18665 in josm


Ignore:
Timestamp:
2023-02-16T16:01:49+01:00 (14 months ago)
Author:
taylor.smock
Message:

Fix several coverity issues

CID-1504572: Synchronization on java.util.concurrent objects

The lock wasn't necessary after moving from AtomicBoolean to
CountDownLatch

CID-1504570: Dereference null value -- the default getApiUrl return was null
CID-1504570: Explicit null dereferenced

Consumer<IOAuthToken> to Consumer<Optional<IOAuthToken>>

CID-1476014, CID-1476013, CID-1476011: Resource leak

These resource leaks are unlikely to happen outside of plugin code,
and are all related to StringSelection#getTransferData, which may
return a Closeable object, but is unlikely to be the Transferable
for any of the affected classes.


This also fixes some recently introduced SonarLint issues. It also suppresses
squid:S100 ("Method names should comply with a naming convention") for the MapCSS
Functions class, since the convention for that method names in that class is
^[a-zA-Z][a-zA-Z0-9_]*$

Location:
trunk
Files:
14 edited

Legend:

Unmodified
Added
Removed

  • trunk/src/org/openstreetmap/josm/actions/mapmode/SelectAction.java

    r18554 r18665  
    2020import java.util.LinkedList;
    2121import java.util.List;
     22import java.util.Objects;
    2223import java.util.Optional;
    2324
    … …  
    195196     * set would have to be checked.
    196197     */
    197     private transient Optional<OsmPrimitive> currentHighlight = Optional.empty();
     198    private transient OsmPrimitive currentHighlight;
    198199
    199200    /**
    … …  
    276277        determineMapMode(c.isPresent());
    277278
    278         Optional<OsmPrimitive> newHighlight = Optional.empty();
    279 
    280279        virtualManager.clear();
    281280        if (mode == Mode.MOVE && !dragInProgress() && virtualManager.activateVirtualNodeNearPoint(e.getPoint())) {
    … …  
    286285            mv.setNewCursor(SelectActionCursor.virtual_node.cursor(), this);
    287286            // don't highlight anything else if a virtual node will be
    288             return repaintIfRequired(newHighlight);
     287            return repaintIfRequired(null);
    289288        }
    290289
    … …  
    293292        // return early if there can't be any highlights
    294293        if (!drawTargetHighlight || (mode != Mode.MOVE && mode != Mode.SELECT) || !c.isPresent())
    295             return repaintIfRequired(newHighlight);
     294            return repaintIfRequired(null);
    296295
    297296        // CTRL toggles selection, but if while dragging CTRL means merge
    298297        final boolean isToggleMode = platformMenuShortcutKeyMask && !dragInProgress();
    299         if (c.isPresent() && (isToggleMode || !c.get().isSelected())) {
     298        OsmPrimitive newHighlight = null;
     299        if (isToggleMode || !c.get().isSelected()) {
    300300            // only highlight primitives that will change the selection
    301301            // when clicked. I.e. don't highlight selected elements unless
    302302            // we are in toggle mode.
    303             newHighlight = c;
     303            newHighlight = c.get();
    304304        }
    305305        return repaintIfRequired(newHighlight);
    … …  
    377377            ds.clearHighlightedVirtualNodes();
    378378        }
    379         if (!currentHighlight.isPresent()) {
     379        if (currentHighlight == null) {
    380380            return needsRepaint;
    381         } else {
    382             currentHighlight.get().setHighlighted(false);
    383         }
    384         currentHighlight = Optional.empty();
     381        }
     382        currentHighlight.setHighlighted(false);
     383        currentHighlight = null;
    385384        return true;
    386385    }
    387386
    388     private boolean repaintIfRequired(Optional<OsmPrimitive> newHighlight) {
    389         if (!drawTargetHighlight || currentHighlight.equals(newHighlight))
     387    private boolean repaintIfRequired(OsmPrimitive newHighlight) {
     388        if (!drawTargetHighlight || Objects.equals(currentHighlight, newHighlight))
    390389            return false;
    391         currentHighlight.ifPresent(osm -> osm.setHighlighted(false));
    392         newHighlight.ifPresent(osm -> osm.setHighlighted(true));
     390        if (currentHighlight != null) {
     391            currentHighlight.setHighlighted(false);
     392        }
     393        if (newHighlight != null) {
     394            newHighlight.setHighlighted(true);
     395        }
    393396        currentHighlight = newHighlight;
    394397        return true;
    … …  
    533536            if (p != null) {
    534537                p.setHighlighted(true);
    535                 currentHighlight = Optional.of(p);
     538                currentHighlight = p;
    536539                needsRepaint = true;
    537540            }
    … …  
    905908
    906909    static void checkCommandForLargeDistance(Command lastCommand) {
     910        if (lastCommand == null) {
     911            return;
     912        }
    907913        final int moveCount = lastCommand.getParticipatingPrimitives().size();
    908914        if (lastCommand instanceof MoveCommand) {

  • trunk/src/org/openstreetmap/josm/data/oauth/IOAuthAuthorization.java

    r18650 r18665  
    22package org.openstreetmap.josm.data.oauth;
    33
     4import java.util.Optional;
    45import java.util.function.Consumer;
    56
    … …  
    1617     * @param scopes The scopes to ask for
    1718     */
    18     void authorize(IOAuthParameters parameters, Consumer<IOAuthToken> consumer, Enum<?>... scopes);
     19    void authorize(IOAuthParameters parameters, Consumer<Optional<IOAuthToken>> consumer, Enum<?>... scopes);
    1920}

  • trunk/src/org/openstreetmap/josm/data/oauth/IOAuthParameters.java

    r18650 r18665  
    9191    }
    9292
     93    /**
     94     * Store the preferences for these parameters
     95     */
    9396    void rememberPreferences();
    9497}

  • trunk/src/org/openstreetmap/josm/data/oauth/IOAuthToken.java

    r18650 r18665  
    1313     * Sign a client
    1414     * @param client The client to sign
     15     * @throws OAuthException if the OAuth token type is unknown (AKA we do't know how to handle it)
    1516     */
    1617    void sign(HttpClient client) throws OAuthException;

  • trunk/src/org/openstreetmap/josm/data/oauth/OAuth20Authorization.java

    r18662 r18665  
    1313import java.util.Map;
    1414import java.util.Objects;
     15import java.util.Optional;
    1516import java.util.UUID;
    1617import java.util.function.Consumer;
    … …  
    4546
    4647    @Override
    47     public void authorize(IOAuthParameters parameters, Consumer<IOAuthToken> consumer, Enum<?>... scopes) {
     48    public void authorize(IOAuthParameters parameters, Consumer<Optional<IOAuthToken>> consumer, Enum<?>... scopes) {
    4849        final String state = UUID.randomUUID().toString();
    4950        final String codeVerifier = UUID.randomUUID().toString(); // Cryptographically random string (ASCII)
    … …  
    6263        private final String state;
    6364        private final IOAuthParameters parameters;
    64         private final Consumer<IOAuthToken> consumer;
     65        private final Consumer<Optional<IOAuthToken>> consumer;
    6566        private final String codeVerifier;
    6667
    67         OAuth20AuthorizationHandler(String state, String codeVerifier, IOAuthParameters parameters, Consumer<IOAuthToken> consumer) {
     68        OAuth20AuthorizationHandler(String state, String codeVerifier, IOAuthParameters parameters, Consumer<Optional<IOAuthToken>> consumer) {
    6869            this.state = state;
    6970            this.parameters = parameters;
    … …  
    9899                    HttpClient.Response response = tradeCodeForToken.getResponse();
    99100                    OAuth20Token oAuth20Token = new OAuth20Token(parameters, response.getContentReader());
    100                     consumer.accept(oAuth20Token);
     101                    consumer.accept(Optional.of(oAuth20Token));
    101102                } catch (IOException | OAuth20Exception e) {
    102                     consumer.accept(null);
     103                    consumer.accept(Optional.empty());
    103104                    throw new JosmRuntimeException(e);
    104105                } finally {

  • trunk/src/org/openstreetmap/josm/data/oauth/OAuth20Token.java

    r18650 r18665  
    2020import org.openstreetmap.josm.tools.HttpClient;
    2121import org.openstreetmap.josm.tools.JosmRuntimeException;
     22import org.openstreetmap.josm.tools.Utils;
    2223
    2324/**
    … …  
    8182    @Override
    8283    public void sign(HttpClient client) throws OAuthException {
    83         if (!this.oauthParameters.getApiUrl().contains(client.getURL().getHost())) {
     84        if (!Utils.isBlank(this.oauthParameters.getApiUrl())
     85                && !this.oauthParameters.getApiUrl().contains(client.getURL().getHost())) {
    8486            String host = URI.create(this.oauthParameters.getAccessTokenUrl()).getHost();
    8587            throw new IllegalArgumentException("Cannot sign URL with token for different host: Expected " + host

  • trunk/src/org/openstreetmap/josm/gui/datatransfer/importers/FilePaster.java

    r17556 r18665  
    44import java.awt.datatransfer.DataFlavor;
    55import java.awt.datatransfer.UnsupportedFlavorException;
     6import java.io.Closeable;
    67import java.io.File;
    78import java.io.IOException;
    … …  
    1516import org.openstreetmap.josm.gui.io.importexport.Options;
    1617import org.openstreetmap.josm.gui.layer.OsmDataLayer;
     18import org.openstreetmap.josm.tools.Utils;
    1719
    1820/**
    … …  
    3234    public boolean importData(TransferSupport support, OsmDataLayer layer, EastNorth pasteAt)
    3335            throws UnsupportedFlavorException, IOException {
    34         @SuppressWarnings("unchecked")
    35         List<File> files = (List<File>) support.getTransferable().getTransferData(df);
    36         OpenFileAction.OpenFileTask task = new OpenFileAction.OpenFileTask(files, null);
    37         task.setOptions(Options.RECORD_HISTORY);
    38         MainApplication.worker.submit(task);
    39         return true;
     36        final Object data = support.getTransferable().getTransferData(df);
     37        if (data instanceof List) {
     38            @SuppressWarnings("unchecked")
     39            List<File> files = (List<File>) data;
     40            OpenFileAction.OpenFileTask task = new OpenFileAction.OpenFileTask(files, null);
     41            task.setOptions(Options.RECORD_HISTORY);
     42            MainApplication.worker.submit(task);
     43            return true;
     44        }
     45        // We should never hit this code -- Coverity thinks that it is possible for this to be called with a
     46        // StringSelection transferable, which is not currently possible with our code. It *could* be done from
     47        // a plugin though.
     48        if (data instanceof Closeable) {
     49            Utils.close((Closeable) data);
     50        }
     51        throw new UnsupportedFlavorException(df);
    4052    }
    4153}

  • trunk/src/org/openstreetmap/josm/gui/datatransfer/importers/TagTransferPaster.java

    r10604 r18665  
    33
    44import java.awt.datatransfer.UnsupportedFlavorException;
     5import java.io.Closeable;
    56import java.io.IOException;
    67import java.util.Map;
    … …  
    910
    1011import org.openstreetmap.josm.gui.datatransfer.data.TagTransferData;
     12import org.openstreetmap.josm.tools.Utils;
    1113
    1214/**
    … …  
    2527    @Override
    2628    protected Map<String, String> getTags(TransferSupport support) throws UnsupportedFlavorException, IOException {
    27         TagTransferData data = (TagTransferData) support.getTransferable().getTransferData(df);
    28         return data.getTags();
     29        final Object data = support.getTransferable().getTransferData(df);
     30        if (data instanceof TagTransferData) {
     31            return ((TagTransferData) data).getTags();
     32        }
     33        // We should never hit this code -- Coverity thinks that it is possible for this to be called with a
     34        // StringSelection transferable, which is not currently possible with our code. It *could* be done from
     35        // a plugin though.
     36        if (data instanceof Closeable) {
     37            Utils.close((Closeable) data);
     38        }
     39        throw new UnsupportedFlavorException(df);
    2940    }
    3041}

  • trunk/src/org/openstreetmap/josm/gui/dialogs/layer/LayerListTransferHandler.java

    r16438 r18665  
    66import java.awt.datatransfer.Transferable;
    77import java.awt.datatransfer.UnsupportedFlavorException;
     8import java.io.Closeable;
    89import java.io.IOException;
    910import java.util.ArrayList;
    … …  
    2223import org.openstreetmap.josm.gui.layer.OsmDataLayer;
    2324import org.openstreetmap.josm.tools.Logging;
     25import org.openstreetmap.josm.tools.Utils;
    2426
    2527/**
    … …  
    3234 */
    3335public class LayerListTransferHandler extends TransferHandler {
     36    private static final long serialVersionUID = -5924044609120394316L;
     37
    3438    @Override
    3539    public int getSourceActions(JComponent c) {
    … …  
    4852
    4953    private static boolean onlyDataLayersSelected(LayerListModel tableModel) {
    50         return tableModel.getSelectedLayers().stream().allMatch(l -> l instanceof OsmDataLayer);
     54        return tableModel.getSelectedLayers().stream().allMatch(OsmDataLayer.class::isInstance);
    5155    }
    5256
    … …  
    7983            LayerListModel tableModel = (LayerListModel) ((JTable) support.getComponent()).getModel();
    8084
    81             LayerTransferable.Data layers = (LayerTransferable.Data) support.getTransferable()
     85            final Object data = support.getTransferable()
    8286                    .getTransferData(LayerTransferable.LAYER_DATA);
     87            final LayerTransferable.Data layers;
     88            if (data instanceof LayerTransferable.Data) {
     89                layers = (LayerTransferable.Data) data;
     90            } else if (data instanceof Closeable) {
     91                // We should never hit this code -- Coverity thinks that it is possible for this to be called with a
     92                // StringSelection transferable, which is not currently possible with our code. It *could* be done from
     93                // a plugin though.
     94                Utils.close((Closeable) data);
     95                throw new UnsupportedFlavorException(LayerTransferable.LAYER_DATA);
     96            } else {
     97                throw new UnsupportedFlavorException(LayerTransferable.LAYER_DATA);
     98            }
    8399
    84100            int dropLocation;

  • trunk/src/org/openstreetmap/josm/gui/mappaint/mapcss/Functions.java

    r18664 r18665  
    5656 * @since 15245 (extracted from {@link ExpressionFactory})
    5757 */
    58 @SuppressWarnings("UnusedDeclaration")
     58@SuppressWarnings({"UnusedDeclaration", "squid:S100"})
    5959public final class Functions {
    6060
    … …  
    313313     * @param args arguments. First one is used as separator
    314314     * @return assembled string
    315      * @see String#join
     315     * @see String#join(CharSequence, CharSequence...)
    316316     */
    317317    @NullableArguments
    … …  
    325325     * @param values collection of objects
    326326     * @return assembled string
    327      * @see String#join
     327     * @see String#join(CharSequence, Iterable)
    328328     */
    329329    public static String join_list(final String separator, final List<String> values) {
    … …  
    739739     * @see Object#equals(Object)
    740740     */
     741    @SuppressWarnings("squid:S1221")
    741742    public static boolean equal(Object a, Object b) {
    742743        if (a.getClass() == b.getClass()) return a.equals(b);
    … …  
    775776
    776777    /**
    777      * Obtains the JOSM'key {@link org.openstreetmap.josm.data.Preferences} string for key {@code key},
     778     * Obtains the JOSM key {@link org.openstreetmap.josm.data.Preferences} string for key {@code key},
    778779     * and defaults to {@code def} if that is null.
    779780     *

  • trunk/src/org/openstreetmap/josm/gui/preferences/server/AuthenticationPreferencesPanel.java

    r18662 r18665  
    118118    public final void initFromPreferences() {
    119119        final String authMethod = OsmApi.getAuthMethod();
    120         if ("basic".equals(authMethod)) {
    121             rbBasicAuthentication.setSelected(true);
    122         } else if ("oauth".equals(authMethod)) {
    123             rbOAuth.setSelected(true);
    124         } else if ("oauth20".equals(authMethod)) {
    125             rbOAuth20.setSelected(true);
    126         } else {
    127             Logging.warn(tr("Unsupported value in preference ''{0}'', got ''{1}''. Using authentication method ''Basic Authentication''.",
    128                     "osm-server.auth-method", authMethod));
    129             rbBasicAuthentication.setSelected(true);
     120        switch (authMethod) {
     121            case "basic":
     122                rbBasicAuthentication.setSelected(true);
     123                break;
     124            case "oauth":
     125                rbOAuth.setSelected(true);
     126                break;
     127            case "oauth20":
     128                rbOAuth20.setSelected(true);
     129                break;
     130            default:
     131                Logging.warn(tr("Unsupported value in preference ''{0}'', got ''{1}''. Using authentication method ''Basic Authentication''.",
     132                        "osm-server.auth-method", authMethod));
     133                rbBasicAuthentication.setSelected(true);
    130134        }
    131135        pnlBasicAuthPreferences.initFromPreferences();

  • trunk/src/org/openstreetmap/josm/gui/preferences/server/OAuthAuthenticationPreferencesPanel.java

    r18650 r18665  
    374374                    // Clean up old token/password
    375375                    OAuthAccessTokenHolder.getInstance().setAccessToken(null);
    376                     OAuthAccessTokenHolder.getInstance().setAccessToken(OsmApi.getOsmApi().getServerUrl(), token);
     376                    OAuthAccessTokenHolder.getInstance().setAccessToken(OsmApi.getOsmApi().getServerUrl(), token.orElse(null));
    377377                    OAuthAccessTokenHolder.getInstance().save(CredentialsManager.getInstance());
    378378                    GuiHelper.runInEDT(OAuthAuthenticationPreferencesPanel.this::refreshView);

  • trunk/src/org/openstreetmap/josm/io/OsmConnection.java

    r18651 r18665  
    1111import java.util.Base64;
    1212import java.util.Objects;
     13import java.util.Optional;
    1314import java.util.concurrent.CountDownLatch;
    1415import java.util.concurrent.TimeUnit;
    … …  
    215216        }
    216217        CountDownLatch done = new CountDownLatch(1);
    217         Consumer<IOAuthToken> consumer = authToken -> {
     218        Consumer<Optional<IOAuthToken>> consumer = authToken -> {
    218219                    if (!remoteControlIsRunning) {
    219220                        RemoteControl.stop();
    … …  
    221222                    // Clean up old token/password
    222223                    OAuthAccessTokenHolder.getInstance().setAccessToken(null);
    223                     OAuthAccessTokenHolder.getInstance().setAccessToken(OsmApi.getOsmApi().getServerUrl(), authToken);
     224                    OAuthAccessTokenHolder.getInstance().setAccessToken(OsmApi.getOsmApi().getServerUrl(), authToken.orElse(null));
    224225                    OAuthAccessTokenHolder.getInstance().save(CredentialsManager.getInstance());
    225226                    done.countDown();
    … …  
    229230                OsmScopes.read_prefs, OsmScopes.write_prefs,
    230231                OsmScopes.write_api, OsmScopes.write_notes);
    231         synchronized (done) {
    232             // Only wait at most 5 minutes
    233             int counter = 0;
    234             while (done.getCount() >= 0 && counter < 5) {
    235                 try {
    236                     if (done.await(1, TimeUnit.MINUTES)) {
    237                         break;
    238                     }
    239                 } catch (InterruptedException e) {
    240                     Thread.currentThread().interrupt();
    241                     Logging.trace(e);
    242                     consumer.accept(null);
    243                     throw new MissingOAuthAccessTokenException(e);
     232        // Only wait at most 5 minutes
     233        int counter = 0;
     234        while (done.getCount() >= 0 && counter < 5) {
     235            try {
     236                if (done.await(1, TimeUnit.MINUTES)) {
     237                    break;
    244238                }
    245                 counter++;
    246             }
     239            } catch (InterruptedException e) {
     240                Thread.currentThread().interrupt();
     241                Logging.trace(e);
     242                consumer.accept(null);
     243                throw new MissingOAuthAccessTokenException(e);
     244            }
     245            counter++;
    247246        }
    248247    }

  • trunk/test/unit/org/openstreetmap/josm/data/oauth/OAuth20AuthorizationTest.java

    r18650 r18665  
    33
    44import static org.junit.jupiter.api.Assertions.assertEquals;
     5import static org.junit.jupiter.api.Assertions.assertFalse;
    56import static org.junit.jupiter.api.Assertions.assertNotNull;
    6 import static org.junit.jupiter.api.Assertions.assertNull;
    77import static org.junit.jupiter.api.Assertions.assertTrue;
    88
    … …  
    1010import java.util.HashMap;
    1111import java.util.Map;
     12import java.util.Optional;
    1213import java.util.concurrent.atomic.AtomicReference;
    1314import java.util.stream.Collectors;
    … …  
    166167    void testAuthorize(WireMockRuntimeInfo wireMockRuntimeInfo) throws IOException {
    167168        final OAuth20Authorization authorization = new OAuth20Authorization();
    168         final AtomicReference<IOAuthToken> consumer = new AtomicReference<>();
     169        final AtomicReference<Optional<IOAuthToken>> consumer = new AtomicReference<>();
    169170        OAuth20Parameters parameters = (OAuth20Parameters) OAuthParameters.createDefault(OsmApi.getOsmApi().getBaseUrl(), OAuthVersion.OAuth20);
    170171        RemoteControl.start();
    … …  
    181182        }
    182183        assertNotNull(consumer.get());
    183         assertEquals(OAuthVersion.OAuth20, consumer.get().getOAuthType());
    184         OAuth20Token token = (OAuth20Token) consumer.get();
     184        assertTrue(consumer.get().isPresent());
     185        assertEquals(OAuthVersion.OAuth20, consumer.get().get().getOAuthType());
     186        OAuth20Token token = (OAuth20Token) consumer.get().get();
    185187        assertEquals("test_access_token", token.getBearerToken());
    186188    }
    … …  
    190192        oauthServer.stateToReturn = "Bad_State";
    191193        final OAuth20Authorization authorization = new OAuth20Authorization();
    192         final AtomicReference<IOAuthToken> consumer = new AtomicReference<>();
     194        final AtomicReference<Optional<IOAuthToken>> consumer = new AtomicReference<>();
    193195        OAuth20Parameters parameters = (OAuth20Parameters) OAuthParameters.createDefault(OsmApi.getOsmApi().getBaseUrl(), OAuthVersion.OAuth20);
    194196        RemoteControl.start();
    … …  
    206208            client.disconnect();
    207209        }
    208         assertNull(consumer.get());
     210        assertFalse(consumer.get().isPresent());
    209211    }
    210212}
Note: See TracChangeset for help on using the changeset viewer.