Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix for https://github.com/vorburger/MariaDB4j/issues/233 #96

Merged
merged 2 commits into from
Apr 17, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions src/main/java/ch/vorburger/exec/ManagedProcess.java
Original file line number Diff line number Diff line change
Expand Up @@ -67,7 +67,7 @@ public class ManagedProcess implements ManagedProcessState {

private final CommandLine commandLine;
private final Executor executor = new DefaultExecutor();
private final ExecuteWatchdog watchDog = new ExecuteWatchdog(ExecuteWatchdog.INFINITE_TIMEOUT);
private final StopCheckExecuteWatchdog watchDog = new StopCheckExecuteWatchdog(ExecuteWatchdog.INFINITE_TIMEOUT);
private final ProcessDestroyer shutdownHookProcessDestroyer = new LoggingShutdownHookProcessDestroyer();
private final Map<String, String> environment;
private final CompositeExecuteResultHandler resultHandler;
Expand Down Expand Up @@ -473,7 +473,7 @@ public ManagedProcess waitForExitMaxMsOrDestroy(long maxWaitUntilDestroyTimeout)
}

protected void assertWaitForIsValid() throws ManagedProcessException {
if (!isAlive() && !resultHandler.hasResult()) {
if (!watchDog.isStopped() && !isAlive() && !resultHandler.hasResult()) {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

6% of developers fix this issue

THREAD_SAFETY_VIOLATION: Read/Write race. Non-private method ManagedProcess.assertWaitForIsValid() indirectly reads without synchronization from this.isAlive. Potentially races with write in method ManagedProcess.startExecute().
Reporting because another access to the same memory occurs on a background thread, although this access may not.


ℹ️ Expand to see all @sonatype-lift commands

You can reply with the following commands. For example, reply with @sonatype-lift ignoreall to leave out all findings.

Command Usage
@sonatype-lift ignore Leave out the above finding from this PR
@sonatype-lift ignoreall Leave out all the existing findings from this PR
@sonatype-lift exclude <file|issue|path|tool> Exclude specified file|issue|path|tool from Lift findings by updating your config.toml file

Note: When talking to LiftBot, you need to refresh the page to see its response.
Click here to add LiftBot to another repo.


Help us improve LIFT! (Sonatype LiftBot external survey)

Was this a good recommendation for you? Answering this survey will not impact your Lift settings.

[ 🙁 Not relevant ] - [ 😕 Won't fix ] - [ 😑 Not critical, will fix ] - [ 🙂 Critical, will fix ] - [ 😊 Critical, fixing now ]

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#99

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@vorburger it doesn't seem like this patch introduced this thread safety violation. Can we merge in this patch? It would be a useful improvement even if it preserved the thread safety issue.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mosesn thank you very much for having reviewed this; I agree - so (finally, sorry) MERGE this now!

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you!!

throw new ManagedProcessException("Asked to waitFor " + getProcLongName()
+ ", but it was never even start()'ed!");
}
Expand Down
51 changes: 51 additions & 0 deletions src/main/java/ch/vorburger/exec/StopCheckExecuteWatchdog.java
Original file line number Diff line number Diff line change
@@ -0,0 +1,51 @@
/*
* #%L
* ch.vorburger.exec
* %%
* Copyright (C) 2012 - 2018 Michael Vorburger
* %%
* Licensed 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.
* #L%
*/

package ch.vorburger.exec;
cardamon marked this conversation as resolved.
Show resolved Hide resolved

import org.apache.commons.exec.ExecuteWatchdog;

public class StopCheckExecuteWatchdog extends ExecuteWatchdog {
private volatile boolean stopped = false;

/**
* Creates a new watchdog with a given timeout.
*
* @param timeout
* the timeout for the process in milliseconds. It must be
* greater than 0 or 'INFINITE_TIMEOUT'
*/
public StopCheckExecuteWatchdog(long timeout) {
super(timeout);
}

/**
* {@inheritDoc}
*/
@Override
public synchronized void stop() {
super.stop();
stopped = true;
}

public boolean isStopped() {
return stopped;
}
}