Why does this pointer-fu segfault?

Question

I seem to have hit the limits of my Pointer-Fu and am appealing for help (or some sort of brain medicine).

A rough outline of the project: An embedded ARM video-encoder board running Linux, using a manufacturer-supplied ill-documented poorly-supported SDK. Among its vast sprawl of code is a huge pile that is generated by gSoap from some WSDL, and it's this that is causing the headache.

In part of a huge data structure auto-generated by gSoap, we have a place to write some data (or, a place to write a pointer to the place we've written some data):

 struct tt__IPAddress
 {
    enum tt__IPType Type;   /* required element of type tt:IPType */
    char *IPv4Address;  /* optional element of type tt:IPv4Address */
    char *IPv6Address;  /* optional element of type tt:IPv6Address */
 };

Then we have this code which, in short, should be writing a string to the IPv4Address:

DNSInformation->DNSManual = ((struct tt__IPAddress *)soap_malloc(soap, sizeof(struct tt__IPAddress)));
DNSInformation->DNSManual->IPv4Address = (char **)soap_malloc(soap, sizeof(char *));
DNSInformation->DNSManual->IPv4Address[0] = (char *)soap_malloc(soap, sizeof(char) * LARGE_INFO_LENGTH);
// Code crashes at this next line:
strncpy(*DNSInformation->DNSManual->IPv4Address, dns_string, LARGE_INFO_LENGTH-1);

The dns_string is what you'd expect - something like "192.168.2.254". It's correctly null-terminated, the value of LARGE_INFO_LENGTH is something big (like 1024) so plenty of room for the string. I changed from strcpy() to strncpy() for safety.

My background is smaller embedded stuff (no OS, no use of malloc()) so I'm having a bit of trouble convincing myself I understand what this code is doing. The code is auto-generated / part of the SDK so it's not my creation and it's not documented / commented.

Here's what I think it's doing:

DNSInformation->DNSManual = ((struct tt__IPAddress *)soap_malloc(soap, sizeof(struct tt__IPAddress)));

Is allocating a lump of RAM, pointed to by DNSManual, where a tt__IPAddress struct will live.

DNSInformation->DNSManual->IPv4Address = (char **)soap_malloc(soap, sizeof(char *));

Is allocating a lump of RAM, pointed to by IPv4Address, where a pointer to a string containing the address will be wrote.

DNSInformation->DNSManual->IPv4Address[0] = (char *)soap_malloc(soap, sizeof(char) * LARGE_INFO_LENGTH);

Now this one throws me a bit, it looks like it's trying to allocate RAM to hold the string which will be pointed to by IPv4Address[0], except that looks to me like they're trying to write a (32-bit) pointer to a char, possibly.

This code has worked previously, however after some changes elsewhere it now segfaults, always at or during the strncpy().

My questions are twofold:

Can someone help me properly understand what is going on with the mallocs / pointer-fu?
Any guidance on how to go about tracing / debugging this?

We do not have GDB facility with this setup unfortunately - yes I'm sure it's possible to set it up, but for now let's just assume that's not practical for many lame and tedious reasons.

Currently I have debugging printf's scattered liberally through the code, in fact on every line in this little snippet, and it always stops with a SIGSEGV at the strncpy() line.

Edit to close as WhozCraig has hit the answer:

For reasons best known to itself, gSoap had changed the struct tt__IPAddress, perhaps it had run out of asterisks, but what it had been in previous versions, and what it should be, is this:

struct tt__IPAddress
 {
    enum tt__IPType Type; 
    char **IPv4Address;  /* note ptr to ptr */
    char **IPv6Address;
 };

The first, immediately obvious error is that [you are casting the return value of `malloc()`](http://stackoverflow.com/questions/605845/do-i-cast-the-result-of-malloc/605858#605858). — , Aug 27 '13 at 16:57
@unwind huh, have you never seen me linking to that answer of yours? :) I've been doing it for quite a long time. (Unfortunately, it's often needed. Too often.) — , Aug 27 '13 at 17:00
The code's not exactly consistent, is it? Your compiler should certainly be giving you a warning for the implicit conversion of (char**) to (char*). Surely the declaration of the struct is wrong! — Nicholas Wilson, Aug 27 '13 at 17:02
@H2CO3 - In my defence, **I'm** not casting anything, the horrible generated code I'm forced to modify is doing it. Unfortunately, I need a bit of help on part 1 of my question to fully grasp what's going on. — John U, Aug 27 '13 at 17:02
@JohnU Whoops, then sorry for accusing you with that. (In this case, the generated code really is horrible.) As for an explanation, Nicholas Wilson made some fair points. (Also, I liked the "pointer-fu" word :P) — , Aug 27 '13 at 17:05
Without more context, it's hard to say which is wrong, the struct definition, or the code posted. Either IPvAddress should be a `char**`, or the code later shouldn't do the `sizeof(char)` allocation, and should treat the field as a pointer to string rather than a pointer to a table of pointers. — Nicholas Wilson, Aug 27 '13 at 17:05
@H2CO3 I'm sure I have, didn't mean to sound as if I disregarded your previous efforts. And I of course agree, it's scary how often code with casts is posted. :| — unwind, Aug 27 '13 at 17:05
Can you show us the `DNSInformation` and `DNSManual` stucts? — Uchia Itachi, Aug 27 '13 at 17:06
@unwind Ah no, I was just surprised :) Me too, I often can't help but just shake my head when *someone again did the darn wrong cast...* — , Aug 27 '13 at 17:10

