Fix issues reported by Coverity

[cristgl]

Related issue
#3621

Description

This PR fixes most of the defects reported by Coverity at the mentioned issue. Some of them are false positives and the ones that haven't been fixed (not marked as done at the issue) are related to tainted variables.

Tests

• Compilation without warnings in every supported platform
  • Linux
  • MAC OS X
• Source installation
• Source upgrade
• Memory tests
  • Valgrind report for affected components
    ossec-analysisd:

==30493== LEAK SUMMARY:
==30493==    definitely lost: 100 bytes in 9 blocks
==30493==    indirectly lost: 0 bytes in 0 blocks
==30493==      possibly lost: 28,832 bytes in 106 blocks
==30493==    still reachable: 12,147,091 bytes in 58,196 blocks
==30493==         suppressed: 0 bytes in 0 blocks
==30493== Reachable blocks (those to which a pointer was found) are not shown.

wazuh-modulesd (threads and database errors, same as in version 3.9.2):

==3397== 89 bytes in 1 blocks are possibly lost in loss record 54 of 93
==3397==    at 0x4C2FB0F: malloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==3397==    by 0x4012C94: allocate_dtv_entry (dl-tls.c:582)
==3397==    by 0x4012C94: allocate_and_init (dl-tls.c:607)
==3397==    by 0x4012C94: tls_get_addr_tail (dl-tls.c:787)
==3397==    by 0x4019A27: __tls_get_addr (tls_get_addr.S:55)
==3397==    by 0xD2F6FF1: ??? (in /lib/x86_64-linux-gnu/libnss_systemd.so.2)
==3397==    by 0xD2FCA35: ??? (in /lib/x86_64-linux-gnu/libnss_systemd.so.2)
==3397==    by 0xD2F74B2: ??? (in /lib/x86_64-linux-gnu/libnss_systemd.so.2)
==3397==    by 0xD30C48E: ??? (in /lib/x86_64-linux-gnu/libnss_systemd.so.2)
==3397==    by 0xD30C8CB: ??? (in /lib/x86_64-linux-gnu/libnss_systemd.so.2)
==3397==    by 0xD2F1B56: _nss_systemd_getpwuid_r (in /lib/x86_64-linux-gnu/libnss_systemd.so.2)
==3397==    by 0x5B6AD34: getpwuid_r@@GLIBC_2.2.5 (getXXbyYY_r.c:315)
==3397==    by 0x5B6A497: getpwuid (getXXbyYY.c:135)
==3397==    by 0x5233729: user_from_uid (pwcache.c:42)
==3397== 
==3397== 272 bytes in 1 blocks are possibly lost in loss record 70 of 93
==3397==    at 0x4C31B25: calloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==3397==    by 0x40134A6: allocate_dtv (dl-tls.c:286)
==3397==    by 0x40134A6: _dl_allocate_tls (dl-tls.c:530)
==3397==    by 0x5870227: allocate_stack (allocatestack.c:627)
==3397==    by 0x5870227: pthread_create@@GLIBC_2.2.5 (pthread_create.c:644)
==3397==    by 0x13C5A4: CreateThreadJoinable (pthreads_op.c:47)
==3397==    by 0x13C64B: CreateThread (pthreads_op.c:62)
==3397==    by 0x111EC0: main (main.c:102)
==3397== 
==3397== 1,632 bytes in 6 blocks are possibly lost in loss record 85 of 93
==3397==    at 0x4C31B25: calloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==3397==    by 0x40134A6: allocate_dtv (dl-tls.c:286)
==3397==    by 0x40134A6: _dl_allocate_tls (dl-tls.c:530)
==3397==    by 0x5870227: allocate_stack (allocatestack.c:627)
==3397==    by 0x5870227: pthread_create@@GLIBC_2.2.5 (pthread_create.c:644)
==3397==    by 0x13C5A4: CreateThreadJoinable (pthreads_op.c:47)
==3397==    by 0x111E1F: main (main.c:95)
==3397== 
==3397== 4,104 bytes in 1 blocks are possibly lost in loss record 89 of 93
==3397==    at 0x4C2FB0F: malloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==3397==    by 0x50FEDAA: sqlite3MemMalloc (sqlite3.c:20790)
==3397==    by 0x50FF7A2: mallocWithAlarm (sqlite3.c:24464)
==3397==    by 0x50FF84B: sqlite3Malloc (sqlite3.c:24494)
==3397==    by 0x510DC36: pcache1Alloc (sqlite3.c:45336)
==3397==    by 0x510DF24: sqlite3PageMalloc (sqlite3.c:45479)
==3397==    by 0x511E55F: allocateTempSpace (sqlite3.c:61560)
==3397==    by 0x51208CA: btreeCursor (sqlite3.c:63234)
==3397==    by 0x5120A3C: sqlite3BtreeCursor (sqlite3.c:63276)
==3397==    by 0x513C44F: sqlite3VdbeExec (sqlite3.c:81807)
==3397==    by 0x5135036: sqlite3Step (sqlite3.c:76693)
==3397==    by 0x51351E4: sqlite3_step (sqlite3.c:76754)
==3397== 
==3397== LEAK SUMMARY:
==3397==    definitely lost: 0 bytes in 0 blocks
==3397==    indirectly lost: 0 bytes in 0 blocks
==3397==      possibly lost: 6,097 bytes in 9 blocks
==3397==    still reachable: 369,222 bytes in 194 blocks
==3397==                       of which reachable via heuristic:
==3397==                         length64           : 223,896 bytes in 124 blocks
==3397==         suppressed: 0 bytes in 0 blocks
==3397== Reachable blocks (those to which a pointer was found) are not shown.
==3397== To see them, rerun with: --leak-check=full --show-leak-kinds=all

