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.

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

zlog_reload can cause a crash by freeing memory used by zlog_spec_write_time. #7

Closed
pattheaux opened this issue Sep 19, 2012 · 3 comments

Comments

@pattheaux
Copy link

The last_time_format stored per thread in a_thread->event->last_time_fmt points to memory allocated in a zlog_spec_t. When zlog_reload deletes and recreates the the spec array it frees this memory but the thread storage still holds a pointer to it. The quick fix is to disable caching the last time format in spec.c:

static int zlog_spec_write_time(zlog_spec_t * a_spec, zlog_thread_t * a_thread, zlog_buf_t * a_buf)
{
/* do fetch time every event once */
zlog_spec_fetch_time;

/* strftime %d() is slow too, do it when 
 * time_fmt changed(event go through another spec) */
if (a_thread->event->last_time_fmt != a_spec->time_fmt) {
            /* The last_time_fmt memory can be free'd when zlog_reload deletes the formats */
            /* disable this for now.                                                       */
    //a_thread->event->last_time_fmt = a_spec->time_fmt;    <----- Comment out this line
@HardySimpson
Copy link
Owner

Yes, you are right. I fix it and launch zlog 1.2.2....

@HardySimpson
Copy link
Owner

The last fix is not correct, simplely kick the cache will cause sometimes not fetch time. so I fix it again....

I think this fix will work, isn't it ?

static int zlog_spec_write_time(zlog_spec_t * a_spec, zlog_thread_t * a_thread, zlog_buf_t * a_buf)
{
        if (!a_thread->event->time_stamp.tv_sec) {
                gettimeofday(&(a_thread->event->time_stamp), NULL);
        }

        /* a_event->time_str is a cache, as strftime is slow.
         * It is modified when this event meets another time specifier, or time slips one second.
         * So it is a weak cache, just work when a event goes through only one time specifier.
         * If there is any better way, please tell me!
         */
        if (
             (a_thread->event->time_stamp.tv_sec != a_thread->event->time_last)
             || (a_spec->time_fmt != a_thread->event->time_fmt_last)
           ) {
                a_thread->event->time_last = a_thread->event->time_stamp.tv_sec;
                a_thread->event->time_fmt_last = a_spec->time_fmt;

                localtime_r(&(a_thread->event->time_stamp.tv_sec),
                            &(a_thread->event->time_local));

                a_thread->event->time_str_len = strftime(a_thread->event->time_str,
                        sizeof(a_thread->event->time_str),
                        a_spec->time_fmt, &(a_thread->event->time_local));
        }
        return zlog_buf_append(a_buf, a_thread->event->time_str, a_thread->event->time_str_len);
}

@HardySimpson
Copy link
Owner

I improve it on 1.2.5,
NO time_fmt in zlog_event_t anymore,
where every time spec can have it's own strftime cache!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants