Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Code-Hex/vz segfaults when NewFileSerialPortAttachment returns an error #56

Closed
cfergeau opened this issue Oct 7, 2022 · 11 comments · Fixed by #71
Closed

Code-Hex/vz segfaults when NewFileSerialPortAttachment returns an error #56

cfergeau opened this issue Oct 7, 2022 · 11 comments · Fixed by #71

Comments

@cfergeau
Copy link
Contributor

cfergeau commented Oct 7, 2022

318c157 was probably fixing a similar issue.
cfergeau added a commit to cfergeau/vz that referenced this issue Oct 7, 2022
@cfergeau
Remove autoreleasepool from newVZFileSerialPortAttachment
1841875 
This fixes TestNonExistingFileSerialPortAttachment() which is
crashing before this commit:

% go test .
fatal error: unexpected signal during runtime execution
[signal SIGSEGV: segmentation violation code=0x2 addr=0xcf6cce7eb130 pc=0x19e33a5a0]

runtime stack:
runtime.throw({0x100ce0ded?, 0x100ccf234?})
	/usr/local/go/src/runtime/panic.go:1047 +0x40 fp=0x16f293700 sp=0x16f2936d0 pc=0x100ba2870
runtime.sigpanic()
	/usr/local/go/src/runtime/signal_unix.go:819 +0x1e4 fp=0x16f293730 sp=0x16f293700 pc=0x100bb9754

goroutine 19 [syscall]:
runtime.cgocall(0x100ccf214, 0x14000040d98)
	/usr/local/go/src/runtime/cgocall.go:158 +0x54 fp=0x14000040d60 sp=0x14000040d20 pc=0x100b6fbc4
github.com/Code-Hex/vz/v2._Cfunc_convertNSError2Flat(0x6000022cb120)
	_cgo_gotypes.go:344 +0x3c fp=0x14000040d90 sp=0x14000040d60 pc=0x100ccc11c
github.com/Code-Hex/vz/v2.newNSError.func1(0x1010c0a68?)
	/Users/teuf/dev/vz/objcutil.go:234 +0x48 fp=0x14000040df0 sp=0x14000040d90 pc=0x100cccd58
github.com/Code-Hex/vz/v2.newNSError(0x0?)
	/Users/teuf/dev/vz/objcutil.go:234 +0x30 fp=0x14000040e80 sp=0x14000040df0 pc=0x100cccbe0
github.com/Code-Hex/vz/v2.NewFileSerialPortAttachment({0x100cd9765?, 0x100be659c?}, 0x68?)
	/Users/teuf/dev/vz/console.go:92 +0x104 fp=0x14000040f10 sp=0x14000040e80 pc=0x100ccc6c4
github.com/Code-Hex/vz/v2.TestNonExistingFileSerialPortAttachment(0x0?)
	/Users/teuf/dev/vz/error_test.go:10 +0x30 fp=0x14000040f60 sp=0x14000040f10 pc=0x100ccbf10
testing.tRunner(0x14000107040, 0x100d81878)
	/usr/local/go/src/testing/testing.go:1446 +0x10c fp=0x14000040fb0 sp=0x14000040f60 pc=0x100c2d10c
testing.(*T).Run.func1()
	/usr/local/go/src/testing/testing.go:1493 +0x2c fp=0x14000040fd0 sp=0x14000040fb0 pc=0x100c2de4c
runtime.goexit()
	/usr/local/go/src/runtime/asm_arm64.s:1165 +0x4 fp=0x14000040fd0 sp=0x14000040fd0 pc=0x100bd4834
created by testing.(*T).Run
	/usr/local/go/src/testing/testing.go:1493 +0x300
[snip]

This fixes Code-Hex#56
@cfergeau cfergeau mentioned this issue Oct 7, 2022
Closed
cfergeau added a commit to cfergeau/vz that referenced this issue Oct 7, 2022
@cfergeau
Remove autoreleasepool from newVZFileSerialPortAttachment
21eab2b 
This fixes TestNonExistingFileSerialPortAttachment() which is
crashing before this commit:

