)]}'
{"/PATCHSET_LEVEL":[{"author":{"_account_id":1002047,"name":"Evgeniy Naydanov","email":"eugnay@gmail.com","username":"en-sc"},"change_message_id":"3b5df4a3ce09890109f06b88e11e020161a0ea55","unresolved":true,"context_lines":[],"source_content_type":"","patch_set":1,"id":"512687b3_e6274fb6","updated":"2024-12-09 14:54:14.000000000","message":"Marc, thank you for the patch. I\u0027m working on target-type specific [`configure/cget` for RISC-V](https://github.com/riscv-collab/riscv-openocd/pull/1168) and I\u0027ve made the same mistake as the one you are fixing here. Thanks!\n\nConsidering this, wouldn\u0027t it be safer to deal with it on the target-type-independent level?\nE.g. by calling `target-\u003etype-\u003edeinit_target()` [after `target_configure()` fails](https://review.openocd.org/c/openocd/+/8637/1/src/target/target.c#5846).","commit_id":"07a05b249bbd5064639fca49e4c43ff6f0b1dfc8"},{"author":{"_account_id":1002047,"name":"Evgeniy Naydanov","email":"eugnay@gmail.com","username":"en-sc"},"change_message_id":"ee3d58933fe842b87cd61dcf08166411df5b337a","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":1,"id":"1a3f5285_7deb73c3","updated":"2024-12-10 11:00:43.000000000","message":"There is also another issue -- freeing the`private_config` messes up with the following `cget`:\nAt current master (8a3723022):\n```\n\u003e openocd -c \u0027adapter driver dummy; jtag newtap tap cpu -irlen 2; dap create mydap -chain-position tap.cpu; target create test.cpu hla_target -dap mydap; catch {test.cpu configure -ap-num}; echo \"DAP: [test.cpu cget -dap]\"; shutdown\u0027\nOpen On-Chip Debugger 0.12.0+dev-00779-g8a3723022 (2024-12-10-13:38)\nLicensed under GNU GPL v2\nFor bug reports, read\n\thttp://openocd.org/doc/doxygen/bugs.html\nInfo : only one transport option; autoselecting \u0027jtag\u0027\nDAP: mydap\nshutdown command invoked\n```\nYour version:\n```\n\u003e openocd -c \u0027adapter driver dummy; jtag newtap tap cpu -irlen 2; dap create mydap -chain-position tap.cpu; target create test.cpu hla_target -dap mydap; catch {test.cpu configure -ap-num}; echo \"DAP: [test.cpu cget -dap]\"; shutdown\u0027\nOpen On-Chip Debugger 0.12.0+dev-00803-g07a05b249 (2024-12-10-13:56)\nLicensed under GNU GPL v2\nFor bug reports, read\n\thttp://openocd.org/doc/doxygen/bugs.html\nInfo : only one transport option; autoselecting \u0027jtag\u0027\nfree(): double free detected in tcache 2\n[1]    1463086 IOT instruction (core dumped)\n```","commit_id":"07a05b249bbd5064639fca49e4c43ff6f0b1dfc8"}],"src/target/aarch64.c":[{"author":{"_account_id":1000687,"name":"Tomas Vanek","display_name":"Tomas Vanek","email":"vanekt@fbl.cz","username":"vanekt"},"change_message_id":"a93d4a1e8d0aecac3771358cd5ce5900b325a953","unresolved":true,"context_lines":[{"line_number":2920,"context_line":"\t * and JIM_ERR if an error occurred during parameter evaluation."},{"line_number":2921,"context_line":"\t * For JIM_CONTINUE, we check our own params."},{"line_number":2922,"context_line":"\t */"},{"line_number":2923,"context_line":"\te \u003d adiv5_jim_configure_ext(target, goi, \u0026pc-\u003eadiv5_config, ADI_CONFIGURE_DAP_COMPULSORY);"},{"line_number":2924,"context_line":"\tif (e !\u003d JIM_CONTINUE)"},{"line_number":2925,"context_line":"\t\treturn e;"},{"line_number":2926,"context_line":""}],"source_content_type":"text/x-csrc","patch_set":1,"id":"c6c2f4d6_bb460fb4","side":"PARENT","line":2923,"range":{"start_line":2923,"start_character":42,"end_line":2923,"end_character":59},"updated":"2024-12-10 09:55:03.000000000","message":"See here","commit_id":"2b79c32c76c3f5f54811d1c07159fe3dcd8a482b"}],"src/target/arm_adi_v5.c":[{"author":{"_account_id":1000687,"name":"Tomas Vanek","display_name":"Tomas Vanek","email":"vanekt@fbl.cz","username":"vanekt"},"change_message_id":"a93d4a1e8d0aecac3771358cd5ce5900b325a953","unresolved":true,"context_lines":[{"line_number":2459,"context_line":""},{"line_number":2460,"context_line":"\tint e \u003d adiv5_jim_spot_configure(goi, \u0026pc-\u003edap, \u0026pc-\u003eap_num, NULL);"},{"line_number":2461,"context_line":"\tif (e !\u003d JIM_OK) {"},{"line_number":2462,"context_line":"\t\tfree(pc);"},{"line_number":2463,"context_line":"\t\treturn e;"},{"line_number":2464,"context_line":"\t}"},{"line_number":2465,"context_line":""}],"source_content_type":"text/x-csrc","patch_set":1,"id":"6797ed0b_3876acbc","line":2462,"updated":"2024-12-10 09:55:03.000000000","message":"No!!\nIf `pc` has been allocated, it was stored as `target-\u003eprivate_config` and caller/upper layer will free it.\nIf `pc` was passed as a parameter it should not be freed at all!","commit_id":"07a05b249bbd5064639fca49e4c43ff6f0b1dfc8"},{"author":{"_account_id":1000853,"name":"zapb","display_name":"Marc Schink","email":"dev@zapb.de","username":"zapb"},"change_message_id":"884ba25478c8239e189e21cdc02e0252b4c14dc6","unresolved":true,"context_lines":[{"line_number":2459,"context_line":""},{"line_number":2460,"context_line":"\tint e \u003d adiv5_jim_spot_configure(goi, \u0026pc-\u003edap, \u0026pc-\u003eap_num, NULL);"},{"line_number":2461,"context_line":"\tif (e !\u003d JIM_OK) {"},{"line_number":2462,"context_line":"\t\tfree(pc);"},{"line_number":2463,"context_line":"\t\treturn e;"},{"line_number":2464,"context_line":"\t}"},{"line_number":2465,"context_line":""}],"source_content_type":"text/x-csrc","patch_set":1,"id":"3584c469_f17a2b81","line":2462,"in_reply_to":"1d70b6b0_fc0b04ca","updated":"2024-12-10 12:33:55.000000000","message":"As I said, they free `private_config` but you cannot call them because they perform operations which are not valid during the initialization / configuration. For example, if you call `cortex_m_deinit_target() in target.c:5847, you generate a segfault because of invalid reads. So I would suggest to modify the `xxx_deinit_target()` functions in a way they can be called at any time / during configuration. Do you agree?","commit_id":"07a05b249bbd5064639fca49e4c43ff6f0b1dfc8"},{"author":{"_account_id":1000687,"name":"Tomas Vanek","display_name":"Tomas Vanek","email":"vanekt@fbl.cz","username":"vanekt"},"change_message_id":"423983f7fa134fa93b3fb5a8732e3bfc9627fa39","unresolved":true,"context_lines":[{"line_number":2459,"context_line":""},{"line_number":2460,"context_line":"\tint e \u003d adiv5_jim_spot_configure(goi, \u0026pc-\u003edap, \u0026pc-\u003eap_num, NULL);"},{"line_number":2461,"context_line":"\tif (e !\u003d JIM_OK) {"},{"line_number":2462,"context_line":"\t\tfree(pc);"},{"line_number":2463,"context_line":"\t\treturn e;"},{"line_number":2464,"context_line":"\t}"},{"line_number":2465,"context_line":""}],"source_content_type":"text/x-csrc","patch_set":1,"id":"37796439_176dcbfa","line":2462,"in_reply_to":"1d70b6b0_fc0b04ca","updated":"2024-12-10 12:54:05.000000000","message":"Evgeniy, see 8642: target: free private_config if target itialisation fails | https://review.openocd.org/c/openocd/+/8642\nIt should fix the leak you revealed.","commit_id":"07a05b249bbd5064639fca49e4c43ff6f0b1dfc8"},{"author":{"_account_id":1000853,"name":"zapb","display_name":"Marc Schink","email":"dev@zapb.de","username":"zapb"},"change_message_id":"873b62a7de40b53e14f86ac6cbe5ae48f370401c","unresolved":false,"context_lines":[{"line_number":2459,"context_line":""},{"line_number":2460,"context_line":"\tint e \u003d adiv5_jim_spot_configure(goi, \u0026pc-\u003edap, \u0026pc-\u003eap_num, NULL);"},{"line_number":2461,"context_line":"\tif (e !\u003d JIM_OK) {"},{"line_number":2462,"context_line":"\t\tfree(pc);"},{"line_number":2463,"context_line":"\t\treturn e;"},{"line_number":2464,"context_line":"\t}"},{"line_number":2465,"context_line":""}],"source_content_type":"text/x-csrc","patch_set":1,"id":"91b6a57e_1b081c76","line":2462,"in_reply_to":"37796439_176dcbfa","updated":"2024-12-10 13:14:59.000000000","message":"Next time, rather than releasing an additional patch, explain what kind of solution you prefer and why.","commit_id":"07a05b249bbd5064639fca49e4c43ff6f0b1dfc8"},{"author":{"_account_id":1000853,"name":"zapb","display_name":"Marc Schink","email":"dev@zapb.de","username":"zapb"},"change_message_id":"41c08980247bc385514720324b2b100d0a1eed3d","unresolved":true,"context_lines":[{"line_number":2459,"context_line":""},{"line_number":2460,"context_line":"\tint e \u003d adiv5_jim_spot_configure(goi, \u0026pc-\u003edap, \u0026pc-\u003eap_num, NULL);"},{"line_number":2461,"context_line":"\tif (e !\u003d JIM_OK) {"},{"line_number":2462,"context_line":"\t\tfree(pc);"},{"line_number":2463,"context_line":"\t\treturn e;"},{"line_number":2464,"context_line":"\t}"},{"line_number":2465,"context_line":""}],"source_content_type":"text/x-csrc","patch_set":1,"id":"d9292957_be046a48","line":2462,"in_reply_to":"6797ed0b_3876acbc","updated":"2024-12-10 11:01:39.000000000","message":"Thanks Evgeniy! You\u0027re totally right Tomas - except that the upper layer is not able to handle it properly at the moment. Where would you free() private_config? There is no `deinit` in arm_adi_v5.c (which allocated private_config). `private_config` is free\u0027d in the target specific \"deinit\" code but this code is not safe to be called during the configuration stage. Then, you can implement a \"catch all\" solution and free it in the general target layer. This is not a good option in my opinion because the target layer did not allocate it. Did I miss something?\n\nSo I would recommend to make the target \"deinit\" functions safe to be called during the configuration stage? I tested cortex_m and aarch64, at least these two targets must be fixed.","commit_id":"07a05b249bbd5064639fca49e4c43ff6f0b1dfc8"},{"author":{"_account_id":1002047,"name":"Evgeniy Naydanov","email":"eugnay@gmail.com","username":"en-sc"},"change_message_id":"ee3d58933fe842b87cd61dcf08166411df5b337a","unresolved":true,"context_lines":[{"line_number":2459,"context_line":""},{"line_number":2460,"context_line":"\tint e \u003d adiv5_jim_spot_configure(goi, \u0026pc-\u003edap, \u0026pc-\u003eap_num, NULL);"},{"line_number":2461,"context_line":"\tif (e !\u003d JIM_OK) {"},{"line_number":2462,"context_line":"\t\tfree(pc);"},{"line_number":2463,"context_line":"\t\treturn e;"},{"line_number":2464,"context_line":"\t}"},{"line_number":2465,"context_line":""}],"source_content_type":"text/x-csrc","patch_set":1,"id":"36f11a74_5649d23f","line":2462,"in_reply_to":"6797ed0b_3876acbc","updated":"2024-12-10 11:00:43.000000000","message":"There are cases though when the `target-\u003eprivate_config` leaks:\n```\n\u003e valgrind --leak-check\u003dfull openocd -c \u0027adapter driver jlink; jtag newtap tap cpu -irlen 2; target create test.cpu hla_target -dap\u0027\n...\n\u003d\u003d343799\u003d\u003d HEAP SUMMARY:\n\u003d\u003d343799\u003d\u003d     in use at exit: 16 bytes in 1 blocks\n\u003d\u003d343799\u003d\u003d   total heap usage: 11,272 allocs, 11,271 frees, 949,927 bytes allocated\n\u003d\u003d343799\u003d\u003d\n\u003d\u003d343799\u003d\u003d 16 bytes in 1 blocks are definitely lost in loss record 1 of 1\n\u003d\u003d343799\u003d\u003d    at 0x483DD99: calloc (in /usr/lib/x86_64-linux-gnu/valgrind/vgpreload_memcheck-amd64-linux.so)\n\u003d\u003d343799\u003d\u003d    by 0x1F5C90: adiv5_jim_configure_ext (arm_adi_v5.c:2447)\n\u003d\u003d343799\u003d\u003d    by 0x1F5E47: adiv5_jim_configure (arm_adi_v5.c:2481)\n\u003d\u003d343799\u003d\u003d    by 0x20E6CF: target_configure (target.c:5046)\n\u003d\u003d343799\u003d\u003d    by 0x210DA2: target_create (target.c:5952)\n\u003d\u003d343799\u003d\u003d    by 0x211963: jim_target_create (target.c:6185)\n\u003d\u003d343799\u003d\u003d    by 0x25A471: exec_command (command.c:496)\n\u003d\u003d343799\u003d\u003d    by 0x25B7C9: jim_command_dispatch (command.c:931)\n\u003d\u003d343799\u003d\u003d    by 0x485400: JimInvokeCommand (jim.c:10931)\n\u003d\u003d343799\u003d\u003d    by 0x485685: Jim_EvalObjVector (jim.c:11009)\n\u003d\u003d343799\u003d\u003d    by 0x485793: Jim_EvalObjPrefix (jim.c:11030)\n\u003d\u003d343799\u003d\u003d    by 0x25B650: jim_command_dispatch (command.c:895)\n\u003d\u003d343799\u003d\u003d\n\u003d\u003d343799\u003d\u003d LEAK SUMMARY:\n\u003d\u003d343799\u003d\u003d    definitely lost: 16 bytes in 1 blocks\n\u003d\u003d343799\u003d\u003d    indirectly lost: 0 bytes in 0 blocks\n\u003d\u003d343799\u003d\u003d      possibly lost: 0 bytes in 0 blocks\n\u003d\u003d343799\u003d\u003d    still reachable: 0 bytes in 0 blocks\n\u003d\u003d343799\u003d\u003d         suppressed: 0 bytes in 0 blocks\n\u003d\u003d343799\u003d\u003d\n\u003d\u003d343799\u003d\u003d For lists of detected and suppressed errors, rerun with: -s\n\u003d\u003d343799\u003d\u003d ERROR SUMMARY: 1 errors from 1 contexts (suppressed: 0 from 0)\n```","commit_id":"07a05b249bbd5064639fca49e4c43ff6f0b1dfc8"},{"author":{"_account_id":1000687,"name":"Tomas Vanek","display_name":"Tomas Vanek","email":"vanekt@fbl.cz","username":"vanekt"},"change_message_id":"c1b9304848a829e9c6da382a3c2278fe441060be","unresolved":false,"context_lines":[{"line_number":2459,"context_line":""},{"line_number":2460,"context_line":"\tint e \u003d adiv5_jim_spot_configure(goi, \u0026pc-\u003edap, \u0026pc-\u003eap_num, NULL);"},{"line_number":2461,"context_line":"\tif (e !\u003d JIM_OK) {"},{"line_number":2462,"context_line":"\t\tfree(pc);"},{"line_number":2463,"context_line":"\t\treturn e;"},{"line_number":2464,"context_line":"\t}"},{"line_number":2465,"context_line":""}],"source_content_type":"text/x-csrc","patch_set":1,"id":"e8f50788_a4536ebf","line":2462,"in_reply_to":"91b6a57e_1b081c76","updated":"2024-12-10 14:05:38.000000000","message":"Marc,\nplease do not take it as an aggressive self-promotion.\nI just looked for the reason of the leak found by Evgeniy and when I found it, submitting the patch looked as the fastest and simplest way.\n\nOf course the code with many `free()` at each error return is not ideal and could be error prone. Feel free to rework it as you want.\n\nJust one note: we should not spent so much time debugging every minimal memory leak on the error paths. There are more important topics...","commit_id":"07a05b249bbd5064639fca49e4c43ff6f0b1dfc8"},{"author":{"_account_id":1000687,"name":"Tomas Vanek","display_name":"Tomas Vanek","email":"vanekt@fbl.cz","username":"vanekt"},"change_message_id":"4a690bcac05cd5fed4a63916fcdae35db191ea0e","unresolved":true,"context_lines":[{"line_number":2459,"context_line":""},{"line_number":2460,"context_line":"\tint e \u003d adiv5_jim_spot_configure(goi, \u0026pc-\u003edap, \u0026pc-\u003eap_num, NULL);"},{"line_number":2461,"context_line":"\tif (e !\u003d JIM_OK) {"},{"line_number":2462,"context_line":"\t\tfree(pc);"},{"line_number":2463,"context_line":"\t\treturn e;"},{"line_number":2464,"context_line":"\t}"},{"line_number":2465,"context_line":""}],"source_content_type":"text/x-csrc","patch_set":1,"id":"1d70b6b0_fc0b04ca","line":2462,"in_reply_to":"d9292957_be046a48","updated":"2024-12-10 12:21:29.000000000","message":"At least cortex_m and aarch64 have `free(target-\u003eprivate_config)` in `xxx_deinit_target()`. The question is if deinit is called after error in initialisation.","commit_id":"07a05b249bbd5064639fca49e4c43ff6f0b1dfc8"},{"author":{"_account_id":1000853,"name":"zapb","display_name":"Marc Schink","email":"dev@zapb.de","username":"zapb"},"change_message_id":"a4ef61c46850d06505a81cf72e8821cc53e57123","unresolved":false,"context_lines":[{"line_number":2459,"context_line":""},{"line_number":2460,"context_line":"\tint e \u003d adiv5_jim_spot_configure(goi, \u0026pc-\u003edap, \u0026pc-\u003eap_num, NULL);"},{"line_number":2461,"context_line":"\tif (e !\u003d JIM_OK) {"},{"line_number":2462,"context_line":"\t\tfree(pc);"},{"line_number":2463,"context_line":"\t\treturn e;"},{"line_number":2464,"context_line":"\t}"},{"line_number":2465,"context_line":""}],"source_content_type":"text/x-csrc","patch_set":1,"id":"e4051f1a_75e97987","line":2462,"in_reply_to":"e8f50788_a4536ebf","updated":"2024-12-12 12:19:23.000000000","message":"\u003e Marc,\n\u003e please do not take it as an aggressive self-promotion.\n\u003e I just looked for the reason of the leak found by Evgeniy and when I found it, submitting the patch looked as the fastest and simplest way.\n\nTo be honest, it annoyed me a little that you just pushed a new patch that fixes the leak in a way I proposed (not my favorite way though) instead of answering my questions. That my comments were not unfounded can now be seen in #8642.\n\n\u003e Just one note: we should not spent so much time debugging every minimal memory leak on the error paths. There are more important topics...\n\nIn general I agree with you and I can well understand that you don\u0027t want to spend much time on this and that there are more important topics. But I don\u0027t think it was more efficient to come to the same conclusion via the detour of a new patch.\n\nBut let\u0027s not make this bigger than it is, I hope my point is clear and a little understandable. I appreciate your reviews and your contributions. Let\u0027s come to a good solution constructively.","commit_id":"07a05b249bbd5064639fca49e4c43ff6f0b1dfc8"}]}