Page MenuHomeFreeBSD

In ypbind, eliminate error: dereferencing type-punned pointer will break strict-aliasing rules
ClosedPublic

Authored by araujo on May 31 2015, 8:20 AM.
Tags
None
Referenced Files
Unknown Object (File)
Tue, Dec 3, 5:54 AM
Unknown Object (File)
Tue, Dec 3, 3:34 AM
Unknown Object (File)
Mon, Dec 2, 9:17 AM
Unknown Object (File)
Wed, Nov 20, 5:11 AM
Unknown Object (File)
Mon, Nov 11, 3:32 AM
Unknown Object (File)
Mon, Nov 11, 12:51 AM
Unknown Object (File)
Fri, Nov 8, 12:56 AM
Unknown Object (File)
Nov 1 2024, 8:09 PM

Details

Summary

Without this fix, compiling ypbind with gcc 4.9 will give warnings such as this:

/opt2/branches/head/usr.sbin/ypbind/ypbind.c: In function 'rpc_received':
/opt2/branches/head/usr.sbin/ypbind/ypbind.c:948:2: warning: dereferencing type-punned pointer will break strict-aliasing rules [-Wstrict-aliasing]
  *(u_int32_t *)&ybr.ypbind_resp_u.ypbind_bindinfo.ypbind_binding_addr = raddrp->sin_addr.s_addr;
  ^

Diff Detail

Repository
rS FreeBSD src repository - subversion
Lint
Lint Passed
Unit
No Test Coverage

Event Timeline

rodrigc retitled this revision from to In ypbind, eliminate error: dereferencing type-punned pointer will break strict-aliasing rules.
rodrigc updated this object.
rodrigc edited the test plan for this revision. (Show Details)
rodrigc added reviewers: araujo, bapt, emaste, dim.
rodrigc set the repository for this revision to rS FreeBSD src repository - subversion.
rodrigc added a subscriber: Unknown Object (MLST).

Hi Craig,

Could you take a look on this patch too? https://people.freebsd.org/~araujo/ypbind.patch
I already integrated your changes from this revision.

Maybe we should consider to sync it with the current OpenBSD version, although I'm not familiar with ypbind, maybe it is possible.
http://cvsweb.openbsd.org/cgi-bin/cvsweb/src/usr.sbin/ypbind/ypbind.c?annotate=1.63

Best,

usr.sbin/ypbind/ypbind.c
951–954

Missing an & here?

Sorry for the late response. Please address @dim 's comment, if you have not
addressed it already.

Also, removing the type-alias punned warnings is trickier than the other types
of fixes that we have done to clean up the code.
For this change, I would feel more comfortable if we actually tested ypbind with this code change.

I know, this is a headcahe. :(
But still, we have to do the right thing.

Regarding importing ypbind from OpenBSD. I think this is a good idea, because it looks like they more actively maintain the code.
However, this code in FreeBSD is *old*....almost 17 years old. Even if we import from OpenBSD, I don't know if
there are changes to functionality, command-line options, etc. that will be introduced.
This will require testing also.

It depends on how much interest you have in ypbind. :)
I don't have a lot of interest, and was just looking at doing minimal code changes to fix compiler warnings.

Let me know what approach you want to take. I will support you.

Sorry for the late response. Please address @dim 's comment, if you have not
addressed it already.

Yes, I have addressed @dim's comment already.

Also, removing the type-alias punned warnings is trickier than the other types
of fixes that we have done to clean up the code.
For this change, I would feel more comfortable if we actually tested ypbind with this code change.

I know, this is a headcahe. :(
But still, we have to do the right thing.

I would prefer make it in two steps, first fix the CLANG issues with this patch, I will make the tests and report to you as soon as I get the results. You are right and we must do it.

Regarding importing ypbind from OpenBSD. I think this is a good idea, because it looks like they more actively maintain the code.
However, this code in FreeBSD is *old*....almost 17 years old. Even if we import from OpenBSD, I don't know if
there are changes to functionality, command-line options, etc. that will be introduced.
This will require testing also.

It depends on how much interest you have in ypbind. :)
I don't have a lot of interest, and was just looking at doing minimal code changes to fix compiler warnings.