```
% go test .
fatal error: unexpected signal during runtime execution
[signal SIGSEGV: segmentation violation code=0x2 addr=0xcf6cce7eb130 pc=0x19e33a5a0]

runtime stack:
runtime.throw({0x100ce0ded?, 0x100ccf234?})
	/usr/local/go/src/runtime/panic.go:1047 +0x40 fp=0x16f293700 sp=0x16f2936d0 pc=0x100ba2870
runtime.sigpanic()
	/usr/local/go/src/runtime/signal_unix.go:819 +0x1e4 fp=0x16f293730 sp=0x16f293700 pc=0x100bb9754

goroutine 19 [syscall]:
runtime.cgocall(0x100ccf214, 0x14000040d98)
	/usr/local/go/src/runtime/cgocall.go:158 +0x54 fp=0x14000040d60 sp=0x14000040d20 pc=0x100b6fbc4
github.com/Code-Hex/vz/v2._Cfunc_convertNSError2Flat(0x6000022cb120)
	_cgo_gotypes.go:344 +0x3c fp=0x14000040d90 sp=0x14000040d60 pc=0x100ccc11c
github.com/Code-Hex/vz/v2.newNSError.func1(0x1010c0a68?)
	/Users/teuf/dev/vz/objcutil.go:234 +0x48 fp=0x14000040df0 sp=0x14000040d90 pc=0x100cccd58
github.com/Code-Hex/vz/v2.newNSError(0x0?)
	/Users/teuf/dev/vz/objcutil.go:234 +0x30 fp=0x14000040e80 sp=0x14000040df0 pc=0x100cccbe0
github.com/Code-Hex/vz/v2.NewFileSerialPortAttachment({0x100cd9765?, 0x100be659c?}, 0x68?)
	/Users/teuf/dev/vz/console.go:92 +0x104 fp=0x14000040f10 sp=0x14000040e80 pc=0x100ccc6c4
github.com/Code-Hex/vz/v2.TestNonExistingFileSerialPortAttachment(0x0?)
	/Users/teuf/dev/vz/error_test.go:10 +0x30 fp=0x14000040f60 sp=0x14000040f10 pc=0x100ccbf10
testing.tRunner(0x14000107040, 0x100d81878)
	/usr/local/go/src/testing/testing.go:1446 +0x10c fp=0x14000040fb0 sp=0x14000040f60 pc=0x100c2d10c
testing.(*T).Run.func1()
	/usr/local/go/src/testing/testing.go:1493 +0x2c fp=0x14000040fd0 sp=0x14000040fb0 pc=0x100c2de4c
runtime.goexit()
	/usr/local/go/src/runtime/asm_arm64.s:1165 +0x4 fp=0x14000040fd0 sp=0x14000040fd0 pc=0x100bd4834
created by testing.(*T).Run
	/usr/local/go/src/testing/testing.go:1493 +0x300
[snip]
```

This fixes Code-Hex#56
@Code-Hex
Copy link
Owner

Code-Hex commented Oct 7, 2022

This problem should fix error handling instead of removing autorelease pool

@cfergeau
Copy link
Contributor Author

This commit is doing the same as 318c157
I don't know objective C, I don't know autoreleasepools, I cannot easily do more than this.

@Code-Hex
Copy link
Owner

@cfergeau So why you send PR? I can't believe

@cfergeau
Copy link
Contributor Author

I wrote a test case to reproduce the issue. I found commit 318c157 . Your commit is also removing autoreleasepool (but the commit log does not explain why). My commit fixes the test case, and is similar to an existing commit, so it seemed good for sending.

cfergeau added a commit to cfergeau/vz that referenced this issue Oct 10, 2022
@cfergeau
Remove autoreleasepool from newVZFileSerialPortAttachment
b378b24 
This fixes TestNonExistingFileSerialPortAttachment() which is
crashing before this commit:

