Skip to content

Commit cda37f6

Browse files
wxiaoguangwxiaoguang
committed
fix
1 parent d70af38 commit cda37f6

File tree

10 files changed

+169
-74
lines changed

10 files changed

+169
-74
lines changed

‎models/db/context.go‎

Lines changed: 64 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,87 @@ 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
34-
}
35-
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
31+
engine Engine
4732
}
4833

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

72-
// GetEngine will get a db Engine from this context or return an Engine restricted to this context
89+
// GetEngine gets an existing db Engine/Statement or creates a new Session
7390
func GetEngine(ctx context.Context) Engine {
74-
if e := getEngine(ctx); e != nil {
91+
if e := getExistingEngine(ctx); e != nil {
7592
return e
7693
}
7794
return x.Context(ctx)
7895
}
7996

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()
97+
// getExistingEngine gets an existing db Engine/Statement from this context or returns nil
98+
func getExistingEngine(ctx context.Context) (e Engine) {
99+
defer func() { contextSafetyCheck(e) }()
100+
if engined, ok := ctx.(*Context); ok {
101+
return engined.engine
84102
}
85-
enginedInterface := ctx.Value(enginedContextKey)
86-
if enginedInterface != nil {
87-
return enginedInterface.(Engined).Engine()
103+
if engined, ok := ctx.Value(engineContextKey).(*Context); ok {
104+
return engined.engine
88105
}
89106
return nil
90107
}
@@ -132,23 +149,23 @@ func (c *halfCommitter) Close() error {
132149
// d. It doesn't mean rollback is forbidden, but always do it only when there is an error, and you do want to rollback.
133150
func TxContext(parentCtx context.Context) (*Context, Committer, error) {
134151
if sess, ok := inTransaction(parentCtx); ok {
135-
return newContext(parentCtx, sess, true), &halfCommitter{committer: sess}, nil
152+
return newContext(parentCtx, sess), &halfCommitter{committer: sess}, nil
136153
}
137154

138155
sess := x.NewSession()
139156
if err := sess.Begin(); err != nil {
140-
sess.Close()
157+
_ = sess.Close()
141158
return nil, nil, err
142159
}
143160

144-
return newContext(DefaultContext, sess, true), sess, nil
161+
return newContext(DefaultContext, sess), sess, nil
145162
}
146163

147164
// WithTx represents executing database operations on a transaction, if the transaction exist,
148165
// this function will reuse it otherwise will create a new one and close it when finished.
149166
func WithTx(parentCtx context.Context, f func(ctx context.Context) error) error {
150167
if sess, ok := inTransaction(parentCtx); ok {
151-
err := f(newContext(parentCtx, sess, true))
168+
err := f(newContext(parentCtx, sess))
152169
if err != nil {
153170
// rollback immediately, in case the caller ignores returned error and tries to commit the transaction.
154171
_ = sess.Close()
@@ -165,7 +182,7 @@ func txWithNoCheck(parentCtx context.Context, f func(ctx context.Context) error)
165182
return err
166183
}
167184

168-
if err := f(newContext(parentCtx, sess, true)); err != nil {
185+
if err := f(newContext(parentCtx, sess)); err != nil {
169186
return err
170187
}
171188

@@ -312,7 +329,7 @@ func InTransaction(ctx context.Context) bool {
312329
}
313330

314331
func inTransaction(ctx context.Context) (*xorm.Session, bool) {
315-
e := getEngine(ctx)
332+
e := getExistingEngine(ctx)
316333
if e == nil {
317334
return nil, false
318335
}

‎models/db/context_test.go‎

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

‎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)