[cristgl]

The TempFile function has more than one task, among which we can find the creation of a temporary file, the acquisition of a second file's permissions, the copy of these permissions to the created file and the copy of the second file's content to the created one.

This is confusing since the task that we might think this function would do by its title is to create a temporary file. We propose dividing TempFile into two different functions, one that creates this temporary file and another one that copies the content. Also, in case that the second file doesn't exist, we would call umask with the permissions that this file frequently have.

[chemamartinez]

Testing LogCollector behavior (reading audit format logs):

Write log in two different times

Inputs

# echo -n "type=USER_LOGIN msg=audit(1563533983.190:208): pid=49556 uid=0" >> /var/log/audit/audit.log
# echo " auid=0 ses=9 msg='op=login id=0 exe="/usr/sbin/sshd" hostname=172.16.98.1 addr=172.16.98.1 terminal=/dev/pts/2 res=success'" >> /var/log/audit/audit.log

Events output:

2019 Jul 19 04:04:06 ubuntu->/var/log/audit/audit.log type=USER_LOGIN msg=audit(1563533983.190:208): pid=49556 uid=0 auid=0 ses=9 msg='op=login id=0 exe=/usr/sbin/sshd hostname=172.16.98.1 addr=172.16.98.1 terminal=/dev/pts/2 res=success'

The log is not read until \n is found.

Truncate file while LogCollector is reading it

Configuration

<localfile>
    <log_format>audit</log_format>
    <location>/root/test/test.log</location>
</localfile>

Stressing LogCollector for that file:

# for i in `seq 1 1000000`; do echo "type=USER_LOGIN msg=audit(1563534870.202:$i): test" >> /root/test/test.log; done;

While reading the 1,000,000 lines, we truncate the file:

# echo -n > /root/test/test.log

LogCollector output:

