Fix oacr build warnings #68
Conversation
kasontey
requested review from
sslivins,
Sreekarch,
QuinnDamerell-MS,
rohangop and
dgarrett-microsoft
libftl/logging.c
Outdated
| @@ -32,7 +32,7 @@ void ftl_log_msg(ftl_stream_configuration_private_t *ftl, ftl_log_severity_t log | |||
|
|
|||
| m.msg.log.log_level = log_level; | |||
| va_start(args, fmt); | |||
| vsnprintf(m.msg.log.string, sizeof(m.msg.log.string), fmt, args); | |||
| vsnprintf_s(m.msg.log.string, sizeof(m.msg.log.string), sizeof(m.msg.log.string), fmt, args); | |||
There was a problem hiding this comment.
I believe you want something like strlen here, sizeof will return the size of the type which I think is char*, so 4 bytes.
There was a problem hiding this comment.
Can you call strlen on a char[] ?
There was a problem hiding this comment.
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
There was a problem hiding this comment.
hmmm, but there should be a nice way to get the count
There was a problem hiding this comment.
Yeah, strlen will look through the array until it finds a '/0'
There was a problem hiding this comment.
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]?
There was a problem hiding this comment.
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;
There was a problem hiding this comment.
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 :)
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Good call. Thanks everyone!
libftl/media.c
Outdated
| @@ -842,7 +842,7 @@ static int _media_send_slot(ftl_stream_configuration_private_t *ftl, nack_slot_t | |||
| int pkt_len; | |||
|
|
|||
| os_lock_mutex(&ftl->media.mutex); | |||
| memcpy(pkt, slot->packet, slot->len); | |||
| memcpy_s(pkt, sizeof(pkt), slot->packet, slot->len); | |||
There was a problem hiding this comment.
This should be sizeof(pkt) * MAX_PACKET_BUFFER
|
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 |
|
@QuinnDamerell-MS New commit is up. Can you see if Linux still throws those build errors? |
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.