Skip to content
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
Original file line number Diff line number Diff line change
@@ -0,0 +1,75 @@
/*******************************************************************************
* Copyright 2025 Espressif Systems (Shanghai) PTE LTD. All rights reserved.
* Use is subject to license terms.
*******************************************************************************/
package com.espressif.idf.core.logging;

import java.io.BufferedWriter;
import java.io.File;
import java.io.FileWriter;
import java.io.IOException;
import java.io.PrintWriter;
import java.io.Writer;
import java.time.LocalDateTime;
import java.util.Map;
import java.util.concurrent.ConcurrentHashMap;

public class LogFileWriterManager
{
private static final Map<String, PrintWriter> writers = new ConcurrentHashMap<>();

private LogFileWriterManager()
{
}

public static PrintWriter getWriter(String path, boolean append)
{
if (path == null || path.isEmpty())
{
return new PrintWriter(Writer.nullWriter());
}

return writers.computeIfAbsent(path, p -> {
try
{
File file = new File(p);
File parent = file.getParentFile();
if (parent != null && !parent.exists())
{
parent.mkdirs();
Copy link

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Potential security issue: Directory creation without validation.

The mkdirs() call could create directories in unintended locations if the path is malicious or contains path traversal sequences (e.g., ../../../etc/passwd).

Add path validation before creating directories:

if (parent != null && !parent.exists())
{
+   // Validate parent path to prevent directory traversal attacks
+   String canonicalParent = parent.getCanonicalPath();
+   String expectedParent = parent.getAbsolutePath();
+   if (!canonicalParent.equals(expectedParent))
+   {
+       throw new IOException("Invalid path detected: " + path);
+   }
    parent.mkdirs();
}

Committable suggestion skipped: line range outside the PR's diff.

🤖 Prompt for AI Agents
In
bundles/com.espressif.idf.core/src/com/espressif/idf/core/logging/LogFileWriterManager.java
at line 39, the call to parent.mkdirs() creates directories without validating
the path, which can lead to security risks like path traversal. Before calling
mkdirs(), add validation to ensure the directory path is within an allowed base
directory and does not contain any traversal sequences or invalid characters.
Reject or sanitize paths that do not meet these criteria to prevent creating
directories in unintended locations.

}
if (!file.exists())
{
file.createNewFile();
}
return new PrintWriter(new BufferedWriter(new FileWriter(file, append)), true);
}
catch (IOException e)
{
Logger.log(e);
return new PrintWriter(Writer.nullWriter());
}
});
}
Comment on lines +25 to +53
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue

Fix append mode handling for existing writers.

The current implementation ignores the append parameter for existing writers. If a writer already exists for a path, the append mode from the first call is used for all subsequent calls, which could lead to unexpected behavior.

Consider this scenario:

