Fix reading scap files with PPM_ENABLE_SENTINEL enabled #1259
Conversation
| readsize = gzread(f, handle->m_file_evt_buf, readlen); | ||
| // read a ppm_scap_evt_hdr struct leaving some space at the beginning of the buffer | ||
| // so that we can cast the whole buffer as ppm_evt_hdr | ||
| readsize = gzread(f, handle->m_file_evt_buf + sizeof(struct ppm_evt_hdr) - sizeof(struct ppm_scap_evt_hdr), readlen); |
There was a problem hiding this comment.
If you really leave 4 bytes at the beginning because the sentinel is enabled, shouldn't you modify also, e.g., line 2779? Otherwise aren't you reading garbage instead of the CPUID? If I'm right, same for the other usages of handle->m_file_evt_buf.
There was a problem hiding this comment.
Yeah, this is reading junk into cpuid and handle->m_last_evt_dump_flags (they don't change their position regardless of the sentinel). The event should be read properly (with junk in the sentinel but that's another story).
| readsize = gzread(f, handle->m_file_evt_buf, readlen); | ||
| // read a ppm_scap_evt_hdr struct leaving some space at the beginning of the buffer | ||
| // so that we can cast the whole buffer as ppm_evt_hdr | ||
| readsize = gzread(f, handle->m_file_evt_buf + sizeof(struct ppm_evt_hdr) - sizeof(struct ppm_scap_evt_hdr), readlen); |
There was a problem hiding this comment.
There is a (very remote) possibility of a buffer overrun because you're starting at an offset into the buffer. We ensure above that readlen <= FILE_READ_BUF_SIZE, but if readlen + (sizeof(struct ppm_evt_hdr) - sizeof(struct ppm_scap_evt_hdr)) > FILE_READ_BUF_SIZE then we could potentially walk off the end of the buffer.
| /// it must be identical to ppm_evt_hdr. Otherwise, it must match the *end* | ||
| /// of ppm_evt_hdr (scap files don't have the sentinel at the beginning of the struct) | ||
| /// see scap_next_offline() in scap_savefile.c | ||
| struct ppm_scap_evt_hdr { |
There was a problem hiding this comment.
I worry about diversion between this struct and ppm_evt_hdr. I suggest adding some compile-time checks to make sure everything's done properly:
#ifdef PPM_ENABLE_SENTINEL
#define PPM_STRUCT_DIFF sizeof(uint32_t)
#else
#define PPM_STRUCT_DIFF 0
#endif
#if sizeof(ppm_scap_evt_hdr) - sizeof(ppm_evt_hdr) != PPM_STRUCT_DIFF
#error Mismatch between ppm_scap_evt_hdr and ppm_evt_hdr
#endif
I think something like that will work
There was a problem hiding this comment.
I have the same concern as @nathan-b . Another way to avoid this would be to directly populate ppm_evt_hdr with ppm_scap_evt_hdr as a member and get rid of lines 1418-1422. But that will require changes to all files where ppm_evt_hdr is used.
|
This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. |
scap files don't have the sentinel stored regardless of PPM_ENABLE_SENTINEL being defined or not. This leads to scap files being loaded incorrectly when
#define PPM_ENABLE_SENTINEL