```
% go test .
fatal error: unexpected signal during runtime execution
[signal SIGSEGV: segmentation violation code=0x2 addr=0xcf6cce7eb130 pc=0x19e33a5a0]

runtime stack:
runtime.throw({0x100ce0ded?, 0x100ccf234?})
	/usr/local/go/src/runtime/panic.go:1047 +0x40 fp=0x16f293700 sp=0x16f2936d0 pc=0x100ba2870
runtime.sigpanic()
	/usr/local/go/src/runtime/signal_unix.go:819 +0x1e4 fp=0x16f293730 sp=0x16f293700 pc=0x100bb9754

goroutine 19 [syscall]:
runtime.cgocall(0x100ccf214, 0x14000040d98)
	/usr/local/go/src/runtime/cgocall.go:158 +0x54 fp=0x14000040d60 sp=0x14000040d20 pc=0x100b6fbc4
github.com/Code-Hex/vz/v2._Cfunc_convertNSError2Flat(0x6000022cb120)
	_cgo_gotypes.go:344 +0x3c fp=0x14000040d90 sp=0x14000040d60 pc=0x100ccc11c
github.com/Code-Hex/vz/v2.newNSError.func1(0x1010c0a68?)
	/Users/teuf/dev/vz/objcutil.go:234 +0x48 fp=0x14000040df0 sp=0x14000040d90 pc=0x100cccd58
github.com/Code-Hex/vz/v2.newNSError(0x0?)
	/Users/teuf/dev/vz/objcutil.go:234 +0x30 fp=0x14000040e80 sp=0x14000040df0 pc=0x100cccbe0
github.com/Code-Hex/vz/v2.NewFileSerialPortAttachment({0x100cd9765?, 0x100be659c?}, 0x68?)
	/Users/teuf/dev/vz/console.go:92 +0x104 fp=0x14000040f10 sp=0x14000040e80 pc=0x100ccc6c4
github.com/Code-Hex/vz/v2.TestNonExistingFileSerialPortAttachment(0x0?)
	/Users/teuf/dev/vz/error_test.go:10 +0x30 fp=0x14000040f60 sp=0x14000040f10 pc=0x100ccbf10
testing.tRunner(0x14000107040, 0x100d81878)
	/usr/local/go/src/testing/testing.go:1446 +0x10c fp=0x14000040fb0 sp=0x14000040f60 pc=0x100c2d10c
testing.(*T).Run.func1()
	/usr/local/go/src/testing/testing.go:1493 +0x2c fp=0x14000040fd0 sp=0x14000040fb0 pc=0x100c2de4c
runtime.goexit()
	/usr/local/go/src/runtime/asm_arm64.s:1165 +0x4 fp=0x14000040fd0 sp=0x14000040fd0 pc=0x100bd4834
created by testing.(*T).Run
	/usr/local/go/src/testing/testing.go:1493 +0x300
[snip]
```

This fixes Code-Hex#56
@cfergeau
Copy link
Contributor Author

I propose a working patch to fix a crash in Code-Hex/vz master with a reproducer. You reject it, which is fine. I am not asking for teaching of objective-c and Go, but I expect feedback I can act on. Then I can provide a PR which will suit you. Your feedback so far in this issue has been too vague and non actionable for me, especially with my lack of familiarity with objective-c.
It is the first time in this issue you mention problems with the test case in this commit and its external dependencies. I can easily fix this, but I cannot guess it is not welcome!

@Code-Hex
Copy link
Owner

@cfergeau Test case welcome but no thank you third-party deps.

I think I should fix the my code

cfergeau added a commit to cfergeau/vz that referenced this issue Oct 10, 2022
@cfergeau
Retain error in newVZFileSerialPortAttachment
7470311 
If VZFileSerialPortAttachment:initWithURL returns an error, we need to
call `retain` on it, otherwise it will be freed when exiting from the
@autoreleasepool block.

