5 ms·

The trouble with struct sockaddr's fake flexible array

96 points by signa11 2y ago

pm2222 2y ago
Why not just use TLV style structure?
- asveikau 2y ago
  That's essentially what it is. You can determine the size by looking at sa_family. Some platforms also have an sa_len field. It's still unsafe if you allocate for the "base" type of struct sockaddr then try to use it for something larger, but that generally is not done. People usually allocate for the exact type they want, and only pointers to the structure are passed around, often opaquely.
arisudesu 2y ago
Either I don't understand the problem completely, or why wasn't it possible to introduce something like 'ex' address family that allowed to pass and disambiguate extended parameter format(s) which would include array sizes etc? We had these *Ex functions everywhere in Win32 API for an eternity, why unices couldn't do the same trick?
- dwattttt 2y ago
  You can introduce new APIs and new types to resolve the "is it flexible, or is it 14 chars"; that's more or less one of the explored approaches in the article. It won't make existing uses any clearer or safer though, requiring rewrites to take advantage of them.
  - kazinator 2y ago
    The array should not be touched so the question is moot. The struck sockaddr type should only be used for pointers, which are cast to the correct type according to their family before they're are dereferenced, with the exception that the sa_family member can be accessed through the sockaddr base. For defining or allocating an object that can hold any address, sa_storage should be used, mentioned in the article.
    - dwattttt 2y ago
      I imagine the problem they're trying to address is ensuring that everyone _does_ only use correctly cast pointers; as defined, it's legal to use that 14 char array, it's just that it's never what you're meant to do.
      - kazinator 2y ago
        In my opinion, the array should be marked obsolescent, and removed (not necessarily physically, but the name gone from the member namespace). The 14 bytes is not enough for it to be define storage for all types, which is why sockaddr_storage is there. It cannot meaningfully access anything. Implementations could rename it to some __sa_data or whatever name so that the size of the structure doesn't change, if that is important to them.
- Retr0id 2y ago
  If the goal here is to reduce attack surface or bug-prone code in the kernel, leaving the non-ex variants around unchanged wouldn't help much with that.
- kazinator 2y ago
  You don't need the Ex thing with socket addresses because they have a run-time type field: sa_family. A function that takes a sockaddr * can dereference it to get at that field, and then know exactly which address type it's dealing with.
  - dblohm7 2y ago
    Respectfully, I think you're missing the point. Windows uses idioms like that too (though usually via filling an initial field with the size of the structure in byte), but GP's point of an Ex-style API would be to completely eliminate kludges such as runtime type fields at the beginning of structs and move to something safer.
    - kazinator 2y ago
      I don't see what. Type fields in structs is more or less the pinnacle of what it means to make things safer in C. :) :) The same bind() API has to work for any kind of socket: AF_UNIX, AF_INET, AF_INET6, AF_X25 or what have you. That any kind of socket pairs with the matching address type, whose type is erased at the API level down to struct sockaddr *. But from the socket type, it is inferred what type it must be and the appropriate network stack that is called can check the type of the address matches. I don't see how you'd get rid of this with a new bind_ex function, or why you would want to. Of course we could have a dedicated API for every address family: bind_inet, bind_inet6, bind_unix, ... which is bletcherous. Strictly speaking, I think we could drop the address family field from addresses, and then just assume they are the right type. The system API's all have a socket argument from which the type can be assumed. Having the type field in the address structure lets there be generic functions that just work with addresses. E.g. an address to text function that works with any sockaddr.
dataflow 2y ago
Here's what I don't understand: > As long as this usage remains, the checking tools built into both compilers must treat any trailing array in a structure as if it were flexible; that can disable overflow checking on that array entirely. No, they don't? Why couldn't they just have a mechanism to suppress the check for this particular struct (like a whitelist)?
- oersted 2y ago
  Yeah it's interesting that a (relatively) small quirk in the Linux source has the "political leverage" to impact the whole C ecosystem. I suppose C was made for Unix at Bell Labs and GCC is also inextricably tied, you could say that C is first and foremost the language of Linux, fair enough.
  - nine_k 2y ago
    I suspect that the problem is a problem in many code bases, but only a hugely important project like Linux has the leverage to try and have it addressed at the ecosystem level.
- AshamedCaptain 2y ago
  I'd think this is an extremely common pattern anyway (with the declared array length been 1, or even some other "minimal storage value"), and likely struct sockaddr's use of it is the cherry on the top.
  - dataflow 2y ago
    The only embedded sizes I've ever seen are flex, 0, 1, and 14 (sockaddr's). It's trivial enough to exclude all of them.
    - AshamedCaptain 2y ago
      No, I have seen many people use an arbitrary value to indicate "this is the amount it makes most sense to allocate this structure with", e.g. when you allocate it on the stack. If you need more than that you allocate it with a malloc wrapper or the like, which returns one of arbitrary long size. What I have not seen is this happening in the middle of the struct, for obvious reasons; it's always the last element in the struct.
