Skip to content

Commit 9516580

Browse files
xaionaro@dx.centerxaionaro@dx.center
authored andcommitted
fix: handle TF_STATUS_CODE, prevent mutex deadlock, fix binder token writing
Bug 3: In BR_REPLY handling, check the TF_STATUS_CODE flag (0x08). When set, the 4-byte reply data is a kernel status_t error code, not a regular parcel. Added tfStatusCode constant to protocol.go. Bug 4: Transact() held d.mu for the entire transaction (send + wait for reply). The read loop also needs d.mu to acknowledge BR_INCREFS/BR_ACQUIRE callbacks. If the server sends callbacks before replying, this causes deadlock. Fixed by extracting lockedIoctl() and scoping the mutex to individual ioctl calls. Bug 2: IBinder fields in generated parcelable MarshalParcel would panic on nil Token (calling .Handle() on nil). Added nil guards that write null binder objects. Also fixed codegen to generate these guards for all IBinder/interface parcelable fields. Bug 1: AttributionSourceState uses @utf8InCpp String fields but codegen wrote UTF-16 (WriteString16). Fixed by merging field-level annotations into TypeSpecifier before marshal lookup, so @utf8InCpp is visible to marshalForType. Manually patched attributionsourcestate.go to use WriteString/ReadString (will be regenerated correctly now).
1 parent bed0657 commit 9516580

4 files changed

Lines changed: 131 additions & 62 deletions

File tree

android/content/attributionsourcestate.go

Lines changed: 13 additions & 7 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

kernelbinder/driver.go

