Skip to content

Fix Int cast flagged by copilot #416

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.

Sign up for GitHub

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

Merged
biglittlebigben merged 1 commit into from
Feb 11, 2026
Merged

Fix Int cast flagged by copilot #416

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
2 changes: 1 addition & 1 deletion pkg/ipc/ipc.pb.go
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 1 addition & 1 deletion pkg/ipc/ipc_grpc.pb.go
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

19 changes: 14 additions & 5 deletions pkg/service/debug.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,10 +21,11 @@ import (
"strconv"
"strings"

"github.com/livekit/ingress/pkg/errors"
"github.com/livekit/protocol/logger"
"github.com/livekit/protocol/pprof"
"github.com/livekit/psrpc"

"github.com/livekit/ingress/pkg/errors"
)

const (
Expand Down Expand Up @@ -80,14 +81,22 @@ func (s *Service) handlePProf(w http.ResponseWriter, r *http.Request) {
var err error
var b []byte

timeout, _ := strconv.Atoi(r.URL.Query().Get("timeout"))
debug, _ := strconv.Atoi(r.URL.Query().Get("debug"))
timeout, err := strconv.ParseInt(r.URL.Query().Get("timeout"), 10, 32)
Copy link
Copy Markdown
Contributor

@paulwe paulwe Feb 10, 2026

Choose a reason for hiding this comment

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

this doesn't really make sense. iirc valid values for debug are like 0, 1, or 2 and valid values for timeout are... some reasonable number of seconds. would it make more sense to bounds check? i'm not sure limiting the range to int32 is doing much for us

Copy link
Copy Markdown
Contributor

@paulwe paulwe Feb 10, 2026

Choose a reason for hiding this comment

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

also negative numbers make no sense since this is... an enum and a number of seconds so if we're trying to make the parser validate the input why parse it as signed?

Copy link
Copy Markdown
Contributor Author

@biglittlebigben biglittlebigben Feb 10, 2026 •
edited
Loading

Choose a reason for hiding this comment

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

My take is, the report doesn't make sense in the first place: If we parse an in64 > INT32_MAX, we'll end up passing an invalid integer to pprof that will either ignore it or fail. The pprof API takes negative integers (and supposedly fails when given that input).

This is meant to quiet down automated reports with the lowest amount of work while not impacting functionality, security or maintainability.

The automated report is a compliance blocker impacting sales.

Alternatively, we may be able to close the report by explaining why we believe this is not an issue, but we should expect scrutiny doing that. Would you rather we go that route?

Copy link
Copy Markdown
Contributor

@paulwe paulwe Feb 10, 2026

Choose a reason for hiding this comment

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

the intent of the flag makes sense - we're missing input validation. it's fair to argue that the library does validation on its own and so this is wasted effort. but if we were compelled to add it it seemed like we might as well actually validate the input. if the goal is just checking a box idc

Copy link
Copy Markdown
Contributor Author

@biglittlebigben biglittlebigben Feb 11, 2026

Choose a reason for hiding this comment

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

Valid values for "debug" depend on the profiler used. What does maintaining validation logic for tool name -> valid debug values get us? I can buy that adding an upper limit for the timeout may somewhat mitigate some DOS attempts if anybody ever got access to the pprof endpoint. However, we'd need to make the limit at least 5 min and that's plenty of time to create about as many goroutines as you want.

This validation would however likely make most sense in https://github.com/livekit/protocol/blob/2504a42c8d96f69d22e49462e9b130222a9143b9/pprof/pprof.go#L35. So we would still need to deal with the casting check.

I can make all parameters to all these functions int64 instead, but this feels about as ridiculous.

Copy link
Copy Markdown
Contributor

@paulwe paulwe Feb 11, 2026

Choose a reason for hiding this comment

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

if parsing as int32 silences the warning let's ship it...

if err != nil {
http.Error(w, "malformed timeout", http.StatusBadRequest)
return
}
debug, err := strconv.ParseInt(r.URL.Query().Get("debug"), 10, 32)
if err != nil {
http.Error(w, "malformed debug", http.StatusBadRequest)
return
}

pathElements := strings.Split(r.URL.Path, "/")
switch len(pathElements) {
case 3:
// profile main service
b, err = pprof.GetProfileData(context.Background(), pathElements[2], timeout, debug)
b, err = pprof.GetProfileData(context.Background(), pathElements[2], int(timeout), int(debug))

case 4:
resourceID := pathElements[2]
Expand All @@ -97,7 +106,7 @@ func (s *Service) handlePProf(w http.ResponseWriter, r *http.Request) {
return
}

b, err = api.GetProfileData(context.Background(), pathElements[3], timeout, debug)
b, err = api.GetProfileData(context.Background(), pathElements[3], int32(timeout), int32(debug))
if err != nil {
return
}
Expand Down
2 changes: 1 addition & 1 deletion pkg/service/process_manager.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -297,7 +297,7 @@ func (p *process) WHIPRTCConnectionNotify(ctx context.Context, req *rpc.WHIPRTCC
return &google_protobuf2.Empty{}, nil
}

func (p *process) GetProfileData(ctx context.Context, profileName string, timeout int, debug int) (b []byte, err error) {
func (p *process) GetProfileData(ctx context.Context, profileName string, timeout int32, debug int32) (b []byte, err error) {
grpcClient := p.grpcClient.Load()
if grpcClient == nil {
return nil, errors.ErrIngressNotFound
Expand Down
4 changes: 2 additions & 2 deletions pkg/service/service.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -562,8 +562,8 @@ type localSessionAPI struct {
sessionCloser
}

func (a *localSessionAPI) GetProfileData(ctx context.Context, profileName string, timeout int, debug int) (b []byte, err error) {
return pprof.GetProfileData(ctx, profileName, timeout, debug)
func (a *localSessionAPI) GetProfileData(ctx context.Context, profileName string, timeout int32, debug int32) (b []byte, err error) {
return pprof.GetProfileData(ctx, profileName, int(timeout), int(debug))
}

func (a *localSessionAPI) GetPipelineDot(ctx context.Context) (string, error) {
Expand Down
2 changes: 1 addition & 1 deletion pkg/types/session_api.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ type SessionAPI interface {
MediaStatsUpdater
MediaStatGatherer

GetProfileData(ctx context.Context, profileName string, timeout int, debug int) (b []byte, err error)
GetProfileData(ctx context.Context, profileName string, timeout int32, debug int32) (b []byte, err error)
GetPipelineDot(ctx context.Context) (string, error)
CloseSession(ctx context.Context)
}
Loading