libcam: NULL out freed `ccb.cdm.matches` and `ccb.cdm.patterns` pointers to avoid double frees

[ngie]

Summary

libcam: NULL out freed ccb.cdm.matches and ccb.cdm.patterns pointers
to avoid double frees

MFC after: 1 week
Sponsored by: Dell EMC Isilon

Test Plan

$ sudo ln -sfh 'abort:true,junk:true' /etc/malloc.conf                                                                                                                              
$ (cd lib/libcam/; make; sudo make install; cd tests; sudo make check)                                                                                                              
===> tests (all)
(cd /usr/src/lib/libcam/tests &&  DEPENDFILE=.depend.libcam_test  NO_SUBDIR=1 make -f /usr/src/lib/libcam/tests/Makefile _RECURSING_PROGS=t  PROG=libcam_test )
install  -C -o root -g wheel -m 444   libcam.a /usr/lib/
install  -C -o root -g wheel -m 444   libcam_p.a /usr/lib/
install  -s -o root -g wheel -m 444     libcam.so.7 /lib/
install  -o root -g wheel -m 444    libcam.so.7.debug /usr/lib/debug/lib/
install -l rs  /lib/libcam.so.7  /usr/lib/libcam.so
install  -C -o root -g wheel -m 444  /usr/src/lib/libcam/camlib.h /usr/include/
install  -o root -g wheel -m 444 cam.3.gz  /usr/share/man/man3/
install  -o root -g wheel -m 444 cam_cdbparse.3.gz  /usr/share/man/man3/
rm -f /usr/share/man/man3/cam_open_device.3 /usr/share/man/man3/cam_open_device.3.gz;  install -l h  /usr/share/man/man3/cam.3.gz /usr/share/man/man3/cam_open_device.3.gz
rm -f /usr/share/man/man3/cam_open_spec_device.3 /usr/share/man/man3/cam_open_spec_device.3.gz;  install -l h  /usr/share/man/man3/cam.3.gz /usr/share/man/man3/cam_open_spec_device.3.gz
rm -f /usr/share/man/man3/cam_open_btl.3 /usr/share/man/man3/cam_open_btl.3.gz;  install -l h  /usr/share/man/man3/cam.3.gz /usr/share/man/man3/cam_open_btl.3.gz
rm -f /usr/share/man/man3/cam_open_pass.3 /usr/share/man/man3/cam_open_pass.3.gz;  install -l h  /usr/share/man/man3/cam.3.gz /usr/share/man/man3/cam_open_pass.3.gz
rm -f /usr/share/man/man3/cam_close_device.3 /usr/share/man/man3/cam_close_device.3.gz;  install -l h  /usr/share/man/man3/cam.3.gz /usr/share/man/man3/cam_close_device.3.gz
rm -f /usr/share/man/man3/cam_close_spec_device.3 /usr/share/man/man3/cam_close_spec_device.3.gz;  install -l h  /usr/share/man/man3/cam.3.gz /usr/share/man/man3/cam_close_spec_device.3.gz
rm -f /usr/share/man/man3/cam_getccb.3 /usr/share/man/man3/cam_getccb.3.gz;  install -l h  /usr/share/man/man3/cam.3.gz /usr/share/man/man3/cam_getccb.3.gz
rm -f /usr/share/man/man3/cam_send_ccb.3 /usr/share/man/man3/cam_send_ccb.3.gz;  install -l h  /usr/share/man/man3/cam.3.gz /usr/share/man/man3/cam_send_ccb.3.gz
rm -f /usr/share/man/man3/cam_freeccb.3 /usr/share/man/man3/cam_freeccb.3.gz;  install -l h  /usr/share/man/man3/cam.3.gz /usr/share/man/man3/cam_freeccb.3.gz
rm -f /usr/share/man/man3/cam_path_string.3 /usr/share/man/man3/cam_path_string.3.gz;  install -l h  /usr/share/man/man3/cam.3.gz /usr/share/man/man3/cam_path_string.3.gz
rm -f /usr/share/man/man3/cam_device_dup.3 /usr/share/man/man3/cam_device_dup.3.gz;  install -l h  /usr/share/man/man3/cam.3.gz /usr/share/man/man3/cam_device_dup.3.gz
rm -f /usr/share/man/man3/cam_device_copy.3 /usr/share/man/man3/cam_device_copy.3.gz;  install -l h  /usr/share/man/man3/cam.3.gz /usr/share/man/man3/cam_device_copy.3.gz
rm -f /usr/share/man/man3/cam_get_device.3 /usr/share/man/man3/cam_get_device.3.gz;  install -l h  /usr/share/man/man3/cam.3.gz /usr/share/man/man3/cam_get_device.3.gz
rm -f /usr/share/man/man3/csio_build.3 /usr/share/man/man3/csio_build.3.gz;  install -l h  /usr/share/man/man3/cam_cdbparse.3.gz /usr/share/man/man3/csio_build.3.gz
rm -f /usr/share/man/man3/csio_build_visit.3 /usr/share/man/man3/csio_build_visit.3.gz;  install -l h  /usr/share/man/man3/cam_cdbparse.3.gz /usr/share/man/man3/csio_build_visit.3.gz
rm -f /usr/share/man/man3/csio_decode.3 /usr/share/man/man3/csio_decode.3.gz;  install -l h  /usr/share/man/man3/cam_cdbparse.3.gz /usr/share/man/man3/csio_decode.3.gz
rm -f /usr/share/man/man3/csio_decode_visit.3 /usr/share/man/man3/csio_decode_visit.3.gz;  install -l h  /usr/share/man/man3/cam_cdbparse.3.gz /usr/share/man/man3/csio_decode_visit.3.gz
rm -f /usr/share/man/man3/buff_decode.3 /usr/share/man/man3/buff_decode.3.gz;  install -l h  /usr/share/man/man3/cam_cdbparse.3.gz /usr/share/man/man3/buff_decode.3.gz
rm -f /usr/share/man/man3/buff_decode_visit.3 /usr/share/man/man3/buff_decode_visit.3.gz;  install -l h  /usr/share/man/man3/cam_cdbparse.3.gz /usr/share/man/man3/buff_decode_visit.3.gz
rm -f /usr/share/man/man3/csio_encode.3 /usr/share/man/man3/csio_encode.3.gz;  install -l h  /usr/share/man/man3/cam_cdbparse.3.gz /usr/share/man/man3/csio_encode.3.gz
rm -f /usr/share/man/man3/csio_encode_visit.3 /usr/share/man/man3/csio_encode_visit.3.gz;  install -l h  /usr/share/man/man3/cam_cdbparse.3.gz /usr/share/man/man3/csio_encode_visit.3.gz
rm -f /usr/share/man/man3/buff_encode_visit.3 /usr/share/man/man3/buff_encode_visit.3.gz;  install -l h  /usr/share/man/man3/cam_cdbparse.3.gz /usr/share/man/man3/buff_encode_visit.3.gz
===> tests (install)
install  -o root  -g wheel -m 444  Kyuafile  /usr/tests/lib/libcam/Kyuafile
(cd /usr/src/lib/libcam/tests &&  DEPENDFILE=.depend.libcam_test  NO_SUBDIR=1 make -f /usr/src/lib/libcam/tests/Makefile _RECURSING_PROGS=t  PROG=libcam_test  install)
install  -s -o root -g wheel -m 555   libcam_test /usr/tests/lib/libcam/libcam_test
install  -o root -g wheel -m 444  libcam_test.debug /usr/lib/debug/usr/tests/lib/libcam/libcam_test.debug
libcam_test:cam_close_device_negative_test_NULL  ->  passed  [0.003s]
libcam_test:cam_freeccb_negative_test_NULL  ->  passed  [0.003s]
libcam_test:cam_getccb_positive_test  ->  passed  [0.004s]
libcam_test:cam_open_device_negative_test_O_RDONLY  ->  passed  [0.003s]
libcam_test:cam_open_device_negative_test_nonexistent  ->  passed  [0.003s]
libcam_test:cam_open_device_negative_test_unprivileged  ->  passed  [0.003s]
libcam_test:cam_open_device_positive_test  ->  passed  [0.003s]

