Created attachment 114868 [details] test case Hello. The attached test-case demonstrates the problem. It creates 2 pages of shared memory, uninitializes one page and initializes another. Even though both pages contain valid values, valgrind doesn't understand shared memory and reports an error. I have the program that uses shared memory a lot, and gets hundreds of false errors from valgrind.
At my work, we are also using shared memory (shared between several processes). In the same process, the shared memory is mapped only once. In that setup, a piece of shared memory can be initialised by one process, but read by another process. So, for this, we are using a valgrind client request to declare the shared memory as initialised after attach. In your test case, the same memory is mapped twice (at different addresses) by the same process. This is an unusual usage pattern of shared memory. Is that representative of your real application ? If yes, then I think it will be quite unlikely this can be improved in valgrind: even if valgrind could maybe track the system calls to detect that the same piece of memory is in fact mapped twice, handling this in memcheck will very probably imply a specific data structure to be looked at for each access, and that will bring a very high cost on programs not using this unusual shared memory usage. So, in summary, it would be better to map the memory only once and/or declare it initialised after map.
Created attachment 114889 [details] 2-process test-case Hi, thanks for your reply. Here is the test-case that does the same thing but with 2 processes scheme. Same valgrind error. While the first test-case represents my real program more, I really see no difference. As for the difficulty of fixing this - yes, I perfectly understand it. While I can think of some ugly work-arounds (like, for example, checking if the values have changed, and consider them initialized in that case), nothing good can probably be done. How about downgrading the error to warning if it comes from the shared mem?
(In reply to Stas Sergeev from comment #2) > Created attachment 114889 [details] > 2-process test-case > > Hi, thanks for your reply. > Here is the test-case that does the same > thing but with 2 processes scheme. > Same valgrind error. > While the first test-case represents my real > program more, I really see no difference. For valgrind, there is no way a first valgrind process can track what another valgrind process does to the shared memory. In one single process, I think all would work ok if the memory/file would be mapped only once. It is not very clear to me what is the reason to map twice the same file. At my work, we are using shared memory between several processes. We just declare the shared memory as initialised just after mmap. This avoids false positive (and of course, introduces the risk of a false negative). > > As for the difficulty of fixing this - yes, > I perfectly understand it. While I can think > of some ugly work-arounds (like, for example, > checking if the values have changed, and consider > them initialized in that case), nothing good can > probably be done. How about downgrading the error > to warning if it comes from the shared mem? There is no concept of 'undefined error' and 'undefined warning', and introducing this distinction seems not too desirable (e.g. impact on suppression files, etc). So, if you really have to keep the multiple mmap in a single process, the best way to avoid false positive is to declare the memory as initialised after mmap using VALGRIND_MAKE_MEM_DEFINED or disable error reporting for the mmap-ed address range using VALGRIND_DISABLE_ADDR_ERROR_REPORTING_IN_RANGE
Created attachment 114906 [details] 2 processes and VALGRIND_MAKE_MEM_DEFINED (In reply to Philippe Waroquiers from comment #3) > It is not very clear to me what is the reason > to map twice the same file. Mapping file twice may not be needed, but mapping shared memory twice (which is what my test-case does) allows you to create the mem aliases with different access rights. Consider writing the JIT. I am sure you, as a valgrind developer, write JIT every day actually. :) To trap self-modified code, you would execute the JITted code in PROT_EXEC window (no PROT_WRITE), and the JIT itself will access it via PROT_READ|PROT_WRITE window (very rough picture, it functions differently of course, but multiple windows with different perms are needed). But I would suggest to forget this exotic case and assume I use 2 processes as my second test-case does. This is because I simply don't see any difference. If you explain me why 2 processes+shmem are safe, I guess I'll deal with the rest on my own. Since valgrind doesn't know how the actions on shmem in one process affects another, it just exactly the same way doesn't know how writes within a single process can affect another mem window in the same process. > At my work, we are using shared memory between > several processes. We just declare the shared memory > as initialised just after mmap. See my next test-case where I did exactly that. I don't think it was needed, as AFAIK mmap()ed memory is always zeroed and valgrind knows this already, but I did that to follow your suggestion. > There is no concept of 'undefined error' and 'undefined warning', > and introducing this distinction seems not too desirable > (e.g. impact on suppression files, etc). Then you should at least document the possible sources of false-positives, including this one. I spent many hours trying to understand what valgrind is up to. A very long route was passed to track things back to the shared memory use (--track-origins just points to the stack allocation that didn't give me any clue). If that would have been documented, I would immediately understand that I hit exactly that case. > or disable error reporting for the mmap-ed address > range using > VALGRIND_DISABLE_ADDR_ERROR_REPORTING_IN_RANGE Thanks. If nothing else, I can do that. But since I don't want to deal with valgrind instrumentation headers, I can as well add an extra initializers just to silence down the errors. So basically I _will_ find the ways to silence down the errors, but getting things improved (or at least documented) on valgrind side would be a good result after my many-hours research of those false-positives. :)
Think of the following use-case. Consider a very silly shm IPC that uses just one large struct with many input and output fields. Client fills in the output fields and copies the entire struct to the shm buffer. Server reads it, fills in the input fields and puts it back to shm. Client reads the struct back with all fields now properly initialized. valgrind in this scenario would still think some fields are uninitialized because it can't understand that server have already replaced the entire content in this buffer. This is what my test-case is trying to demonstrate. It doesn't matter whether the server is in the same process or another, as valgrind will behave the same way in both cases.
(In reply to Stas Sergeev from comment #5) > Think of the following use-case. > Consider a very silly shm IPC that > uses just one large struct with many > input and output fields. Client fills > in the output fields and copies the > entire struct to the shm buffer. Server > reads it, fills in the input fields > and puts it back to shm. Client reads > the struct back with all fields now > properly initialized. > valgrind in this scenario would still > think some fields are uninitialized > because it can't understand that server > have already replaced the entire content > in this buffer. Yes, indeed. That's why we have syscall and ioctl wrappers in Valgrind. They describe what the other side (kernel) expects from the buffers sent there and in what shape they come back with buffers received. It should be quite easy to extend this mechanism to an arbitrary client-server protocol, be it via shm, sockets or any other IPC.
(In reply to Stas Sergeev from comment #4) > Created attachment 114906 [details] > 2 processes and VALGRIND_MAKE_MEM_DEFINED > > (In reply to Philippe Waroquiers from comment #3) > > It is not very clear to me what is the reason > > to map twice the same file. > > Mapping file twice may not be needed, but > mapping shared memory twice (which is what > my test-case does) allows you to create the > mem aliases with different access rights. > Consider writing the JIT. I am sure you, > as a valgrind developer, write JIT every > day actually. :) To trap self-modified > code, you would execute the JITted code > in PROT_EXEC window (no PROT_WRITE), and > the JIT itself will access it via > PROT_READ|PROT_WRITE window (very rough > picture, it functions differently of course, > but multiple windows with different perms > are needed). Ok, yes that clarifies. > > But I would suggest to forget this exotic > case and assume I use 2 processes as my > second test-case does. This is because I > simply don't see any difference. If you explain > me why 2 processes+shmem are safe, I guess > I'll deal with the rest on my own. Since > valgrind doesn't know how the actions on > shmem in one process affects another, it > just exactly the same way doesn't know how > writes within a single process can affect > another mem window in the same process. Yes, for sure, valgrind cannot follow what another process does to shared memory. > > > At my work, we are using shared memory between > > several processes. We just declare the shared memory > > as initialised just after mmap. > > See my next test-case where I did exactly > that. I don't think it was needed, as AFAIK > mmap()ed memory is always zeroed and valgrind > knows this already, but I did that to follow > your suggestion. If mmap would necessarily zero out memory, then that would create major difficulties to use shared memory. I think that what the kernel guarantees is that the first process that creates new memory with mmap gets zero-ed memory (otherwise, that would be an info leak from the previous process that used this memory). Initialising yourself the memory via one ptr but accessing it via another mapping is not the same as declaring the memory defined after *each* mmap. In the first case, valgrind can just detect that the memory at ptr1 address is initialised, but it cannot see this initialisation via the second mapping. > > > There is no concept of 'undefined error' and 'undefined warning', > > and introducing this distinction seems not too desirable > > (e.g. impact on suppression files, etc). > > Then you should at least document the > possible sources of false-positives, including > this one. I spent many hours trying to > understand what valgrind is up to. A very > long route was passed to track things back > to the shared memory use (--track-origins > just points to the stack allocation that didn't > give me any clue). If that would have been > documented, I would immediately understand > that I hit exactly that case. Yes, that is a good idea. Do you have a suggestion about the best place to add this piece of information in the documentation ? > > > or disable error reporting for the mmap-ed address > > range using > > VALGRIND_DISABLE_ADDR_ERROR_REPORTING_IN_RANGE > > Thanks. If nothing else, I can do that. > But since I don't want to deal with valgrind > instrumentation headers, I can as well add > an extra initializers just to silence down > the errors. So basically I _will_ find the > ways to silence down the errors, but getting > things improved (or at least documented) on > valgrind side would be a good result after > my many-hours research of those false-positives. :) Yes, the doc must be good, in particular to document (surprising) limitations.
(In reply to Philippe Waroquiers from comment #7) > If mmap would necessarily zero out memory, then > that would create major difficulties to use > shared memory. > I think that what the kernel guarantees is that > the first process that creates new memory > with mmap gets zero-ed memory Yes but I don't think valgrind knows if this shm page is mapped first time or N-th time. It simply doesn't have such info, unless I am missing something obvious. So it should probably assume the memory is initialized. Or does it check if the mapping is MAP_SHARED and assumes uninitialized only in that case? In either case, my latest test-case does mmap() then VALGRIND_MAKE_MEM_DEFINED() then fork(). No second mapping any more. > Initialising yourself the memory via one ptr > but accessing it via another mapping > is not the same as declaring the memory > defined after *each* mmap. > In the first case, valgrind can just detect that > the memory at ptr1 address is initialised, > but it cannot see this initialisation via > the second mapping. Well, my last test-case does only one mmap() and then fork(). So even if I think hard I don't see how can I satisfy your current suggestion.
(In reply to Ivo Raisr from comment #6) > Yes, indeed. That's why we have syscall and ioctl wrappers in Valgrind. > They describe what the other side (kernel) expects from the buffers sent > there > and in what shape they come back with buffers received. > > It should be quite easy to extend this mechanism to an arbitrary > client-server protocol, be it via shm, sockets or any other IPC. I don't see a big use of it. Unlike the kernel ioctls, in this case users will have to create these protocol wrappers on their own for every prog. I don't think too many people would be interested in doing so. I think the mission of valgrind is to make it easier for the people to track the source of the problem. As in this case the source of the problem is valgrind itself, I think the adequate help for people would be the proper documentation. It will allow them to track the problems to valgrind instead of poking their own code in vain.
(In reply to Stas Sergeev from comment #8) > > Initialising yourself the memory via one ptr > > but accessing it via another mapping > > is not the same as declaring the memory > > defined after *each* mmap. > > In the first case, valgrind can just detect that > > the memory at ptr1 address is initialised, > > but it cannot see this initialisation via > > the second mapping. > > Well, my last test-case does only one mmap() > and then fork(). If you declare the memory initialised before forking, using VALGRIND_MAKE_MEM_DEFINED() both the parent and the child valgrind will consider it initialised. But in any case, as you suggested, we should improve the doc to mention the multi-mapping. Any suggestion where to put this information ? (or asked otherwise, where did you search in the doc ?) Thanks
(In reply to Philippe Waroquiers from comment #10) > If you declare the memory initialised before forking, > using VALGRIND_MAKE_MEM_DEFINED() ... which is what my latest test-case already does. Please suggest how to improve the test-case. > But in any case, as you suggested, we should > improve the doc to mention the multi-mapping. > > Any suggestion where to put this information ? > (or asked otherwise, where did you search in > the doc ?) In google, of course. :) "valgrind memcheck false positives" or "valgrind uninitialized false positives" etc. google will find it in a wiki or in off-line docs, so whatever place you prefer, is only a question of the valgrind-established practices. google will find it anywhere. Since valgrind knows that the error comes from the shared mem, it can print more description itself, referring to the docs. In this case the user will certainly not miss it.
Limitations about shared memory and false positive documented in ee5464ce3. Apart of documenting the limitations, there is not much more that can be done without significant additional complexity and slowness, so closing the bug on this basis.
Will it help to print an additional warning when the uninitialized mem is copied to the shared region? The chances are very high this will result in a false-positives later, so maybe catching it at that stage would be better? If such warning is printed, valgrind may just consider them initialized from now on, to avoid the false-positive error later. What do you think about such strategy?
(In reply to Stas Sergeev from comment #13) > Will it help to print an additional warning when > the uninitialized mem is copied to the shared region? > The chances are very high this will result in a false-positives > later, so maybe catching it at that stage would be better? > If such warning is printed, valgrind may just consider > them initialized from now on, to avoid the false-positive > error later. > What do you think about such strategy? Giving errors/warnings when uninitialised memory is copied will as far as I can see slow down valgrind (as it means that each copy operation must contain a check) and will cause many false positive. So, IMO, the easiest is to use the client requests (the downside of this is that this implies modification of the code).
(In reply to Philippe Waroquiers from comment #14) > So, IMO, the easiest is to use the client requests Which ones? I did all the modifications you asked for, they all in my latest test-case. > (the downside of this is that this implies modification of the code). No, that downside is that you didn't suggest any further modifications. :)
(In reply to Stas Sergeev from comment #15) > (In reply to Philippe Waroquiers from comment #14) > > So, IMO, the easiest is to use the client requests > > Which ones? > I did all the modifications you asked for, they > all in my latest test-case. > > > (the downside of this is that this implies modification of the code). > > No, that downside is that you didn't suggest > any further modifications. :) Here is the new part of the doc describing these false positives: <para>As explained above, Memcheck maintains 8 V bits for each byte in your process, including for bytes that are in shared memory. However, the same piece of shared memory can be mapped multiple times, by several processes or even by the same process (for example, if the process wants a read-only and a read-write mapping of the same page). For such multiple mappings, Memcheck tracks the V bits for each mapping independently. This can lead to false positive errors, as the shared memory can be initialised via a first mapping, and accessed via another mapping. The access via this other mapping will have its own V bits, which have not been changed when the memory was initialised via the first mapping. The bypass for these false positives is to use Memcheck's client requests <varname>VALGRIND_MAKE_MEM_DEFINED</varname> and <varname>VALGRIND_MAKE_MEM_UNDEFINED</varname> to inform Memcheck about what your program does (or what another process does) to these shared memory mappings. Alternatively, you can also use <varname>VALGRIND_DISABLE_ADDR_ERROR_REPORTING_IN_RANGE</varname>. </para> So, the easiest is to use VALGRIND_DISABLE_ADDR_ERROR_REPORTING_IN_RANGE. If you still want to have proper error detection, then you must ensure that whenever your program initialises (or uninitialises) a byte, that the equivalent V bits are modified the same way via VALGRIND_MAKE_MEM_*DEFINED in the other(s) mapping. For example, if you have a read mapping and a write mapping of a page, after each modification via the "write" mapping, you have to 'copy' the V bits from the write mapping to the read mapping. That might not be straightforward to do, in particular if you have multiple threads (you probably need a critical section around the write operations) or processes (this will even be more complex, as you will probably need a side channel to inform all other processes about what was done). This logic is of course application dependent.
(In reply to Philippe Waroquiers from comment #16) > That might not be straightforward to do, in particular if you have > multiple threads (you probably need a critical section around the > write operations) or processes (this will even be more complex, as you > will probably need a side channel to inform all other processes about what > was done). Exactly, that's the point. You need a side-channel that also knows about the relationship between the pointers to shm in different processes - something only the kernel knows about normally (a bit simpler if shm is mapped before fork() - then the pointers are equal, but this is not always the case). Thant's why I think VALGRIND_MAKE_MEM_DEFINED doesn't actually work for this case, and yet you suggested it all along the way here (and I was updating the test-case) and also in the docs. So I think there is some mistake. This all seems to suggest it might be better to enable the errors from shm with some extra command-line switch, leaving it disabled by default...
(In reply to Stas Sergeev from comment #17) > (In reply to Philippe Waroquiers from comment #16) > > That might not be straightforward to do, in particular if you have > > multiple threads (you probably need a critical section around the > > write operations) or processes (this will even be more complex, as you > > will probably need a side channel to inform all other processes about what > > was done). > Exactly, that's the point. > You need a side-channel that also knows > about the relationship between the pointers > to shm in different processes - something > only the kernel knows about normally (a bit > simpler if shm is mapped before fork() - then > the pointers are equal, but this is not always > the case). > Thant's why I think VALGRIND_MAKE_MEM_DEFINED > doesn't actually work for this case, and yet > you suggested it all along the way here (and > I was updating the test-case) and also in the > docs. The above is exactly what we do with shared memory at my day job, so, it works in at least one case :). But depending on the application, it might be (very) difficult to be precise. Then just be less precise: e.g. before each access to shared memory, declare it initialised. That will for sure work. > So I think there is some mistake. > This all seems to suggest it might be better > to enable the errors from shm with some extra > command-line switch, leaving it disabled by > default... When a 'usage of undefined' bit is detected, it is not known anymore that this undefined value came from (or through) shared memory, so it is not clear to me how we could implement this, and if we could, what would be the impact on the performance. At best, it would mean to do something like --track-origin=yes and have 'shared memory' origin having priority over other origins. Which mean that we would lose the real origin of a bug in case it is a real bug not originating from shared memory. Sorry I cannot help you more on that, I would have liked to propose something better.
(In reply to Philippe Waroquiers from comment #18) > When a 'usage of undefined' bit is detected, > it is not known anymore that this undefined > value came from (or through) shared memory, Which made me think that VALGRIND_DISABLE_ADDR_ERROR_REPORTING_IN_RANGE is in fact useless, so I removed this from the doc, Always good to have enthusiastic discussions :). So currently, this then only leaves us with MAKE_MEM_DEFINED approach (in the worst case, declare your shared memory initialised before each access).