kazinator 2y ago
I believe struct sockaddr was supposed to serve the role for which sockaddr_storage was later invented. That's why it has the padding array sa_data: so that if you actually define or allocate such an object it will be big enough to hold any address. In no way is that a flexible array, and you never want to be accessing it. Probably, sockaddr_storage was introduced rather than making sockaddr's array bigger because by that time the definition of sockaddr had become a mystical sacred cow, not to be touched. Plus that size of 14 is the same on every system. It is a one size fits all approach which is not a way to define the maximum size structure over all the address types if you want every platform to be able to make it a tight fit. The size is going to vary with implementation due to different alignment and padding requirements.
rjsw 2y ago
The article could point out that real BSD sockaddrs only have an 8-bit field for the address family which leaves another 8 bits for the length of the structure.
- asveikau 2y ago
  Some platforms have a field called sa_len.
OptionOfT 2y ago
> Needless to say, these casts can be error prone; casting a pointer between different structure types is also deemed to be undefined behavior in current C. This surprised me. What kind of mechanism is there in place to prevent the compiler from formatting /dev/sda when you write code like this?
- cratermoon 2y ago
  Nothing in the language, though the permissions mechanism of the system may disallow it. Unless the compiler is running with root permissions. C is not a safe language. The more abrasive among fans of the language would say "skill issue" and "git gud" if you want to avoid footguns.
- sseagull 2y ago
  What kind of mechanism is there in place to prevent any compiler from just replacing your main() function with formatting /dev/sda? That's nothing to do with the language, that's always on the compiler. Nothing stops the compiler from taking even correctly-specified code and doing whatever it wants.
  - ivanbakel 2y ago
    That's a rathee obtuse response. You can typically operate under the assumption that the compiler works correctly. The question of the GP was - what prevents the compiler from (correctly) exploiting UB to change the program behaviour from the desired one? If the Linux devs want to rely on their compilers' output, they have to somehow be obeying the contract around UB.
    - shawnz 2y ago
      I think the more reasonable assumption is that the practical needs of the biggest users will probably trump what any specification demands. The thing stopping the compiler from doing dangerous behaviours in response to commonly abused UB is obviously that people wouldn't use the compiler if it did that. Just like how the thing stopping the compiler from doing dangerous behaviours in response to spec-legal code is that people wouldn't use it if it did that.
jandrese 2y ago
It wouldn’t help the kernel case here, but I’ve long thought that the socket TCP and UDP APIs were bad in the general use case. Having to build and shuffle around the sockaddr structure is too low level for the majority use case. I think most programmers would prefer something like: int inet_connect(AF_TYPE, destination, port, options, &error); Also: int inet_listen(AF_TYPE, port, options, &error); So for example: int sock; inet_err err = NULL; sock = inet_connect(SOCK_STREAM, “google.com”, 443, NULL, &err); if ( sock < 0 ) { printf(“Failed to connect: %s\n”, err->message); return -1; } Basically one level more abstraction in the API. The old API could stick around for people who are doing something weird, but this would cover most use cases I think. Added bonus: programs using this API are trivial to add support for IPv6, it is just a recompile.
- asveikau 2y ago
  > sock = inet_connect(SOCK_STREAM, “google.com”, 443, NULL, &err); You just added a DNS lookup to connect(2). Also, you linked socket creation to all of this. This would make it hard to do an asynchronous version. Today you can create a socket and do a non-blocking connect(2) (after doing your asynchronous DNS lookup) Lastly, I think if you tasked someone with creating that API, they would probably use the old APIs to build it ...
  - ape4 2y ago
    That second parameter could accept a text IP-address eg "192.168.100.100". Like the address bar of a browser.
    - braiamp 2y ago
      So, the dns lookup is still there, it's just optional. You would have to pass a flag to tell it not to try to do the lookup or you would have to do something fancy trying to know if the passed parameter is an IP.
    - asveikau 2y ago
      This is already how all the DNS lookup APIs work.
rurban 2y ago
It's not really the struct sockaddr sa_data[14] problem, it's still a compiler problem - treating explicitly sized buffers at the end of structs to be not explicitly sized. It's a compiler bug. 14 bytes is 14 bytes, and [1] is one byte not a many or some. And the API's should follow. For the folks who need more bytes, invent your own type. Or set socket attributes as everyone else.

