Fix oacr build warnings #68

2018-06-11T14:42:55-05:00

kasontey commented

2018-06-11 14:42:55 -05:00

(Migrated from github.com)

Fixed TVS oacr warnings that appear when built for console code.
Windows wants us to check buffer size, while this is not required on Linux, so two new defines added for memcpy_s and vsnprintf_s.
Also Windows suggest we use GetAddrInfoW which takes a windows wide string, this method does not work with Linux. So we will suppress the warning.

Fixed TVS oacr warnings that appear when built for console code. Windows wants us to check buffer size, while this is not required on Linux, so two new defines added for memcpy_s and vsnprintf_s. Also Windows suggest we use GetAddrInfoW which takes a windows wide string, this method does not work with Linux. So we will suppress the warning.

rohangop (Migrated from github.com) reviewed

2018-06-11 14:42:55 -05:00

sslivins (Migrated from github.com) reviewed

2018-06-11 14:42:55 -05:00

dgarrett-microsoft (Migrated from github.com) reviewed

2018-06-11 14:42:55 -05:00

Sreekarch (Migrated from github.com) approved these changes

2018-06-12 12:32:24 -05:00

QuinnDamerell-MS (Migrated from github.com) requested changes

2018-06-12 12:59:29 -05:00

libftl/logging.c

I believe you want something like strlen here, sizeof will return the size of the type which I think is char*, so 4 bytes.

libftl/media.c

					
				@ -842,7 +842,7 @@ static int _media_send_slot(ftl_stream_configuration_private_t *ftl, nack_slot_t

				  int pkt_len;

This should be sizeof(pkt) * MAX_PACKET_BUFFER

kasontey (Migrated from github.com) reviewed

2018-06-19 13:24:26 -05:00

libftl/logging.c

Can you call strlen on a char[] ?

Doridian (Migrated from github.com) reviewed

2018-06-19 13:26:09 -05:00

libftl/logging.c

Of course, [] and * is the same in C anyway. (Both are a pointer, it is just syntactic sugar, really).
Also sizeof(char*) is 8 bytes on 64-bit :p

Of course, [] and * is the same in C anyway. (Both are a pointer, it is just syntactic sugar, really). Also sizeof(char*) is 8 bytes on 64-bit :p

kasontey (Migrated from github.com) reviewed

2018-06-19 13:26:14 -05:00

libftl/logging.c

hmmm, but there should be a nice way to get the count

QuinnDamerell-MS (Migrated from github.com) reviewed

2018-06-19 13:27:20 -05:00

libftl/logging.c

Yeah, strlen will look through the array until it finds a '/0'

TomyLobo (Migrated from github.com) reviewed

2018-06-19 13:52:46 -05:00

libftl/logging.c

I'm not much of a C guy (more of a C++ heathen), but I'd usually refer to the standard in such cases:
http://www.open-std.org/jtc1/sc22/WG14/www/docs/n1256.pdf page 92, 6.5.3.4 3rd paragraph

And voila, like C++, C's arrays degrade into pointers when passed to a function that accepts pointers, but the sizeof operator actually returns the full size of the array.
The standard even gives you a neat example covering your use case g

Btw, according to wikipedia where it is linked, that is the "Final version of the C99 standard with corrigenda TC1, TC2, and TC3 included, formatted as a draft".
If that standardization process is anything like C++'s, that's about as close as you can get to the real standard without paying money :)

I'm not much of a C guy (more of a C++ heathen), but I'd usually refer to the standard in such cases: http://www.open-std.org/jtc1/sc22/WG14/www/docs/n1256.pdf page 92, 6.5.3.4 3rd paragraph And voila, like C++, C's arrays degrade into pointers when passed to a function that accepts pointers, but the sizeof operator actually returns the full size of the array. The standard even gives you a neat example covering your use case *g* Btw, according to wikipedia where it is linked, that is the "Final version of the C99 standard with corrigenda TC1, TC2, and TC3 included, formatted as a draft". If that standardization process is anything like C++'s, that's about as close as you can get to the real standard without paying money :)

kasontey (Migrated from github.com) reviewed

2018-06-19 14:00:50 -05:00

libftl/logging.c

strlen now used in the next iteration.

kasontey (Migrated from github.com) reviewed

2018-06-19 14:01:23 -05:00

libftl/logging.c

thnkas guys for the tips, I've been living in c# world for a long time 😄

dgarrett-microsoft (Migrated from github.com) reviewed

2018-06-19 14:13:45 -05:00

libftl/logging.c