Let me know what approach you want to take. I will support you.

I don't have too many interest on ypbind, but I do believe to sync it with OpenBSD won't be that hard, but will cost time. I will do it in a second time, first I will check what are the changes and the impact as we are using a very old ypbind.

Thanks for the support, very appreciated, I will start it soon.

araujo edited reviewers, added: rodrigc; removed: araujo.

I will take it to be able to update the patch that I'm gonna start to test as dicussed with @rodrigc.

Update @rodrigc's patch with dim's review and with my changes too.
I'm gonna start to test it today!

usr.sbin/ypbind/ypbind.c
622

While we are here, can we please update this to a post-K&R definition? E.g.:

static bool_t broadcast_result(caddr_t out, struct sockaddr_in *addr)
dim edited edge metadata.

For the rest, LGTM.

This revision is now accepted and ready to land.Jul 5 2015, 11:00 AM
araujo edited edge metadata.

Address dim@ comment related with the function: broadcast_result().
Also I setup my machine following this page: https://www.freebsd.org/doc/handbook/network-nis.html

Seems everything works fine.

Result:
root@srcCODE:~ # ypcat passwd
smmsp:*:25:25:Sendmail Submission User:/var/spool/clientmqueue:/usr/sbin/nologin
hast:*:845:845:HAST unprivileged user:/var/empty:/usr/sbin/nologin
bin:*:3:7:Binaries Commands and Source:/:/usr/sbin/nologin
tty:*:4:65533:Tty Sandbox:/:/usr/sbin/nologin
kmem:*:5:65533:KMem Sandbox:/:/usr/sbin/nologin
sshd:*:22:22:Secure Shell Daemon:/var/empty:/usr/sbin/nologin
proxy:*:62:62:Packet Filter pseudo-user:/nonexistent:/usr/sbin/nologin
unbound:*:59:59:Unbound DNS Resolver:/var/unbound:/usr/sbin/nologin
_pflogd:*:64:64:pflogd privsep user:/var/empty:/usr/sbin/nologin
nobody:*:65534:65534:Unprivileged user:/nonexistent:/usr/sbin/nologin
root:*:0:0:Charlie &:/root:/bin/csh
toor:*:0:0:Bourne-again Superuser:/root:
daemon:*:1:1:Owner of many system processes:/root:/usr/sbin/nologin
operator:*:2:5:System &:/:/usr/sbin/nologin
man:*:9:9:Mister Man Pages:/usr/share/man:/usr/sbin/nologin
games:*:7:13:Games pseudo-user:/usr/games:/usr/sbin/nologin
news:*:8:8:News Subsystem:/:/usr/sbin/nologin
bind:*:53:53:Bind Sandbox:/:/usr/sbin/nologin
uucp:*:66:66:UUCP pseudo-user:/var/spool/uucppublic:/usr/local/libexec/uucp/uucico
www:*:80:80:World Wide Web Owner:/nonexistent:/usr/sbin/nologin
mailnull:*:26:26:Sendmail Default User:/var/spool/mqueue:/usr/sbin/nologin
_dhcp:*:65:65:dhcp programs:/var/empty:/usr/sbin/nologin
pop:*:68:6:Post Office Owner:/nonexistent:/usr/sbin/nologin
auditdistd:*:78:77:Auditdistd unprivileged user:/var/empty:/usr/sbin/nologin
git_daemon:*:964:964:git daemon:/nonexistent:/usr/sbin/nologin

This revision now requires review to proceed.Jul 12 2015, 2:13 PM
araujo edited edge metadata.

Minor change to respect style(9).

rodrigc edited edge metadata.
This revision is now accepted and ready to land.Jul 14 2015, 5:30 PM