96 points by signa11 2y ago

pm2222 2y ago

Why not just use TLV style structure?

asveikau 2y ago
That's essentially what it is. You can determine the size by looking at sa_family. Some platforms also have an sa_len field. It's still unsafe if you allocate for the "base" type of struct sockaddr then try to use it for something larger, but that generally is not done. People usually allocate for the exact type they want, and only pointers to the structure are passed around, often opaquely.

arisudesu 2y ago

Either I don't understand the problem completely, or why wasn't it possible to introduce something like 'ex' address family that allowed to pass and disambiguate extended parameter format(s) which would include array sizes etc? We had these *Ex functions everywhere in Win32 API for an eternity, why unices couldn't do the same trick?

dwattttt 2y ago
You can introduce new APIs and new types to resolve the "is it flexible, or is it 14 chars"; that's more or less one of the explored approaches in the article. It won't make existing uses any clearer or safer though, requiring rewrites to take advantage of them.
- kazinator 2y ago
  The array should not be touched so the question is moot. The struck sockaddr type should only be used for pointers, which are cast to the correct type according to their family before they're are dereferenced, with the exception that the sa_family member can be accessed through the sockaddr base. For defining or allocating an object that can hold any address, sa_storage should be used, mentioned in the article.
  - dwattttt 2y ago
    I imagine the problem they're trying to address is ensuring that everyone _does_ only use correctly cast pointers; as defined, it's legal to use that 14 char array, it's just that it's never what you're meant to do.
    - kazinator 2y ago
      In my opinion, the array should be marked obsolescent, and removed (not necessarily physically, but the name gone from the member namespace). The 14 bytes is not enough for it to be define storage for all types, which is why sockaddr_storage is there. It cannot meaningfully access anything. Implementations could rename it to some __sa_data or whatever name so that the size of the structure doesn't change, if that is important to them.
Retr0id 2y ago
If the goal here is to reduce attack surface or bug-prone code in the kernel, leaving the non-ex variants around unchanged wouldn't help much with that.
kazinator 2y ago
You don't need the Ex thing with socket addresses because they have a run-time type field: sa_family. A function that takes a sockaddr * can dereference it to get at that field, and then know exactly which address type it's dealing with.
- dblohm7 2y ago
  Respectfully, I think you're missing the point. Windows uses idioms like that too (though usually via filling an initial field with the size of the structure in byte), but GP's point of an Ex-style API would be to completely eliminate kludges such as runtime type fields at the beginning of structs and move to something safer.
  - kazinator 2y ago
    I don't see what. Type fields in structs is more or less the pinnacle of what it means to make things safer in C. :) :) The same bind() API has to work for any kind of socket: AF_UNIX, AF_INET, AF_INET6, AF_X25 or what have you. That any kind of socket pairs with the matching address type, whose type is erased at the API level down to struct sockaddr *. But from the socket type, it is inferred what type it must be and the appropriate network stack that is called can check the type of the address matches. I don't see how you'd get rid of this with a new bind_ex function, or why you would want to. Of course we could have a dedicated API for every address family: bind_inet, bind_inet6, bind_unix, ... which is bletcherous. Strictly speaking, I think we could drop the address family field from addresses, and then just assume they are the right type. The system API's all have a socket argument from which the type can be assumed. Having the type field in the address structure lets there be generic functions that just work with addresses. E.g. an address to text function that works with any sockaddr.

dataflow 2y ago

Here's what I don't understand: > As long as this usage remains, the checking tools built into both compilers must treat any trailing array in a structure as if it were flexible; that can disable overflow checking on that array entirely. No, they don't? Why couldn't they just have a mechanism to suppress the check for this particular struct (like a whitelist)?

oersted 2y ago
Yeah it's interesting that a (relatively) small quirk in the Linux source has the "political leverage" to impact the whole C ecosystem. I suppose C was made for Unix at Bell Labs and GCC is also inextricably tied, you could say that C is first and foremost the language of Linux, fair enough.
- nine_k 2y ago
  I suspect that the problem is a problem in many code bases, but only a hugely important project like Linux has the leverage to try and have it addressed at the ecosystem level.
AshamedCaptain 2y ago
I'd think this is an extremely common pattern anyway (with the declared array length been 1, or even some other "minimal storage value"), and likely struct sockaddr's use of it is the cherry on the top.
- dataflow 2y ago
  The only embedded sizes I've ever seen are flex, 0, 1, and 14 (sockaddr's). It's trivial enough to exclude all of them.
  - AshamedCaptain 2y ago
    No, I have seen many people use an arbitrary value to indicate "this is the amount it makes most sense to allocate this structure with", e.g. when you allocate it on the stack. If you need more than that you allocate it with a malloc wrapper or the like, which returns one of arbitrary long size. What I have not seen is this happening in the middle of the struct, for obvious reasons; it's always the last element in the struct.

