frames: avoid possible undefined behavior

Code analyzer flagged FrameCopy as a possible source of UB due to
both pointers passed to memcpy being the same.

app-layer-frames.c: In function ‘FrameCopy’:
app-layer-frames.c:236:5: warning: overlapping buffers passed as arguments to ‘memcpy’ [-Wanalyzer-overlapping-buffers]
  236 |     memcpy(dst, src, sizeof(*dst));
      |     ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
  ‘FramePrune’: events 1-8
    │
    │  750 | static void FramePrune(Frames *frames, const TcpStream *stream, const bool eof)
    │      |             ^~~~~~~~~~
    │      |             |
    │      |             (1) entry to ‘FramePrune’
    │......
    │  766 |     for (uint16_t i = 0; i < frames->cnt; i++) {
    │      |                          ~~~~~~~~~~~~~~~
    │      |                            |
    │      |                            (2) following ‘true’ branch... ─>─┐
    │      |                                                              │
    │      |                                                              │
    │      |┌─────────────────────────────────────────────────────────────┘
    │  767 |│        if (i < FRAMES_STATIC_CNT) {
    │      |│           ~
    │      |│           |
    │      |└──────────>(3) ...to here
    │      |            (4) following ‘true’ branch (when ‘i <= 2’)... ─>─┐
    │      |                                                              │
    │      |                                                              │
    │      |┌─────────────────────────────────────────────────────────────┘
    │  768 |│            Frame *frame = &frames->sframes[i];
    │      |│                            ~~~~~~~~~~~~~~~~~~
    │      |│                                           |
    │      |└──────────────────────────────────────────>(5) ...to here
    │  769 |             FrameDebug("prune(s)", frames, frame);
    │  770 |             if (eof || FrameIsDone(frame, acked)) {
    │      |                ~
    │      |                |
    │      |                (6) following ‘false’ branch... ─>─┐
    │      |                                                   │
    │......
    │      |                                                   │
    │      |┌──────────────────────────────────────────────────┘
    │  779 |│                const uint64_t fle = FrameLeftEdge(stream, frame);
    │      |│                                     ~~~~~~~~~~~~~~~~~~~~~~~~~~~~
    │      |│                                     |
    │      |└────────────────────────────────────>(7) ...to here
    │      |                                      (8) calling ‘FrameLeftEdge’ from ‘FramePrune’
    │
    └──> ‘FrameLeftEdge’: event 9
           │
           │  257 | static inline uint64_t FrameLeftEdge(const TcpStream *stream, const Frame *frame)
           │      |                        ^~~~~~~~~~~~~
           │      |                        |
           │      |                        (9) entry to ‘FrameLeftEdge’
           │
         ‘FrameLeftEdge’: event 10
           │
           │suricata-common.h:323:27:
           │  323 |         #define BUG_ON(x) assert(!(x))
           │      |                           ^~~~~~
           │      |                           |
           │      |                           (10) following ‘false’ branch (when ‘frame_offset <= app_progress’)... ─>─┐
           │      |                                                                                                     │
util-validate.h:95:36: note: in expansion of macro ‘BUG_ON’
           │   95 | #define DEBUG_VALIDATE_BUG_ON(exp) BUG_ON((exp))
           │      |                                    ^~~~~~
app-layer-frames.c:266:5: note: in expansion of macro ‘DEBUG_VALIDATE_BUG_ON’
           │  266 |     DEBUG_VALIDATE_BUG_ON(frame_offset > app_progress);
           │      |     ^~~~~~~~~~~~~~~~~~~~~
           │
         ‘FrameLeftEdge’: event 11
           │
           │      |                                                                                                     │
           │      |┌────────────────────────────────────────────────────────────────────────────────────────────────────┘
           │  269 |│    if (frame->len < 0) {
           │      |│        ~~~~~^~~~~
           │      |│             |
           │      |└────────────>(11) ...to here
           │
    <──────┘
    │
  ‘FramePrune’: events 12-13
    │
    │  779 |                 const uint64_t fle = FrameLeftEdge(stream, frame);
    │      |                                      ^~~~~~~~~~~~~~~~~~~~~~~~~~~~
    │      |                                      |
    │      |                                      (12) returning to ‘FramePrune’ from ‘FrameLeftEdge’
    │......
    │  783 |                 FrameCopy(nframe, frame);
    │      |                 ~~~~~~~~~~~~~~~~~~~~~~~~
    │      |                 |
    │      |                 (13) calling ‘FrameCopy’ from ‘FramePrune’
    │
    └──> ‘FrameCopy’: events 14-15
           │
           │  234 | static void FrameCopy(Frame *dst, Frame *src)
           │      |             ^~~~~~~~~
           │      |             |
           │      |             (14) entry to ‘FrameCopy’
           │  235 | {
           │  236 |     memcpy(dst, src, sizeof(*dst));
           │      |     ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
           │      |     |
           │      |     (15) ⚠️  overlapping buffers passed as arguments to ‘memcpy’
           │
In file included from suricata-common.h:129,
                 from app-layer-frames.c:25:
/usr/include/string.h:47:14: note: the behavior of ‘memcpy’ is undefined for overlapping buffers
   47 | extern void *memcpy (void *__restrict __dest, const void *__restrict __src,
      |              ^~~~~~
pull/15636/head
Victor Julien 1 month ago
parent 188376b577
commit a5effad936

@ -233,6 +233,7 @@ static void FrameClean(Frame *frame)
static void FrameCopy(Frame *dst, Frame *src)
{
DEBUG_VALIDATE_BUG_ON(dst == src);
memcpy(dst, src, sizeof(*dst));
}
@ -353,8 +354,8 @@ static int FrameSlide(const char *ds, Frames *frames, const TcpStream *stream, c
#endif
} else {
Frame *nframe = &frames->sframes[x];
FrameCopy(nframe, frame);
if (frame != nframe) {
FrameCopy(nframe, frame);
FrameClean(frame);
}
le = MIN(le, FrameLeftEdge(stream, nframe));
@ -378,8 +379,8 @@ static int FrameSlide(const char *ds, Frames *frames, const TcpStream *stream, c
} else {
nframe = &frames->sframes[x];
}
FrameCopy(nframe, frame);
if (frame != nframe) {
FrameCopy(nframe, frame);
FrameClean(frame);
}
le = MIN(le, FrameLeftEdge(stream, nframe));
@ -780,8 +781,8 @@ static void FramePrune(Frames *frames, const TcpStream *stream, const bool eof)
le = MIN(le, fle);
SCLogDebug("le %" PRIu64 ", frame fle %" PRIu64, le, fle);
Frame *nframe = &frames->sframes[x];
FrameCopy(nframe, frame);
if (frame != nframe) {
FrameCopy(nframe, frame);
FrameClean(frame);
}
x++;
@ -808,8 +809,8 @@ static void FramePrune(Frames *frames, const TcpStream *stream, const bool eof)
} else {
nframe = &frames->sframes[x];
}
FrameCopy(nframe, frame);
if (frame != nframe) {
FrameCopy(nframe, frame);
FrameClean(frame);
}
x++;

Loading…
Cancel
Save