4 ms·

I'm also no C guru, but this seems to dereference a pointer before checking if it's in-bounds: struct pollfd *pfd = &pfds[p]; if (fd == -1) retu

by faho 5y ago

I'm also no C guru, but this seems to dereference a pointer before checking if it's in-bounds: struct pollfd *pfd = &pfds[p]; if (fd == -1) return; if (p == -1 || (u_int)p >= npfd) fatal_f("channel %d: bad pfd %d (max %u)", c->self, p, npfd); dump_channel_poll(__func__, what, c, p, pfd); If p is < 0 or >= npfd (the number of elements in pfds), this should read out of bounds of the pfds array, and the check happens too late. This is similar to a famous Linux kernel bug from ages ago, where a !NULL check was optimized away in such a situation, leading to an exploit: https://lwn.net/Articles/342330/ https://lwn.net/Articles/342330/ In this case I'm not aware if something like that happens, or if it happens to be harmless.

aaronmdjones 5y ago
This: struct pollfd *pfd = &pfds[p]; ... does not dereference pfds. It looks that way, but you need to account for the address-of operator before it. What it's actually doing is: struct pollfd *pfd = pfds + p; ... and thus it works even if pfds is NULL or if p is out of bounds. The following check before using pfd is correct.
- faho 5y ago
  Ah, okay. Personally I'd still want to do the check before, because someone could still be adding something in-between the assignment and the check, but if that's the order it's resolved in it seems to be working now. (I'm guessing they declare and assign the variable first because of C version constraints - wasn't that something that changed in C99?)
  - aaronmdjones 5y ago
    Yes; it's C99 that let you declare variables anywhere in a function, rather than only before any statements. So it could be rewritten as: if (fd == -1) return; if (p == -1 || (u_int)p >= npfd) fatal_f("channel %d: bad pfd %d (max %u)", c->self, p, npfd); struct pollfd *pfd = &pfds[p]; dump_channel_poll(__func__, what, c, p, pfd); This is how I would have written it (I exclusively write C99), but the OpenSSH developers target a lot more platforms that may not have reliable C99 compilers, and are understandably reticent to rely upon the GNU extensions to C89 that would also let them do this.
    - mdaniel 5y ago
      For consideration, "declare variable" is just that; one could still choose to only assign to it after having done the validation (assuming fatal_f causes control flow termination) struct pollfd *pfd; if (p == -1 || (u_int)p >= npfd) fatal_f("channel %d: bad pfd %d (max %u)", c->self, p, npfd); pfd = &pfds[p]; right?
      - fullstop 5y ago
        A good compiler would produce the same code in both cases.
    - nybble41 5y ago
      You don't need C99 for this, just some extra braces: if (fd == -1) return; if (p == -1 || (u_int)p >= npfd) fatal_f("channel %d: bad pfd %d (max %u)", c->self, p, npfd); { struct pollfd *pfd = &pfds[p]; dump_channel_poll(__func__, what, c, p, pfd); } It's perfectly valid C89 to declare a variable at the start of a nested compound statement no matter where it appears in the outer block, so in practice you can declare variables wherever you want so long as you don't mind the extra clutter.
