From: Vladislav Shpilevoy <v.shpilevoy@tarantool.org> To: Cyrill Gorcunov <gorcunov@gmail.com> Cc: TML <tarantool-patches@dev.tarantool.org> Subject: Re: [Tarantool-patches] [PATCH v4 4/4] crash: report crash data to the feedback server Date: Sun, 20 Dec 2020 15:48:14 +0100 [thread overview] Message-ID: <0bb6f2db-5ef8-9d71-be61-67e113c7e0ad@tarantool.org> (raw) In-Reply-To: <20201216111606.GA14556@grain> >>> | "backtrace": "IzAgIDB4NjMxZDAzIGluIGNyYXNoX2NvbGxlY3QrYmYKIzEgID" >>> | "B4NjMyZWRhIGluIGNyYXNoX3NpZ25hbF9jYis1MQojMiAgMHg3ZjFkNDkyMzlhO" >>> | "TAgaW4gZnVubG9ja2ZpbGUrNjAKIzMgIDB4N2YxZDQ4ZGQxYzVlIGluIGVwb2xs" >>> | "X3dhaXQrNWUKIzQgIDB4ODM3NzEzIGluIGVwb2xsX3BvbGwrOTkKIzUgIDB4ODN" >>> | "hZDQ4IGluIGV2X3J1bis0ODcKIzYgIDB4NjBmMGFlIGluIHRhcmFudG9vbF9sdW" >>> | "FfcnVuX3NjcmlwdCsxMzgKIzcgIDB4NDQ2NWY1IGluIG1haW4rNWFjCiM4ICAwe" >>> | "DdmMWQ0OGNmNzA0MiBpbiBfX2xpYmNfc3RhcnRfbWFpbitmMgojOSAgMHg0NDRh" >>> | "MGUgaW4gX3N0YXJ0KzJlCg==", >>> | "timestamp": "2020-12-10 17:08:42 MSK" >>> | } >>> | } >> >> 3. And what is the reason you encode it all into base64 second time? > > Because backtrace contains newlines and other special symbols, this is very inconvenient > when you print the json-dump into console. I didn't ask about backtrace base64. I ask about why do you encode crashdump.data field into base64 again. Currently you do base64 encoding 2 times: one for signal.backtrace field, and second time you encode the whole {uname: ..., build: ..., signal: ...}. Signal.backtrace here is already in base64. I ask about the second encoding. >>> + >>> + char *p = static_alloc(SMALL_STATIC_SIZE); >>> + char *e = &p[SMALL_STATIC_SIZE - 1]; >>> + char *head = p; >>> + char *tail = &p[SMALL_STATIC_SIZE - 8]; >> >> 10. Why -8? > > To have at least a gap before the end of a buffer. Sorry, but this is not an answer. Why not -7, -100, -3? I don't see where these 8 bytes are used for anything, even as a special 8-byte gap. >>> + >>> + /* >>> + * Lets reuse tail of the buffer as a temp space. >>> + */ >>> + struct utsname *uname_ptr = (void *)&tail[-sizeof(struct utsname)]; >>> + if (p >= (char *)(void *)uname_ptr) >> >> 11. Why do you need double cast? I can't imagine that it does anything >> except adds confusion. > > Compiler doesn't like to compare a char pointer with a structure pointer > > src/lib/core/crash.c:251:8: error: comparison of distinct pointer types lacks a cast [-Werror] > 251 | if (p >= (void *)uname_ptr) > | ^~ Can you just use (char *) cast instead of (void *)? Could you simply try that on your own without me suggesting it? >>> + if (uname(uname_ptr) != 0) { >>> + pr_syserr("uname call failed, ignore"); >>> + memset(uname_ptr, 0, sizeof(struct utsname)); >>> + } >>> + >>> + snprintf_safe("{"); >> >> 12. snprintf_safe uses memory [begin, e]. Where 'begin' is the initial >> value of 'p'. 'uname_ptr' occupies [e - 7 - sizeof(*uname_ptr), e - 7]. >> Assume static size is 12KB, and sizeof(*uname_ptr) is 640. >> >> [begin = 0, snprintf e = 12287] >> [11640 uname 12280] >> >> So snprintf and uname buffers intersect. If there is a big enough dump, >> it will corrupt uname object. >> >> Is it correct? > > This is undefined behaviour. I though about it and I don't like this but > there is no other way if we want to use a single buffer for all. This was > the reason while I've been using preallocated memory for all this and you > said to reuse static alloc instead. Another option is to adjust @e pointer > to be sure that we're not jumping into the tail. Letme think maybe I manage > to make this more clear and readable... It is not about clear/readable, for now. You just use buffer which can be corrupted. It is a bug. Regardless of how clear will it look. >>> + if (cinfo->signo == SIGSEGV) { >>> + if (cinfo->sicode == SEGV_MAPERR) { >>> + snprintf_safe("\"si_code_str\":\"%s\",", >>> + "SEGV_MAPERR"); >>> + } else if (cinfo->sicode == SEGV_ACCERR) { >>> + snprintf_safe("\"si_code_str\":\"%s\",", >>> + "SEGV_ACCERR"); >>> + } >>> + snprintf_safe("\"si_addr\":\"0x%llx\",", >>> + (long long)cinfo->siaddr); >>> + } >> >> 13. Instead of using all these global many KB buffers you >> could also create a pipe to the new process, which the >> child would read as stdin from Lua, without any tiny limits >> on how many KBs it can use. And do write() to it directly. > > Pipes use kernel memory for own buffers and what is worse this > memory is not accountable as far as I remember. I don't see any problem with kernel using memory to support pipes. But I do see a problem in the code above, which can simply return garbage or crash again if the dump will be big enough. >>> + >>> + char *timestamp_rt_str = &tail[-128]; >>> + if (p >= timestamp_rt_str) >>> + goto out; >>> + ns_to_localtime(cinfo->timestamp_rt, timestamp_rt_str, 128); >> >> 14. Why 128? And why is it hardcoded? > > This should fit any possible timestring I think. The timestring is bound > to locale and I think it might vary in size. I choose a big value but > to be honest I'm not 100% if this will be enough forever. Ok, then the second question remains - why is it hardcoded in 2 places? I remember you sent some commits to the other source files where you changed such places to use sizeof() or const variable or something like that. How is it different here? >>> + snprintf_safe("\"timestamp\":\"%s\"", timestamp_rt_str); >>> + snprintf_safe("}"); >>> + snprintf_safe("}"); >>> + >>> + size_t report_len = p - head; >>> + size_t report_elen = base64_bufsize(report_len, BASE64_NOWRAP); >>> + >>> + char *report_base64 = &tail[-report_elen-8]; >> >> 15. Why -8? > > Already explained. Didn't understand, sorry. >>> + pid_t pid = vfork(); >>> + if (pid == 0) { >>> + /* >>> + * The script must exit at the end but there >>> + * is no simple way to make sure from inside >>> + * of a signal crash handler. So just hope it >>> + * is running fine. >>> + */ >>> + execve(exec_argv[0], exec_argv, NULL); >>> + pr_crit("exec(%s,[%s,%s,%s,NULL]) failed", >>> + exec_argv[0], exec_argv[0], >>> + exec_argv[1], exec_argv[2]); >> >> 17. What will happen if this tarantool also will crash? Will there >> be an infinite chain of crashes? For instance, if we will have an >> issue with curl again. Or somebody else will build with his own >> curl leading to a crash on any usage, like it already happened one >> time. > > Strictly speaking -- yes, you're right. We might have a chain of crashes > and I think we might need a command line option for this? If it will crash before checking the command line option, it won't help. I see that feedback_daemon is disabled until first box.cfg. I suggest you do the same here - disable crash sending by default. Enable it with first box.cfg. In your script you don't use box.cfg, so a chain crash won't happen. Unless some bug will override the flag responsible for crash send. I also realized, that before box.cfg you don't know the URL. So if I crash the process before it, I get this: SystemError curl: URL using bad/illegal format or missing URL: Invalid argument fatal error, exiting the event loop This is because you try to send the dump before the crash module is configured.
next prev parent reply other threads:[~2020-12-20 14:48 UTC|newest] Thread overview: 38+ messages / expand[flat|nested] mbox.gz Atom feed top 2020-12-10 16:18 [Tarantool-patches] [PATCH v4 0/4] crash: implement sending feedback Cyrill Gorcunov 2020-12-10 16:18 ` [Tarantool-patches] [PATCH v4 1/4] util: introduce strlcpy helper Cyrill Gorcunov 2020-12-11 7:34 ` Serge Petrenko 2020-12-11 7:58 ` Serge Petrenko 2020-12-11 10:04 ` Cyrill Gorcunov 2020-12-11 11:07 ` Serge Petrenko 2020-12-11 11:38 ` Cyrill Gorcunov 2020-12-14 22:54 ` Vladislav Shpilevoy 2020-12-14 22:54 ` Vladislav Shpilevoy 2020-12-15 8:47 ` Cyrill Gorcunov 2020-12-10 16:18 ` [Tarantool-patches] [PATCH v4 2/4] backtrace: allow to specify destination buffer Cyrill Gorcunov 2020-12-11 7:50 ` Serge Petrenko 2020-12-10 16:18 ` [Tarantool-patches] [PATCH v4 3/4] crash: move fatal signal handling in Cyrill Gorcunov 2020-12-11 9:31 ` Serge Petrenko 2020-12-11 10:38 ` Cyrill Gorcunov 2020-12-11 11:12 ` Serge Petrenko 2020-12-14 22:54 ` Vladislav Shpilevoy 2020-12-15 8:16 ` Cyrill Gorcunov 2020-12-20 14:48 ` Vladislav Shpilevoy 2020-12-20 15:49 ` Cyrill Gorcunov 2020-12-20 16:07 ` Vladislav Shpilevoy 2020-12-20 16:58 ` Cyrill Gorcunov 2020-12-20 15:45 ` Vladislav Shpilevoy 2020-12-10 16:18 ` [Tarantool-patches] [PATCH v4 4/4] crash: report crash data to the feedback server Cyrill Gorcunov 2020-12-11 12:57 ` Serge Petrenko 2020-12-12 16:19 ` Cyrill Gorcunov 2020-12-12 17:07 ` Cyrill Gorcunov 2020-12-14 9:41 ` Serge Petrenko 2020-12-14 22:54 ` Vladislav Shpilevoy 2020-12-16 11:16 ` Cyrill Gorcunov 2020-12-16 20:31 ` Cyrill Gorcunov 2020-12-20 15:16 ` Vladislav Shpilevoy 2020-12-20 18:26 ` Cyrill Gorcunov 2020-12-20 14:48 ` Vladislav Shpilevoy [this message] 2020-12-20 18:21 ` Cyrill Gorcunov 2020-12-20 18:41 ` Vladislav Shpilevoy 2020-12-20 19:16 ` Cyrill Gorcunov 2020-12-21 17:01 ` Vladislav Shpilevoy
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=0bb6f2db-5ef8-9d71-be61-67e113c7e0ad@tarantool.org \ --to=v.shpilevoy@tarantool.org \ --cc=gorcunov@gmail.com \ --cc=tarantool-patches@dev.tarantool.org \ --subject='Re: [Tarantool-patches] [PATCH v4 4/4] crash: report crash data to the feedback server' \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: link
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox