This is an archive of the discontinued LLVM Phabricator instance.

Custom wrappers for DFSanitizing sprintf & snprintf
ClosedPublic

Authored by martignlo on Oct 1 2014, 6:41 AM.

Details

Summary

Introduce custom wrappers for sprintf & snprintf to propagate the labels of the arguments to the output string.

Diff Detail

Event Timeline

martignlo updated this revision to Diff 14282.Oct 1 2014, 6:41 AM
martignlo retitled this revision from to Custom wrappers for DFSanitizing sprintf & snprintf.
martignlo updated this object.
martignlo edited the test plan for this revision. (Show Details)
martignlo added a reviewer: pcc.
martignlo added a subscriber: Unknown Object (MLST).
kcc added a subscriber: kcc.Oct 1 2014, 9:04 AM

Note that internal_vsnprintf does not implement the full spec of vsnprintf

pcc edited edge metadata.Oct 1 2014, 1:42 PM

Note that internal_vsnprintf does not implement the full spec of vsnprintf

Looks like a naming conflict -- the code does end up calling the libc vsnprintf. I've asked for the name to be changed to avoid this confusion.

lib/dfsan/dfsan_custom.cc
878

It is confusing to give this function the same name as an existing function in sanitizer_common.

909

Likewise, this should probably have a different name.

1101

This can just be va_arg(ap, dfsan_label *) as pointer types are not affected by default argument promotion.

test/dfsan/custom.c
777

Please add a test for each occurrence of the PRINTF macro in the implementation.

martignlo updated this revision to Diff 14325.Oct 2 2014, 7:07 AM
martignlo edited edge metadata.
martignlo added inline comments.Oct 2 2014, 7:10 AM
lib/dfsan/dfsan_custom.cc
878

I realize the name was not optimal but I did not realize the collision. Renamed.

909

Done

1101

Done

test/dfsan/custom.c
777

Done

pcc added inline comments.Oct 2 2014, 6:25 PM
lib/dfsan/dfsan_custom.cc
916

Can you use InternalMmapVector here?

test/dfsan/custom.c
805

Please modify this macro to check labels (in both the unlabelled and labelled cases). You already have a test for %s above so you don't need to check it here.

Though I would much prefer if this could be done without macros. Maybe we can move these tests to a .cc file and use templates?

martignlo updated this revision to Diff 14455.Oct 6 2014, 8:32 AM
martignlo updated this revision to Diff 14456.Oct 6 2014, 8:37 AM
martignlo added inline comments.Oct 6 2014, 8:41 AM
lib/dfsan/dfsan_custom.cc
916

Done.

test/dfsan/custom.c
805

I renamed the file to custom.cc so that we can use a template. Now the tests also labels the arguments and makes sure the labels are applied to the correct part of the output buffer.

Do you think it is necessary to try the same passing a zero-label?

martignlo updated this revision to Diff 14459.Oct 6 2014, 9:22 AM
martignlo added inline comments.
test/dfsan/custom.c
805

Ok. Now each string is formatted twice, with and without a labelled argument.

pcc added inline comments.Oct 6 2014, 1:08 PM
lib/dfsan/dfsan_custom.cc
911

InternalMmapVector<Chunk> should be simpler?

martignlo updated this revision to Diff 14494.Oct 7 2014, 3:42 AM
martignlo added inline comments.
lib/dfsan/dfsan_custom.cc
911

Done

pcc accepted this revision.Oct 7 2014, 10:39 AM
pcc edited edge metadata.

LGTM

This revision is now accepted and ready to land.Oct 7 2014, 10:39 AM
martignlo closed this revision.Oct 8 2014, 3:12 AM
martignlo updated this revision to Diff 14553.

Closed by commit rL219293 (authored by @martignlo).