Fix memory safety and type mismatch bugs (#63, #65, #66)#67
Fix memory safety and type mismatch bugs (#63, #65, #66)#67dcpagotto wants to merge 1 commit intonasa-jpl:integrationfrom
Conversation
…asa-jpl#66) - libams.c: dereference content pointer before calling MRELEASE (nasa-jpl#66) MRELEASE(content) was freeing the char** stack pointer instead of the actual buffer at *content, causing either a crash or memory leak. Other MRELEASE calls in the same function already use *content correctly. - dtkaadmin.c: remove unused sprintf and undersized stack buffer (nasa-jpl#65) The char test[5] buffer and sprintf call in manageLeadTime were dead code (result never used). sprintf with %u on a 5-byte buffer can overflow for values > 9999, which is valid since the only lower bound check is >= 20. - ipnfw.c: use sm_list_insert_last instead of sdr_list_insert_last (nasa-jpl#63) viaPassageways is created with sm_list_create (shared memory list) but was being populated with sdr_list_insert_last (SDR list function). Changed to sm_list_insert_last with ionwm partition, matching how the list is created and destroyed in the rest of the codebase.
|
Hey! Just noticed issue #65 got closed today — nice to see it's being addressed. Quick question though: are you folks planning to review this PR, or did you end up fixing it differently on your side? Totally fine either way, just wanted to check so I know whether to keep this open or close it out. Thanks! |
|
#63 is sorta fixed because IONe's IRF code is supposed to replace the HIRR code, so those HIRR functions would go away. But at the same time, they don't want to merge the IRF code into the default branch because it isn't tested. IMO we should remove the HIRR code and worry about IRF stuff at a later time, but I wanted feedback from the developers first. As pointed out in #66 (comment), I think the memory ownership of the ams functions are sus. Someone with knowledge about AMS should clean that up (maybe @SkyDeBaun ?), but I think people are busy. If a cleanup is not forthcoming, then I think they should merge your AMS change. |
|
Hey @SkyDeBaun, saw your comment on #66 — glad the MRELEASE issue is confirmed! You mentioned there might be a conflict with your current branch. Happy to rebase this PR against your changes if that helps, or if you'd rather just fold these fixes into your work directly, that's cool too. Whatever makes integration easier on your end. Just let me know how you'd like to handle it 👍 |
@dcpagotto I may have to retract that statement.. I think your fix looks promising (makes sense!) but am seeing a conflict with RAMS.. when the node in question is also an AMS subscriber. Will need to dig into this a bit. |
|
@SkyDeBaun Please check if this PR can be closed or if we defer to 4.2 - update milestone please. Thx! |
|
Investigating ongoing. |
4 participants
Fixes for three open bugs, all verified against the source. Minimal changes, one fix per file.
ams/library/libams.c— fix MRELEASE dereference (#66)constructMessagereceivescontentaschar **. At line 1118,MRELEASE(content)frees the stack pointer itself instead of the buffer it points to. Every other MRELEASE call in the same function correctly uses*content.tc/dtka/dtkaadmin.c— remove dead code with buffer overflow risk (#65)manageLeadTimedeclareschar test[5]and callssprintf(test, "%u", ...)but the result is never read. The buffer is too small for the full range ofunsigned int(up to 10 digits), and the only input validation is>= 20, so any value above 9999 would overflow. Removed both the buffer and the sprintf since they serve no purpose.bpv7/ipn/ipnfw.c— fix SDR/SM list type mismatch (#63)initializeHIRRcreatesviaPassagewayswithsm_list_create(ionwm)(shared memory list), but then tries to populate it usingsdr_list_insert_last(SDR persistent store function). These are completely different list implementations. Changed tosm_list_insert_last(ionwm, ...)to match the list type. This is consistent with how the list is destroyed inlibcgr.cusingsm_list_destroy.All three are single-line changes. No functional additions, just correcting existing behavior to match what the code clearly intended.