Conversation
ndev2
force-pushed
the
bug_fix
branch
5 times, most recently
from
1841506 to
f24c3d1
Compare
tc/tc_util.c
Outdated
| #include <arpa/inet.h> | ||
| #include <string.h> | ||
| #include <math.h> | ||
| #include <ctype.h> |
There was a problem hiding this comment.
I don't think we need this include anymore.
tc/tc_util.c
Outdated
| long max_rate_bits; | ||
| int ret; | ||
| double perc, rate_bits; | ||
| char *char_perc, *p; |
There was a problem hiding this comment.
Let's change char_perc to str_perc to more accurately reflect what it represents.
tc/tc_util.c
Outdated
|
|
||
| if (perc > 100.0 || perc < 0.0) { | ||
|
|
||
| if (perc > 100.0 || perc < 0.0 || errno == ERANGE) { |
There was a problem hiding this comment.
This check for errno == ERANGE is for strtod(), but we call free() between the call to strtod() and here. It's possible that in some implementations free() could set errno.
So let's do the error checking for strtod() in this way:
int saved_errno;
...
perc = strtod(char_perc, &p);
saved_errno = errno;
free(str_perc);
/* Make sure there's only one percent sign and it's at the end. */
if (*p != '%' || *(p++) != '\0')
goto malf;
if (perc > 100.0 || perc < 0.0 || saved_errno == ERANGE) {
fprintf(stderr, "Invalid rate specified; should be between [0,100]%% but is %s\n", str);
return -1;
}
There was a problem hiding this comment.
This should catch any cases where there are multiple percentage signs, or a percentage sign that's not at the end. Please test these cases.
There was a problem hiding this comment.
Hey Cody, *p is pointing to char_perc which is allocated a new buffer, so if we free char_perc, and then check for *p, it doesn't lead anywhere (I ran into this issue when I was testing it out), but yes, I'll check for double percentage signs, thanks.
There was a problem hiding this comment.
Actually, the current code catches double percentage signs as I'm testing it right now, I guess strtod() set *p to where it stopped converting the string to double, and so, the other (*p++) != '\0' condition catches double percentage signs
There was a problem hiding this comment.
Right, sorry, I should have been clearer -- the code you wrote before already did that, I just added the comment to make it clear what it was doing.
But we definitely need to save errno in a variable so that we can use it after. Doing so also simplifies how we can can call free(str_perc). So please add those parts in as well, following the suggested code that I wrote above.
There was a problem hiding this comment.
Right I see, thanks, will make the changes now
|
Hi Cody, I've made the necessary changes and it looks like the code does handle double percentage signs by showing errors, so I didn't change that part. Do let me know what you think, thanks. |
|
Hi Cody, made the changes, hope the code is good, thanks |
tc/tc_util.c
Outdated
| saved_errno = errno; | ||
| free(str_perc); | ||
|
|
||
|
|
There was a problem hiding this comment.
Just remove one of these extra blank lines and I think this patch will be good to go.
|
Once you've fixed the above comment, I think you can squash these commits back down to just one and add go back to the mailing list. |
2 participants
Bug fix for malformed rate strings