Lines changed: 60 additions & 39 deletions
Original file line numberDiff line numberDiff line change
@@ -186,9 +186,6 @@ func (d *Driver) Transact(
186186
runtime.LockOSThread()
187187
defer runtime.UnlockOSThread()
188188

189-
d.mu.Lock()
190-
defer d.mu.Unlock()
191-
192189
dataBytes := data.Data()
193190
objects := data.Objects()
194191

@@ -232,29 +229,17 @@ func (d *Driver) Transact(
232229
readBuffer: uint64(uintptr(unsafe.Pointer(&readBuf[0]))),
233230
}
234231

235-
for {
236-
_, _, errno := unix.Syscall(
237-
unix.SYS_IOCTL,
238-
uintptr(d.fd),
239-
binderWriteReadIoctl,
240-
uintptr(unsafe.Pointer(&bwr)),
241-
)
242-
switch errno {
243-
case 0:
244-
// Success — proceed to parse.
245-
case unix.EINTR:
246-
// Retry on interrupted system call.
247-
// Reset write buffer (already consumed) but keep read buffer.
248-
bwr.writeSize = 0
249-
continue
250-
default:
251-
return nil, &aidlerrors.BinderError{Op: "ioctl(BINDER_WRITE_READ)", Err: errno}
252-
}
253-
break
232+
// Lock only around each individual ioctl call, not across the entire
233+
// transaction. Holding the mutex during a blocking read-wait would
234+
// prevent the read loop from acknowledging BR_INCREFS/BR_ACQUIRE
235+
// callbacks, causing deadlock when the kernel expects acknowledgment
236+
// before sending BR_REPLY.
237+
if err := d.lockedIoctl(&bwr); err != nil {
238+
return nil, err
254239
}
255240

256241
// Parse the read buffer for BR codes. The kernel may split
257-
// BR_TRANSACTION_COMPLETE and BR_REPLY across separate ioctl reads
242+
// BR_TRANSACTION_COMPLETE and BR_REPLY across separate ioctl reads --
258243
// if we got TC but no reply yet, read again to wait for BR_REPLY.
259244
isOneway := flags&binder.FlagOneway != 0
260245
for {
@@ -269,27 +254,14 @@ func (d *Driver) Transact(
269254
return reply, nil
270255
}
271256

272-
// BR_TRANSACTION_COMPLETE without BR_REPLY the service hasn't
257+
// BR_TRANSACTION_COMPLETE without BR_REPLY -- the service hasn't
273258
// responded yet. Issue a read-only ioctl to wait for BR_REPLY.
274259
logger.Debugf(ctx, "Transact: got BR_TRANSACTION_COMPLETE without BR_REPLY, reading again")
275260
bwr.writeSize = 0
276261
bwr.writeConsumed = 0
277262
bwr.readConsumed = 0
278-
for {
279-
_, _, errno := unix.Syscall(
280-
unix.SYS_IOCTL,
281-
uintptr(d.fd),
282-
binderWriteReadIoctl,
283-
uintptr(unsafe.Pointer(&bwr)),
284-
)
285-
switch errno {
286-
case 0:
287-
case unix.EINTR:
288-
continue
289-
default:
290-
return nil, &aidlerrors.BinderError{Op: "ioctl(BINDER_WRITE_READ/read)", Err: errno}
291-
}
292-
break
263+
if err := d.lockedIoctl(&bwr); err != nil {
264+
return nil, err
293265
}
294266
}
295267
}
@@ -343,15 +315,35 @@ func (d *Driver) parseReadBuffer(
343315
// Copy data from the mmap'd region into a new parcel.
344316
replyData := d.copyFromMapped(txn.dataBuffer, txn.dataSize)
345317

318+
// When the kernel sets TF_STATUS_CODE, the 4-byte data is a
319+
// status_t error code, not a regular parcel response.
320+
if txn.flags&tfStatusCode != 0 {
321+
d.mu.Lock()
322+
freeErr := d.freeBuffer(ctx, txn.dataBuffer)
323+
d.mu.Unlock()
324+
if freeErr != nil {
325+
logger.Warnf(ctx, "failed to free binder buffer: %v", freeErr)
326+
}
327+
if len(replyData) >= 4 {
328+
statusCode := int32(binary.LittleEndian.Uint32(replyData))
329+
if statusCode != 0 {
330+
return nil, true, fmt.Errorf("binder: kernel status error: %d (0x%x)", statusCode, uint32(statusCode))
331+
}
332+
}
333+
return parcel.New(), true, nil
334+
}
335+
346336
// Acquire references for any binder handles in the reply
347337
// BEFORE freeing the buffer, otherwise the kernel drops
348338
// the handle references.
339+
d.mu.Lock()
349340
d.acquireReplyHandles(ctx, replyData, txn.offsetsBuffer, txn.offsetsSize)
350341

351342
// Free the mmap'd buffer.
352343
if err := d.freeBuffer(ctx, txn.dataBuffer); err != nil {
353344
logger.Warnf(ctx, "failed to free binder buffer: %v", err)
354345
}
346+
d.mu.Unlock()
355347

356348
return parcel.FromBytes(replyData), true, nil
357349

@@ -573,6 +565,35 @@ func (d *Driver) writeCommand(
573565
}
574566
}
575567

568+
// lockedIoctl executes a BINDER_WRITE_READ ioctl while holding d.mu,
569+
// retrying on EINTR.
570+
func (d *Driver) lockedIoctl(
571+
bwr *binderWriteRead,
572+
) error {
573+
d.mu.Lock()
574+
defer d.mu.Unlock()
575+
576+
for {
577+
_, _, errno := unix.Syscall(
578+
unix.SYS_IOCTL,
579+
uintptr(d.fd),
580+
binderWriteReadIoctl,
581+
uintptr(unsafe.Pointer(bwr)),
582+
)
583+
switch errno {
584+
case 0:
585+
return nil
586+
case unix.EINTR:
587+
// Retry on interrupted system call.
588+
// Reset write buffer (already consumed) but keep read buffer.
589+
bwr.writeSize = 0
590+
continue
591+
default:
592+
return &aidlerrors.BinderError{Op: "ioctl(BINDER_WRITE_READ)", Err: errno}
593+
}
594+
}
595+
}
596+
576597
// copyStructToBytes copies a struct's raw memory into a byte slice.
577598
func copyStructToBytes(
578599
dst []byte,

kernelbinder/protocol.go

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -33,6 +33,10 @@ const binderPtrCookieSize = 2 * unsafe.Sizeof(uintptr(0)) // = 16 on 64-bit
3333
// __attribute__((packed)), so there is NO alignment padding between fields.
3434
const binderHandleCookieSize = unsafe.Sizeof(uint32(0)) + unsafe.Sizeof(uintptr(0)) // = 12 on 64-bit
3535

36+
// tfStatusCode is TF_STATUS_CODE (0x08). When the kernel sets this flag on a
37+
// BR_REPLY, the 4-byte data payload is a status_t error code, not a regular parcel.
38+
const tfStatusCode = uint32(0x08)
39+
3640
// Binder return (BR) codes -- read from the driver.
3741
var (
3842
brError = uint32(ior('r', 0, unsafe.Sizeof(int32(0))))

tools/pkg/codegen/parcelable_gen.go

Lines changed: 54 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -73,6 +73,22 @@ func GenerateParcelable(
7373
return f.Bytes()
7474
}
7575

76+
// fieldTypeWithAnnotations returns a TypeSpecifier that includes both the
77+
// field-level and type-level annotations. In AIDL, annotations like
78+
// @utf8InCpp and @nullable are placed before the type in field declarations
79+
// but the parser attaches them to the FieldDecl, not the TypeSpecifier.
80+
// Merging them ensures marshalForType sees annotations like @utf8InCpp.
81+
func fieldTypeWithAnnotations(
82+
field *parser.FieldDecl,
83+
) *parser.TypeSpecifier {
84+
if len(field.Annots) == 0 {
85+
return field.Type
86+
}
87+
merged := *field.Type
88+
merged.Annots = append(append([]*parser.Annotation(nil), field.Annots...), field.Type.Annots...)
89+
return &merged
90+
}
91+
7692
// hasBinderField returns true if any field requires the binder import
7793
// (either an AIDL interface type or the IBinder primitive type).
7894
func hasBinderField(
@@ -137,31 +153,50 @@ func writeMarshalParcel(
137153
for _, field := range fields {
138154
goFieldName := AIDLToGoName(field.FieldName)
139155
fieldAccess := "s." + goFieldName
156+
// Merge field-level annotations (e.g. @utf8InCpp) into the type
157+
// so marshalForType sees them when choosing the write method.
158+
fieldType := fieldTypeWithAnnotations(field)
140159

141-
if field.Type.IsArray || field.Type.Name == "List" {
142-
writeArrayFieldToParcel(f, field.Type, fieldAccess, "p", opts, typeRef)
160+
if fieldType.IsArray || fieldType.Name == "List" {
161+
writeArrayFieldToParcel(f, fieldType, fieldAccess, "p", opts, typeRef)
143162
continue
144163
}
145164

146-
if field.Type.Name == "Map" {
147-
writeMapFieldToParcel(f, field.Type, fieldAccess, "p", opts, typeRef)
165+
if fieldType.Name == "Map" {
166+
writeMapFieldToParcel(f, fieldType, fieldAccess, "p", opts, typeRef)
148167
continue
149168
}
150169

151-
info := marshalForTypeWithCycleCheck(field.Type, opts, typeRef)
170+
info := marshalForTypeWithCycleCheck(fieldType, opts, typeRef)
152171
if info.WriteExpr == "" {
153172
continue
154173
}
155174

156-
writeExpr := fmt.Sprintf(info.WriteExpr, fieldAccess)
157-
writeExpr = strings.ReplaceAll(writeExpr, "_data", "p")
158-
159-
if strings.Contains(info.WriteExpr, ".MarshalParcel") {
160-
writeExpr = fmt.Sprintf("%s.MarshalParcel(p)", fieldAccess)
175+
// IBinder/interface fields need a nil guard: write a null binder
176+
// object when nil, otherwise use WriteStrongBinder with the handle.
177+
// MarshalParcel does not have ctx/transport, so WriteBinderToParcel
178+
// cannot be used here; local StubBinders must be pre-registered.
179+
switch {
180+
case info.IsIBinder:
181+
f.P("\tif %s == nil {", fieldAccess)
182+
f.P("\t\tp.WriteNullStrongBinder()")
183+
f.P("\t} else {")
184+
f.P("\t\tp.WriteStrongBinder(%s.Handle())", fieldAccess)
185+
f.P("\t}")
186+
case info.IsInterface:
187+
f.P("\tif %s == nil {", fieldAccess)
188+
f.P("\t\tp.WriteNullStrongBinder()")
189+
f.P("\t} else {")
190+
f.P("\t\tp.WriteStrongBinder(%s.AsBinder().Handle())", fieldAccess)
191+
f.P("\t}")
192+
case strings.Contains(info.WriteExpr, ".MarshalParcel"):
193+
writeExpr := fmt.Sprintf("%s.MarshalParcel(p)", fieldAccess)
161194
f.P("\tif _err := %s; _err != nil {", writeExpr)
162195
f.P("\t\treturn _err")
163196
f.P("\t}")
164-
} else {
197+
default:
198+
writeExpr := fmt.Sprintf(info.WriteExpr, fieldAccess)
199+
writeExpr = strings.ReplaceAll(writeExpr, "_data", "p")
165200
f.P("\t%s", writeExpr)
166201
}
167202
}
@@ -227,19 +262,22 @@ func writeUnmarshalParcel(
227262
arrayIdx := 0
228263
for _, field := range fields {
229264
goFieldName := AIDLToGoName(field.FieldName)
265+
// Merge field-level annotations (e.g. @utf8InCpp) into the type
266+
// so marshalForType sees them when choosing the read method.
267+
fieldType := fieldTypeWithAnnotations(field)
230268

231-
if field.Type.IsArray || field.Type.Name == "List" {
232-
readArrayFieldFromParcel(f, field.Type, "s."+goFieldName, "p", arrayIdx, opts, typeRef)
269+
if fieldType.IsArray || fieldType.Name == "List" {
270+
readArrayFieldFromParcel(f, fieldType, "s."+goFieldName, "p", arrayIdx, opts, typeRef)
233271
arrayIdx++
234272
continue
235273
}
236274

237-
if field.Type.Name == "Map" {
238-
readMapFieldFromParcel(f, field.Type, "s."+goFieldName, "p", opts, typeRef)
275+
if fieldType.Name == "Map" {
276+
readMapFieldFromParcel(f, fieldType, "s."+goFieldName, "p", opts, typeRef)
239277
continue
240278
}
241279

242-
info := marshalForTypeWithCycleCheck(field.Type, opts, typeRef)
280+
info := marshalForTypeWithCycleCheck(fieldType, opts, typeRef)
243281
if info.ReadExpr == "" {
244282
continue
245283
}

0 commit comments

Comments
 (0)