This fixes TestNonExistingFileSerialPortAttachment() which is
crashing before this commit:

```
% go test .
fatal error: unexpected signal during runtime execution
[signal SIGSEGV: segmentation violation code=0x2 addr=0xcf6cce7eb130 pc=0x19e33a5a0]

runtime stack:
runtime.throw({0x100ce0ded?, 0x100ccf234?})
	/usr/local/go/src/runtime/panic.go:1047 +0x40 fp=0x16f293700 sp=0x16f2936d0 pc=0x100ba2870
runtime.sigpanic()
	/usr/local/go/src/runtime/signal_unix.go:819 +0x1e4 fp=0x16f293730 sp=0x16f293700 pc=0x100bb9754

goroutine 19 [syscall]:
runtime.cgocall(0x100ccf214, 0x14000040d98)
	/usr/local/go/src/runtime/cgocall.go:158 +0x54 fp=0x14000040d60 sp=0x14000040d20 pc=0x100b6fbc4
github.com/Code-Hex/vz/v2._Cfunc_convertNSError2Flat(0x6000022cb120)
	_cgo_gotypes.go:344 +0x3c fp=0x14000040d90 sp=0x14000040d60 pc=0x100ccc11c
github.com/Code-Hex/vz/v2.newNSError.func1(0x1010c0a68?)
	/Users/teuf/dev/vz/objcutil.go:234 +0x48 fp=0x14000040df0 sp=0x14000040d90 pc=0x100cccd58
github.com/Code-Hex/vz/v2.newNSError(0x0?)
	/Users/teuf/dev/vz/objcutil.go:234 +0x30 fp=0x14000040e80 sp=0x14000040df0 pc=0x100cccbe0
github.com/Code-Hex/vz/v2.NewFileSerialPortAttachment({0x100cd9765?, 0x100be659c?}, 0x68?)
	/Users/teuf/dev/vz/console.go:92 +0x104 fp=0x14000040f10 sp=0x14000040e80 pc=0x100ccc6c4
github.com/Code-Hex/vz/v2.TestNonExistingFileSerialPortAttachment(0x0?)
	/Users/teuf/dev/vz/error_test.go:10 +0x30 fp=0x14000040f60 sp=0x14000040f10 pc=0x100ccbf10
testing.tRunner(0x14000107040, 0x100d81878)
	/usr/local/go/src/testing/testing.go:1446 +0x10c fp=0x14000040fb0 sp=0x14000040f60 pc=0x100c2d10c
testing.(*T).Run.func1()
	/usr/local/go/src/testing/testing.go:1493 +0x2c fp=0x14000040fd0 sp=0x14000040fb0 pc=0x100c2de4c
runtime.goexit()
	/usr/local/go/src/runtime/asm_arm64.s:1165 +0x4 fp=0x14000040fd0 sp=0x14000040fd0 pc=0x100bd4834
created by testing.(*T).Run
	/usr/local/go/src/testing/testing.go:1493 +0x300
[snip]
```

This fixes Code-Hex#56
@Code-Hex
Copy link
Owner

@cfergeau Please stop sending your PR to me. I have a plan to commit to this project.
I have my own priorities for some tasks.
It is annoying to review code one by one and to leave comments, as it disrupts my priorities. If you can give me PR after these, you are more than welcome to do so. Just please wait a while for v3 to be released.

#59

@cfergeau cfergeau mentioned this issue Oct 10, 2022
Closed
@Code-Hex
Copy link
Owner

@cfergeau Not that it matters, but I don't know why my contribution to the repository is your business. It is annoying.
Especially, Your EM comments in particular make me uncomfortable.

@cfergeau
Copy link
Contributor Author

If you can give me PR after these, you are more than welcome to do so. Just please wait a while for v3 to be released.

Sorry, I filed PR #60 before seeing your message. You can ignore it until v3 is released!

