sqlite: fix nil deref on missing response #32
Conversation
pkg/db/sqlite/sqlite.go
Outdated
| if err != nil { | ||
| return fmt.Errorf("could not query request headers: %v", err) | ||
| } | ||
| reqLogs[i].Request.Header = headers | ||
| reqLog.Request.Header = headers |
There was a problem hiding this comment.
Manually tested this, and the retrieved request headers aren't included in the API response. I believe it's because reqLog.Request is a value and not a pointer, so the original reqLogs that is returned upstream won't be mutated. But because http.Header's underlying type is a map (which holds a reference), we could keep the for _, reqLog (I agree it's more idiomatic) and fill (copy) the found headers like so:
| reqLog.Request.Header = headers | |
| for key, value := range headers { | |
| reqLog.Request.Header[key] = value | |
| } |
Curious to hear your thoughts @holiman.
There was a problem hiding this comment.
Hm, maybe it would be better to change the method signature (and really, in general) to use pointers instead? So you'd use reqLogs []*reqlog.Request. I find that often makes it easier to reason about
There was a problem hiding this comment.
Your proposed variant uses the fact that the copied Request has copied the same pointer to a map, and thus you access the same underlying map. So mutating the original headers-map although the caller passed a copy. It's a bit of a hacky solution to the problem.
There was a problem hiding this comment.
This might be a bit personal/subjective, but I typically avoid using/passing around slices of pointers; don't really see the added benefit in this use case. Thinking about this some more IMHO it's actually not that bad to range over the slice and use the index to mutate the values of the slice, e.g. what we now do in master. I'd opt for this PR to be just this change.
There was a problem hiding this comment.
I've changed the PR to now be only that change. However, since the egress-object is by-value (only the ingress Response field is a reference), it means that none of the request headers become set. So this PR fixes the nil deref-panic, but doesn't fix the request headers.
There was a problem hiding this comment.
However, since the egress-object is by-value (only the ingress
Responsefield is a reference), it means that none of the request headers become set. So this PR fixes the nil deref-panic, but doesn't fix the request headers.
Are you sure about the request headers not being set? Just tested this manually, and the request headers are getting set. While indeed the Request field on the reqlog.Request struct is a value and not a pointer, because we're using the index when ranging (ref) and setting the headers via reqLogs[i].Request.Header (ref), we're actually mutating the items referenced by the slice.
There was a problem hiding this comment.
Are you sure about the request headers not being set? Just tested this manually,
You're right. I tested it too, but apparently I tested against the wrong binary.
|
Unless you change the api to take slices (which I think would be best), I could change my PR to look something like this .. ? That properly closes the statements before doing the next one, instead of deferring both to the end. func copyHeaders(dst, src http.Header) {
for k, v := range src {
dst[k] = v
}
}
func (c *Client) queryHeaders(
ctx context.Context,
query httpRequestLogsQuery,
reqLogs []reqlog.Request,
) error {
q := func(cols []string, response bool) error {
if len(cols) == 0 {
return nil
}
headersQuery, _, err := sq.
Select(cols...).
From("http_headers").Where("req_id = ?").
ToSql()
if err != nil {
return fmt.Errorf("could not parse query: %v", err)
}
stmt, err := c.db.PrepareContext(ctx, headersQuery)
if err != nil {
return fmt.Errorf("could not prepare statement: %v", err)
}
defer stmt.Close()
for _, reqLog := range reqLogs {
headers, err := findHeaders(ctx, stmt, reqLog.ID)
if err != nil {
return fmt.Errorf("could not query request headers: %v", err)
}
if !response {
copyHeaders(reqLog.Request.Header, headers)
} else {
copyHeaders(reqLog.Response.Response.Header, headers)
}
}
return nil
}
if err := q(query.requestHeaderCols, false); err != nil {
return err
}
if err := q(query.responseHeaderCols, true); err != nil {
return err
}
return nil
}EDIT: that^ code doesn't work, I made a separate PR instead to solve the root problem too |
holiman
force-pushed
the
fix_nil_deref
branch
from
5c4d57a
to
f975d1c
Compare
I tested it out a bit, and immediately hit this error:
It seems that if the response either is not saved (or perhaps not yet saved?), the
reqLogs[i].Responsewasnil, causing a crash.I also rewrote the loops to be more go-idiomatic.