Project

General

Profile

Actions

Optimization #3658

closed

Use WARN_UNUSED for ByteExtract* functions

Added by Philippe Antoine over 4 years ago. Updated about 2 years ago.

Status:
Closed
Priority:
Normal
Assignee:
Target version:
Effort:
Difficulty:
Label:
Beginner, C, Outreachy

Description

The return value should be checked
This helps the compiler finding where it is not the case.

And fix the code which do not check these return values


Files

Actions #1

Updated by Victor Julien over 4 years ago

  • Status changed from New to Assigned
  • Assignee set to Shivani Bhardwaj
  • Target version changed from TBD to 6.0.0beta1
Actions #2

Updated by Victor Julien over 4 years ago

  • Target version changed from 6.0.0beta1 to 7.0.0-beta1
Actions #3

Updated by Shivani Bhardwaj about 4 years ago

  • Label Beginner, C, Outreachy added
Actions #4

Updated by Shivani Bhardwaj about 4 years ago

  • Assignee changed from Shivani Bhardwaj to Community Ticket
Actions #5

Updated by Janani Ramjee about 4 years ago

  • Assignee changed from Community Ticket to Janani Ramjee
Actions #6

Updated by Victor Julien over 3 years ago

  • Status changed from Assigned to New
  • Assignee deleted (Janani Ramjee)
  • Target version deleted (7.0.0-beta1)
Actions #7

Updated by Haleema Khan about 2 years ago

I want to work on this issue.
As per my understanding, I need to add the WARN_UNUSED macro to all the ByteExtract* functions e.g

 int ByteExtractUint16(res, BYTE_LITTLE_ENDIAN, sizeof(uint16_t),
            (const uint8_t *) (input + *offset));

would become:
   int WARN_UNUSED ByteExtractUint16(res, BYTE_LITTLE_ENDIAN, sizeof(uint16_t),
            (const uint8_t *) (input + *offset));

Actions #8

Updated by Haleema Khan about 2 years ago

  • Assignee set to Haleema Khan
Actions #9

Updated by Philippe Antoine about 2 years ago

Exactly, then the warnings need to be fixed

Actions #10

Updated by Haleema Khan about 2 years ago

Do I need to run unit tests to generate those warnings or the clippy tool?

Actions #11

Updated by Philippe Antoine about 2 years ago

These warnings should come out of clang, not clippy...

Actions #12

Updated by Haleema Khan about 2 years ago

I added the macro and ran clang filname.c.

I am getting a bunch of warnings and errors. Were you referring to those and how can I go about fixing those. e.g Following is a portion of the log that I am getting:

In file included from src/util-byte.c:26:
src/suricata-common.h:44:2: warning: "L1 cache line size not detected during build. Assuming 64 bytes." [-W#warnings]
#warning "L1 cache line size not detected during the build. Assuming 64 bytes." 
 ^
In file included from src/util-byte.c:26:
In file included from src/suricata-common.h:141:
/usr/local/include/pcre2.h:969:2: error: PCRE2_CODE_UNIT_WIDTH must be defined before including pcre2.h.
#error PCRE2_CODE_UNIT_WIDTH must be defined before including pcre2.h.
 ^
/usr/local/include/pcre2.h:970:2: error: Use 8, 16, or 32; or 0 for a multi-width application.
#error Use 8, 16, or 32; or 0 for a multi-width application.

EDIT: I don't think they are the warnings I am looking for. I am trying to use scan-build but due to some installation issue its not working for me.
To me, they don't seem to be connected to the changes I made.

Actions #13

Updated by Haleema Khan about 2 years ago

When I try to run the Clang check I get errors. One of these is related to #include <pcre2.h> and #include <byteswap.h>.
These files are not found. I searched in my local repo and then Suricata's main repo to make sure I have not messed up the files but confirmed that they are not there. What should I do about this? Should I remove the include statements altogether?

Actions #14

Updated by Philippe Antoine about 2 years ago

You should compile Suricata with ./configure then make
make will call clang with the right flags, including those for PCRE2

Actions #15

Updated by Haleema Khan about 2 years ago

I am currently working on fixing the WARN_UNUSED warning from this.
I am having a hard time understanding this function. The description says "Extract 16 bits and move up the offset".
the ByteExtract function return value is never used, hence the error. So I understand it is mainly extracting the bytes from the given string for "ENIPEtxtract" but then doing nothing.
What I don't understand is the intention of "ENIPExtractUint16" function. It is returning 1 so the output of ByteExtract is to be added to the offset? that seems to be the only way the result can be used .
Another way I can think of is adding an If{} block that returns 1 if "ByteExtract" returns an output.

Actions #17

Updated by Haleema Khan about 2 years ago

  • Status changed from New to In Progress
Actions #18

Updated by Philippe Antoine about 2 years ago

What about something like

diff --git a/src/app-layer-enip-common.c b/src/app-layer-enip-common.c
index 91f06e731..407f7cc05 100644
--- a/src/app-layer-enip-common.c
+++ b/src/app-layer-enip-common.c
@@ -70,8 +70,10 @@ static int ENIPExtractUint16(uint16_t *res, const uint8_t *input, uint16_t *offs
         return 0;
     }

-    ByteExtractUint16(res, BYTE_LITTLE_ENDIAN, sizeof(uint16_t),
-            (const uint8_t *) (input + *offset));
+    if (ByteExtractUint16(res, BYTE_LITTLE_ENDIAN, sizeof(uint16_t),
+                          (const uint8_t *) (input + *offset)) < 0) {
+        return 0;
+    }
     *offset += sizeof(uint16_t);
     return 1;
 }

Actions #19

Updated by Shivani Bhardwaj about 2 years ago

  • Target version set to 7.0.0-beta1
Actions #20

Updated by Haleema Khan about 2 years ago

  • Status changed from In Progress to In Review
Actions #21

Updated by Victor Julien about 2 years ago

  • Status changed from In Review to Closed
Actions

Also available in: Atom PDF