GNOME Bugzilla – Bug 636741
Fix leak when processing the set cookie header
Last modified: 2010-12-10 20:02:23 UTC
When the first_party variable is NULL, the loop continues without freeing or storing the parsed cookie, which results in a leak. Potential fix attached.
Created attachment 176036 [details] [review] Fix leak when processing the set cookie header Valgrind trace: ==4617== 401 (96 direct, 305 indirect) bytes in 2 blocks are definitely lost in loss record 15,500 of 16,590 ==4617== at 0x4C244E8: malloc (vg_replace_malloc.c:236) ==4617== by 0x103D73AA: g_malloc (gmem.c:164) ==4617== by 0x103F0EB4: g_slice_alloc (gslice.c:842) ==4617== by 0x103F0F0C: g_slice_alloc0 (gslice.c:854) ==4617== by 0xE1CCD04: parse_one_cookie (soup-cookie.c:218) ==4617== by 0xE1CD2B3: soup_cookies_from_response (soup-cookie.c:763) ==4617== by 0xE1CE332: process_set_cookie_header (soup-cookie-jar.c:562) ==4617== by 0xFB4AC36: g_cclosure_marshal_VOID__VOID (gmarshal.c:79) ==4617== by 0xFB2F59E: g_closure_invoke (gclosure.c:766) ==4617== by 0xFB4A2B1: signal_emit_unlocked_R (gsignal.c:3252) ==4617== by 0xFB490EE: g_signal_emit_valist (gsignal.c:2983) ==4617== by 0xFB4966C: g_signal_emit (gsignal.c:3040) ==4617== by 0xE1DA0DF: io_read (soup-message-io.c:903)
Comment on attachment 176036 [details] [review] Fix leak when processing the set cookie header > if (first_party == NULL && >- priv->accept_policy == SOUP_COOKIE_JAR_ACCEPT_NO_THIRD_PARTY) >+ priv->accept_policy == SOUP_COOKIE_JAR_ACCEPT_NO_THIRD_PARTY) { if you were going to keep that, then fix the indentation level to be back how it was before. However, the leak just points out that the code was kind of messy before, and freeing the cookie in two different places doesn't really make it any better. I think it would also work to just remove this first check, and add "first_party != NULL" to the appropriate place in the remaining check, right?
oops, sorry about the indentation changes. I agree about the code being a bit messy. I took the path of least resistance since it was a quick fix, but you're right, I should fix it properly.
Created attachment 176102 [details] [review] Fix leak when processing the set cookie header Valgrind trace: ==4617== 401 (96 direct, 305 indirect) bytes in 2 blocks are definitely lost in loss record 15,500 of 16,590 ==4617== at 0x4C244E8: malloc (vg_replace_malloc.c:236) ==4617== by 0x103D73AA: g_malloc (gmem.c:164) ==4617== by 0x103F0EB4: g_slice_alloc (gslice.c:842) ==4617== by 0x103F0F0C: g_slice_alloc0 (gslice.c:854) ==4617== by 0xE1CCD04: parse_one_cookie (soup-cookie.c:218) ==4617== by 0xE1CD2B3: soup_cookies_from_response (soup-cookie.c:763) ==4617== by 0xE1CE332: process_set_cookie_header (soup-cookie-jar.c:562) ==4617== by 0xFB4AC36: g_cclosure_marshal_VOID__VOID (gmarshal.c:79) ==4617== by 0xFB2F59E: g_closure_invoke (gclosure.c:766) ==4617== by 0xFB4A2B1: signal_emit_unlocked_R (gsignal.c:3252) ==4617== by 0xFB490EE: g_signal_emit_valist (gsignal.c:2983) ==4617== by 0xFB4966C: g_signal_emit (gsignal.c:3040) ==4617== by 0xE1DA0DF: io_read (soup-message-io.c:903)
Comment on attachment 176102 [details] [review] Fix leak when processing the set cookie header looks good
pushed 00561929c2a61bd204f98ba0dcd2925219534104 to master