Just dropping in here, sorry for the confusion.
The type of m.msg.log.string is char[]. It's an allocated array in the struct and has a size. sizeof(char[]) is correct here and will give the number of bytes in the total array. strlen() is incorrect here as the array isn't yet initialized. It will just search through uninitialized memory until it happens to find a 0.

@TomyLobo I think you're misinterpreting the standard. Footnote 88 on page 80 under 6.5.3.4 explains how sizeof() applies to a parameter degraded array, which is that it returns the size of the pointer type, not the whole array.
Anyway, that doesn't apply here since m.msg.log.string isn't a function parameter.

Just dropping in here, sorry for the confusion. The type of m.msg.log.string is char[]. It's an allocated array in the struct and has a size. sizeof(char[]) is correct here and will give the number of bytes in the total array. strlen() is incorrect here as the array isn't yet initialized. It will just search through uninitialized memory until it happens to find a 0. @TomyLobo I think you're misinterpreting the standard. Footnote 88 on page 80 under 6.5.3.4 explains how sizeof() applies to a parameter degraded array, which is that it returns the size of the pointer type, not the whole array. Anyway, that doesn't apply here since m.msg.log.string isn't a function parameter.

dgarrett-microsoft (Migrated from github.com) reviewed

2018-06-19 14:20:07 -05:00

libftl/logging.c

There is however an issue with the second sizeof() parameter (the count parameter). You either need to subtract 1 to allow for the null terminator and add error handling code to check if vsnprintf_s failed to write the whole string, or since this is just for logging, you can just replace it with _TRUNCATE and let it automatically truncate the string.

TomyLobo (Migrated from github.com) reviewed

2018-06-19 14:27:53 -05:00

libftl/logging.c

I was confused about your page numbers there for a while.
Apparently, pdf page 92 is standard page 80 :)

Anyway, I actually have no idea what type the "string" field has. I assumed that, being a field it was not char[], but char[x] (which behave differently in sizeof, at least in C++)

So, what is it? char[] or char[x]?

I was confused about your page numbers there for a while. Apparently, pdf page 92 is standard page 80 :) Anyway, I actually have no idea what type the "string" field has. I assumed that, being a field it was not char[], but char[x] (which behave differently in sizeof, at least in C++) So, what is it? char[] or char[x]?

dgarrett-microsoft (Migrated from github.com) reviewed

2018-06-19 14:43:38 -05:00

libftl/logging.c

Ah, sorry about the page confusion.
The "string" variable is a char[1024] in a struct which is in the msg union, which is in the m struct.

typedef struct {
  int log_level;
  char string[1024];
}ftl_status_log_msg_t;

Ah, sorry about the page confusion. The "string" variable is a char[1024] in a struct which is in the msg union, which is in the m struct. ``` typedef struct { int log_level; char string[1024]; }ftl_status_log_msg_t; ```

TomyLobo (Migrated from github.com) reviewed

2018-06-19 14:50:43 -05:00

libftl/logging.c

https://ideone.com/1YQUTV there we go
So this means that "m.msg.log.string" cannot actually be a char[], assuming this piece of code compiles in gcc 6.3 :)

This means it's likely char[x] and sizeof was correct.
If, however, it is char*, you cannot use sizeof. In that case, you need the size from somewhere else. If it's not initialized and you did not store the length somewhere alongside the pointer, good luck figuring out the length :)

https://ideone.com/1YQUTV there we go So this means that "m.msg.log.string" cannot actually be a char[], assuming this piece of code compiles in gcc 6.3 :) This means it's likely char[x] and sizeof was correct. If, however, it is char*, you cannot use sizeof. In that case, you need the size from somewhere else. If it's not initialized and you did not [store the length somewhere alongside the pointer](https://en.cppreference.com/w/cpp/string/basic_string), good luck figuring out the length :)

kasontey (Migrated from github.com) reviewed

2018-06-19 17:33:14 -05:00

libftl/logging.c

Yup so seems like this is a char[x], so I'll move this back to sizeof.
And for the count we'll use the _TRUNCATE for string truncation.

Yup so seems like this is a char[x], so I'll move this back to sizeof. And for the count we'll use the _TRUNCATE for string truncation.

QuinnDamerell-MS (Migrated from github.com) reviewed

2018-06-22 10:02:09 -05:00

libftl/logging.c

Good call. Thanks everyone!

QuinnDamerell-MS commented

2018-06-22 10:06:51 -05:00

(Migrated from github.com)

It looks like this doesn't build on Linux currently, can you take a look? Here are the build errors.