kazinator 2y ago

I believe struct sockaddr was supposed to serve the role for which sockaddr_storage was later invented. That's why it has the padding array sa_data: so that if you actually define or allocate such an object it will be big enough to hold any address. In no way is that a flexible array, and you never want to be accessing it. Probably, sockaddr_storage was introduced rather than making sockaddr's array bigger because by that time the definition of sockaddr had become a mystical sacred cow, not to be touched. Plus that size of 14 is the same on every system. It is a one size fits all approach which is not a way to define the maximum size structure over all the address types if you want every platform to be able to make it a tight fit. The size is going to vary with implementation due to different alignment and padding requirements.

rjsw 2y ago

The article could point out that real BSD sockaddrs only have an 8-bit field for the address family which leaves another 8 bits for the length of the structure.

asveikau 2y ago
Some platforms have a field called sa_len.

OptionOfT 2y ago

> Needless to say, these casts can be error prone; casting a pointer between different structure types is also deemed to be undefined behavior in current C. This surprised me. What kind of mechanism is there in place to prevent the compiler from formatting /dev/sda when you write code like this?

cratermoon 2y ago
Nothing in the language, though the permissions mechanism of the system may disallow it. Unless the compiler is running with root permissions. C is not a safe language. The more abrasive among fans of the language would say "skill issue" and "git gud" if you want to avoid footguns.
sseagull 2y ago
What kind of mechanism is there in place to prevent any compiler from just replacing your main() function with formatting /dev/sda? That's nothing to do with the language, that's always on the compiler. Nothing stops the compiler from taking even correctly-specified code and doing whatever it wants.
- ivanbakel 2y ago
  That's a rathee obtuse response. You can typically operate under the assumption that the compiler works correctly. The question of the GP was - what prevents the compiler from (correctly) exploiting UB to change the program behaviour from the desired one? If the Linux devs want to rely on their compilers' output, they have to somehow be obeying the contract around UB.
  - shawnz 2y ago
    I think the more reasonable assumption is that the practical needs of the biggest users will probably trump what any specification demands. The thing stopping the compiler from doing dangerous behaviours in response to commonly abused UB is obviously that people wouldn't use the compiler if it did that. Just like how the thing stopping the compiler from doing dangerous behaviours in response to spec-legal code is that people wouldn't use it if it did that.

jandrese 2y ago

It wouldn’t help the kernel case here, but I’ve long thought that the socket TCP and UDP APIs were bad in the general use case. Having to build and shuffle around the sockaddr structure is too low level for the majority use case. I think most programmers would prefer something like: int inet_connect(AF_TYPE, destination, port, options, &error); Also: int inet_listen(AF_TYPE, port, options, &error); So for example: int sock; inet_err err = NULL; sock = inet_connect(SOCK_STREAM, “google.com”, 443, NULL, &err); if ( sock < 0 ) { printf(“Failed to connect: %s\n”, err->message); return -1; } Basically one level more abstraction in the API. The old API could stick around for people who are doing something weird, but this would cover most use cases I think. Added bonus: programs using this API are trivial to add support for IPv6, it is just a recompile.

asveikau 2y ago
> sock = inet_connect(SOCK_STREAM, “google.com”, 443, NULL, &err); You just added a DNS lookup to connect(2). Also, you linked socket creation to all of this. This would make it hard to do an asynchronous version. Today you can create a socket and do a non-blocking connect(2) (after doing your asynchronous DNS lookup) Lastly, I think if you tasked someone with creating that API, they would probably use the old APIs to build it ...
- ape4 2y ago
  That second parameter could accept a text IP-address eg "192.168.100.100". Like the address bar of a browser.
  - braiamp 2y ago
    So, the dns lookup is still there, it's just optional. You would have to pass a flag to tell it not to try to do the lookup or you would have to do something fancy trying to know if the passed parameter is an IP.
  - asveikau 2y ago
    This is already how all the DNS lookup APIs work.

rurban 2y ago

It's not really the struct sockaddr sa_data[14] problem, it's still a compiler problem - treating explicitly sized buffers at the end of structs to be not explicitly sized. It's a compiler bug. 14 bytes is 14 bytes, and [1] is one byte not a many or some. And the API's should follow. For the folks who need more bytes, invent your own type. Or set socket attributes as everyone else.