Skip to content

Commit d3458da

Browse files
wxiaoguangwxiaoguang
committed
fix
1 parent d70af38 commit d3458da

File tree

10 files changed

+167
-75
lines changed

10 files changed

+167
-75
lines changed

‎models/db/context.go‎

Lines changed: 62 additions & 47 deletions
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,12 @@ package db
66
import (
77
"context"
88
"database/sql"
9+
"errors"
10+
"runtime"
11+
"slices"
12+
"sync"
13+
14+
"code.gitea.io/gitea/modules/setting"
915

1016
"xorm.io/builder"
1117
"xorm.io/xorm"
@@ -15,76 +21,85 @@ import (
1521
// will be overwritten by Init with HammerContext
1622
var DefaultContext context.Context
1723

18-
// contextKey is a value for use with context.WithValue.
19-
type contextKey struct {
20-
name string
21-
}
24+
type engineContextKeyType struct{}
2225

23-
// enginedContextKey is a context key. It is used with context.Value() to get the current Engined for the context
24-
var (
25-
enginedContextKey = &contextKey{"engined"}
26-
_ Engined = &Context{}
27-
)
26+
var engineContextKey = engineContextKeyType{}
2827

2928
// Context represents a db context
3029
type Context struct {
3130
context.Context
32-
e Engine
33-
transaction bool
31+
engine Engine
3432
}
3533

36-
func newContext(ctx context.Context, e Engine, transaction bool) *Context {
37-
return &Context{
38-
Context: ctx,
39-
e: e,
40-
transaction: transaction,
41-
}
42-
}
43-
44-
// InTransaction if context is in a transaction
45-
func (ctx *Context) InTransaction() bool {
46-
return ctx.transaction
47-
}
48-
49-
// Engine returns db engine
50-
func (ctx *Context) Engine() Engine {
51-
return ctx.e
34+
func newContext(ctx context.Context, e Engine) *Context {
35+
return &Context{Context: ctx, engine: e}
5236
}
5337

5438
// Value shadows Value for context.Context but allows us to get ourselves and an Engined object
5539
func (ctx *Context) Value(key any) any {
56-
if key == enginedContextKey {
40+
if key == engineContextKey {
5741
return ctx
5842
}
5943
return ctx.Context.Value(key)
6044
}
6145

6246
// WithContext returns this engine tied to this context
6347
func (ctx *Context) WithContext(other context.Context) *Context {
64-
return newContext(ctx, ctx.e.Context(other), ctx.transaction)
48+
return newContext(ctx, ctx.engine.Context(other))
6549
}
6650

67-
// Engined structs provide an Engine
68-
type Engined interface {
69-
Engine() Engine
51+
var contextSafetyOnce sync.Once
52+
var contextSafetyDeniedFuncPCs []uintptr
53+
54+
func contextSafetyCheck(e Engine) {
55+
if setting.IsProd && !setting.IsInTesting {
56+
return
57+
}
58+
// Only do this check for non-end-users. If the problem could be fixed in the future, this code could be removed.
59+
contextSafetyOnce.Do(func() {
60+
// try to figure out the bad functions to deny
61+
type m struct{}
62+
_ = e.SQL("SELECT 1").Iterate(&m{}, func(int, any) error {
63+
callers := make([]uintptr, 32)
64+
callerNum := runtime.Callers(1, callers)
65+
for i := 0; i < callerNum; i++ {
66+
if funcName := runtime.FuncForPC(callers[i]).Name(); funcName == "xorm.io/xorm.(*Session).Iterate" {
67+
contextSafetyDeniedFuncPCs = append(contextSafetyDeniedFuncPCs, callers[i])
68+
}
69+
}
70+
return nil
71+
})
72+
if len(contextSafetyDeniedFuncPCs) != 1 {
73+
panic(errors.New("unable to determine the functions to deny"))
74+
}
75+
})
76+
77+
// it should be very fast: xxxx ns/op
78+
callers := make([]uintptr, 32)
79+
callerNum := runtime.Callers(3, callers) // skip 3: runtime.Callers, contextSafetyCheck, GetEngine
80+
for i := 0; i < callerNum; i++ {
81+
if slices.Contains(contextSafetyDeniedFuncPCs, callers[i]) {
82+
panic(errors.New("using database context in an iterator would cause corrupted results"))
83+
}
84+
}
7085
}
7186

72-
// GetEngine will get a db Engine from this context or return an Engine restricted to this context
87+
// GetEngine gets an existing db Engine/Statement or creates a new Session
7388
func GetEngine(ctx context.Context) Engine {
74-
if e := getEngine(ctx); e != nil {
89+
if e := getExistingEngine(ctx); e != nil {
7590
return e
7691
}
7792
return x.Context(ctx)
7893
}
7994

80-
// getEngine will get a db Engine from this context or return nil
81-
func getEngine(ctx context.Context) Engine {
82-
if engined, ok := ctx.(Engined); ok {
83-
return engined.Engine()
95+
// getExistingEngine gets an existing db Engine/Statement from this context or returns nil
96+
func getExistingEngine(ctx context.Context) (e Engine) {
97+
defer func() { contextSafetyCheck(e) }()
98+
if engined, ok := ctx.(*Context); ok {
99+
return engined.engine
84100
}
85-
enginedInterface := ctx.Value(enginedContextKey)
86-
if enginedInterface != nil {
87-
return enginedInterface.(Engined).Engine()
101+
if engined, ok := ctx.Value(engineContextKey).(*Context); ok {
102+
return engined.engine
88103
}
89104
return nil
90105
}
@@ -132,23 +147,23 @@ func (c *halfCommitter) Close() error {
132147
// d. It doesn't mean rollback is forbidden, but always do it only when there is an error, and you do want to rollback.
133148
func TxContext(parentCtx context.Context) (*Context, Committer, error) {
134149
if sess, ok := inTransaction(parentCtx); ok {
135-
return newContext(parentCtx, sess, true), &halfCommitter{committer: sess}, nil
150+
return newContext(parentCtx, sess), &halfCommitter{committer: sess}, nil
136151
}
137152

138153
sess := x.NewSession()
139154
if err := sess.Begin(); err != nil {
140-
sess.Close()
155+
_ = sess.Close()
141156
return nil, nil, err
142157
}
143158

144-
return newContext(DefaultContext, sess, true), sess, nil
159+
return newContext(DefaultContext, sess), sess, nil
145160
}
146161

147162
// WithTx represents executing database operations on a transaction, if the transaction exist,
148163
// this function will reuse it otherwise will create a new one and close it when finished.
149164
func WithTx(parentCtx context.Context, f func(ctx context.Context) error) error {
150165
if sess, ok := inTransaction(parentCtx); ok {
151-
err := f(newContext(parentCtx, sess, true))
166+
err := f(newContext(parentCtx, sess))
152167
if err != nil {
153168
// rollback immediately, in case the caller ignores returned error and tries to commit the transaction.
154169
_ = sess.Close()
@@ -165,7 +180,7 @@ func txWithNoCheck(parentCtx context.Context, f func(ctx context.Context) error)
165180
return err
166181
}
167182

168-
if err := f(newContext(parentCtx, sess, true)); err != nil {
183+
if err := f(newContext(parentCtx, sess)); err != nil {
169184
return err
170185
}
171186

@@ -312,7 +327,7 @@ func InTransaction(ctx context.Context) bool {
312327
}
313328

314329
func inTransaction(ctx context.Context) (*xorm.Session, bool) {
315-
e := getEngine(ctx)
330+
e := getExistingEngine(ctx)
316331
if e == nil {
317332
return nil, false
318333
}

‎models/db/context_test.go‎

Lines changed: 44 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -9,7 +9,6 @@ import (
99

1010
"code.gitea.io/gitea/models/db"
1111
"code.gitea.io/gitea/models/unittest"
12-
1312
"github.com/stretchr/testify/assert"
1413
)
1514

@@ -84,3 +83,47 @@ func TestTxContext(t *testing.T) {
8483
}))
8584
}
8685
}
86+
87+
func TestContextSafety(t *testing.T) {
88+
type TestModel1 struct {
89+
ID int64
90+
}
91+
type TestModel2 struct {
92+
ID int64
93+
}
94+
assert.NoError(t, unittest.GetXORMEngine().Sync(&TestModel1{}, &TestModel2{}))
95+
assert.NoError(t, db.TruncateBeans(db.DefaultContext, &TestModel1{}, &TestModel2{}))
96+
testCount := 10
97+
for i := 1; i <= testCount; i++ {
98+
assert.NoError(t, db.Insert(db.DefaultContext, &TestModel1{ID: int64(i)}))
99+
assert.NoError(t, db.Insert(db.DefaultContext, &TestModel2{ID: int64(-i)}))
100+
}
101+
102+
actualCount := 0
103+
// here: db.GetEngine(db.DefaultContext) is a new *Session created from *Engine
104+
_ = db.WithTx(db.DefaultContext, func(ctx context.Context) error {
105+
_ = db.GetEngine(ctx).Iterate(&TestModel1{}, func(i int, bean any) error {
106+
// here: db.GetEngine(ctx) is always the unclosed "Iterate" *Session with autoResetStatement=false,
107+
// and the internal states (including "cond" and others) are always there and not be reset in this callback.
108+
m1 := bean.(*TestModel1)
109+
assert.EqualValues(t, i+1, m1.ID)
110+
111+
// here: XORM bug, it fails because the SQL becomes "WHERE id=-1", "WHERE id=-1 AND id=-2", "WHERE id=-1 AND id=-2 AND id=-3" ...
112+
// and it conflicts with the "Iterate"'s internal states.
113+
// has, err := db.GetEngine(ctx).Get(&TestModel2{ID: -m1.ID})
114+
115+
actualCount++
116+
return nil
117+
})
118+
return nil
119+
})
120+
assert.EqualValues(t, testCount, actualCount)
121+
122+
// deny the bad usages
123+
assert.PanicsWithError(t, "using database context in an iterator would cause corrupted results", func() {
124+
_ = unittest.GetXORMEngine().Iterate(&TestModel1{}, func(i int, bean any) error {
125+
_ = db.GetEngine(db.DefaultContext)
126+
return nil
127+
})
128+
})
129+
}

‎models/db/engine.go‎

Lines changed: 1 addition & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -161,10 +161,7 @@ func InitEngine(ctx context.Context) error {
161161
// SetDefaultEngine sets the default engine for db
162162
func SetDefaultEngine(ctx context.Context, eng *xorm.Engine) {
163163
x = eng
164-
DefaultContext = &Context{
165-
Context: ctx,
166-
e: x,
167-
}
164+
DefaultContext = &Context{Context: ctx, engine: x}
168165
}
169166

170167
// UnsetDefaultEngine closes and unsets the default engine

‎models/db/install/db.go‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -11,7 +11,7 @@ import (
1111
)
1212

1313
func getXORMEngine() *xorm.Engine {
14-
return db.DefaultContext.(*db.Context).Engine().(*xorm.Engine)
14+
return db.GetEngine(db.DefaultContext).(*xorm.Engine)
1515
}
1616

1717
// CheckDatabaseConnection checks the database connection

‎models/db/iterate.go‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -11,7 +11,7 @@ import (
1111
"xorm.io/builder"
1212
)
1313

14-
// Iterate iterate all the Bean object
14+
// Iterate iterates all the Bean object
1515
func Iterate[Bean any](ctx context.Context, cond builder.Cond, f func(ctx context.Context, bean *Bean) error) error {
1616
var start int
1717
batchSize := setting.Database.IterateBufferSize

‎models/packages/debian/search.go‎

Lines changed: 16 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -75,26 +75,27 @@ func ExistPackages(ctx context.Context, opts *PackageSearchOptions) (bool, error
7575
}
7676

7777
// SearchPackages gets the packages matching the search options
78-
func SearchPackages(ctx context.Context, opts *PackageSearchOptions, iter func(*packages.PackageFileDescriptor)) error {
79-
return db.GetEngine(ctx).
78+
func SearchPackages(ctx context.Context, opts *PackageSearchOptions) ([]*packages.PackageFileDescriptor, error) {
79+
var pkgFiles []*packages.PackageFile
80+
err := db.GetEngine(ctx).
8081
Table("package_file").
8182
Select("package_file.*").
8283
Join("INNER", "package_version", "package_version.id = package_file.version_id").
8384
Join("INNER", "package", "package.id = package_version.package_id").
8485
Where(opts.toCond()).
85-
Asc("package.lower_name", "package_version.created_unix").
86-
Iterate(new(packages.PackageFile), func(_ int, bean any) error {
87-
pf := bean.(*packages.PackageFile)
88-
89-
pfd, err := packages.GetPackageFileDescriptor(ctx, pf)
90-
if err != nil {
91-
return err
92-
}
93-
94-
iter(pfd)
95-
96-
return nil
97-
})
86+
Asc("package.lower_name", "package_version.created_unix").Find(&pkgFiles)
87+
if err != nil {
88+
return nil, err
89+
}
90+
pfds := make([]*packages.PackageFileDescriptor, 0, len(pkgFiles))
91+
for _, pf := range pkgFiles {
92+
pfd, err := packages.GetPackageFileDescriptor(ctx, pf)
93+
if err != nil {
94+
return nil, err
95+
}
96+
pfds = append(pfds, pfd)
97+
}
98+
return pfds, nil
9899
}
99100

100101
// GetDistributions gets all available distributions

‎models/unittest/fixtures.go‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -25,7 +25,7 @@ func GetXORMEngine(engine ...*xorm.Engine) (x *xorm.Engine) {
2525
if len(engine) == 1 {
2626
return engine[0]
2727
}
28-
return db.DefaultContext.(*db.Context).Engine().(*xorm.Engine)
28+
return db.GetEngine(db.DefaultContext).(*xorm.Engine)
2929
}
3030

3131
// InitFixtures initialize test fixtures for a test database

‎services/packages/cleanup/cleanup.go‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -22,7 +22,7 @@ import (
2222
rpm_service "code.gitea.io/gitea/services/packages/rpm"
2323
)
2424

25-
// Task method to execute cleanup rules and cleanup expired package data
25+
// CleanupTask executes cleanup rules and cleanup expired package data
2626
func CleanupTask(ctx context.Context, olderThan time.Duration) error {
2727
if err := ExecuteCleanupRules(ctx); err != nil {
2828
return err

‎services/packages/debian/repository.go‎

Lines changed: 5 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -206,7 +206,11 @@ func buildPackagesIndices(ctx context.Context, ownerID int64, repoVersion *packa
206206
w := io.MultiWriter(packagesContent, gzw, xzw)
207207

208208
addSeparator := false
209-
if err := debian_model.SearchPackages(ctx, opts, func(pfd *packages_model.PackageFileDescriptor) {
209+
pfds, err := debian_model.SearchPackages(ctx, opts)
210+
if err != nil {
211+
return err
212+
}
213+
for _, pfd := range pfds {
210214
if addSeparator {
211215
fmt.Fprintln(w)
212216
}
@@ -220,10 +224,7 @@ func buildPackagesIndices(ctx context.Context, ownerID int64, repoVersion *packa
220224
fmt.Fprintf(w, "SHA1: %s\n", pfd.Blob.HashSHA1)
221225
fmt.Fprintf(w, "SHA256: %s\n", pfd.Blob.HashSHA256)
222226
fmt.Fprintf(w, "SHA512: %s\n", pfd.Blob.HashSHA512)
223-
}); err != nil {
224-
return err
225227
}
226-
227228
gzw.Close()
228229
xzw.Close()
229230

0 commit comments

Comments
 (0)