allocate space for buf on heap #1324
Conversation
Avoids | src/epggrab/module/xmltv.c:204:47: error: '%s' directive output may be truncated writing between 2 and 2147483645 bytes into a region of size 115 [-Werror=format-truncation=] | 204 | snprintf(buf, sizeof(buf)-1, "ddprogid://%s/%s", mod->id, s); | | ^~ Upstream-Status: Pending Signed-off-by: Khem Raj <raj.khem@gmail.com>
| if (strlen(s) < 2) return; | ||
|
|
||
| char* buf = (char *)malloc(strlen(s) + strlen(mod->id) + 13); | ||
| buf[strlen(s) + strlen(mod->id) + 12] = '\0'; |
There was a problem hiding this comment.
There are a few things that can be improved in your change:
- There is no need to null terminate
buf[](line 203) when usingsnprintf(), as it always null terminates. - Line 215 can be removed as
snprintf()returns the number of characters put intobuf[](excluding the training'\0'). - There are multiple calls to
strlen()on the same strings:strlen(s)happens three times &strlen(mod->id)happens twice.
Consider doing this:
const int s_len = strlen(s);
if( s_len < 2) return;
const int buf_size = s_len + strlen(mod->id) + 13;
char * buf = (char *) malloc( buf_size);
/* Raw URI */
int e = snprintf( buf, buf_size, "ddprogid://%s/%s", mod->id, s);
...and then lines 215 & 216 become:
while( --e && buf[e] != '.') {}
There was a problem hiding this comment.
@yafiyogi Not sure if you didn't catch this one or if it was a wanted decision: If e is the length of the string, buf[e] would be the null/terminating char, so we would do an unnecessary comparison there. We could start with e-1 directly and do one less comparison in 215/216.
There was a problem hiding this comment.
@Flole998 Yes, by having int e = snprintf( buf, buf_size, "ddprogid://%s/%s", mod->id, s) - 1;, it means means line 215 can be removed & 216 can remain the same, which is in general a good thing.
I don't like having '- 1' at the end of a longish line. It can get overlooked when looking through code.
My code avoids a comparison with the null terminator by doing --e before comparing buf[e] == '.'.
There was a problem hiding this comment.
Ah yeah now I see it ;) That was one of my first thoughts aswell but I then thought it could get overlooked easily, and apparently that is true. Anyways, I like that solution and if this PR doesn't get updated I will put your solution in when I have time to do so (unless you want to make it a PR which I would instantly merge ;) ).
| /* Raw URI */ | ||
| snprintf(buf, sizeof(buf)-1, "ddprogid://%s/%s", mod->id, s); | ||
| snprintf(buf, strlen(s) + strlen(mod->id) + 12, "ddprogid://%s/%s", mod->id, s); |
There was a problem hiding this comment.
Here, by using + 12 rather than + 13, the last character of string s will not be copied to buf[]. snprintf() copies size - 1 characters an then null terminates. This appears to be a bug.
|
Changes are in, thanks @yafiyogi for your help! |
|
@Flole998 you're welcome. |
Avoids
| src/epggrab/module/xmltv.c:204:47: error: '%s' directive output may be truncated writing between 2 and 2147483645 bytes into a region of size 115 [-Werror=format-truncation=]
| 204 | snprintf(buf, sizeof(buf)-1, "ddprogid://%s/%s", mod->id, s);
| | ^~
Upstream-Status: Pending
Signed-off-by: Khem Raj raj.khem@gmail.com