Bug #42
closedSMB app layer assumes int for pointer arith
Description
app-layer-smb.c assumes that pointer arith results in "int", but on 64-bit linux this is "long int".
EX: static int SMBGetByteCount(Flow *f, void *smb_state, AppLayerParserState *pstate, uint8_t *input, uint32_t input_len, AppLayerParserResult *output)
uint8_t *p = input;
...
SCReturnInt(p - input);
Should return "int", not "long int". On 64-bit Linux and causes compile to fail with debug on (due to debugging output around SCReturnInt() macro) when using '-Wall -Werror -Wextra -Wno-unused-parameter' on 64-bit linux:
if gcc -DHAVE_CONFIG_H -I. -I. -I.. -I/home/brectanus/projects/oisf/deps/include -Wall -Werror -Wextra -Wno-unused-parameter -g -Wall -fno-strict-aliasing -D_BSD_SOURCE -D__BSD_SOURCE -D__FAVOR_BSD -DHAVE_NET_ETHERNET_H -I /usr/include -DLIBPCAP_VERSION_MAJOR=1 -DUNITTESTS -DDEBUG -MT app-layer-smb.o -MD -MP -MF ".deps/app-layer-smb.Tpo" -c -o app-layer-smb.o app-layer-smb.c; \
then mv -f ".deps/app-layer-smb.Tpo" ".deps/app-layer-smb.Po"; else rm -f ".deps/app-layer-smb.Tpo"; exit 1; fi
cc1: warnings being treated as errors
app-layer-smb.c: In function ‘SMBGetByteCount’:
app-layer-smb.c:425: error: format ‘%d’ expects type ‘int’, but argument 4 has type ‘long int’
app-layer-smb.c:425: error: format ‘%d’ expects type ‘int’, but argument 4 has type ‘long int’
app-layer-smb.c: In function ‘SMBParseWordCount’:
app-layer-smb.c:458: error: format ‘%d’ expects type ‘int’, but argument 4 has type ‘long int’
app-layer-smb.c:458: error: format ‘%d’ expects type ‘int’, but argument 4 has type ‘long int’
app-layer-smb.c: In function ‘SMBParseByteCount’:
app-layer-smb.c:499: error: format ‘%d’ expects type ‘int’, but argument 4 has type ‘long int’
app-layer-smb.c:499: error: format ‘%d’ expects type ‘int’, but argument 4 has type ‘long int’
app-layer-smb.c: In function ‘NBSSParseHeader’:
app-layer-smb.c:543: error: format ‘%d’ expects type ‘int’, but argument 4 has type ‘long int’
app-layer-smb.c:543: error: format ‘%d’ expects type ‘int’, but argument 4 has type ‘long int’
app-layer-smb.c: In function ‘SMBParseHeader’:
app-layer-smb.c:701: error: format ‘%d’ expects type ‘int’, but argument 4 has type ‘long int’
app-layer-smb.c:701: error: format ‘%d’ expects type ‘int’, but argument 4 has type ‘long int’
make2: *** [app-layer-smb.o] Error 1
Files
Updated by Kirby Kuehl over 14 years ago
In util-debug.h
Could you modify
#define SCReturnInt(x) do { \
if (sc_log_global_log_level >= SC_LOG_DEBUG) { \
SCLogDebug("Returning: %d ... <<", x); \
SCLogCheckFDFilterExit(FUNCTION); \
} \
return x; \
} while(0)
To look like this (NOTE the only difference is the inclusion of the 't'.
#define SCReturnInt(x) do { \
if (sc_log_global_log_level >= SC_LOG_DEBUG) { \
SCLogDebug("Returning: %td ... <<", x); \
SCLogCheckFDFilterExit(FUNCTION); \
} \
return x; \
} while(0)
In C99, such code can use %td; the t size modifier corresponds to the
ptrdiff_t type.
Updated by Kirby Kuehl over 14 years ago
If that works, let me know and I will do a submit a correct patch. If it doesn't work, I will create a SCReturnLongInt() macro and cast the
difference to a long and use %ld
Updated by Brian Rectanus over 14 years ago
It does solve the issue there, but there is the other issue of passing an int:
cc1: warnings being treated as errors
flow.c: In function ‘FlowClearMemory’:
flow.c:747: error: format ‘%td’ expects type ‘ptrdiff_t’, but argument 4 has type ‘int’
flow.c:747: error: format ‘%td’ expects type ‘ptrdiff_t’, but argument 4 has type ‘int’
While this may solve the warning for the debug output, I think the real issue is that the function returns "int" and you are passing a "long int" in the return macro. Probably better would be to do proper casting instead of assuming that it is an int? Though the issue is not there w/o debug because the compiler sees that it needs to be an int in the return. I prefer casts, which shows that the programmer knows the issue is there.
Updated by Kirby Kuehl over 14 years ago
Please try the attached patch. It introduces a new SCReturnLongInt() macro. Rather than use ptrdiff_t or the t format specifier, I casted everything to long int.
I use pointer arithmetic throughout smb, smb2, and dcerpc decoding so I have also made the appropriate modifications there as well.
If this works well for you, we can close the ticket.
Updated by Kirby Kuehl over 14 years ago
Grr. Wait. Another patch coming soon :)
Updated by Kirby Kuehl over 14 years ago
Try this one. Note to Victor, I also compile cleanly on Fedora 12.
-Wall -Werror -Wextra -Wno-unused-parameter
Updated by Victor Julien over 14 years ago
Wouldn't it make more sense to use a fixed size type like uint32_t?
Updated by Brian Rectanus over 14 years ago
Kirby: That patch does indeed remove the warnings for me, however...
Victor: Agreed, uint32_t seems adequate (providing no negative returns). Though "int" seems fine as well, given proper handling/casts. Or maybe just use ptrdiff_t, but there is no return Macro for this.
Updated by Kirby Kuehl over 14 years ago
Here is a modified version that uses uint32_t.
Updated by Brian Rectanus over 14 years ago
That last patch works for me as well (I like it better).
Updated by Victor Julien over 14 years ago
After applying the patch the unittest DCERPCParserTest01 gets stuck an an endless loop.
Updated by Kirby Kuehl over 14 years ago
Updated by Victor Julien over 14 years ago
- Status changed from New to Closed
Patch applied, thanks guys!