WhozCraig · Accepted Answer · 2013-08-27T17:12:34.970

The code doesn't follow the structure layout. The layout is:

 struct tt__IPAddress
 {
    enum tt__IPType Type;   /* required element of type tt:IPType */
    char *IPv4Address;  /* optional element of type tt:IPv4Address */
    char *IPv6Address;  /* optional element of type tt:IPv6Address */
 };

meaning: IPv4Address is a char pointer. yet this:

DNSInformation->DNSManual->IPv4Address = (char **)soap_malloc(soap, sizeof(char *));

is assigning a char ** cast to it. but the type is still char * so this:

strncpy(*DNSInformation->DNSManual->IPv4Address, dns_string, LARGE_INFO_LENGTH-1);

is dereferencing said pointer to a single char, which I can assure you is NOT compatible with a char * on your platform (and likely any other for that matter).

There should be warnings running amok on this compilation at a minimum, and outright errors if your compiler has any brains at all. This looks like it was original intended to be this:

 struct tt__IPAddress
 {
    enum tt__IPType Type; 
    char **IPv4Address;  /* note ptr to ptr */
    char **IPv6Address;
 };

for having a dynamic pointer array, each pointer being dynamically allocated memory for a single IP address. if it were like this, it would make much more sense. That said, if you intend on only a single IPv4 address per structure then this should be changed:

DNSInformation->DNSManual = soap_malloc(soap, sizeof(struct tt__IPAddress)));
if (DNSInformation->DNSManual)
{
    DNSInformation->DNSManual->IPv4Address = soap_malloc(soap, sizeof(char) * LARGE_INFO_LENGTH);
    if (DNSInformation->DNSManual->IPv4Address)
    {
        strncpy(DNSInformation->DNSManual->IPv4Address, dns_string, LARGE_INFO_LENGTH-1);
        DNSInformation->DNSManual->IPv4Address[LARGE_INFO_LENGTH-1] = 0;
    }
}

Or something similar to that.

**You have hit the nail on the nose Sir** - inspection of a previous version of auto-generated code revealed that there were indeed two stars. Compiler warnings were there, but even when "working" (as supplied) the SDK generates about 10,000 warnings, so they got a little lost in the noise. Why gSoap suddenly decided to knock a star off is anyone's guess, but now I know to keep a close eye on it. Thanks very much! — John U, Aug 28 '13 at 08:05
@JohnU 10,000 warnings? Ouch. I'd take serious stock in looking for either an addendum or outright replacement. As you can see, they usually mean something along the lines of "I'm not so sure this is a great idea; did you *really* mean to do that?". Anyway, I'm glad it helped. — WhozCraig, Aug 28 '13 at 15:57
It's big, and unfortunately it's the only thing that includes the features our customer requires for the hardware that's available, without having an extra zero or three on the end of the budgets to hire a team of programmers. Switching verbose mode on the compiler outputs one million lines of text - I've taken to writing parsers just to catch things! — John U, Aug 29 '13 at 07:51