[ 38%] Building C object CMakeFiles/ftl.dir/libftl/logging.c.o
In file included from /var/lib/jenkins/workspace/tl-sdk_fixoacrbuildwarnings-4OYIR3M7N4Z4DCVGVF3LAQBGZZPEVQSZO7DAGCWVR4O427WAWWQA/ftl-sdk/libftl/logging.c:26:0:
/var/lib/jenkins/workspace/tl-sdk_fixoacrbuildwarnings-4OYIR3M7N4Z4DCVGVF3LAQBGZZPEVQSZO7DAGCWVR4O427WAWWQA/ftl-sdk/libftl/ftl_private.h:135:43: warning: VA_ARGS can only appear in the expansion of a C99 variadic macro [enabled by default]
#define vsnprintf_s(buf, bufsz, cnt, fmt, VA_ARGS) vsnprintf(buf, cnt, fmt, VA_ARGS)
^
/var/lib/jenkins/workspace/tl-sdk_fixoacrbuildwarnings-4OYIR3M7N4Z4DCVGVF3LAQBGZZPEVQSZO7DAGCWVR4O427WAWWQA/ftl-sdk/libftl/ftl_private.h:135:81: warning: VA_ARGS can only appear in the expansion of a C99 variadic macro [enabled by default]
#define vsnprintf_s(buf, bufsz, cnt, fmt, VA_ARGS) vsnprintf(buf, cnt, fmt, VA_ARGS)
^
/var/lib/jenkins/workspace/tl-sdk_fixoacrbuildwarnings-4OYIR3M7N4Z4DCVGVF3LAQBGZZPEVQSZO7DAGCWVR4O427WAWWQA/ftl-sdk/libftl/logging.c: In function ‘ftl_log_msg’:
/var/lib/jenkins/workspace/tl-sdk_fixoacrbuildwarnings-4OYIR3M7N4Z4DCVGVF3LAQBGZZPEVQSZO7DAGCWVR4O427WAWWQA/ftl-sdk/libftl/logging.c:35:59: error: ‘_TRUNCATE’ undeclared (first use in this function)
vsnprintf_s(m.msg.log.string, sizeof(m.msg.log.string), _TRUNCATE, fmt, args);
^
/var/lib/jenkins/workspace/tl-sdk_fixoacrbuildwarnings-4OYIR3M7N4Z4DCVGVF3LAQBGZZPEVQSZO7DAGCWVR4O427WAWWQA/ftl-sdk/libftl/ftl_private.h:135:71: note: in definition of macro ‘vsnprintf_s’
#define vsnprintf_s(buf, bufsz, cnt, fmt, VA_ARGS) vsnprintf(buf, cnt, fmt, VA_ARGS)
^
/var/lib/jenkins/workspace/tl-sdk_fixoacrbuildwarnings-4OYIR3M7N4Z4DCVGVF3LAQBGZZPEVQSZO7DAGCWVR4O427WAWWQA/ftl-sdk/libftl/logging.c:35:59: note: each undeclared identifier is reported only once for each function it appears in
vsnprintf_s(m.msg.log.string, sizeof(m.msg.log.string), _TRUNCATE, fmt, args);
^
/var/lib/jenkins/workspace/tl-sdk_fixoacrbuildwarnings-4OYIR3M7N4Z4DCVGVF3LAQBGZZPEVQSZO7DAGCWVR4O427WAWWQA/ftl-sdk/libftl/ftl_private.h:135:71: note: in definition of macro ‘vsnprintf_s’
#define vsnprintf_s(buf, bufsz, cnt, fmt, VA_ARGS) vsnprintf(buf, cnt, fmt, VA_ARGS)
^
make[2]: *** [CMakeFiles/ftl.dir/libftl/logging.c.o] Error 1

