Small fixes. #721

Closed
wants to merge 2 commits into
from

Conversation

Projects
None yet
3 participants
@ghost

ghost commented Feb 26, 2013

Changes:

  • inet.c:64 l is unsigned so checking for <= 0 is pointless.
  • unix/getaddrinfo.c:152 buf is never free'd.
  • win/fs.c:1051 buf is never free'd.
  • win/tty.c:99 handle is assigned a value in line 103.
  • win/tty.c:989 FOREGROUND_RED is bitwise-OR'd with FOREGROUND_RED.

I'm not really that familiar with the code-base so excuse any stupidity but
these are a few small things I noticed.

Changes:
  - _inet.c:64_ `l` is unsigned so checking for `<= 0` is pointless.
  - _unix/getaddrinfo.c:152_ `buf` is never free'd.
  - _win/fs.c:1051_ `buf` is never free'd.
  - _win/tty.c:99_ `handle` is assigned a value in line 103.
  - _win/tty.c:989_ `FOREGROUND_RED` is bitwise-OR'd with `FOREGROUND_RED`.
Contributor

bnoordhuis commented Feb 26, 2013

unix/getaddrinfo.c:152 buf is never free'd.

That's incorrect - the memory is freed but the logic is rather subtle - so please revert that.

I'll leave it to @piscisaureus to review the other changes.

Revert _unix/getaddrinfo.c:152_
@bnoordhuis says that the memory is free'd but the logic isn't immediately
obvious.
@@ -61,7 +61,7 @@ static uv_err_t inet_ntop4(const unsigned char *src, char *dst, size_t size) {
#else
l = _snprintf(tmp, sizeof(tmp), fmt, src[0], src[1], src[2], src[3]);
#endif
- if (l <= 0 || l >= size) {
+ if (l == 0 || l >= size) {
@piscisaureus

piscisaureus Mar 5, 2013

Member

In my book snprintf returns an int and can actually return a negative value on failure. So the correct fix would be to make l of type int and not size_t.

@@ -1048,6 +1048,8 @@ static void fs__sendfile(uv_fs_t* req) {
}
}
+ free(buf);
@piscisaureus

piscisaureus Mar 5, 2013

Member

Seems right.

Member

piscisaureus commented Apr 10, 2013

Thanks. I landed the pieces I agree about in 7570a35 and 603915d.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment