Segfault thrown on one line of code

Question

I am using this library for libgps and having a few issues with it getting it to run properly.

The error from my debugger after it says segfault is:

Cannot find bounds of current function

The line of code throwing this is located in this file, on line 132.

uint8_t checksum= (uint8_t)strtol(strchr(message, '*')+1, NULL, 16);

I don't know the context of this at all, and I dont know why it would / wouldn't throw a segfault.

My code:

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

/*
 * 
 */
int main(int argc, char** argv) {

    // Open
    gps_init();
    gps_on();

    loc_t data;

    gps_location(&data);
    printf("%lf %lf\n", data.latitude, data.longitude);

    return (EXIT_SUCCESS);
}

The function gps_location() takes you into gps.c and from there it runs into serial.c, once it runs:

void serial_readln(char *buffer, int len)
{
    char c;
    char *b = buffer;
    int rx_length = -1;
    while(1) {
        rx_length = read(uart0_filestream, (void*)(&c), 1);

        if (rx_length <= 0) {
            //wait for messages
            sleep(1);
        } else {
            if (c == '\n') {
                *b++ = '\0';
                break;
            }
            *b++ = c;
        }
    }
}

On the break it returns to gps.c goes into:

switch (nmea_get_message_type(buffer)) {

which takes it into nmea.c for nmea_get_message_type above.

It then runs the line:

if ((checksum = nmea_valid_checksum(message)) != _EMPTY)

taking it down to: uint8_t checksum= (uint8_t)strtol(strchr(message, '*')+1, NULL, 16); which is where the error is.

What is causing this?

Edit:

uint8_t is defined as: typedef unsigned char uint8_t;

If there is no `'*'` in the string, `strchr()` returns `NULL`, which `strtol()` isn't designed to handle. Add a check for `NULL` before passing the result of `strchr()` to `strtol()`. — EOF, Aug 03 '15 at 21:14
I do not see enough code to know if this is contributing to your problem, but `serial_readln()` ignores `len`, so could be writing past the end of `buffer`. I'd fix it in any case, to remove all doubt. — donjuedo, Aug 03 '15 at 21:26
@MattMcNabb it's not my code - I cannot find any reliable and up to date libraries for use with GPSD (GPD Deamon). I think message being null is the correct answer here. My programming knowledge is no where near good enough to make my own library for this sort of thing — I Garratt, Aug 03 '15 at 21:39
@donjuedo I think this is the root cause of the issue here - the buffer is going out of bounds, which to me sounds like it is writing past the end of the buffer as you say. I'm not sure how to stop it, but I will look into it — I Garratt, Aug 03 '15 at 21:45

score 0 · Answer 1 · answered Aug 03 '15 at 21:15

0

Segmentation fault is not a "thrown exception" per se, it is a hardware-issued problem ("you said go there, but I don't see anything named 'there'").

As for your problem: what happens when strchr() does not find the specified character? I suggest you try it and find out.

answered Aug 03 '15 at 21:15

inetknght

4,300
1
26
52

1

Thanks for the input, I have had a look at the `strchr` function, and from what I can see it is taking two args, one is a string and the other is the search parameter? So in this instance, it is searching the variable `message` for a `*`? The contents of `message` at this point is: `0 '\\000'`, I have no idea what that is – I Garratt Aug 03 '15 at 21:24
1

@IGarratt that means that `message` points to an empty string, so your code causes [undefined behaviour](http://stackoverflow.com/a/4105123/1505939) because `strchr` will return a null pointer. – M.M Aug 03 '15 at 21:36
1

@MattMcNabb thanks for that, this code is riddled with bugs. `*b++ = c;` is going out of bounds in `serial_readln` – I Garratt Aug 03 '15 at 21:43
1

@IGarratt Yes, this code needs a major overhaul to be used in anything except a toy program – M.M Aug 03 '15 at 21:53

score 0 · Answer 2 · answered Aug 03 '15 at 21:48

The code you are working with is horrible and has no error checking anywhere. So it may go haywire for any unexpected input. This could be a potential security vulnerability too.

To fix this particular instance, change the code to:

if ( !message )
    return NMEA_CHECKSUM_ERR;  // possibly `exit` or something, this shouldn't happen

char *star = strchr(message, '*'); 
if ( !star )
    return NMEA_CHECKSUM_ERR;

uint8_t checksum = strtol(star, NULL, 16);

The nmea_parse_gpgga and nmea_parse_gprmc also have multiple instances of a similar problem.

These functions might be acceptable if there was a parser or a regexp check that sanitizes the input before calling these functions. However, based on your question (I didn't check the codebase), it seems data is passed directly from read which is inexcusable.

The segfaulting function was not designed to handle an empty message or in fact any message not matching the expected form.

Another disastrous blunder is that the serial_readln function never checks that it does not write beyond len.

Thanks for this - you have solved one half of the problem here! Which is as you say no error checking in case it doesn't behave as expected. I think the overarching problem here is that `serial_readln` is going "out of bounds" somehow, giving the error in the debugger for variable b `0xbf000000
`, so it's not actually getting to the `strchr` bit in the first place. What a horrible library. I wouldnt use it if there were alternatives around — I Garratt, Aug 03 '15 at 22:01
@IGarratt try fixing `serial_readln` to abort when it read `len - 1` characters. The calling code is `char buffer[256];` so that can't really go wrong — M.M, Aug 03 '15 at 22:31

Segfault thrown on one line of code

2 Answers2