diff --git a/internal/web/service/inbound.go b/internal/web/service/inbound.go index 0f6385215..ec9ea653a 100644 --- a/internal/web/service/inbound.go +++ b/internal/web/service/inbound.go @@ -930,18 +930,11 @@ func (s *InboundService) AddInbound(inbound *model.Inbound) (*model.Inbound, boo return inbound, false, err } - conflict, err := s.checkPortConflict(inbound, 0) - if err != nil { - return inbound, false, err - } - if conflict != nil { - return inbound, false, common.NewError(conflict.String()) - } - - inbound.Tag, err = s.resolveInboundTag(inbound, 0) + tag, err := s.resolveInboundTag(inbound, 0) if err != nil { return inbound, false, err } + inbound.Tag = tag clients, err := s.GetClients(inbound) if err != nil { @@ -1027,10 +1020,16 @@ func (s *InboundService) AddInbound(inbound *model.Inbound) (*model.Inbound, boo } } - db := database.GetDB() needRestart := false var postCommitApply func() - err = db.Transaction(func(tx *gorm.DB) error { + err = runSerializedTx(func(tx *gorm.DB) error { + conflict, cErr := checkPortConflictTx(tx, inbound, 0) + if cErr != nil { + return cErr + } + if conflict != nil { + return common.NewError(conflict.String()) + } markDirty := false if err := tx.Omit("ClientStats").Save(inbound).Error; err != nil { return err @@ -1416,14 +1415,6 @@ func (s *InboundService) UpdateInbound(inbound *model.Inbound) (*model.Inbound, // stays scoped to its own node (the payload's nodeId is unreliable, often absent). inbound.NodeID = oldInbound.NodeID - conflict, err := s.checkPortConflict(inbound, inbound.Id) - if err != nil { - return inbound, false, err - } - if conflict != nil { - return inbound, false, common.NewError(conflict.String()) - } - // Capture the pre-edit protocol and routing state before oldInbound is // overwritten with the new values further down, then ensure a routed // inbound keeps a stable egress port (reusing the one already stored). @@ -1441,6 +1432,13 @@ func (s *InboundService) UpdateInbound(inbound *model.Inbound) (*model.Inbound, var postCommitApply func() txErr := runSerializedTx(func(tx *gorm.DB) error { + conflict, cErr := checkPortConflictTx(tx, inbound, inbound.Id) + if cErr != nil { + return cErr + } + if conflict != nil { + return common.NewError(conflict.String()) + } if err := s.updateClientTraffics(tx, oldInbound, inbound); err != nil { return err } diff --git a/internal/web/service/inbound_create_race_test.go b/internal/web/service/inbound_create_race_test.go new file mode 100644 index 000000000..a44d5d065 --- /dev/null +++ b/internal/web/service/inbound_create_race_test.go @@ -0,0 +1,99 @@ +package service + +import ( + "fmt" + "sync" + "testing" + + "github.com/mhsanaei/3x-ui/v3/internal/database" + "github.com/mhsanaei/3x-ui/v3/internal/database/model" +) + +// A wildcard listener and a specific one on the same port overlap, but they are +// two different rows: only the in-transaction check can reject the pair, and it +// can only do so if the check and the insert cannot interleave. +func TestAddInboundConcurrentOverlappingListenersSingleWinner(t *testing.T) { + setupConflictDB(t) + + const rounds = 25 + for round := range rounds { + port := 24000 + round + claims := []*model.Inbound{ + { + Tag: fmt.Sprintf("race-%d-wildcard", round), Listen: "", + Port: port, Protocol: model.VLESS, + StreamSettings: `{"network":"tcp"}`, Settings: `{"clients":[]}`, + }, + { + Tag: fmt.Sprintf("race-%d-specific", round), Listen: "127.0.0.1", + Port: port, Protocol: model.Trojan, + StreamSettings: `{"network":"tcp"}`, Settings: `{"clients":[]}`, + }, + } + + start := make(chan struct{}) + errs := make(chan error, len(claims)) + var wg sync.WaitGroup + for _, claim := range claims { + wg.Add(1) + go func(inbound *model.Inbound) { + defer wg.Done() + <-start + _, _, err := (&InboundService{}).AddInbound(inbound) + errs <- err + }(claim) + } + close(start) + wg.Wait() + close(errs) + + committed := 0 + rejections := make([]string, 0, len(claims)) + for err := range errs { + if err == nil { + committed++ + continue + } + rejections = append(rejections, err.Error()) + } + if committed != 1 { + t.Fatalf("round %d port %d: concurrent AddInbound committed=%d, want exactly 1 (rejections: %v)", + round, port, committed, rejections) + } + } +} + +// Editing an inbound onto a port another one already holds must be rejected — +// the check moved inside the transaction, and nothing else guards this path. +func TestUpdateInboundRejectsPortTakenByAnother(t *testing.T) { + setupConflictDB(t) + + svc := &InboundService{} + first := &model.Inbound{ + Tag: "update-holder", Listen: "", Port: 25101, Protocol: model.VLESS, + StreamSettings: `{"network":"tcp"}`, Settings: `{"clients":[]}`, + } + if _, _, err := svc.AddInbound(first); err != nil { + t.Fatalf("seed holder: %v", err) + } + second := &model.Inbound{ + Tag: "update-mover", Listen: "", Port: 25102, Protocol: model.VLESS, + StreamSettings: `{"network":"tcp"}`, Settings: `{"clients":[]}`, + } + if _, _, err := svc.AddInbound(second); err != nil { + t.Fatalf("seed mover: %v", err) + } + + second.Port = first.Port + if _, _, err := svc.UpdateInbound(second); err == nil { + t.Fatal("moving an inbound onto a port already in use was accepted") + } + + var stored model.Inbound + if err := database.GetDB().First(&stored, second.Id).Error; err != nil { + t.Fatal(err) + } + if stored.Port != 25102 { + t.Fatalf("rejected update still changed the stored port to %d", stored.Port) + } +} diff --git a/internal/web/service/port_conflict.go b/internal/web/service/port_conflict.go index f36a2f9b7..9737782bf 100644 --- a/internal/web/service/port_conflict.go +++ b/internal/web/service/port_conflict.go @@ -8,6 +8,8 @@ import ( "github.com/mhsanaei/3x-ui/v3/internal/database" "github.com/mhsanaei/3x-ui/v3/internal/database/model" "github.com/mhsanaei/3x-ui/v3/internal/util/common" + + "gorm.io/gorm" ) type transportBits uint8 @@ -158,7 +160,13 @@ func reservedAPIPort() int { return defaultXrayAPIPort } +// checkPortConflict reads outside any transaction; callers that must not race a +// concurrent create use checkPortConflictTx inside their own transaction. func (s *InboundService) checkPortConflict(inbound *model.Inbound, ignoreId int) (*portConflictDetail, error) { + return checkPortConflictTx(database.GetDB(), inbound, ignoreId) +} + +func checkPortConflictTx(db *gorm.DB, inbound *model.Inbound, ignoreId int) (*portConflictDetail, error) { newBits := inboundTransports(inbound.Protocol, inbound.StreamSettings, inbound.Settings) // The internal Xray API inbound (tag "api", loopback TCP) isn't a DB row, @@ -175,8 +183,6 @@ func (s *InboundService) checkPortConflict(inbound *model.Inbound, ignoreId int) }, nil } - db := database.GetDB() - var candidates []*model.Inbound q := db.Model(model.Inbound{}).Where("port = ?", inbound.Port) if ignoreId > 0 {