Results file id is usr_tests_lib_libcam.20170312-060125-145326
Results saved to /root/.kyua/store/results.usr_tests_lib_libcam.20170312-060125-145326.db

7/7 passed (0 failed)
$ sudo camcontrol devlist
<VMware, VMware Virtual S 1.0>     at scbus2 target 0 lun 0 (da0,pass0)
<VMware, Virtual CD-ROM 1.0>       at scbus2 target 1 lun 0 (pass1,cd0)
$

[imp]

I like setting persistent things to NULL after free, but setting local variables to NULL after free on the 'exit' path of the function is actually harmful: Either the compiler will generate code that wastes cycles in what could be a critical path, or it will optimize it away and maybe issue a warning.... Neither outcome is good, and there's no benefit to doing this.

lib/libcam/camlib.c

97 ↗

(On Diff #26172)

This is completely useless. ccb leaves scope the very next line. Some compilers will warn about writes that have no effect. Clang can't be far behind.

664 ↗

(On Diff #26172)

This one is also useless.

679 ↗

(On Diff #26172)

This is useless. dev leaves scope the very next line.

[ngie]

lib/libcam/camlib.c

97 ↗

(On Diff #26172)

cam_freeccb is a public API. Calling cam_freeccb on the same ccb twice will result in a double free. This is what I'm trying to prevent.

664 ↗

(On Diff #26172)

I disagree. This prevents a seemingly wild pointer from being returned via this function, which is used in cam_open_btl, cam_open_pass, and cam_open_spec_device

679 ↗

(On Diff #26172)

Again, cam_close_device is a public API. Calling it on the same dev twice will result in a double free. This is what I'm trying to prevent.

[ngie]

In D9970#206077, @imp wrote:

I like setting persistent things to NULL after free, but setting local variables to NULL after free on the 'exit' path of the function is actually harmful: Either the compiler will generate code that wastes cycles in what could be a critical path, or it will optimize it away and maybe issue a warning.... Neither outcome is good, and there's no benefit to doing this.

I forgot to reply to this portion of the comment: could you please provide an example of this? I know that running free/munmap on a large collection of memory pages can eat up a lot of time unnecessarily on exit, but I'm not quite sure I understand the negative effects you're mentioning above.

[imp]

In D9970#206301, @ngie wrote:

In D9970#206077, @imp wrote:

I like setting persistent things to NULL after free, but setting local variables to NULL after free on the 'exit' path of the function is actually harmful: Either the compiler will generate code that wastes cycles in what could be a critical path, or it will optimize it away and maybe issue a warning.... Neither outcome is good, and there's no benefit to doing this.

I forgot to reply to this portion of the comment: could you please provide an example of this? I know that running free/munmap on a large collection of memory pages can eat up a lot of time unnecessarily on exit, but I'm not quite sure I understand the negative effects you're mentioning above.

Right. The variables you set are local variables. This doesn't affect the caller. As such, setting them is totally useless. It's a write to a variable that's subsequently not used. Compilers will warn about that. If you were setting it in the caller somehow, I'd agree with you. But these are useless.

In other words

foo(char *ptr) { free(ptr); ptr=NULL);

bar(...)
{ char *p = stuff(); /*stuff */
foo(p);
/*here*/
foo(p); /*double free */
}

At /*here*/ p will still contain the return value of stuff() because ptr is local to foo(). setting it in foo won't prevent a subsequent foo() call from freeing it again.

So it can't possibly prevent anything. And compiler technology has started warning for this sort of thing (though maybe not this exact thing). See gcc's -Wunused-but-set-variable and -Wunused-but-set-parameter warnings.

lib/libcam/camlib.c

664 ↗

(On Diff #26172)

It won't prevent anything. Setting it to NULL doesn't affect the caller at all. And you exit the function a few lines later. There's no effect here what so ever. Since it won't affect the caller, it won't prevent a double free.

679 ↗

(On Diff #26172)

No it won't. Since dev is a local variable (a parameter), setting it to NULL here will not set it in the caller. Calling this twice will still result in a double free.

[ngie]

lib/libcam/camlib.c

97 ↗

(On Diff #26172)

Oh bah, you're right. Ugh, this API seems broken in that regard.

664 ↗

(On Diff #26172)

Oh bah, you're right... it might be better to make the parameter const so it's unambiguously read-only.

679 ↗

(On Diff #26172)

Oh bah, you're right. Ugh, this API seems broken.

[imp]

lib/libcam/camlib.c

664 ↗

(On Diff #26172)

Except if you mark it as const foo *, you'll generate -Wqual-cast warnings because you cast the const-ness away when you call free()...

[ngie]

lib/libcam/camlib.c

664 ↗

(On Diff #26172)

The part I wasn't explicit about in my previous statement is that the value would be copied on entry in to the function, with something like malloc+memcpy, but that seems to be a lot more work than it's worth for an internal function...

[ngie]

Remove useless variable NULLing after free for values.

As noted by imp, the values are local to the function and not pointers of
pointers, so NULLing them out after free is trivially wasteful and
obfuscating.

This is a reminder to me to pay attention to types more before just making
blanket changes like this.

[ngie]

Ping?

[imp]

Thanks for making the suggested changes...