Skip to content

Commit

Permalink
Fix accidental double-freeing of log entries and fix LogEntry API (de…
Browse files Browse the repository at this point in the history
…ephaven#4060)

This logging code was accidentally misusing the LogEntry API, calling endl() multiple times. This resulted in the same entry being put into the pool multiple time. Later on, the entry could be used by multiple threads logging at the same time. Most of the time this would result in garbage logs, but occasionally it would result in an NPE.

This also breaks LogEntry binary and (potentially) source compatibility to ensure users can't string together fluent end()/endl().

Note: it's still possible for callers to double-free

```java
le = log.info();
le.append("hello");
le.append("world");
le.endl();
le.endl();
```

but this is a much rarer pattern. Adding additional runtime safety checks would be possible, but would not be free.

Fixes deephaven#4051
  • Loading branch information
devinrsmith authored Jun 23, 2023
1 parent 452df84 commit feae9d0
Show file tree
Hide file tree
Showing 8 changed files with 27 additions and 27 deletions.
20 changes: 14 additions & 6 deletions IO/src/main/java/io/deephaven/io/log/LogEntry.java
Original file line number Diff line number Diff line change
Expand Up @@ -20,9 +20,17 @@ public interface LogEntry extends LogOutput, LogSink.Element {

LogEntry start(LogSink sink, LogLevel level, long currentTimeMicros, Throwable t);

LogEntry end();
/**
* Completes the log entry. Callers should not use {@code this} after completion. End or {@link #endl()} should be
* called exactly once.
*/
void end();

LogEntry endl();
/**
* Completes the log entry with a newline. Callers should not use {@code this} after completion. Endl or
* {@link #end()} should be called exactly once.
*/
void endl();

LogEntry append(boolean b);

Expand Down Expand Up @@ -213,13 +221,13 @@ public LogEntry start(LogSink sink, LogLevel level, long currentTimeMicros, Thro
}

@Override
public LogEntry end() {
return this;
public void end() {

}

@Override
public LogEntry endl() {
return this;
public void endl() {

}

@Override
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -117,18 +117,16 @@ public LogEntry start(final LogSink sink, final LogLevel level, final long curre
}

@Override
public LogEntry end() {
public void end() {
// noinspection unchecked
sink.write(this);
ends.getAndIncrement();
return this;
}

@Override
public LogEntry endl() {
public void endl() {
nl();
end();
return this;
}

@Override
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -144,17 +144,15 @@ public LogEntry start(final LogSink logSink, final LogLevel level, final long cu
}

@Override
public LogEntry end() {
public void end() {
// noinspection unchecked
logSink.write(this);
return this;
}

@Override
public LogEntry endl() {
public void endl() {
nl();
end();
return this;
}

@Override
Expand Down
6 changes: 2 additions & 4 deletions IO/src/main/java/io/deephaven/io/log/impl/LogEntryImpl.java
Original file line number Diff line number Diff line change
Expand Up @@ -72,18 +72,16 @@ public LogEntry start(final LogSink sink, final LogLevel level, final long curre
}

@Override
public LogEntry end() {
public void end() {
close();
sink.write(this);
ends.getAndIncrement();
return this;
}

@Override
public LogEntry endl() {
public void endl() {
nl();
end();
return this;
}

// ------------------------------------------------------------------------------------------------
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -83,15 +83,14 @@ public LogEntry start(LogSink sink, LogLevel level, long currentTimeMicros, Thro
}

@Override // from LogEntry
public LogEntry end() {
public void end() {
throw Assert.statementNeverExecuted();
}

@Override // from LogEntry
public LogEntry endl() {
public void endl() {
m_monitor.endl(builder.toString());
builder.setLength(0);
return this;
}

// ################################################################
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -66,14 +66,14 @@ public Optional<AuthContext> login(String payload, MetadataResponseListener list
@Override
public void initialize(String targetUrl) {
// Noisily log this, so the user can find the link to click easily
logger.warn().endl().endl().endl().endl().endl();
logger.warn().nl().nl().nl().nl().endl();
logger.warn().append("================================================================================").endl();
logger.warn().append("Superuser access through pre-shared key is enabled - use ").append(PSK)
.append(" to connect").endl();
logger.warn().append("Connect automatically to Web UI with ").append(targetUrl).append("/?psk=")
.append(PSK)
.endl();
logger.warn().append("================================================================================").endl();
logger.warn().endl().endl().endl().endl().endl();
logger.warn().nl().nl().nl().nl().endl();
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -27,14 +27,14 @@ public String getAuthType() {
@Override
public void initialize(String targetUrl) {
if (WARN_ANONYMOUS_ACCESS) {
log.warn().endl().endl().endl().endl().endl();
log.warn().nl().nl().nl().nl().endl();
log.warn().append("================================================================================")
.endl();
log.warn().append("WARNING! Anonymous authentication is enabled. This is not recommended!").endl();
log.warn().append(" Listening on ").append(targetUrl).endl();
log.warn().append("================================================================================")
.endl();
log.warn().endl().endl().endl().endl().endl();
log.warn().nl().nl().nl().nl().endl();
} else {
log.info().append("Anonymous authentication is enabled. Listening on ").append(targetUrl).endl();
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -92,9 +92,8 @@ public boolean isFatal() {
}

@Override
public LogEntry endl() {
public void endl() {
super.end();
return this;
}
}

Expand Down

0 comments on commit feae9d0

Please sign in to comment.