It looks like this doesn't build on Linux currently, can you take a look? Here are the build errors. [ 38%] Building C object CMakeFiles/ftl.dir/libftl/logging.c.o In file included from /var/lib/jenkins/workspace/tl-sdk_fixoacrbuildwarnings-4OYIR3M7N4Z4DCVGVF3LAQBGZZPEVQSZO7DAGCWVR4O427WAWWQA/ftl-sdk/libftl/logging.c:26:0: /var/lib/jenkins/workspace/tl-sdk_fixoacrbuildwarnings-4OYIR3M7N4Z4DCVGVF3LAQBGZZPEVQSZO7DAGCWVR4O427WAWWQA/ftl-sdk/libftl/ftl_private.h:135:43: warning: __VA_ARGS__ can only appear in the expansion of a C99 variadic macro [enabled by default] #define vsnprintf_s(buf, bufsz, cnt, fmt, __VA_ARGS__) vsnprintf(buf, cnt, fmt, __VA_ARGS__) ^ /var/lib/jenkins/workspace/tl-sdk_fixoacrbuildwarnings-4OYIR3M7N4Z4DCVGVF3LAQBGZZPEVQSZO7DAGCWVR4O427WAWWQA/ftl-sdk/libftl/ftl_private.h:135:81: warning: __VA_ARGS__ can only appear in the expansion of a C99 variadic macro [enabled by default] #define vsnprintf_s(buf, bufsz, cnt, fmt, __VA_ARGS__) vsnprintf(buf, cnt, fmt, __VA_ARGS__) ^ /var/lib/jenkins/workspace/tl-sdk_fixoacrbuildwarnings-4OYIR3M7N4Z4DCVGVF3LAQBGZZPEVQSZO7DAGCWVR4O427WAWWQA/ftl-sdk/libftl/logging.c: In function ‘ftl_log_msg’: /var/lib/jenkins/workspace/tl-sdk_fixoacrbuildwarnings-4OYIR3M7N4Z4DCVGVF3LAQBGZZPEVQSZO7DAGCWVR4O427WAWWQA/ftl-sdk/libftl/logging.c:35:59: error: ‘_TRUNCATE’ undeclared (first use in this function) vsnprintf_s(m.msg.log.string, sizeof(m.msg.log.string), _TRUNCATE, fmt, args); ^ /var/lib/jenkins/workspace/tl-sdk_fixoacrbuildwarnings-4OYIR3M7N4Z4DCVGVF3LAQBGZZPEVQSZO7DAGCWVR4O427WAWWQA/ftl-sdk/libftl/ftl_private.h:135:71: note: in definition of macro ‘vsnprintf_s’ #define vsnprintf_s(buf, bufsz, cnt, fmt, __VA_ARGS__) vsnprintf(buf, cnt, fmt, __VA_ARGS__) ^ /var/lib/jenkins/workspace/tl-sdk_fixoacrbuildwarnings-4OYIR3M7N4Z4DCVGVF3LAQBGZZPEVQSZO7DAGCWVR4O427WAWWQA/ftl-sdk/libftl/logging.c:35:59: note: each undeclared identifier is reported only once for each function it appears in vsnprintf_s(m.msg.log.string, sizeof(m.msg.log.string), _TRUNCATE, fmt, args); ^ /var/lib/jenkins/workspace/tl-sdk_fixoacrbuildwarnings-4OYIR3M7N4Z4DCVGVF3LAQBGZZPEVQSZO7DAGCWVR4O427WAWWQA/ftl-sdk/libftl/ftl_private.h:135:71: note: in definition of macro ‘vsnprintf_s’ #define vsnprintf_s(buf, bufsz, cnt, fmt, __VA_ARGS__) vsnprintf(buf, cnt, fmt, __VA_ARGS__) ^ make[2]: *** [CMakeFiles/ftl.dir/libftl/logging.c.o] Error 1

kasontey commented

2018-06-22 11:19:55 -05:00

(Migrated from github.com)

@QuinnDamerell-MS New commit is up. Can you see if Linux still throws those build errors?

This pull request has changes conflicting with the target branch.

libftl/handshake.c
libftl/ingest.c

View command line instructions

Manual merge helper

Use this merge commit message when completing the merge manually.

Merge commit title

Merge pull request 'Fix oacr build warnings' (#68) from fixoacrbuildwarnings into master

Merge commit body

Reviewed-on: https://git.richey.dev/gh-microsoft/ftl-sdk/pulls/68

Merge pull request 'Fix oacr build warnings' (#68) from fixoacrbuildwarnings into master

Reviewed-on: https://git.richey.dev/gh-microsoft/ftl-sdk/pulls/68

Checkout

From your project repository, check out a new branch and test the changes.

git fetch -u origin 'fixoacrbuildwarnings:fixoacrbuildwarnings'

git switch 'fixoacrbuildwarnings'

Merge

Merge the changes and update on Forgejo.

Warning: The "Autodetect manual merge" setting is not enabled for this repository, you will have to mark this pull request as manually merged afterwards.

git switch 'master'

git merge --no-ff 'fixoacrbuildwarnings'

git switch 'fixoacrbuildwarnings'

git rebase 'master'

git switch 'master'

git merge --ff-only 'fixoacrbuildwarnings'

git switch 'fixoacrbuildwarnings'

git rebase 'master'

git switch 'master'

git merge --no-ff 'fixoacrbuildwarnings'

git switch 'master'

git merge --squash 'fixoacrbuildwarnings'