  1. First call: getWriter("file.log", false) - creates writer in overwrite mode
  2. Second call: getWriter("file.log", true) - returns same writer, still in overwrite mode

Apply this refactor to include append mode in the key:

-return writers.computeIfAbsent(path, p -> {
+String key = path + ":" + append;
+return writers.computeIfAbsent(key, k -> {

Also update the closeWriter method accordingly:

-public static void closeWriter(String path)
+public static void closeWriter(String path, boolean append)
{
-    if (path == null || path.isEmpty())
-        return;
-    PrintWriter writer = writers.remove(path);
+    if (path == null || path.isEmpty())
+        return;
+    String key = path + ":" + append;
+    PrintWriter writer = writers.remove(key);

Committable suggestion skipped: line range outside the PR's diff.

🤖 Prompt for AI Agents
In
bundles/com.espressif.idf.core/src/com/espressif/idf/core/logging/LogFileWriterManager.java
between lines 25 and 53, the getWriter method currently uses only the file path
as the key in the writers map, ignoring the append parameter for existing
writers. To fix this, modify the key to combine both the path and append mode
(e.g., a composite key or a string concatenation) so that writers with different
append modes are stored separately. Also update the closeWriter method to accept
and use this new key format to correctly close the intended writer.

⚠️ Potential issue

Critical issue: Append parameter is ignored for existing writers.

The computeIfAbsent method only creates a new writer if the path doesn't exist in the map. However, the append parameter is only considered during writer creation, not when retrieving an existing writer. This means:

  1. If a writer is requested with append=false, it's created and cached
  2. Subsequent requests with append=true for the same path will return the same writer (which was created with append=false)

This could lead to unexpected behavior where the append mode doesn't match the caller's intention.

Consider this approach to ensure append behavior is consistent:

-public static PrintWriter getWriter(String path, boolean append)
+public static PrintWriter getWriter(String path, boolean append)
{
    if (path == null || path.isEmpty())
    {
        return new PrintWriter(Writer.nullWriter());
    }

-   return writers.computeIfAbsent(path, p -> {
+   // Create a composite key that includes append mode
+   String key = path + "|append=" + append;
+   return writers.computeIfAbsent(key, k -> {
        try
        {
-           File file = new File(p);
+           File file = new File(path);
            File parent = file.getParentFile();
            if (parent != null && !parent.exists())
            {
                parent.mkdirs();
            }
            if (!file.exists())
            {
                file.createNewFile();
            }
            return new PrintWriter(new BufferedWriter(new FileWriter(file, append)), true);
        }
        catch (IOException e)
        {
            Logger.log(e);
            return new PrintWriter(Writer.nullWriter());
        }
    });
}

And update the closeWriter method accordingly:

public static void closeWriter(String path)
{
    if (path == null || path.isEmpty())
        return;
-   PrintWriter writer = writers.remove(path);
+   // Remove both append modes for the path
+   PrintWriter writer1 = writers.remove(path + "|append=true");
+   PrintWriter writer2 = writers.remove(path + "|append=false");
+   PrintWriter writer = writer1 != null ? writer1 : writer2;
    if (writer != null)
    {
        writer.println("=== Session ended at " + LocalDateTime.now() + " ===");
        writer.close();
    }
}

Committable suggestion skipped: line range outside the PR's diff.

🤖 Prompt for AI Agents
In
bundles/com.espressif.idf.core/src/com/espressif/idf/core/logging/LogFileWriterManager.java
between lines 25 and 53, the getWriter method ignores the append parameter for
existing writers because it caches writers by path only. To fix this, modify the
caching key to include both path and append mode (e.g., use a composite key or a
map keyed by path and append flag) so that writers with different append modes
are stored separately. Update the closeWriter method to remove the writer from
the cache using the same composite key to ensure proper cleanup.


public static void closeWriter(String path)
{
if (path == null || path.isEmpty())
return;
PrintWriter writer = writers.remove(path);
if (writer != null)
{
writer.println("=== Session ended at " + LocalDateTime.now() + " ==="); //$NON-NLS-1$ //$NON-NLS-2$
writer.close();
}
}

public static void closeAll()
{
for (PrintWriter writer : writers.values())
{
writer.close();
}
writers.clear();
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -10,26 +10,28 @@
import java.util.Map;

import org.eclipse.cdt.dsf.gdb.launching.GDBProcess;
import org.eclipse.core.runtime.CoreException;
import org.eclipse.debug.core.DebugException;
import org.eclipse.debug.core.DebugPlugin;
import org.eclipse.debug.core.ILaunch;
import org.eclipse.debug.core.model.IStreamsProxy;
import org.eclipse.debug.core.model.RuntimeProcess;
import org.eclipse.debug.internal.core.NullStreamsProxy;
import org.eclipse.debug.ui.IDebugUIConstants;

import com.espressif.idf.core.util.StringUtil;
import com.espressif.idf.debug.gdbjtag.openocd.dsf.process.monitors.StreamsProxy;

/**
* Customised process class for the
* Idf based processes that will require a
* custom console based on the settings provided in the espressif configurations
* Customised process class for the Idf based processes that will require a custom console based on the settings
* provided in the espressif configurations
*
* @author Ali Azam Rana
*
*/
@SuppressWarnings("restriction")
public class IdfRuntimeProcess extends GDBProcess
{
private boolean fCaptureOutput = true;
private StreamsProxy streamsProxy;

public IdfRuntimeProcess(ILaunch launch, Process process, String name, Map<String, String> attributes)
{
Expand Down Expand Up @@ -60,7 +62,39 @@ protected IStreamsProxy createStreamsProxy()
DebugPlugin.log(e);
}
}
StreamsProxy streamsProxy = new StreamsProxy(this, getSystemProcess(), charset, getLabel());
// Use Eclipse Common tab attribute for output file and append
final String outputFileName = getAttributeSafe(getLaunch().getLaunchConfiguration()::getAttribute,
IDebugUIConstants.ATTR_CAPTURE_IN_FILE, "");

final boolean append = getAttributeSafe(getLaunch().getLaunchConfiguration()::getAttribute,
IDebugUIConstants.ATTR_APPEND_TO_FILE, false);
streamsProxy = new StreamsProxy(this, getSystemProcess(), charset, getLabel(), outputFileName, append);
return streamsProxy;
}

@Override
public void terminate() throws DebugException
{
super.terminate();
streamsProxy.kill();
}

private <T> T getAttributeSafe(AttributeGetter<T> getter, String attribute, T defaultValue)
{
try
{
return getter.get(attribute, defaultValue);
}
catch (CoreException e)
{
DebugPlugin.log(e);
return defaultValue;
}
}

@FunctionalInterface
interface AttributeGetter<T>
{
T get(String attribute, T defaultValue) throws CoreException;
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -4,13 +4,13 @@
*******************************************************************************/
package com.espressif.idf.debug.gdbjtag.openocd.dsf.process;

import java.io.BufferedReader;
import java.io.File;
import java.io.IOException;
import java.io.StringReader;
import java.io.PrintWriter;
import java.nio.charset.Charset;
import java.util.List;

import org.eclipse.core.variables.VariablesPlugin;
import org.eclipse.debug.core.IStreamListener;
import org.eclipse.debug.core.model.IFlushableStreamMonitor;
import org.eclipse.debug.core.model.IProcess;
Expand All @@ -19,6 +19,7 @@
import org.eclipse.ui.console.IOConsoleOutputStream;

import com.espressif.idf.core.build.ReHintPair;
import com.espressif.idf.core.logging.LogFileWriterManager;
import com.espressif.idf.core.logging.Logger;
import com.espressif.idf.core.util.HintsUtil;
import com.espressif.idf.core.util.StringUtil;
Expand All @@ -34,39 +35,46 @@
*
* @author Ali Azam Rana
*/
@SuppressWarnings("restriction")
public class StreamListener implements IStreamListener
{
private static final String OPENOCD_FAQ_LINK = "https://github.com/espressif/openocd-esp32/wiki/Troubleshooting-FAQ"; //$NON-NLS-1$

private IOConsoleOutputStream fConsoleErrorOutputStream;
private IOConsoleOutputStream fConsoleOutputStream;

private IStreamMonitor fErrorStreamMonitor;
private IStreamMonitor fOutputStreamMonitor;

private IdfProcessConsole idfProcessConsole;

/** Flag to remember if stream was already closed. */
private boolean fStreamClosed = false;
private List<ReHintPair> reHintsList;

private final PrintWriter fileWriter;
private final String resolvedOutputFilePath;

public StreamListener(IProcess iProcess, IStreamMonitor errorStreamMonitor, IStreamMonitor outputStreamMonitor,
Charset charset)
Charset charset, String outputFile, boolean append)
{
fErrorStreamMonitor = errorStreamMonitor;
fOutputStreamMonitor = outputStreamMonitor;
this.fErrorStreamMonitor = errorStreamMonitor;
this.fOutputStreamMonitor = outputStreamMonitor;

this.resolvedOutputFilePath = resolveOutputFilePath(outputFile);

this.idfProcessConsole = IdfProcessConsoleFactory.showAndActivateConsole(charset);

idfProcessConsole = IdfProcessConsoleFactory.showAndActivateConsole(charset);
// Clear the console only at the beginning, when OpenOCD starts
if (iProcess.getLabel().contains("openocd"))
if (iProcess.getLabel().contains("openocd")) //$NON-NLS-1$
{
idfProcessConsole.clearConsole();
}
reHintsList = HintsUtil.getReHintsList(new File(HintsUtil.getOpenocdHintsYmlPath()));
fConsoleErrorOutputStream = idfProcessConsole.getErrorStream();
fConsoleErrorOutputStream.setActivateOnWrite(true);
fConsoleOutputStream = idfProcessConsole.getOutputStream();
fConsoleOutputStream.setActivateOnWrite(true);

this.fileWriter = LogFileWriterManager.getWriter(resolvedOutputFilePath, append);
this.reHintsList = HintsUtil.getReHintsList(new File(HintsUtil.getOpenocdHintsYmlPath()));

this.fConsoleErrorOutputStream = idfProcessConsole.getErrorStream();
this.fConsoleErrorOutputStream.setActivateOnWrite(true);
this.fConsoleOutputStream = idfProcessConsole.getOutputStream();
this.fConsoleOutputStream.setActivateOnWrite(true);

flushAndDisableBuffer();
}
Expand All @@ -82,9 +90,8 @@ private void flushAndDisableBuffer()
synchronized (fErrorStreamMonitor)
{
contents = fErrorStreamMonitor.getContents();
if (fErrorStreamMonitor instanceof IFlushableStreamMonitor)
if (fErrorStreamMonitor instanceof IFlushableStreamMonitor m)
{
IFlushableStreamMonitor m = (IFlushableStreamMonitor) fErrorStreamMonitor;
m.flushContents();
m.setBuffered(false);
}
Expand All @@ -94,9 +101,8 @@ private void flushAndDisableBuffer()
synchronized (fOutputStreamMonitor)
{
contents = fOutputStreamMonitor.getContents();
if (fOutputStreamMonitor instanceof IFlushableStreamMonitor)
if (fOutputStreamMonitor instanceof IFlushableStreamMonitor m)
{
IFlushableStreamMonitor m = (IFlushableStreamMonitor) fOutputStreamMonitor;
m.flushContents();
m.setBuffered(false);
}
Expand All @@ -107,29 +113,24 @@ private void flushAndDisableBuffer()
@Override
public void streamAppended(String text, IStreamMonitor monitor)
{
String line;
try (BufferedReader bufferedReader = new BufferedReader(new StringReader(text)))
{
while ((line = bufferedReader.readLine()) != null)
{
text.lines().forEach(line -> {
fileWriter.println(line);

try
{
if (line.startsWith("Error:") && fConsoleErrorOutputStream != null) //$NON-NLS-1$
{
fConsoleErrorOutputStream.write((line + System.lineSeparator()).getBytes());
fConsoleErrorOutputStream.flush();

boolean[] hintMatched = { false };

final String processedLine = line;
reHintsList.stream()
.filter(reHintEntry -> reHintEntry.getRe()
.map(pattern -> pattern.matcher(processedLine).find()
|| processedLine.contains(pattern.toString()))
.map(pattern -> pattern.matcher(line).find() || line.contains(pattern.toString()))
.orElse(false))
.forEach(matchedReHintEntry -> {
try
{
// ANSI escape code for cyan text
hintMatched[0] = true;
String cyanHint = "\u001B[36mHint: " + matchedReHintEntry.getHint() + "\u001B[0m"; //$NON-NLS-1$ //$NON-NLS-2$

Expand All @@ -148,19 +149,18 @@ public void streamAppended(String text, IStreamMonitor monitor)
fConsoleOutputStream.write((Messages.OpenOCDConsole_ErrorGuideMessage + System.lineSeparator()
+ OPENOCD_FAQ_LINK + System.lineSeparator()).getBytes());
}

}
else if (fConsoleOutputStream != null)
{
fConsoleOutputStream.write((line + System.lineSeparator()).getBytes());
fConsoleOutputStream.flush();
}
}
}
catch (IOException e)
{
Logger.log(e);
}
catch (IOException e)
{
Logger.log(e);
}
});
}

public void closeStreams()
Expand All @@ -169,12 +169,10 @@ public void closeStreams()
{
fErrorStreamMonitor.removeListener(this);
}

synchronized (fOutputStreamMonitor)
{
fOutputStreamMonitor.removeListener(this);
}

fStreamClosed = true;
}

Expand All @@ -184,7 +182,50 @@ public void dispose()
{
closeStreams();
}
if (resolvedOutputFilePath != null)
{
LogFileWriterManager.closeWriter(resolvedOutputFilePath);
}
fErrorStreamMonitor = null;
fOutputStreamMonitor = null;
}
}

/**
* Resolves the output file path, expands variables, handles directory case, and ensures file/parent directories
* exist.
*/
private static String resolveOutputFilePath(String outputFile)
{
if (outputFile == null || outputFile.isEmpty())
{
return null;
}
try
{
// Expand Eclipse variables (e.g., ${workspace_loc:...})
String expanded = VariablesPlugin.getDefault().getStringVariableManager()
.performStringSubstitution(outputFile);
File file = new File(expanded);
if (file.isDirectory() || (!file.exists() && expanded.endsWith(File.separator)))
{
// If it's a directory or ends with a separator, append openocd.log
file = new File(file, "openocd.log"); //$NON-NLS-1$
}
File parent = file.getParentFile();
if (parent != null && !parent.exists())
{
parent.mkdirs();
}
if (!file.exists())
{
file.createNewFile();
}
return file.getAbsolutePath();
}
catch (Exception e)
{
Logger.log(e);
return null;
}
}
}
Loading
Loading