Fixes from Coverity Report (12/01/2017) #66
Conversation
Fix Coverity report: CID 200492 (nginx#1 of 1): Uninitialized scalar variable (UNINIT)2. uninit_use_in_call: Using uninitialized value sb.last when calling nxt_sendbuf_mem_coalesce. [show details]
CID 200488 (nginx#1 of 2): Dereference null return value (NULL_RETURNS)11. dereference: Dereferencing a pointer that might be null run_ctx.mem_pool when calling nxt_mp_destroy.
CID 200485 (nginx#1 of 1): Unused value (UNUSED_VALUE)assigned_value: Assigning value st_letter to state here, but that stored value is overwritten before it can be used.
CID 200484 (nginx#1 of 1): Uninitialized scalar variable (UNINIT)2. uninit_use_in_call: Using uninitialized value msg. Field msg.msg_flags is uninitialized when calling recvmsg
CID 200480 (nginx#2 of 2): Resource leak (RESOURCE_LEAK)22. leaked_storage: Variable p going out of scope leaks the storage it points to.
CID 200475 (nginx#1 of 1): Uninitialized scalar variable (UNINIT)2. uninit_use_in_call: Using uninitialized value sb.last when calling nxt_sendbuf_mem_coalesce.
CID 200471 (nginx#1 of 1): Uninitialized scalar variable (UNINIT)5. uninit_use_in_call: Using uninitialized value sb.last when calling nxt_sendbuf_mem_coalesce
| @@ -178,6 +178,12 @@ nxt_process_arguments(nxt_task_t *task, char **orig_argv, char ***orig_envp) | |||
| } | |||
|
|
|||
| done: | |||
| if (p != NULL) { | |||
| nxt_free(p); | |||
There was a problem hiding this comment.
Hi, I just saw this old PR around, and had a fast look at it. nxt_free(), as well as free(3), can handle NULL just fine. The conditionals are not necessary.
| @@ -130,6 +125,11 @@ nxt_process_arguments(nxt_task_t *task, char **orig_argv, char ***orig_envp) | |||
| goto done; | |||
| } | |||
|
|
|||
| p = nxt_malloc(strings_size); | |||
There was a problem hiding this comment.
If you move p after the other goto done, you'll arrive at nxt_free() with an uninitialized pointer, which will result in Undefined Behavior.
That is, consider you jump to done from line 125 after this patch. You'll free p, which is uninitialized, and contains a random value. That will likely crash.
|
@raniervf There are some fixes that seem likely to be correct. Would you mind revisiting this pull request and fixing the issues I mentioned? |
nginx-hg-mirror
force-pushed
the
master
branch
from
8cb7e0b
to
5a37171
Compare
This comment was marked as duplicate.
This comment was marked as duplicate.
Sorry, something went wrong.
There was a problem hiding this comment.
Perhaps not too surprisingly this code no longer exists. nxt_mp_create() is no longer called and was removed by
commit 349717fb90edaf50ae2846db7b72a2da4285541b
Author: Max Romanov <[email protected]>
Date: Thu Jan 11 22:14:20 2018 +0300
Changing relative php scripts paths to real ones.
This is required to run phpMyAdmin.
There was a problem hiding this comment.
This still looks valid, though likely has a pretty slim chance of triggering due to
#if (NXT_HAVE_LINUX_SENDFILE)
.old_sendbuf = nxt_linux_event_conn_io_sendfile,
#elif (NXT_HAVE_FREEBSD_SENDFILE)
.old_sendbuf = nxt_freebsd_event_conn_io_sendfile,
#elif (NXT_HAVE_MACOSX_SENDFILE)
.old_sendbuf = nxt_macosx_event_conn_io_sendfile,
#elif (NXT_HAVE_SOLARIS_SENDFILEV)
.old_sendbuf = nxt_solaris_event_conn_io_sendfilev,
#elif (NXT_HAVE_AIX_SEND_FILE)
.old_sendbuf = nxt_aix_event_conn_io_send_file,
#elif (NXT_HAVE_HPUX_SENDFILE)
.old_sendbuf = nxt_hpux_event_conn_io_sendfile,
#else
.old_sendbuf = nxt_event_conn_io_sendbuf,
#endifand we only possibly take the problematic code path if we are using nxt_event_conn_io_sendbuf().
There was a problem hiding this comment.
This looks valid, assuming that we can go
nxt_linux_event_conn_io_sendfile(() -> nxt_sendbuf_mem_coalesce()
and then
if (nxt_buf_is_mem(b)) {can be false
|
Not entirely sure why this has lingered for so long. @ac000 Do you think we can resolve this as-is this week, or should we close this and start anew with a fresh Coverity run? |
Hi,
Can yours take a look.
Best regards,
Ranier Vilela