@cfergeau cfergeau mentioned this issue Oct 12, 2022
Open
@Code-Hex
Copy link
Owner

Code-Hex commented Oct 13, 2022 •
edited
Loading

I checked this issue, and the NSString stringWithUTF8String: and NSURL fileURLWithPath: methods do not need to be freed respectively, so I tried to reopen #57 but the commit has been removed by your force-pushed.

If you want to send PR for this issue, I can accept your PR. Actually I want only this changes.

NSString *filePathNSString = [NSString stringWithUTF8String:filePath];
    NSURL *fileURL = [NSURL fileURLWithPath:filePathNSString];
    return [[VZFileSerialPortAttachment alloc]
        initWithURL:fileURL
             append:(BOOL)shouldAppend
              error:(NSError *_Nullable *_Nullable)error];

cfergeau added a commit to cfergeau/vz that referenced this issue Oct 13, 2022
@cfergeau
Remove autoreleasepool from newVZFileSerialPortAttachment
53f5cc8 
If we use autoreleasepool, the error returned from
VZFileSerialPortAttachment:initWithURL will be freed before
newVZFileSerialPortAttachment returns. We want the caller to be able to
use it.

This fixes TestNonExistingFileSerialPortAttachment() which is
crashing before this commit:

```
% go test .
fatal error: unexpected signal during runtime execution
[signal SIGSEGV: segmentation violation code=0x2 addr=0xcf6cce7eb130 pc=0x19e33a5a0]

runtime stack:
runtime.throw({0x100ce0ded?, 0x100ccf234?})
	/usr/local/go/src/runtime/panic.go:1047 +0x40 fp=0x16f293700 sp=0x16f2936d0 pc=0x100ba2870
runtime.sigpanic()
	/usr/local/go/src/runtime/signal_unix.go:819 +0x1e4 fp=0x16f293730 sp=0x16f293700 pc=0x100bb9754

goroutine 19 [syscall]:
runtime.cgocall(0x100ccf214, 0x14000040d98)
	/usr/local/go/src/runtime/cgocall.go:158 +0x54 fp=0x14000040d60 sp=0x14000040d20 pc=0x100b6fbc4
github.com/Code-Hex/vz/v2._Cfunc_convertNSError2Flat(0x6000022cb120)
	_cgo_gotypes.go:344 +0x3c fp=0x14000040d90 sp=0x14000040d60 pc=0x100ccc11c
github.com/Code-Hex/vz/v2.newNSError.func1(0x1010c0a68?)
	/Users/teuf/dev/vz/objcutil.go:234 +0x48 fp=0x14000040df0 sp=0x14000040d90 pc=0x100cccd58
github.com/Code-Hex/vz/v2.newNSError(0x0?)
	/Users/teuf/dev/vz/objcutil.go:234 +0x30 fp=0x14000040e80 sp=0x14000040df0 pc=0x100cccbe0
github.com/Code-Hex/vz/v2.NewFileSerialPortAttachment({0x100cd9765?, 0x100be659c?}, 0x68?)
	/Users/teuf/dev/vz/console.go:92 +0x104 fp=0x14000040f10 sp=0x14000040e80 pc=0x100ccc6c4
github.com/Code-Hex/vz/v2.TestNonExistingFileSerialPortAttachment(0x0?)
	/Users/teuf/dev/vz/error_test.go:10 +0x30 fp=0x14000040f60 sp=0x14000040f10 pc=0x100ccbf10
testing.tRunner(0x14000107040, 0x100d81878)
	/usr/local/go/src/testing/testing.go:1446 +0x10c fp=0x14000040fb0 sp=0x14000040f60 pc=0x100c2d10c
testing.(*T).Run.func1()
	/usr/local/go/src/testing/testing.go:1493 +0x2c fp=0x14000040fd0 sp=0x14000040fb0 pc=0x100c2de4c
runtime.goexit()
	/usr/local/go/src/runtime/asm_arm64.s:1165 +0x4 fp=0x14000040fd0 sp=0x14000040fd0 pc=0x100bd4834
created by testing.(*T).Run
	/usr/local/go/src/testing/testing.go:1493 +0x300
[snip]
```

