From 9e8ba039f1c4086d689d6ca1af73f7dc687d2511 Mon Sep 17 00:00:00 2001 From: Dirkjan Bussink Date: Wed, 29 Jan 2025 15:15:33 +0100 Subject: [PATCH] Fix integer parsing logic We have various places where the types of what we parse and then use don't align. This is something CodeQL correctly warns about that it can lead to unexpected behavior with truncation. This fixes those cases that CodeQL is complaining about. Signed-off-by: Dirkjan Bussink --- go/vt/vtcombo/tablet_map.go | 2 +- go/vt/vtctl/grpcvtctldserver/server.go | 7 +++++ .../testutil/test_tmclient.go | 4 +-- .../reparentutil/emergency_reparenter.go | 8 +++--- .../reparentutil/emergency_reparenter_test.go | 28 +++++++++---------- go/vt/vttablet/common/config.go | 28 +++++++++---------- go/vt/vttablet/faketmclient/fake_client.go | 2 +- go/vt/vttablet/grpctmclient/client.go | 4 +-- go/vt/vttablet/grpctmserver/server.go | 2 +- go/vt/vttablet/tabletmanager/rpc_agent.go | 2 +- .../vttablet/tabletmanager/rpc_replication.go | 4 +-- go/vt/vttablet/tabletserver/twopc.go | 2 +- go/vt/vttablet/tmclient/rpc_client_api.go | 2 +- go/vt/vttablet/tmrpctest/test_tm_rpc.go | 4 +-- 14 files changed, 53 insertions(+), 46 deletions(-) diff --git a/go/vt/vtcombo/tablet_map.go b/go/vt/vtcombo/tablet_map.go index 64b49ae2bed..e8c8935f989 100644 --- a/go/vt/vtcombo/tablet_map.go +++ b/go/vt/vtcombo/tablet_map.go @@ -1005,7 +1005,7 @@ func (itmc *internalTabletManagerClient) PopulateReparentJournal(context.Context } // ReadReparentJournalInfo is part of the tmclient.TabletManagerClient interface. -func (itmc *internalTabletManagerClient) ReadReparentJournalInfo(ctx context.Context, tablet *topodatapb.Tablet) (int, error) { +func (itmc *internalTabletManagerClient) ReadReparentJournalInfo(ctx context.Context, tablet *topodatapb.Tablet) (int32, error) { return 0, fmt.Errorf("not implemented in vtcombo") } diff --git a/go/vt/vtctl/grpcvtctldserver/server.go b/go/vt/vtctl/grpcvtctldserver/server.go index baac17dea85..40ce7871902 100644 --- a/go/vt/vtctl/grpcvtctldserver/server.go +++ b/go/vt/vtctl/grpcvtctldserver/server.go @@ -22,6 +22,7 @@ import ( "errors" "fmt" "io" + "math" "net/http" "path/filepath" "runtime/debug" @@ -1509,11 +1510,17 @@ func (s *VtctldServer) GetBackups(ctx context.Context, req *vtctldatapb.GetBacku totalBackups := len(bhs) if req.Limit > 0 { + if req.Limit > math.MaxInt { + return nil, fmt.Errorf("limit %v exceeds maximum allowed value %v", req.DetailedLimit, math.MaxInt) + } totalBackups = int(req.Limit) } totalDetailedBackups := len(bhs) if req.DetailedLimit > 0 { + if req.DetailedLimit > math.MaxInt { + return nil, fmt.Errorf("detailed_limit %v exceeds maximum allowed value %v", req.DetailedLimit, math.MaxInt) + } totalDetailedBackups = int(req.DetailedLimit) } diff --git a/go/vt/vtctl/grpcvtctldserver/testutil/test_tmclient.go b/go/vt/vtctl/grpcvtctldserver/testutil/test_tmclient.go index 768fae5bff4..3b6af754d4e 100644 --- a/go/vt/vtctl/grpcvtctldserver/testutil/test_tmclient.go +++ b/go/vt/vtctl/grpcvtctldserver/testutil/test_tmclient.go @@ -295,7 +295,7 @@ type TabletManagerClient struct { // keyed by tablet alias PopulateReparentJournalResults map[string]error // keyed by tablet alias - ReadReparentJournalInfoResults map[string]int + ReadReparentJournalInfoResults map[string]int32 // keyed by tablet alias. PromoteReplicaDelays map[string]time.Duration // keyed by tablet alias. injects a sleep to the end of the function @@ -974,7 +974,7 @@ func (fake *TabletManagerClient) PopulateReparentJournal(ctx context.Context, ta } // ReadReparentJournalInfo is part of the tmclient.TabletManagerClient interface. -func (fake *TabletManagerClient) ReadReparentJournalInfo(ctx context.Context, tablet *topodatapb.Tablet) (int, error) { +func (fake *TabletManagerClient) ReadReparentJournalInfo(ctx context.Context, tablet *topodatapb.Tablet) (int32, error) { if fake.ReadReparentJournalInfoResults == nil { return 1, nil } diff --git a/go/vt/vtctl/reparentutil/emergency_reparenter.go b/go/vt/vtctl/reparentutil/emergency_reparenter.go index 5f7d3140c7b..513076b8f68 100644 --- a/go/vt/vtctl/reparentutil/emergency_reparenter.go +++ b/go/vt/vtctl/reparentutil/emergency_reparenter.go @@ -805,7 +805,7 @@ func (erp *EmergencyReparenter) findErrantGTIDs( } // Find the maximum length of the reparent journal among all the candidates. - var maxLen int + var maxLen int32 for _, length := range reparentJournalLen { maxLen = max(maxLen, length) } @@ -902,8 +902,8 @@ func (erp *EmergencyReparenter) gatherReparenJournalInfo( validCandidates map[string]replication.Position, tabletMap map[string]*topo.TabletInfo, waitReplicasTimeout time.Duration, -) (map[string]int, error) { - reparentJournalLen := make(map[string]int) +) (map[string]int32, error) { + reparentJournalLen := make(map[string]int32) var mu sync.Mutex errCh := make(chan concurrency.Error) defer close(errCh) @@ -916,7 +916,7 @@ func (erp *EmergencyReparenter) gatherReparenJournalInfo( for candidate := range validCandidates { go func(alias string) { var err error - var length int + var length int32 defer func() { errCh <- concurrency.Error{ Err: err, diff --git a/go/vt/vtctl/reparentutil/emergency_reparenter_test.go b/go/vt/vtctl/reparentutil/emergency_reparenter_test.go index 840df41d6e2..529c2a8f366 100644 --- a/go/vt/vtctl/reparentutil/emergency_reparenter_test.go +++ b/go/vt/vtctl/reparentutil/emergency_reparenter_test.go @@ -4637,7 +4637,7 @@ func TestEmergencyReparenterFindErrantGTIDs(t *testing.T) { { name: "Case 1a: No Errant GTIDs. This is the first reparent. A replica is the most advanced.", tmc: &testutil.TabletManagerClient{ - ReadReparentJournalInfoResults: map[string]int{ + ReadReparentJournalInfoResults: map[string]int32{ "zone1-0000000102": 1, "zone1-0000000103": 1, "zone1-0000000104": 1, @@ -4733,7 +4733,7 @@ func TestEmergencyReparenterFindErrantGTIDs(t *testing.T) { }, }, tmc: &testutil.TabletManagerClient{ - ReadReparentJournalInfoResults: map[string]int{ + ReadReparentJournalInfoResults: map[string]int32{ "zone1-0000000102": 2, "zone1-0000000103": 2, "zone1-0000000104": 2, @@ -4797,7 +4797,7 @@ func TestEmergencyReparenterFindErrantGTIDs(t *testing.T) { }, }, tmc: &testutil.TabletManagerClient{ - ReadReparentJournalInfoResults: map[string]int{ + ReadReparentJournalInfoResults: map[string]int32{ "zone1-0000000102": 2, "zone1-0000000103": 2, "zone1-0000000104": 2, @@ -4861,7 +4861,7 @@ func TestEmergencyReparenterFindErrantGTIDs(t *testing.T) { }, }, tmc: &testutil.TabletManagerClient{ - ReadReparentJournalInfoResults: map[string]int{ + ReadReparentJournalInfoResults: map[string]int32{ "zone1-0000000102": 3, "zone1-0000000103": 2, "zone1-0000000104": 1, @@ -4925,7 +4925,7 @@ func TestEmergencyReparenterFindErrantGTIDs(t *testing.T) { }, }, tmc: &testutil.TabletManagerClient{ - ReadReparentJournalInfoResults: map[string]int{ + ReadReparentJournalInfoResults: map[string]int32{ "zone1-0000000102": 2, "zone1-0000000103": 2, "zone1-0000000104": 1, @@ -4989,7 +4989,7 @@ func TestEmergencyReparenterFindErrantGTIDs(t *testing.T) { }, }, tmc: &testutil.TabletManagerClient{ - ReadReparentJournalInfoResults: map[string]int{ + ReadReparentJournalInfoResults: map[string]int32{ "zone1-0000000102": 2, "zone1-0000000103": 3, "zone1-0000000104": 3, @@ -5052,7 +5052,7 @@ func TestEmergencyReparenterFindErrantGTIDs(t *testing.T) { }, }, tmc: &testutil.TabletManagerClient{ - ReadReparentJournalInfoResults: map[string]int{ + ReadReparentJournalInfoResults: map[string]int32{ "zone1-0000000102": 2, "zone1-0000000103": 3, "zone1-0000000104": 2, @@ -5116,7 +5116,7 @@ func TestEmergencyReparenterFindErrantGTIDs(t *testing.T) { }, }, tmc: &testutil.TabletManagerClient{ - ReadReparentJournalInfoResults: map[string]int{ + ReadReparentJournalInfoResults: map[string]int32{ "zone1-0000000102": 2, "zone1-0000000103": 3, "zone1-0000000104": 2, @@ -5179,7 +5179,7 @@ func TestEmergencyReparenterFindErrantGTIDs(t *testing.T) { }, }, tmc: &testutil.TabletManagerClient{ - ReadReparentJournalInfoResults: map[string]int{ + ReadReparentJournalInfoResults: map[string]int32{ "zone1-0000000102": 3, "zone1-0000000103": 3, "zone1-0000000104": 3, @@ -5243,7 +5243,7 @@ func TestEmergencyReparenterFindErrantGTIDs(t *testing.T) { }, }, tmc: &testutil.TabletManagerClient{ - ReadReparentJournalInfoResults: map[string]int{ + ReadReparentJournalInfoResults: map[string]int32{ "zone1-0000000102": 3, "zone1-0000000103": 3, "zone1-0000000104": 2, @@ -5297,7 +5297,7 @@ func TestEmergencyReparenterFindErrantGTIDs(t *testing.T) { }, }, tmc: &testutil.TabletManagerClient{ - ReadReparentJournalInfoResults: map[string]int{ + ReadReparentJournalInfoResults: map[string]int32{ "zone1-0000000102": 3, "zone1-0000000103": 3, }, @@ -5354,7 +5354,7 @@ func TestEmergencyReparenterFindErrantGTIDs(t *testing.T) { }, }, tmc: &testutil.TabletManagerClient{ - ReadReparentJournalInfoResults: map[string]int{ + ReadReparentJournalInfoResults: map[string]int32{ "zone1-0000000102": 3, "zone1-0000000103": 3, "zone1-0000000104": 3, @@ -5418,7 +5418,7 @@ func TestEmergencyReparenterFindErrantGTIDs(t *testing.T) { }, }, tmc: &testutil.TabletManagerClient{ - ReadReparentJournalInfoResults: map[string]int{ + ReadReparentJournalInfoResults: map[string]int32{ "zone1-0000000102": 2, "zone1-0000000103": 3, "zone1-0000000104": 2, @@ -5481,7 +5481,7 @@ func TestEmergencyReparenterFindErrantGTIDs(t *testing.T) { }, }, tmc: &testutil.TabletManagerClient{ - ReadReparentJournalInfoResults: map[string]int{}, + ReadReparentJournalInfoResults: map[string]int32{}, }, statusMap: map[string]*replicationdatapb.StopReplicationStatus{ "zone1-0000000103": { diff --git a/go/vt/vttablet/common/config.go b/go/vt/vttablet/common/config.go index 72047ce4580..05a989d4cf2 100644 --- a/go/vt/vttablet/common/config.go +++ b/go/vt/vttablet/common/config.go @@ -142,18 +142,18 @@ func NewVReplicationConfig(overrides map[string]string) (*VReplicationConfig, er c.ExperimentalFlags = value } case "vreplication_net_read_timeout": - value, err := strconv.ParseInt(v, 10, 64) + value, err := strconv.Atoi(v) if err != nil { errors = append(errors, getError(k, v)) } else { - c.NetReadTimeout = int(value) + c.NetReadTimeout = value } case "vreplication_net_write_timeout": - value, err := strconv.ParseInt(v, 10, 64) + value, err := strconv.Atoi(v) if err != nil { errors = append(errors, getError(k, v)) } else { - c.NetWriteTimeout = int(value) + c.NetWriteTimeout = value } case "vreplication_copy_phase_duration": value, err := time.ParseDuration(v) @@ -177,18 +177,18 @@ func NewVReplicationConfig(overrides map[string]string) (*VReplicationConfig, er c.MaxTimeToRetryError = value } case "relay_log_max_size": - value, err := strconv.ParseInt(v, 10, 64) + value, err := strconv.Atoi(v) if err != nil { errors = append(errors, getError(k, v)) } else { - c.RelayLogMaxSize = int(value) + c.RelayLogMaxSize = value } case "relay_log_max_items": - value, err := strconv.ParseInt(v, 10, 64) + value, err := strconv.Atoi(v) if err != nil { errors = append(errors, getError(k, v)) } else { - c.RelayLogMaxItems = int(value) + c.RelayLogMaxItems = value } case "vreplication_replica_lag_tolerance": value, err := time.ParseDuration(v) @@ -198,11 +198,11 @@ func NewVReplicationConfig(overrides map[string]string) (*VReplicationConfig, er c.ReplicaLagTolerance = value } case "vreplication_heartbeat_update_interval": - value, err := strconv.ParseInt(v, 10, 64) + value, err := strconv.Atoi(v) if err != nil { errors = append(errors, getError(k, v)) } else { - c.HeartbeatUpdateInterval = int(value) + c.HeartbeatUpdateInterval = value } case "vreplication_store_compressed_gtid": value, err := strconv.ParseBool(v) @@ -212,19 +212,19 @@ func NewVReplicationConfig(overrides map[string]string) (*VReplicationConfig, er c.StoreCompressedGTID = value } case "vreplication-parallel-insert-workers": - value, err := strconv.ParseInt(v, 10, 64) + value, err := strconv.Atoi(v) if err != nil { errors = append(errors, getError(k, v)) } else { - c.ParallelInsertWorkers = int(value) + c.ParallelInsertWorkers = value } case "vstream_packet_size": - value, err := strconv.ParseInt(v, 10, 64) + value, err := strconv.Atoi(v) if err != nil { errors = append(errors, getError(k, v)) } else { c.VStreamPacketSizeOverride = true - c.VStreamPacketSize = int(value) + c.VStreamPacketSize = value } case "vstream_dynamic_packet_size": value, err := strconv.ParseBool(v) diff --git a/go/vt/vttablet/faketmclient/fake_client.go b/go/vt/vttablet/faketmclient/fake_client.go index 78c87d142a9..5a5cd33535f 100644 --- a/go/vt/vttablet/faketmclient/fake_client.go +++ b/go/vt/vttablet/faketmclient/fake_client.go @@ -358,7 +358,7 @@ func (client *FakeTabletManagerClient) PopulateReparentJournal(ctx context.Conte } // ReadReparentJournalInfo is part of the tmclient.TabletManagerClient interface. -func (client *FakeTabletManagerClient) ReadReparentJournalInfo(ctx context.Context, tablet *topodatapb.Tablet) (int, error) { +func (client *FakeTabletManagerClient) ReadReparentJournalInfo(ctx context.Context, tablet *topodatapb.Tablet) (int32, error) { return 10, nil } diff --git a/go/vt/vttablet/grpctmclient/client.go b/go/vt/vttablet/grpctmclient/client.go index 01d51e2993a..f6d154570eb 100644 --- a/go/vt/vttablet/grpctmclient/client.go +++ b/go/vt/vttablet/grpctmclient/client.go @@ -1124,7 +1124,7 @@ func (client *Client) PopulateReparentJournal(ctx context.Context, tablet *topod } // ReadReparentJournalInfo is part of the tmclient.TabletManagerClient interface. -func (client *Client) ReadReparentJournalInfo(ctx context.Context, tablet *topodatapb.Tablet) (int, error) { +func (client *Client) ReadReparentJournalInfo(ctx context.Context, tablet *topodatapb.Tablet) (int32, error) { c, closer, err := client.dialer.dial(ctx, tablet) if err != nil { return 0, err @@ -1134,7 +1134,7 @@ func (client *Client) ReadReparentJournalInfo(ctx context.Context, tablet *topod if err != nil { return 0, err } - return int(resp.Length), nil + return resp.Length, nil } // InitReplica is part of the tmclient.TabletManagerClient interface. diff --git a/go/vt/vttablet/grpctmserver/server.go b/go/vt/vttablet/grpctmserver/server.go index 6f0fd2aa4dc..be1b8ae9372 100644 --- a/go/vt/vttablet/grpctmserver/server.go +++ b/go/vt/vttablet/grpctmserver/server.go @@ -570,7 +570,7 @@ func (s *server) ReadReparentJournalInfo(ctx context.Context, request *tabletman response = &tabletmanagerdatapb.ReadReparentJournalInfoResponse{} length, err := s.tm.ReadReparentJournalInfo(ctx) if err == nil { - response.Length = int32(length) + response.Length = length } return response, err } diff --git a/go/vt/vttablet/tabletmanager/rpc_agent.go b/go/vt/vttablet/tabletmanager/rpc_agent.go index 445d74cb930..e779cfa8ff5 100644 --- a/go/vt/vttablet/tabletmanager/rpc_agent.go +++ b/go/vt/vttablet/tabletmanager/rpc_agent.go @@ -140,7 +140,7 @@ type RPCTM interface { PopulateReparentJournal(ctx context.Context, timeCreatedNS int64, actionName string, tabletAlias *topodatapb.TabletAlias, pos string) error - ReadReparentJournalInfo(ctx context.Context) (int, error) + ReadReparentJournalInfo(ctx context.Context) (int32, error) InitReplica(ctx context.Context, parent *topodatapb.TabletAlias, replicationPosition string, timeCreatedNS int64, semiSync bool) error diff --git a/go/vt/vttablet/tabletmanager/rpc_replication.go b/go/vt/vttablet/tabletmanager/rpc_replication.go index dec94ee6f16..7ac37515b67 100644 --- a/go/vt/vttablet/tabletmanager/rpc_replication.go +++ b/go/vt/vttablet/tabletmanager/rpc_replication.go @@ -428,7 +428,7 @@ func (tm *TabletManager) PopulateReparentJournal(ctx context.Context, timeCreate } // ReadReparentJournalInfo reads the information from reparent journal. -func (tm *TabletManager) ReadReparentJournalInfo(ctx context.Context) (int, error) { +func (tm *TabletManager) ReadReparentJournalInfo(ctx context.Context) (int32, error) { log.Infof("ReadReparentJournalInfo") if err := tm.waitForGrantsToHaveApplied(ctx); err != nil { return 0, err @@ -442,7 +442,7 @@ func (tm *TabletManager) ReadReparentJournalInfo(ctx context.Context) (int, erro if len(res.Rows) != 1 { return 0, vterrors.Errorf(vtrpc.Code_INTERNAL, "unexpected rows when reading reparent journal, got %v", len(res.Rows)) } - return res.Rows[0][0].ToInt() + return res.Rows[0][0].ToInt32() } // InitReplica sets replication primary and position, and waits for the diff --git a/go/vt/vttablet/tabletserver/twopc.go b/go/vt/vttablet/tabletserver/twopc.go index e1212ebe627..67cf3f894cf 100644 --- a/go/vt/vttablet/tabletserver/twopc.go +++ b/go/vt/vttablet/tabletserver/twopc.go @@ -569,7 +569,7 @@ func (tpc *TwoPC) UnresolvedTransactions(ctx context.Context, abandonTime time.T appendCurrentTx() // Extract the transaction state and initialize a new TransactionMetadata - stateID, _ := row[1].ToInt() + stateID, _ := row[1].ToInt32() timeCreated, _ := row[2].ToCastInt64() currentTx = &querypb.TransactionMetadata{ Dtid: dtid, diff --git a/go/vt/vttablet/tmclient/rpc_client_api.go b/go/vt/vttablet/tmclient/rpc_client_api.go index 2b5cc967e9f..7ce4c7f0bf4 100644 --- a/go/vt/vttablet/tmclient/rpc_client_api.go +++ b/go/vt/vttablet/tmclient/rpc_client_api.go @@ -239,7 +239,7 @@ type TabletManagerClient interface { PopulateReparentJournal(ctx context.Context, tablet *topodatapb.Tablet, timeCreatedNS int64, actionName string, tabletAlias *topodatapb.TabletAlias, pos string) error // ReadReparentJournalInfo reads the information from reparent journal - ReadReparentJournalInfo(ctx context.Context, tablet *topodatapb.Tablet) (int, error) + ReadReparentJournalInfo(ctx context.Context, tablet *topodatapb.Tablet) (int32, error) // InitReplica tells a tablet to start replicating from the // passed in primary tablet alias, and wait for the row in the diff --git a/go/vt/vttablet/tmrpctest/test_tm_rpc.go b/go/vt/vttablet/tmrpctest/test_tm_rpc.go index a106b43bf2c..0ec380882cc 100644 --- a/go/vt/vttablet/tmrpctest/test_tm_rpc.go +++ b/go/vt/vttablet/tmrpctest/test_tm_rpc.go @@ -1179,9 +1179,9 @@ func (fra *fakeRPCTM) PopulateReparentJournal(ctx context.Context, timeCreatedNS return nil } -var testReparentJournalLen = 10 +var testReparentJournalLen int32 = 10 -func (fra *fakeRPCTM) ReadReparentJournalInfo(context.Context) (int, error) { +func (fra *fakeRPCTM) ReadReparentJournalInfo(context.Context) (int32, error) { if fra.panics { panic(fmt.Errorf("test-triggered panic")) }