- rssoconnor 5y ago
  `pfds + p` is only defined if both the original pointer and the result pointer are pointing at elements of the same array or one past the end of that array. Note that executing `pfds - 1` when `pfds` points at the first element of an array is undefined behaviour and may fail on some platforms. https://en.cppreference.com/w/c/language/operator_arithmetic#Pointer_arithmetic https://en.cppreference.com/w/c/language/operator_arithmetic...
  - bheadmaster 5y ago
    It's not undefined behavior, but it is implementation-defined behavior [0]: Indirection through an invalid pointer value and passing an invalid pointer value to a deallocation function have undefined behavior. Any other use of an invalid pointer value has implementation-defined behavior. [0] http://eel.is/c++draft/basic.stc.general#4 http://eel.is/c++draft/basic.stc.general#4 Also note that you've quoted C++ reference. Just another example of where C++ stops being "compatible" with C.
    - rssoconnor 5y ago
      cppreference.com hosts references for both the C and C++ languages. My quote is from the C language part of the site. Furthermore, we are not discussing operations on an invalid pointer value. We are talking about the addition operator operating with a pointer value and when its behaviour is defined. Lastly, it is your reference here that is to a C++ language draft, not the C language.
      - paskozdilar 5y ago
        What is the difference between "operations with an invalid pointer value" and "addition operator operating with a pointer value"? EDIT (since I can't reply yet): Official C standard documents are surprisingly hard to come by. The only place I've found one was genesis. Here's a direct quote about pointer arithmetic from ISO/IEC 9899: 1990, page 47: --- When an expression that has integral type is added to or subtracted from a pointer, the result has the type of the pointer operand. If the pointer operand points to an element of an array object, and the array is large enough, the result points to an element offset from the original element such that the difference of the subscripts of the resulting and original array elements equals the integral expression. In other words, if the expression P points to the i-th element of an array object, the expressions (P)+N (equivalently. N+(P) ) and (P)-N (where N has the value n) point to, respectively, the i+n-th and i-n-th elements of the array object, provided they exist. Moreover, if the expression P points to the last element of an array object, the expression (P)+1 points one past the last element of the array object, and if the expression Q points one past the last element of an array object, the expression (Q)-1 points to the last element of the array object. If both the pointer operand and the result point to elements of the same array object, or one past the last element of the array object, the evaluation shall not produce an overflow; otherwise, the behavior is undefined. Unless both the pointer operand and the result point to elements of the same array object, or the pointer operand points one past the last element of an array object and the result points to an element of the same array object, the behavior is undefined if the result is used as an operand of the unary * operator. --- Maybe I just suck at reading technical documents, but I can't figure out which part says that storing an invalid pointer is undefined behavior. Would you care to point it out for me?
paskozdilar 5y ago
The first line is not actually dereferencing anything - it's taking the address of the pointer, and returning another pointer with offset p from the original one. As far as I know, C allows arbitrary pointer arithmetic (including pointing to an out-of-bounds address) as long as you don't read/write into the invalid memory. If the p check fails, nothing will be done with pfd. If I'm wrong, please correct me.
- rssoconnor 5y ago
  https://news.ycombinator.com/item?id=30956370 https://news.ycombinator.com/item?id=30956370
- MrRadar 5y ago
  Unfortunately you are not correct. Pointer arithmetic is only valid if the resulting calculated address is within the bounds of the same array as the original pointer, or calculates an address that is just past the end of the array. Otherwise it is our old friend undefined behavior. See section 6.5.6 of the C11 spec.
fullstop 5y ago
Given that the function fd_ready is static, it is limited to calls within this translation unit. It looks like the function which calls fd_ready has already confirmed that p is a valid index.

by faho 5y ago

aaronmdjones 5y ago

This: struct pollfd *pfd = &pfds[p]; ... does not dereference pfds. It looks that way, but you need to account for the address-of operator before it. What it's actually doing is: struct pollfd *pfd = pfds + p; ... and thus it works even if pfds is NULL or if p is out of bounds. The following check before using pfd is correct.

faho 5y ago
Ah, okay. Personally I'd still want to do the check before, because someone could still be adding something in-between the assignment and the check, but if that's the order it's resolved in it seems to be working now. (I'm guessing they declare and assign the variable first because of C version constraints - wasn't that something that changed in C99?)
- aaronmdjones 5y ago
  Yes; it's C99 that let you declare variables anywhere in a function, rather than only before any statements. So it could be rewritten as: if (fd == -1) return; if (p == -1 || (u_int)p >= npfd) fatal_f("channel %d: bad pfd %d (max %u)", c->self, p, npfd); struct pollfd *pfd = &pfds[p]; dump_channel_poll(__func__, what, c, p, pfd); This is how I would have written it (I exclusively write C99), but the OpenSSH developers target a lot more platforms that may not have reliable C99 compilers, and are understandably reticent to rely upon the GNU extensions to C89 that would also let them do this.
  - mdaniel 5y ago
    For consideration, "declare variable" is just that; one could still choose to only assign to it after having done the validation (assuming fatal_f causes control flow termination) struct pollfd *pfd; if (p == -1 || (u_int)p >= npfd) fatal_f("channel %d: bad pfd %d (max %u)", c->self, p, npfd); pfd = &pfds[p]; right?
    - fullstop 5y ago
      A good compiler would produce the same code in both cases.
  - nybble41 5y ago
    You don't need C99 for this, just some extra braces: if (fd == -1) return; if (p == -1 || (u_int)p >= npfd) fatal_f("channel %d: bad pfd %d (max %u)", c->self, p, npfd); { struct pollfd *pfd = &pfds[p]; dump_channel_poll(__func__, what, c, p, pfd); } It's perfectly valid C89 to declare a variable at the start of a nested compound statement no matter where it appears in the outer block, so in practice you can declare variables wherever you want so long as you don't mind the extra clutter.
rssoconnor 5y ago
`pfds + p` is only defined if both the original pointer and the result pointer are pointing at elements of the same array or one past the end of that array. Note that executing `pfds - 1` when `pfds` points at the first element of an array is undefined behaviour and may fail on some platforms. https://en.cppreference.com/w/c/language/operator_arithmetic#Pointer_arithmetic https://en.cppreference.com/w/c/language/operator_arithmetic...
- bheadmaster 5y ago
  It's not undefined behavior, but it is implementation-defined behavior [0]: Indirection through an invalid pointer value and passing an invalid pointer value to a deallocation function have undefined behavior. Any other use of an invalid pointer value has implementation-defined behavior. [0] http://eel.is/c++draft/basic.stc.general#4 http://eel.is/c++draft/basic.stc.general#4 Also note that you've quoted C++ reference. Just another example of where C++ stops being "compatible" with C.
  - rssoconnor 5y ago
    cppreference.com hosts references for both the C and C++ languages. My quote is from the C language part of the site. Furthermore, we are not discussing operations on an invalid pointer value. We are talking about the addition operator operating with a pointer value and when its behaviour is defined. Lastly, it is your reference here that is to a C++ language draft, not the C language.
    - paskozdilar 5y ago
      What is the difference between "operations with an invalid pointer value" and "addition operator operating with a pointer value"? EDIT (since I can't reply yet): Official C standard documents are surprisingly hard to come by. The only place I've found one was genesis. Here's a direct quote about pointer arithmetic from ISO/IEC 9899: 1990, page 47: --- When an expression that has integral type is added to or subtracted from a pointer, the result has the type of the pointer operand. If the pointer operand points to an element of an array object, and the array is large enough, the result points to an element offset from the original element such that the difference of the subscripts of the resulting and original array elements equals the integral expression. In other words, if the expression P points to the i-th element of an array object, the expressions (P)+N (equivalently. N+(P) ) and (P)-N (where N has the value n) point to, respectively, the i+n-th and i-n-th elements of the array object, provided they exist. Moreover, if the expression P points to the last element of an array object, the expression (P)+1 points one past the last element of the array object, and if the expression Q points one past the last element of an array object, the expression (Q)-1 points to the last element of the array object. If both the pointer operand and the result point to elements of the same array object, or one past the last element of the array object, the evaluation shall not produce an overflow; otherwise, the behavior is undefined. Unless both the pointer operand and the result point to elements of the same array object, or the pointer operand points one past the last element of an array object and the result points to an element of the same array object, the behavior is undefined if the result is used as an operand of the unary * operator. --- Maybe I just suck at reading technical documents, but I can't figure out which part says that storing an invalid pointer is undefined behavior. Would you care to point it out for me?

paskozdilar 5y ago

The first line is not actually dereferencing anything - it's taking the address of the pointer, and returning another pointer with offset p from the original one. As far as I know, C allows arbitrary pointer arithmetic (including pointing to an out-of-bounds address) as long as you don't read/write into the invalid memory. If the p check fails, nothing will be done with pfd. If I'm wrong, please correct me.

rssoconnor 5y ago
https://news.ycombinator.com/item?id=30956370 https://news.ycombinator.com/item?id=30956370
MrRadar 5y ago
Unfortunately you are not correct. Pointer arithmetic is only valid if the resulting calculated address is within the bounds of the same array as the original pointer, or calculates an address that is just past the end of the array. Otherwise it is our old friend undefined behavior. See section 6.5.6 of the C11 spec.

fullstop 5y ago

Given that the function fd_ready is static, it is limited to calls within this translation unit. It looks like the function which calls fd_ready has already confirmed that p is a valid index.