This fixes Code-Hex#56
cfergeau added a commit to cfergeau/vz that referenced this issue Oct 13, 2022
@cfergeau
Remove autoreleasepool from newVZFileSerialPortAttachment
a64a222 
If we use autoreleasepool, the error returned from
VZFileSerialPortAttachment:initWithURL will be freed before
newVZFileSerialPortAttachment returns. We want the caller to be able to
use it.

This fixes TestNonExistingFileSerialPortAttachment() which is
crashing before this commit:

```
% go test .
fatal error: unexpected signal during runtime execution
[signal SIGSEGV: segmentation violation code=0x2 addr=0xcf6cce7eb130 pc=0x19e33a5a0]

runtime stack:
runtime.throw({0x100ce0ded?, 0x100ccf234?})
	/usr/local/go/src/runtime/panic.go:1047 +0x40 fp=0x16f293700 sp=0x16f2936d0 pc=0x100ba2870
runtime.sigpanic()
	/usr/local/go/src/runtime/signal_unix.go:819 +0x1e4 fp=0x16f293730 sp=0x16f293700 pc=0x100bb9754

goroutine 19 [syscall]:
runtime.cgocall(0x100ccf214, 0x14000040d98)
	/usr/local/go/src/runtime/cgocall.go:158 +0x54 fp=0x14000040d60 sp=0x14000040d20 pc=0x100b6fbc4
github.com/Code-Hex/vz/v2._Cfunc_convertNSError2Flat(0x6000022cb120)
	_cgo_gotypes.go:344 +0x3c fp=0x14000040d90 sp=0x14000040d60 pc=0x100ccc11c
github.com/Code-Hex/vz/v2.newNSError.func1(0x1010c0a68?)
	/Users/teuf/dev/vz/objcutil.go:234 +0x48 fp=0x14000040df0 sp=0x14000040d90 pc=0x100cccd58
github.com/Code-Hex/vz/v2.newNSError(0x0?)
	/Users/teuf/dev/vz/objcutil.go:234 +0x30 fp=0x14000040e80 sp=0x14000040df0 pc=0x100cccbe0
github.com/Code-Hex/vz/v2.NewFileSerialPortAttachment({0x100cd9765?, 0x100be659c?}, 0x68?)
	/Users/teuf/dev/vz/console.go:92 +0x104 fp=0x14000040f10 sp=0x14000040e80 pc=0x100ccc6c4
github.com/Code-Hex/vz/v2.TestNonExistingFileSerialPortAttachment(0x0?)
	/Users/teuf/dev/vz/error_test.go:10 +0x30 fp=0x14000040f60 sp=0x14000040f10 pc=0x100ccbf10
testing.tRunner(0x14000107040, 0x100d81878)
	/usr/local/go/src/testing/testing.go:1446 +0x10c fp=0x14000040fb0 sp=0x14000040f60 pc=0x100c2d10c
testing.(*T).Run.func1()
	/usr/local/go/src/testing/testing.go:1493 +0x2c fp=0x14000040fd0 sp=0x14000040fb0 pc=0x100c2de4c
runtime.goexit()
	/usr/local/go/src/runtime/asm_arm64.s:1165 +0x4 fp=0x14000040fd0 sp=0x14000040fd0 pc=0x100bd4834
created by testing.(*T).Run
	/usr/local/go/src/testing/testing.go:1493 +0x300
[snip]
```

This fixes Code-Hex#56
@cfergeau cfergeau mentioned this issue Oct 13, 2022
Merged
@cfergeau
Copy link
Contributor Author

Done in #71. I left error_test.go in the commit, but I can remove it.
Thanks for the link on memory management conventions, this is very useful and interesting!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
2 participants
@cfergeau @Code-Hex