feat: support ssl data event assemble#994
Conversation
dosubot
Bot
added
size:L
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
There was a problem hiding this comment.
🔴 TextHandler.Close() calls writer.Close() twice, causing double-close
The Close() method unconditionally calls h.writer.Close() on line 92, and if that succeeds (no error), it checks h.writer != nil on line 96 (always true since NewTextHandler ensures non-nil) and calls h.writer.Close() a second time on line 97. This double-close can cause errors or panics depending on the writer implementation. This is a pre-existing bug but is now amplified by the PR since GoTlsPayloadHandler also closes the same shared writer before TextHandler.Close() runs (see BUG-0002), resulting in potentially three closes of the same writer during shutdown.
(Refers to lines 91-99)
Was this helpful? React with 👍 or 👎 to provide feedback.
| func (h *GoTlsPayloadHandler) Close() error { | ||
| if closer, ok := h.textOut.(io.Closer); ok { | ||
| return closer.Close() | ||
| } | ||
| return nil |
There was a problem hiding this comment.
🔴 GoTlsPayloadHandler.Close() closes shared textWriter it does not own
In gotls_probe.go:93, the GoTlsPayloadHandler receives p.TextWriter() — the same writer instance that the TextHandler (registered in base_probe.go:116) also holds. During shutdown, Probe.Close() at gotls_probe.go:214 closes GoTlsPayloadHandler first, which calls h.textOut.(io.Closer).Close() at line 49, closing the shared writer. Then BaseProbe.Close() closes TextHandler, which tries to close the same already-closed writer again. The handler should not close a writer it doesn't own.
Shutdown sequence showing the issue
Probe.Close()iteratesp.closer→ callsGoTlsPayloadHandler.Close()→ closes shared textWriterBaseProbe.Close()iteratesp.closers→ callsTextHandler.Close()→ attempts to close already-closed textWriter
| func (h *GoTlsPayloadHandler) Close() error { | |
| if closer, ok := h.textOut.(io.Closer); ok { | |
| return closer.Close() | |
| } | |
| return nil | |
| func (h *GoTlsPayloadHandler) Close() error { | |
| return nil | |
| } |
Was this helpful? React with 👍 or 👎 to provide feedback.
| return err | ||
| } | ||
| return ew.writeToChan(encodedData) | ||
| ew.processor.protoEventCh <- ep |
There was a problem hiding this comment.
🔴 Blocking channel send in eventWorker.Display() can hang event processing
The direct channel send ew.processor.protoEventCh <- ep at line 214 blocks if the channel (128-buffer from ecaptureq/server.go:144) is full. The old code used writeToChan() which has a select/default non-blocking pattern that drops data when the channel is full. The new code will block the eventWorker goroutine indefinitely if the downstream WebSocket broadcast is slow, which can stall the entire event processing pipeline for that connection. This is a regression from non-blocking to blocking behavior.
| ew.processor.protoEventCh <- ep | |
| select { | |
| case ew.processor.protoEventCh <- ep: | |
| default: | |
| } |
Was this helpful? React with 👍 or 👎 to provide feedback.
cfc4n
left a comment
cfc4n
left a comment
There was a problem hiding this comment.
PR改动有点大，辛苦看下这些问题。 核心问题就是“Protobuf”编码格式的支持，能不能再抽象一些。
| Listen string `json:"listen"` | ||
| AddrType uint8 `json:"addr_type"` | ||
| EventWriter io.Writer `json:"-"` | ||
| ProtoChannel chan<- *pb.Event `json:"-"` |
There was a problem hiding this comment.
我觉得这里抽象的不太优雅。 是不是可以再抽象一下 Serializer 层， 这里改为EventChannel chan <- Serializer ？ 这样，跟proto也解耦，也兼容Json 等其他序列化的实现。
type Serializer interface {
Marshal(v interface{}) ([]byte, error)
Unmarshal(data []byte, v interface{}) error
}| } | ||
|
|
||
| // SetProtoChannel sets the ecaptureQ proto event channel. | ||
| func (c *BaseConfig) SetProtoChannel(ch chan<- *pb.Event) { |
| } | ||
|
|
||
| name := fmt.Sprintf("%s-%s", handler.Name(), handler.Writer().Name()) | ||
| name := handler.Name() |
There was a problem hiding this comment.
这里好像不能直接这么改， 会出现相同Handler名被覆盖的情况。 比如，同一个“TextHandler"会有多个不同的Writer。
There was a problem hiding this comment.
为啥要单独为 gotls模块实现一个handler？ 如果真有必要，是不是应该放到probe/gotls/ 目录下？
There was a problem hiding this comment.
这个文件我也没理解，思路是跟gotls_payload_handler.go一样吗？
- Introduce `IsCustomHandler()` to `Event` interface to route complex events (e.g., gotls, openssl) to dedicated payload handlers instead of the default `TextHandler`. - Replace generic `EventWriter` with a dedicated `ProtoChannel` (`chan<- *pb.Event`) for more efficient, direct ecaptureQ structured protobuf delivery. - Remove `OutputWriter` dependency from `EventHandler` to decouple dispatcher registration logic. - Register specific payload handlers (`GoTlsPayloadHandler`, `OpensslPayloadHandler`) for TLS probes to handle payload assembly natively.
Replace the io.Writer-based output pipeline with a composable Encoder abstraction. Each Encoder handles one format/target pair (text, proto, keylog, pcap). Handlers become thin shells that route events through a chain of Encoders.
Convert three standalone handlers to the Encoder pattern, removing the io.Writer / writers.OutputWriter dependency from all handlers.
zenyanle
force-pushed
the
feat/support_ssl_data_assemble
branch
from
f818be4 to
43fccaf
Compare
dosubot
Bot
added
size:XXL
2 participants
Replace the io.Writer-based event output pipeline with a composable Encoder abstraction. Each Encoder handles one format/target pair (text, proto, keylog, pcap). Handlers become thin shells that route events through a chain of Encoders. ecaptureQ is injected as a chan<- *pb.Event rather than disguised as an io.Writer.