Fixes from Coverity Report (12/01/2017) #66

RanierV · 2017-12-01T19:13:10Z

Hi,
Can yours take a look.

Best regards,

Ranier Vilela

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

alejandro-colomar · 2022-09-01T10:47:41Z

src/nxt_process_title.c

@@ -178,6 +178,12 @@ nxt_process_arguments(nxt_task_t *task, char **orig_argv, char ***orig_envp)
    }

 done:
+    if (p != NULL) {
+       nxt_free(p);


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.

alejandro-colomar · 2022-09-01T10:49:55Z

src/nxt_process_title.c

@@ -130,6 +125,11 @@ nxt_process_arguments(nxt_task_t *task, char **orig_argv, char ***orig_envp)
        goto done;
    }

+    p = nxt_malloc(strings_size);


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.

alejandro-colomar · 2022-10-24T23:07:08Z

@raniervf There are some fixes that seem likely to be correct. Would you mind revisiting this pull request and fixing the issues I mentioned?

ac000 · 2023-11-14T19:46:48Z

src/nxt_php_sapi.c

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.

... and as such is no longer flagged by coverity...

ac000 · 2023-11-14T20:09:05Z

src/nxt_conn_write.c

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, #endif

and we only possibly take the problematic code path if we are using nxt_event_conn_io_sendbuf().

ac000 · 2023-11-15T01:34:22Z

src/nxt_linux_sendfile.c

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

callahad · 2024-06-17T08:41:09Z

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?

ac000

This file was removed in commit 22c5100 and as such is no longer flagged by coverity.

ac000

While that would certainly placate the analysers. I think a slightly larger cleanup is in order.

callahad · 2024-10-14T08:57:33Z

With #1441 merged, do we still need this PR open?

ac000 · 2024-10-29T01:54:56Z

With #1441 merged, do we still need this PR open?

That was only one part of this.

Probably need to go through this PR and see what is still open in Coverity...

RanierV added 7 commits December 1, 2017 15:17

Update nxt_conn_write.c

2ee3265

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]

Update nxt_php_sapi.c

676ab95

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.

Update nxt_time_parse.c

ec6cdeb

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.

Update nxt_socketpair.c

858cb4d

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

Update nxt_process_title.c

9173b78

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.

Update nxt_linux_sendfile.c

a67463b

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.

Update nxt_event_conn_job_sendfile.c

d28a21b

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

VBart self-assigned this Dec 1, 2017

alejandro-colomar reviewed Sep 1, 2022

View reviewed changes

alejandro-colomar unassigned VBart Sep 1, 2022

nginx-hg-mirror force-pushed the master branch from 8cb7e0b to 5a37171 Compare January 31, 2023 23:08

This comment was marked as duplicate.

Sign in to view

ac000 reviewed Nov 14, 2023

View reviewed changes

ac000 reviewed Nov 15, 2023

View reviewed changes

ac000 reviewed Sep 23, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixes from Coverity Report (12/01/2017) #66

Fixes from Coverity Report (12/01/2017) #66

RanierV commented Dec 1, 2017

alejandro-colomar Sep 1, 2022

alejandro-colomar Sep 1, 2022 •

edited

Loading

alejandro-colomar commented Oct 24, 2022 •

edited

Loading

This comment was marked as duplicate.

ac000 Nov 14, 2023

ac000 Sep 23, 2024

ac000 Nov 14, 2023

ac000 Nov 15, 2023

callahad commented Jun 17, 2024

ac000 left a comment

ac000 left a comment

callahad commented Oct 14, 2024 •

edited

Loading

ac000 commented Oct 29, 2024

Fixes from Coverity Report (12/01/2017) #66

Are you sure you want to change the base?

Fixes from Coverity Report (12/01/2017) #66

Conversation

RanierV commented Dec 1, 2017

alejandro-colomar Sep 1, 2022

Choose a reason for hiding this comment

alejandro-colomar Sep 1, 2022 • edited Loading

Choose a reason for hiding this comment

alejandro-colomar commented Oct 24, 2022 • edited Loading

This comment was marked as duplicate.

ac000 Nov 14, 2023

Choose a reason for hiding this comment

ac000 Sep 23, 2024

Choose a reason for hiding this comment

ac000 Nov 14, 2023

Choose a reason for hiding this comment

ac000 Nov 15, 2023

Choose a reason for hiding this comment

callahad commented Jun 17, 2024

ac000 left a comment

Choose a reason for hiding this comment

ac000 left a comment

Choose a reason for hiding this comment

callahad commented Oct 14, 2024 • edited Loading

ac000 commented Oct 29, 2024

alejandro-colomar Sep 1, 2022 •

edited

Loading

alejandro-colomar commented Oct 24, 2022 •

edited

Loading

callahad commented Oct 14, 2024 •

edited

Loading