2019/07/19 04:21:05 ossec-logcollector[50139] read_audit.c:92 at read_audit(): DEBUG: Message not complete. Trying again: 'type=USER_LOGIN msg=audit(15635348'
2019/07/19 04:21:05 ossec-logcollector[50139] read_audit.c:130 at read_audit(): DEBUG: Read 5226 lines from /root/test/test.log
2019/07/19 04:21:24 ossec-logcollector[50139] logcollector.c:584 at LogCollectorStart(): DEBUG: File size reduced. /root/test/test.log

After the operation, the log file is being reading again:

2019/07/19 04:21:05 ossec-logcollector[50139] read_audit.c:92 at read_audit(): DEBUG: Message not complete. Trying again: 'type=USER_LOGIN msg=audit(15635348'
2019/07/19 04:21:05 ossec-logcollector[50139] read_audit.c:130 at read_audit(): DEBUG: Read 5226 lines from /root/test/test.log
2019/07/19 04:21:24 ossec-logcollector[50139] logcollector.c:584 at LogCollectorStart(): DEBUG: File size reduced. /root/test/test.log
2019/07/19 04:23:35 ossec-logcollector[50139] read_audit.c:130 at read_audit(): DEBUG: Read 100 lines from /root/test/test.log

It works as expected.

Reloading wildcards

<localfile>
    <log_format>audit</log_format>
    <location>/root/test/*.log</location>
</localfile>

When using wildcards and a file reappears, it takes a while for LogCollector to follow the new file. Logs written during the reloading process have to be read as well.

1. We write the file and delete it:

# for i in `seq 1 100`; do echo "type=USER_LOGIN msg=audit(1563534870.202:$i): test" >> /root/test/test.log; done;
# rm -f /root/test/test.log

LogCollector output:

2019/07/19 04:30:44 ossec-logcollector[50300] read_audit.c:130 at read_audit(): DEBUG: Read 100 lines from /root/test/test.log
2019/07/19 04:31:36 ossec-logcollector[50300] logcollector.c:457 at LogCollectorStart(): INFO: (1959): File '/root/test/test.log' no longer exists.
2019/07/19 04:31:36 ossec-logcollector[50300] logcollector.c:467 at LogCollectorStart(): DEBUG: (1961): Files being monitored: 0/1000.
2019/07/19 04:31:36 ossec-logcollector[50300] logcollector.c:1130 at check_pattern_expand(): DEBUG: (1122): No file found by pattern: '/root/test/*.log'.

2. We create the log file again, and write it before it is recognized by LogCollector:

# touch /root/test/test.log
# for i in `seq 1 100`; do echo "type=USER_LOGIN msg=audit(1563534870.202:$i): test" >> /root/test/test.log; done;

LogCollector output:

2019/07/19 04:32:42 ossec-logcollector[50300] logcollector.c:1154 at check_pattern_expand(): INFO: (1957): New file that matches the '/root/test/*.log' pattern: '/root/test/test.log'.
2019/07/19 04:32:42 ossec-logcollector[50300] logcollector.c:1169 at check_pattern_expand(): DEBUG: (1961): Files being monitored: 1/1000.
2019/07/19 04:32:44 ossec-logcollector[50300] read_audit.c:130 at read_audit(): DEBUG: Read 100 lines from /root/test/test.log

It starts to read at the first line of the file, as expected.

No memory issues during the tests

==50300== LEAK SUMMARY:
==50300==    definitely lost: 0 bytes in 0 blocks
==50300==    indirectly lost: 0 bytes in 0 blocks
==50300==      possibly lost: 1,632 bytes in 6 blocks
==50300==    still reachable: 35,063 bytes in 22 blocks
==50300==         suppressed: 0 bytes in 0 blocks
==50300== Reachable blocks (those to which a pointer was found) are not shown.
==50300== To see them, rerun with: --leak-check=full --show-leak-kinds=all
==50300==
==50300== For counts of detected and suppressed errors, rerun with: -v
==50300== ERROR SUMMARY: 3 errors from 3 contexts (suppressed: 0 from 0)