Hello!

On Tue, Jul 02, 2013 at 02:00:40AM +0900, cubicdaiya wrote:

> Hello!
>
> In Debian squeeze 32bit,
> Valgrind outputs the following message to nginx.
>
> ==17124== Syscall param epoll_ctl(event) points to uninitialised byte(s)
> ==17124== at 0x418F9CE: epoll_ctl (syscall-template.S:82)
> ==17124== by 0x805FB35: ngx_event_process_init (ngx_event.c:853)

[...]

> The following patch eliminates this warning. Could you take a look at it?
>
> # HG changeset patch
> # User Tatsuhiko Kubo <cubicdaiya@gmail.com>
> # Date 1372689447 -32400
> # Node ID cd8fd5cd74294554bb3777821e8703cf0fdf61d7
> # Parent b66ec10e901a6fa0fc19937ceeb52b5ea1fbb706
> Valgrind: the complaint about uninitialized bytes in epoll_data_t.
>
> diff -r b66ec10e901a -r cd8fd5cd7429 src/event/modules/ngx_epoll_module.c
> --- a/src/event/modules/ngx_epoll_module.c Fri Jun 28 13:55:05 2013 +0400
> +++ b/src/event/modules/ngx_epoll_module.c Mon Jul 01 23:37:27 2013 +0900
> @@ -417,6 +417,9 @@
> }
>
> ee.events = events | (uint32_t) flags;
> +
> + ngx_memzero(&ee.data, sizeof(epoll_data_t));
> +
> ee.data.ptr = (void *) ((uintptr_t) c | ev->instance);
>
> ngx_log_debug3(NGX_LOG_DEBUG_EVENT, ev->log, 0,

I can't say I like the patch.

Calls to epoll_ctl() are at hot path, and doing an unneeded
memzero to please Valgrind on some archs doesn't looks like a good
idea. Maybe under #if (NGX_VALGRIND) (and separately from normal
assignments to ee structure; also not sure if we need memzero
here, probably ee.data.u64 = 0 would be better).

Note well that the same coding pattern is used in many places
across the epoll module, and changing only one place doesn't make
sense.

--
Maxim Dounin
http://nginx.org/en/donation.html

_______________________________________________
nginx-devel mailing list
nginx-devel@nginx.org
http://mailman.nginx.org/mailman/listinfo/nginx-devel