Summary: | drd (and also helgrind) crashing on chmod with invalid parameter (with patch) | ||
---|---|---|---|
Product: | [Developer tools] valgrind | Reporter: | Philippe Waroquiers <philippe.waroquiers> |
Component: | drd | Assignee: | Bart Van Assche <bart.vanassche+kde> |
Status: | RESOLVED FIXED | ||
Severity: | crash | CC: | jseward, philippe.waroquiers |
Priority: | NOR | ||
Version: | 3.6.0 | ||
Target Milestone: | --- | ||
Platform: | Unlisted Binaries | ||
OS: | Linux | ||
Latest Commit: | Version Fixed In: |
Description
Philippe Waroquiers
2010-10-23 00:21:55 UTC
I can reproduce the reported behavior. But I'm not sure it is the purpose of Helgrind or DRD to report pointer errors instead of crashing on them - reporting pointer errors is the purpose of memcheck. Furthermore, in the manuals of both Helgrind and DRD it is recommended to make a program memcheck-clean before starting to analyze it with Helgrind or DRD. Is it OK for you to close this bug report ? (In reply to comment #1) > I can reproduce the reported behavior. But I'm not sure it is the purpose of > Helgrind or DRD to report pointer errors instead of crashing on them - > reporting pointer errors is the purpose of memcheck. Furthermore, in the > manuals of both Helgrind and DRD it is recommended to make a program > memcheck-clean before starting to analyze it with Helgrind or DRD. Is it OK for > you to close this bug report ? This error happens in a library for which we do not have the sources => can't fix it. memcheck does not crash on such an error, so we can put it in the memcheck ignore list, but cannot tell drd/helgrind to ignore it, as they both crash. So, to still allow drd/helgrind to be used, it looks better to either ignore such unaddressable bytes (with a msg maybe: the patch I did does that) or alternatively by defining a new kind of drd/helgrind errors (which would tell something like: "unaddressable byte(s) accessed, use memcheck for chasing this error") With this, drd/helgrind would be more usable (and would not change the behaviour of system calls). Philippe, the use of VKI_PROT_NONE in the patch is not quite right. It allows the use of VG_(strlen) even if the area is mapped but does not have read permissions (eg, --- or -x-). Can you test your patch with VKI_PROT_READ instead of VKI_PROT_NONE on your big application? For the small test case, VKI_PROT_READ works OK. Modified version of the patch committed as r11533, for both Helgrind and DRD, on the assumption that VKI_PROT_READ is OK -- shout if not! Thanks for looking into this. (In reply to comment #4) > Modified version of the patch committed as r11533, for both Helgrind > and DRD, on the assumption that VKI_PROT_READ is OK -- shout if not! > Thanks for looking into this. I am quite sure the patch will avoid the problem on the big executable (but I will check in any case) Two comments however: 1. if SHOW_EVENTS is set to >= 1, then I believe the trace will crash. 2. if a page is only PROT_WRITE or PROT_EXEC, posix standard tells the implementation may (implicitely) give read access. I think linux behaves like that (e.g. PROT_WRITE implicitely gives read access, but without having PROT_READ in the protection bits). So, a "fully correct" check implies to check for READ or (if the kernel gives implicit read) for WRITE or for EXEC. Using VKI_PROT_NONE is on the safe side for finding race condition, but is on the unsafe side for crashes: VG still crashes if the protection is only PROT_NONE. Using VKI_PROT_READ is on the safe side to avoid VG crashes, but might cause helgrind/drd to "not see" some reads of a PROT_WRITE or PROT_EXEC. At the end, the VKI_PROT_READ looks more readable/understandable than the VKI_PROT_NONE check, and avoids VG crashes. And probably no one will bitterly complain about a false negative in such a bizarre PROT_WRITE or PROT_EXEC only case. There might however be cases (or other tools) where the above subtility is more important, so this is why I give this (long) comment that you can ignore :). Thanks for this fix and for the threaded fork fix. Philippe (In reply to comment #5) > Two comments however: > 1. if SHOW_EVENTS is set to >= 1, then I believe the trace will crash. At second reading, this belief is wrong: it is the pointer a which is traced, not the string. So, an invalid pointer will not cause a crash in the trace. |