Initialize pthread_t: CID 1375950
ClosedPublic
Actions

Authored by araujo on Jun 12 2017, 6:42 AM.

Details

Reviewers

cem
grehan

Commits

rS319995: Check if pthread_create(3) successfully created the thread prior to call

Summary

Initialize pthread according to CID 1375950.

Diff Detail

Repository

rS FreeBSD src repository - subversion

Lint

Lint Passed

Unit

No Test Coverage

Build Status

Buildable 9810
Build 10245: arc lint + arc unit

Event Timeline

araujo created this revision.Jun 12 2017, 6:42 AM

Herald added a subscriber: imp. · View Herald TranscriptJun 12 2017, 6:42 AM

Harbormaster completed remote builds in B9808: Diff 29472.Jun 12 2017, 6:42 AM

I don't think it's valid to pthread_join() a bogus pthread_t value, which isn't even necessarily a scalar type. I'd just use a new bool value, set it to true when the thread is created, and check before joining the tid value.

Check if pthread_create(3) returns 0.

cem added inline comments.Jun 12 2017, 4:03 PM

usr.sbin/bhyve/rfb.c
768	I'd drop this change — zero isn't necessarily a value that can be assigned to pthread_t.
919–920	This isn't quite right. In the error case pointed out in the email, `error` will be zero due to its initialization at the top of the function, but there will be no thread to join.

araujo added inline comments.Jun 13 2017, 2:08 AM

usr.sbin/bhyve/rfb.c
768	libzfs use it in the same way: cddl/contrib/opensolaris/lib/libzfs/common/libzfs_sendrecv.c: pthread_t tid = 0;
919–920	pthread_create must return 0 if created successfully and the only goto done place is at the end of the switch case, error will be set during that time.

cem added inline comments.Jun 13 2017, 2:16 AM

usr.sbin/bhyve/rfb.c
768	Then libzfs is also wrong.
864	I think you are forgetting about this `goto done;`.
919–920	See the other `goto done` pointed out above.

araujo added inline comments.Jun 13 2017, 2:23 AM

usr.sbin/bhyve/rfb.c
919–920	Yes, you are right! I overlooked that.

Rewrite the attempt to fix CID 1375950.
I made couple tests using 'procstat -Ht <pid>' and also clang
static analisys didn't get anything else related with
the pthread_create(3).

Harbormaster completed remote builds in B9895: Diff 29639.Jun 15 2017, 5:59 AM

This addresses the Coverity warning, thanks! I'm not sure pthread_create failure is actually handled correctly, though.

usr.sbin/bhyve/rfb.c
883–886	Does it make sense to proceed here if pthread_create failed? Maybe instead: if (perror != 0) goto done; pthread_set_name_np(...);

This revision is now accepted and ready to land.Jun 15 2017, 11:42 PM

araujo added inline comments.Jun 16 2017, 1:09 AM

usr.sbin/bhyve/rfb.c
883–886	Yes, I saw that too, but I'm not sure if pthread_create fail the best is go to 'done', I need to make some tests and maybe handle the pthread_create code error in a proper way. @cem thank you again to point out all those issues and to make the review.