score 0 · Answer 2 · answered Aug 27 '13 at 17:04

I think it looks broken.

This:

char *IPv4Address;  /* optional element of type tt:IPv4Address */

says IPv4Address is a single pointer to character data, i.e. a string.

But then it's used like this:

DNSInformation->DNSManual->IPv4Address = (char **)soap_malloc(soap,
                                                              sizeof(char *));

This is just wrong. Assuming a sane return value for soap_malloc() (i.e. void * to comply with malloc()), no cast should be necessary, but the fact that the cast differs from the actual type signal some kind of error.

It treats the IPv4Address struct field as a pointer to pointer, which it clearly is not.

robin.koch · Answer 3 · 2013-08-27T17:25:59.520

0

I'm sure it should look similar to this:

DNSInformation->DNSManual = soap_malloc(soap, sizeof(struct tt__IPAddress)));
DNSInformation->DNSManual->IPv4Address = soap_malloc(soap, sizeof(char) * LARGE_INFO_LENGTH);

strncpy(DNSInformation->DNSManual->IPv4Address, dns_string, LARGE_INFO_LENGTH-1);

Your struct contains pointers to strings, but first it is allocating a array of pointers (char**) and then allocating memory for the first pointer in this array.

and don't forget to set the binary zero after you used strncpy() as it doesn't set it itself.

//Edit: First part was wrong, sorry

edited Aug 27 '13 at 17:25

answered Aug 27 '13 at 17:07

robin.koch

1,223
1
13
20

1

`*x` and `x[0]` are the same. – Shahbaz Aug 27 '13 at 17:18
you are right, thanks for the hint. I didn't really think it through. – robin.koch Aug 27 '13 at 17:26
The truth of the statement ***`*x` and `x[0]`are the same*** depends on what stage of life ***x*** is in. It is true only once ***x*** has been created. Upon creation, declaring ***x*** in the form `int x[0];` does not work. But declaring `int *a;` does. – ryyker Sep 12 '13 at 21:36

kol · Answer 4 · 2013-08-27T17:27:08.493

Here is a working solution (I used malloc instead of soap_malloc etc.):

#include <stdio.h>
#include <stdlib.h>

#define LARGE_INFO_LENGTH 1024

enum tt__IPType { tt__IPv4, tt__IPv6 };

struct tt__IPAddress
{
  enum tt__IPType Type;   /* required element of type tt:IPType */
  char *IPv4Address;  /* optional element of type tt:IPv4Address */
  char *IPv6Address;  /* optional element of type tt:IPv6Address */
};

struct tt__DNSInformation
{
  struct tt__IPAddress* DNSManual;
};

int main()
{
  struct tt__DNSInformation* DNSInformation;
  char dns_string[] = "192.168.2.254";

  DNSInformation = malloc(sizeof(struct tt__DNSInformation));
  DNSInformation->DNSManual = malloc(sizeof(struct tt__IPAddress));
  DNSInformation->DNSManual->IPv4Address = malloc(sizeof(char) * LARGE_INFO_LENGTH);
  strncpy(DNSInformation->DNSManual->IPv4Address, dns_string, LARGE_INFO_LENGTH - 1);

  printf("%s\n", DNSInformation->DNSManual->IPv4Address);
  return 0;
}

Side note: `type *pointer = malloc(size * sizeof(*pointer))` is shorter, less error-prone, more future-proof, easier to maintain and avoids repeating yourself in comparison to `type *pointer = (type *)malloc(size * sizeof(type))` — Shahbaz, Aug 27 '13 at 17:20

Why does this pointer-fu segfault?

4 Answers4