Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

add check for nulled list #1528

Open
wants to merge 1 commit into
base: dev
Choose a base branch
from
Open

add check for nulled list #1528

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 6 additions & 2 deletions panda/include/panda/callbacks/cb-helper-impl.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,9 @@ PANDAENDCOMMENT */
void HELPER(panda_insn_exec)(target_ulong pc) {
// PANDA instrumentation: before basic block
panda_cb_list *plist;
for(plist = panda_cbs[PANDA_CB_INSN_EXEC]; plist != NULL; plist = panda_cb_list_next(plist)) {
for(plist = panda_cbs[PANDA_CB_INSN_EXEC];
(plist != NULL && panda_cbs[PANDA_CB_INSN_EXEC] != NULL) ;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From my reading of this, it seems like some of these changes might not be meaningful (though I'm sure there's something in this PR that's fixing the underlying bug)

Since plist is set to panda_cbs[PANDA_CB_INSN_EXEC] I don't see how plist could ever be NULL while the panda_cbs[PANDA_CB_INSN_EXEC] object is non-NULL - since they're the same.

Though, if there are multiple threads causing race conditions, then that could mean timing is changed by the redundant check (if it's not optimized out) and perhaps improve things - though at that point I'd think the solution would be to do better thread safety instead of duplicating checks.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hang on - staring at this closer and seeing how they could differ - after the initial iteration of the loop the panda_cb_next updates plist and perhaps after the first iteration the entire list could become NULL?

Seems like an easier fix might be to ensure the various callback lists only ever become empty, not null?

plist = panda_cb_list_next(plist)) {
if (plist->enabled) {
plist->entry.insn_exec(plist->context, first_cpu, pc);
}
Expand All @@ -28,7 +30,9 @@ void HELPER(panda_insn_exec)(target_ulong pc) {
void HELPER(panda_after_insn_exec)(target_ulong pc) {
// PANDA instrumentation: after basic block
panda_cb_list *plist;
for(plist = panda_cbs[PANDA_CB_AFTER_INSN_EXEC]; plist != NULL; plist = panda_cb_list_next(plist)) {
for(plist = panda_cbs[PANDA_CB_AFTER_INSN_EXEC];
(plist != NULL && panda_cbs[PANDA_CB_AFTER_INSN_EXEC] != NULL);
plist = panda_cb_list_next(plist)) {
if (plist->enabled){
plist->entry.after_insn_exec(plist->context, first_cpu, pc);
}
Expand Down
12 changes: 6 additions & 6 deletions panda/include/panda/callbacks/cb-macros.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,7 +52,7 @@
void panda_callbacks_ ## name(COMBINE_TYPES(__VA_ARGS__)) { \
panda_cb_list *plist; \
for (plist = panda_cbs[PANDA_CB_ ## name_upper]; \
plist != NULL; \
(plist != NULL && panda_cbs[PANDA_CB_ ## name_upper] != NULL); \
plist = panda_cb_list_next(plist)) { \
if (plist->enabled) \
plist->entry. ENTRY_NAME(name, plist->context, EVERY_SECOND(__VA_ARGS__)); \
Expand All @@ -66,7 +66,7 @@
int panda_callbacks_ ## name(COMBINE_TYPES(__VA_ARGS__)) { \
panda_cb_list *plist; \
for (plist = panda_cbs[PANDA_CB_ ## name_upper]; \
plist != NULL; \
(plist != NULL && panda_cbs[PANDA_CB_ ## name_upper] != NULL); \
plist = panda_cb_list_next(plist)) { \
if (plist->enabled) \
plist->entry. ENTRY_NAME(name, plist->context, EVERY_SECOND(__VA_ARGS__)); \
Expand All @@ -85,7 +85,7 @@
panda_cb_list *plist; \
bool any_true = false; \
for (plist = panda_cbs[PANDA_CB_ ## name_upper]; \
plist != NULL; \
(plist != NULL && panda_cbs[PANDA_CB_ ## name_upper] != NULL); \
plist = panda_cb_list_next(plist)) { \
if (plist->enabled) \
any_true |= plist->entry. ENTRY_NAME(name, plist->context, EVERY_SECOND(__VA_ARGS__)); \
Expand All @@ -112,7 +112,7 @@
if (rr_in_replay()) { \
panda_cb_list *plist; \
for (plist = panda_cbs[PANDA_CB_ ## name_upper]; \
plist != NULL; \
(plist != NULL && panda_cbs[PANDA_CB_ ## name_upper] != NULL); \
plist = panda_cb_list_next(plist)) { \
if (plist->enabled) \
plist->entry. ENTRY_NAME(name, plist->context, EVERY_SECOND(__VA_ARGS__)); \
Expand All @@ -127,7 +127,7 @@
void panda_callbacks_ ## name(void) { \
panda_cb_list *plist; \
for (plist = panda_cbs[PANDA_CB_ ## name_upper]; \
plist != NULL; \
(plist != NULL && panda_cbs[PANDA_CB_ ## name_upper] != NULL); \
plist = panda_cb_list_next(plist)) { \
if (plist->enabled) \
plist->entry. ENTRY_NAME(name, plist->context); \
Expand All @@ -142,7 +142,7 @@
panda_cb_list *plist; \
bool any_true = false; \
for (plist = panda_cbs[PANDA_CB_ ## name_upper]; \
plist != NULL; \
(plist != NULL && panda_cbs[PANDA_CB_ ## name_upper] != NULL); \
plist = panda_cb_list_next(plist)) { \
if (plist->enabled) \
any_true |= plist->entry. ENTRY_NAME(name, plist->context); \
Expand Down
5 changes: 4 additions & 1 deletion panda/src/callbacks.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1510,7 +1510,10 @@ void hmp_panda_list_plugins(Monitor *mon, const QDict *qdict) {
void hmp_panda_plugin_cmd(Monitor *mon, const QDict *qdict) {
panda_cb_list *plist;
const char *cmd = qdict_get_try_str(qdict, "cmd");
for(plist = panda_cbs[PANDA_CB_MONITOR]; plist != NULL; plist = panda_cb_list_next(plist)) {
for(plist = panda_cbs[PANDA_CB_MONITOR];
(plist != NULL && \
panda_cbs[PANDA_CB_MONITOR] != NULL); \
plist = panda_cb_list_next(plist)) {
if (plist->enabled){
plist->entry.monitor(plist->context, mon, cmd);
}
Expand Down
41 changes: 27 additions & 14 deletions panda/src/cb-support.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -84,7 +84,9 @@ MAKE_CALLBACK(void, END_BLOCK_EXEC, end_block_exec,
// Non-macroized version for SBE - if panda_please_retranslate is set, we'll break
void PCB(start_block_exec)(CPUState *cpu, TranslationBlock *tb) {
panda_cb_list *plist;
for (plist = panda_cbs[PANDA_CB_START_BLOCK_EXEC]; plist != NULL; plist = panda_cb_list_next(plist)) {
for (plist = panda_cbs[PANDA_CB_START_BLOCK_EXEC];
(plist != NULL && panda_cbs[PANDA_CB_START_BLOCK_EXEC] != NULL);
plist = panda_cb_list_next(plist)) {
if (plist->enabled)
plist->entry.start_block_exec(plist->context, cpu, tb);
}
Expand Down Expand Up @@ -227,7 +229,8 @@ bool PCB(after_find_fast)(CPUState *cpu, TranslationBlock *tb,
panda_cb_list *plist;
if (!bb_invalidate_done) {
for (plist = panda_cbs[PANDA_CB_BEFORE_BLOCK_EXEC_INVALIDATE_OPT];
plist != NULL; plist = panda_cb_list_next(plist)) {
(plist != NULL && panda_cbs[PANDA_CB_BEFORE_BLOCK_EXEC_INVALIDATE_OPT] != NULL);
plist = panda_cb_list_next(plist)) {
if (plist->enabled)
*invalidate |=
plist->entry.before_block_exec_invalidate_opt(plist->context, cpu, tb);
Expand Down Expand Up @@ -263,8 +266,9 @@ int32_t PCB(before_handle_exception)(CPUState *cpu, int32_t exception_index) {
bool got_new_exception = false;
int32_t new_exception;

for (plist = panda_cbs[PANDA_CB_BEFORE_HANDLE_EXCEPTION]; plist != NULL;
plist = panda_cb_list_next(plist)) {
for (plist = panda_cbs[PANDA_CB_BEFORE_HANDLE_EXCEPTION];
(plist != NULL && panda_cbs[PANDA_CB_BEFORE_HANDLE_EXCEPTION] != NULL);
plist = panda_cb_list_next(plist)) {
if (plist->enabled) {
int32_t new_e = plist->entry.before_handle_exception(plist->context, cpu, exception_index);
if (!got_new_exception && new_e != exception_index) {
Expand All @@ -289,7 +293,8 @@ int32_t PCB(before_handle_interrupt)(CPUState *cpu, int32_t interrupt_request) {
bool got_new_interrupt = false;
int32_t new_interrupt;

for (plist = panda_cbs[PANDA_CB_BEFORE_HANDLE_INTERRUPT]; plist != NULL;
for (plist = panda_cbs[PANDA_CB_BEFORE_HANDLE_INTERRUPT];
(plist != NULL && panda_cbs[PANDA_CB_BEFORE_HANDLE_INTERRUPT] != NULL);
plist = panda_cb_list_next(plist)) {
if (plist->enabled) {
int32_t new_i = plist->entry.before_handle_interrupt(plist->context, cpu, interrupt_request);
Expand Down Expand Up @@ -328,15 +333,17 @@ MEM_CB_TRAMPOLINES(phys)
void PCB(mem_before_read)(CPUState *env, target_ptr_t pc, target_ptr_t addr,
size_t data_size, void *ram_ptr) {
panda_cb_list *plist;
for(plist = panda_cbs[PANDA_CB_VIRT_MEM_BEFORE_READ]; plist != NULL;
for(plist = panda_cbs[PANDA_CB_VIRT_MEM_BEFORE_READ];
plist != NULL && panda_cbs[PANDA_CB_VIRT_MEM_BEFORE_READ] != NULL;
plist = panda_cb_list_next(plist)) {
if (plist->enabled) plist->entry.virt_mem_before_read(plist->context, env, panda_current_pc(env), addr,
data_size);
}
if (panda_cbs[PANDA_CB_PHYS_MEM_BEFORE_READ]) {
hwaddr paddr = get_paddr(env, addr, ram_ptr);
if (paddr == -1) return;
for(plist = panda_cbs[PANDA_CB_PHYS_MEM_BEFORE_READ]; plist != NULL;
for(plist = panda_cbs[PANDA_CB_PHYS_MEM_BEFORE_READ];
plist != NULL && panda_cbs[PANDA_CB_PHYS_MEM_BEFORE_READ] != NULL;
plist = panda_cb_list_next(plist)) {
if (plist->enabled) plist->entry.phys_mem_before_read(plist->context, env, panda_current_pc(env),
paddr, data_size);
Expand All @@ -348,7 +355,8 @@ void PCB(mem_before_read)(CPUState *env, target_ptr_t pc, target_ptr_t addr,
void PCB(mem_after_read)(CPUState *env, target_ptr_t pc, target_ptr_t addr,
size_t data_size, uint64_t result, void *ram_ptr) {
panda_cb_list *plist;
for(plist = panda_cbs[PANDA_CB_VIRT_MEM_AFTER_READ]; plist != NULL;
for(plist = panda_cbs[PANDA_CB_VIRT_MEM_AFTER_READ];
plist != NULL && panda_cbs[PANDA_CB_VIRT_MEM_AFTER_READ] != NULL;
plist = panda_cb_list_next(plist)) {
/* mstamat: Passing &result as the last cb arg doesn't make much sense. */
if (plist->enabled) plist->entry.virt_mem_after_read(plist->context, env, panda_current_pc(env), addr,
Expand All @@ -357,7 +365,8 @@ void PCB(mem_after_read)(CPUState *env, target_ptr_t pc, target_ptr_t addr,
if (panda_cbs[PANDA_CB_PHYS_MEM_AFTER_READ]) {
hwaddr paddr = get_paddr(env, addr, ram_ptr);
if (paddr == -1) return;
for(plist = panda_cbs[PANDA_CB_PHYS_MEM_AFTER_READ]; plist != NULL;
for(plist = panda_cbs[PANDA_CB_PHYS_MEM_AFTER_READ];
plist != NULL && panda_cbs[PANDA_CB_PHYS_MEM_AFTER_READ] != NULL;
plist = panda_cb_list_next(plist)) {
/* mstamat: Passing &result as the last cb arg doesn't make much sense. */
if (plist->enabled) plist->entry.phys_mem_after_read(plist->context, env, panda_current_pc(env), paddr,
Expand All @@ -370,7 +379,8 @@ void PCB(mem_after_read)(CPUState *env, target_ptr_t pc, target_ptr_t addr,
void PCB(mem_before_write)(CPUState *env, target_ptr_t pc, target_ptr_t addr,
size_t data_size, uint64_t val, void *ram_ptr) {
panda_cb_list *plist;
for(plist = panda_cbs[PANDA_CB_VIRT_MEM_BEFORE_WRITE]; plist != NULL;
for(plist = panda_cbs[PANDA_CB_VIRT_MEM_BEFORE_WRITE];
plist != NULL && panda_cbs[PANDA_CB_VIRT_MEM_BEFORE_WRITE] != NULL;
plist = panda_cb_list_next(plist)) {
/* mstamat: Passing &val as the last arg doesn't make much sense. */
if (plist->enabled) plist->entry.virt_mem_before_write(plist->context, env, panda_current_pc(env), addr,
Expand All @@ -379,7 +389,8 @@ void PCB(mem_before_write)(CPUState *env, target_ptr_t pc, target_ptr_t addr,
if (panda_cbs[PANDA_CB_PHYS_MEM_BEFORE_WRITE]) {
hwaddr paddr = get_paddr(env, addr, ram_ptr);
if (paddr == -1) return;
for(plist = panda_cbs[PANDA_CB_PHYS_MEM_BEFORE_WRITE]; plist != NULL;
for(plist = panda_cbs[PANDA_CB_PHYS_MEM_BEFORE_WRITE];
plist != NULL && panda_cbs[PANDA_CB_PHYS_MEM_BEFORE_WRITE] != NULL;
plist = panda_cb_list_next(plist)) {
/* mstamat: Passing &val as the last cb arg doesn't make much sense. */
if (plist->enabled) plist->entry.phys_mem_before_write(plist->context, env, panda_current_pc(env), paddr,
Expand All @@ -392,7 +403,8 @@ void PCB(mem_before_write)(CPUState *env, target_ptr_t pc, target_ptr_t addr,
void PCB(mem_after_write)(CPUState *env, target_ptr_t pc, target_ptr_t addr,
size_t data_size, uint64_t val, void *ram_ptr) {
panda_cb_list *plist;
for (plist = panda_cbs[PANDA_CB_VIRT_MEM_AFTER_WRITE]; plist != NULL;
for (plist = panda_cbs[PANDA_CB_VIRT_MEM_AFTER_WRITE];
(plist != NULL && panda_cbs[PANDA_CB_VIRT_MEM_AFTER_WRITE] != NULL);
plist = panda_cb_list_next(plist)) {
/* mstamat: Passing &val as the last cb arg doesn't make much sense. */
if (plist->enabled) plist->entry.virt_mem_after_write(plist->context, env, panda_current_pc(env), addr,
Expand All @@ -401,8 +413,9 @@ void PCB(mem_after_write)(CPUState *env, target_ptr_t pc, target_ptr_t addr,
if (panda_cbs[PANDA_CB_PHYS_MEM_AFTER_WRITE]) {
hwaddr paddr = get_paddr(env, addr, ram_ptr);
if (paddr == -1) return;
for (plist = panda_cbs[PANDA_CB_PHYS_MEM_AFTER_WRITE]; plist != NULL;
plist = panda_cb_list_next(plist)) {
for (plist = panda_cbs[PANDA_CB_PHYS_MEM_AFTER_WRITE];
(plist != NULL && panda_cbs[PANDA_CB_PHYS_MEM_AFTER_WRITE] != NULL);
plist = panda_cb_list_next(plist)) {
/* mstamat: Passing &val as the last cb arg doesn't make much sense. */
if (plist->enabled) plist->entry.phys_mem_after_write(plist->context, env, panda_current_pc(env), paddr,
data_size, (uint8_t *)&val);
Expand Down
Loading