[SOLVED] Can you tell me why this segfaults?

Unia · 2012-06-11 20:21:51

Hey all,

First of all, you have to know I'm not a programmer - nor am I fluent in any language. The pieces of coding I do are basically copy/pasting and adapting it to the situation. Such is the case here too with this little MPD notifier I am trying to adapt to my own needs. It's written in C, which is a language I want to learn and have some small experience with as I also use DWM.

Now, this piece of code is a notifier to MPD which notifies you in the following circumstances:
1. Change song; eiter done by you by pressing next/previous or automatically when the song ends.
2. Start playback.
3. Pause/stop playback.

What I wanted it to do is show a different message for pause and stop, which it initially doesn't do. The program compiles without errors but when I run it and then make MPD pause or stop, it segfaults and I don't know why. That's what I'm asking help with. I have confirmed that my part works, by replacing the TEXT_STOP parts with my TEXT_PAUSE. You can find the code below and see the changes that I have made between the // unia parts.

/*
 * (C) 2011-2012 by Christian Hesse <mail@eworm.de>
 *
 * This software may be used and distributed according to the terms
 * of the GNU General Public License, incorporated herein by reference.
 */

#include <mpd/client.h>

#include <libnotify/notify.h>

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

#define NOTIFICATION_TIMEOUT	10000

#define TEXT_PLAY	"<b>%s</b>\n- <i>%s</i>"
// unia
#define TEXT_PAUSE  "Paused playback"
// unia
#define TEXT_STOP	"Stopped playback"
#define TEXT_UNKNOWN	"(unknown)"

int main(int argc, char ** argv) {
	struct mpd_connection * conn = mpd_connection_new(NULL, 0, 30000);
	char * title = NULL;
	char * artist = NULL;
	char * album = NULL;
        NotifyNotification * netlink = NULL;
	struct mpd_song * song = NULL;
	char * notification = NULL;
	int errcount = 0;

	GError * error = NULL;

	printf("%s: %s v%s (compiled: %s)\n", argv[0], PROGNAME, VERSION, DATE);

	if (mpd_connection_get_error(conn) != MPD_ERROR_SUCCESS) {
		fprintf(stderr,"%s: %s\n", argv[0], mpd_connection_get_error_message(conn));
		mpd_connection_free(conn);
                exit(EXIT_FAILURE);
	}

        if(!notify_init("Udev-Net-Notification")) {
                fprintf(stderr, "%s: Can't create notify.\n", argv[0]);
                exit(EXIT_FAILURE);
        }

	netlink = notify_notification_new("MPD Notification", NULL, "sound");

	while(mpd_run_idle_mask(conn, MPD_IDLE_PLAYER)) {
		mpd_command_list_begin(conn, true);
		mpd_send_status(conn);
		mpd_send_current_song(conn);
		mpd_command_list_end(conn);

		if (mpd_status_get_state(mpd_recv_status(conn)) == MPD_STATE_PLAY) {
			mpd_response_next(conn);

			song = mpd_recv_song(conn);

			if ((title = g_markup_escape_text(mpd_song_get_tag(song, MPD_TAG_TITLE, 0), -1)) == NULL)
				title = TEXT_UNKNOWN;
			if ((artist = g_markup_escape_text(mpd_song_get_tag(song, MPD_TAG_ARTIST, 0), -1)) == NULL)
				artist = TEXT_UNKNOWN;
			if ((album = g_markup_escape_text(mpd_song_get_tag(song, MPD_TAG_ALBUM, 0), -1)) == NULL)
				album = TEXT_UNKNOWN;

			notification = (char *) malloc(strlen(TEXT_PLAY) + strlen(title) + strlen(artist) + strlen(album));
			sprintf(notification, TEXT_PLAY, title, artist, album);

			mpd_song_free(song);
        // unia
        } else if (mpd_status_get_state(mpd_recv_status(conn)) == MPD_STATE_PAUSE) {
            notification = (char *) malloc(strlen(TEXT_PAUSE));
			sprintf(notification, TEXT_PAUSE);
        // unia
        } else {
            notification = (char *) malloc(strlen(TEXT_STOP));
            sprintf(notification, TEXT_STOP);
		}

		printf("%s: %s\n", argv[0], notification);

		notify_notification_update(netlink, "MPD:", notification, "sound");

		notify_notification_set_timeout(netlink, NOTIFICATION_TIMEOUT);
		notify_notification_set_category(netlink, "MPD-Notification");
		notify_notification_set_urgency (netlink, NOTIFY_URGENCY_NORMAL);

		while(!notify_notification_show(netlink, &error)) {
			if (errcount > 1) {
				fprintf(stderr, "%s: Looks like we can not reconnect to notification daemon... Exiting.\n", argv[0]);
				exit(EXIT_FAILURE);
			} else {
				g_printerr("%s: Error \"%s\" while trying to show notification. Trying to reconnect.\n", argv[0], error->message);
				errcount++;

				g_error_free(error);
				error = NULL;

				notify_uninit();

				usleep(500 * 1000);

				if(!notify_init("mpdnotify")) {
					fprintf(stderr, "%s: Can't create notify.\n", argv[0]);
					exit(EXIT_FAILURE);
				}
			}
		}
		errcount = 0;

		free(notification);
		mpd_response_finish(conn);
	}
	mpd_connection_free(conn);

	return EXIT_SUCCESS;
}

Thanks in advance!

Last edited by Unia (2012-06-15 13:56:40)

drcouzelis · 2012-06-11 20:26:54

Compile it using the "-g" debug option and run it using "gdb app". When it crashes, type "bt" to see a backtrace. It'll tell you exactly where it crashes.

Trilby · 2012-06-11 20:38:23

Or just a quick eyeballing will show that TEXT_PLAY is used as a format string in an sprintf and TEXT_PLAY has placeholders for two variables "%s" but the sprintf function provides three. There may be other errors, but this would cause a segfault.

Edit: odd, that is not a part you editted - I must be missing something, that should not work.

But, within the part you editted you allocate one two few bytes for the string "notification" that you fill. Always "dest = (char *) malloc(strlen(source) + 1);" before a sprintf (or strcopy) from source to dest. The +1 is needed for the '\0' null termination which is not counted by strlen.

Edit2: the original code did not include the +1, but the initial problem I noted wouldn't cause a seg fault, it would just be an unused parameter. That unused parameter (album*) had space allocated for it in "notification" but since it was never copied to notification the string never ran out of space. The original code needs some work - you may want to report that 'upstream' to whomever wrote the original. [Two wrongs don't make a right, but in this case they apparently avoid a segfault]

*note: does the original ever actually report the album? It doesn't look like it could.

Last edited by Trilby (2012-06-11 20:48:20)

Unia · 2012-06-11 20:59:15

drcouzelis wrote:

Compile it using the "-g" debug option and run it using "gdb app". When it crashes, type "bt" to see a backtrace. It'll tell you exactly where it crashes.

Thanks, didn't know about this. However, the output is something above my level:

(gdb) run
Starting program: /usr/bin/mpd-notification
Traceback (most recent call last):
  File "/usr/share/gdb/auto-load/usr/lib/libgobject-2.0.so.0.3200.3-gdb.py", line 9, in <module>
    from gobject import register
  File "/usr/share/glib-2.0/gdb/gobject.py", line 3, in <module>
    import gdb.backtrace
ImportError: No module named backtrace
[Thread debugging using libthread_db enabled]
Using host libthread_db library "/lib/libthread_db.so.1".
/usr/bin/mpd-notification: mpd-notification v0.3.7-1 (compiled: ma jun 11 20:54:52 UTC 2012)
/usr/bin/mpd-notification: <b>100 Degrees</b>
- <i>Fort Minor</i>
[New Thread 0x7ffff5180700 (LWP 19636)]

Program received signal SIGSEGV, Segmentation fault.
0x00007ffff7bd3d50 in mpd_status_get_state () from /usr/lib/libmpdclient.so.2

The things between <b></b> and <i></i> are output from MPD.

And here's the backtrace:

(gdb) bt
#0  0x00007ffff7bd3d50 in mpd_status_get_state () from /usr/lib/libmpdclient.so.2
#1  0x0000000000401500 in ?? ()
#2  0x00007ffff6972455 in __libc_start_main () from /lib/libc.so.6
#3  0x0000000000401149 in ?? ()
#4  0x00007fffffffe1c8 in ?? ()
#5  0x0000000000000000 in ?? ()

Unia · 2012-06-11 21:01:05

Trilby wrote:

Or just a quick eyeballing will show that TEXT_PLAY is used as a format string in an sprintf and TEXT_PLAY has placeholders for two variables "%s" but the sprintf function provides three. There may be other errors, but this would cause a segfault.
Edit: odd, that is not a part you editted - I must be missing something, that should not work.
But, within the part you editted you allocate one two few bytes for the string "notification" that you fill. Always "dest = (char *) malloc(strlen(source) + 1);" before a sprintf (or strcopy) from source to dest. The +1 is needed for the '\0' null termination which is not counted by strlen.
Edit2: the original code did not include the +1, but the initial problem I noted wouldn't cause a seg fault, it would just be an unused parameter. That unused parameter (album*) had space allocated for it in "notification" but since it was never copied to notification the string never ran out of space. The original code needs some work - you may want to report that 'upstream' to whomever wrote the original. [Two wrongs don't make a right, but in this case they apparently avoid a segfault]
*note: does the original ever actually report the album? It doesn't look like it could.

Thanks for your input. I will have to read this again tomorrow, however, as I now don't really understand what you're saying I guess I will forward your reply to the author

Trilby · 2012-06-11 21:11:13

To solve your direct issue, just add one to your malloc.

notification = (char *) malloc(strlen(TEXT_PAUSE) + 1);

ewaller · 2012-06-11 22:11:24

I would take a serious look at

if (mpd_status_get_state(mpd_recv_status(conn)) == MPD_STATE_PLAY) {

and see if mpd_recv_status(conn) is returning NULL.

Edit: Fixed Typo

Last edited by ewaller (2012-06-12 14:52:28)

Unia · 2012-06-12 14:21:28

Trilby wrote:

To solve your direct issue, just add one to your malloc.
notification = (char *) malloc(strlen(TEXT_PAUSE) + 1);

Nope, that doesn't help. It still segfaults.

ewaller wrote:

I would take a seious look at
if (mpd_status_get_state(mpd_recv_status(conn)) == MPD_STATE_PLAY) {
and see if mpd_recv_status(conn) is returning NULL.

How can I check this?

EDIT: Ewaller, do you mean that MPD has both the MPD_STATE_PLAY and the MPD_STATE_PAUSE enabled when I pause a song and that that's interfering with eachother? I don't know, I'm just babbling here

Last edited by Unia (2012-06-12 14:23:07)

ewaller · 2012-06-12 14:49:24

Unia wrote:

[
ewaller wrote:
I would take a seious look at
if (mpd_status_get_state(mpd_recv_status(conn)) == MPD_STATE_PLAY) {
and see if mpd_recv_status(conn) is returning NULL.
How can I check this?

...
 mpd_status* theStatus = mpd_pecv_status(conn);
 if ( ! theStatus)
   /*Don't use a null pointer.  You may want a more graceful error handler */
  exit(1);
if (mpd_status_get_state(theStatus) == MPD_STATE_PLAY)
{
....

I have not tried to compile that, YMMV.

Edit: Hint: I missed a closing parenthesis. I will leave that as an exercise for the reader
Edit2: I missed the MPD_STATE_PLAY clause, so I fixed it all

Last edited by ewaller (2012-06-12 14:55:52)

jdarnold · 2012-06-12 16:52:41

You might want to try 'ddd' - it's a GUI on top of gdb that is pretty easy to use.

Personally, I would just set up a static char buffer and use that instead of malloc / free.

char notification[1000];

Unia · 2012-06-12 17:26:27

I guess I'll have to give up for now as I really don't understand how to apply your suggestions and make things work. My summer project is to learn some C which is what I'm gonna do first before continuing on this.

If any of you know what the problem is and would be so kind as to show it to me and instruct me on how to fix, then that would be superb. I thank you all for your kind help and suggestions and I'm sorry to let you down, but I'm just not there yet.

Thanks!

ewaller · 2012-06-12 17:37:04

Unia wrote:

I guess I'll have to give up for now as I really don't understand how to apply your suggestions and make things work. My summer project is to learn some C which is what I'm gonna do first before continuing on this.
If any of you know what the problem is and would be so kind as to show it to me and instruct me on how to fix, then that would be superb. I thank you all for your kind help and suggestions and I'm sorry to let you down, but I'm just not there yet.
Thanks!

Okay, but take this away: Be a pessimist. Always assume that anything that can go wrong, will go wrong. Never trust a pointer until you know it is valid.

jdarnold · 2012-06-12 19:14:14

Well, one important part of programming is debugging, so this is a good time to learn that. Like I said, install 'ddd' which is a debugger and run your code. It will show exactly where the code is segfaulting (almost certainly overwriting a buffer).

Trilby · 2012-06-12 19:29:25

jdarnold wrote:

Personally, I would just set up a static char buffer and use that instead of malloc / free.

Even better, ditch the "notification" variable all together. There are conditionals within the loop that malloc and set the notification variable with sprintf, but then at the end of the loop, that notification variable is just "printf'ed" to stdout. Cut out the middle-man: just replace all the sprintf(notification,"...",...) with printf("%s: ...",argv[0],...).

In addition to ewallers good "Murphy's law" advice, I'd add "Always simplify". Cut out the middle man when possible. Simple code is simple to understand, simple to debug, and almost always is less resource intensive. The notification variable requires storage space and several function calls to allocate, set, then retrieve it's value. Yet that variable serves no purpose that I can detect other than to clutter up the code and leave more opportunities for errors.

Edit: Oops, I guess that notification variable is used in the function call after the printf. So the above advice can't be taken as is, though the message is still relevant: simplify first, then problem areas will become more clear. Your junior high algebra teacher was right.

Last edited by Trilby (2012-06-12 19:33:23)

Unia · 2012-06-12 19:34:45

jdarnold wrote:

Well, one important part of programming is debugging, so this is a good time to learn that. Like I said, install 'ddd' which is a debugger and run your code. It will show exactly where the code is segfaulting (almost certainly overwriting a buffer).

Gives the same output as running gdb:

Program received signal SIGSEGV, Segmentation fault.
0x00007ffff7bd3d50 in mpd_status_get_state () from /usr/lib/libmpdclient.so.2
(gdb)

So yea, the error is indeed in mpd_status_get_state (). Without my modifications it works, so I guess that MPD_STATE_PLAY and MPD_STATE_PAUSE are somehow interfering.

Trilby · 2012-06-12 19:45:55

Did you modify the "else if" clause, or did you just add it?

If you added it, the second call to mpd_recv_status(conn) may be the problem. If so, the solution would be to do

SomeAppropriateType my_status mpd_recv_status(conn);
if ( my_status == MPD_STATE_PLAY ) {
    ...
}
else if ( my_status == MPD_STATE_PAUSED ) {
    .... your stuff here.
}
else {
    ....
}

Unia · 2012-06-12 19:54:38

Trilby wrote:

Did you modify the "else if" clause, or did you just add it?

Yes, I added this myself. I will have a look at the code you suggested and see if I can somehow get it together. Thanks again guys!

Unia · 2012-06-12 20:31:45

Again, I don't really have an idea what I'm doing. I just got it to compile without errors but something is wrong or missing for sure. It stops segfaulting now, but instead always displays TEXT_STOP with every notification.

Here's what I added:

char * MpdRecvStatus = NULL;
char * my_status = NULL;

...

MpdRecvStatus; my_status; mpd_recv_status(conn);
if (my_status == MPD_STATE_PLAY) {
            mpd_response_next(conn);

            song = mpd_recv_song(conn);

            if ((title = g_markup_escape_text(mpd_song_get_tag(song, MPD_TAG_TITLE, 0), -1)) ==$
                title = TEXT_UNKNOWN;
            if ((artist = g_markup_escape_text(mpd_song_get_tag(song, MPD_TAG_ARTIST, 0), -1)) $
                artist = TEXT_UNKNOWN;
            if ((album = g_markup_escape_text(mpd_song_get_tag(song, MPD_TAG_ALBUM, 0), -1)) ==$
                album = TEXT_UNKNOWN;

            notification = (char *) malloc(strlen(TEXT_PLAY) + strlen(title) + strlen(artist) +$
            sprintf(notification, TEXT_PLAY, title, artist, album);

            mpd_song_free(song);
        // unia
} else if (my_status == MPD_STATE_PAUSE) {
            notification = (char *) malloc(strlen(TEXT_PAUSE));
            sprintf(notification, TEXT_PAUSE);
        // unia
        } else {
            notification = (char *) malloc(strlen(TEXT_STOP));
            sprintf(notification, TEXT_STOP);
        }

The lines that start most left are Trilby's suggestion. I suspect the error to be somewhere here:

MpdRecvStatus; my_status; mpd_recv_status(conn);

That's the line I came up with, with the suggestions I got from the errors I got from compiling. God, I feel like such a noob

EDIT: Anyone got any suggestion for a good (e-)book to learn the basics of C?

Last edited by Unia (2012-06-12 20:33:14)

ewaller · 2012-06-12 21:14:29

Unia wrote:

Anyone got any suggestion for a good (e-)book to learn the basics of C?

The C Programming Language, Brian W. Kernighan, Dennis M. Ritchie

I own three copies (two of them are first edition, pre ANSI). It is available in pdf on line, but I'll not post links here.

It is not a hand holding book, but it is the most compact, concise reference I've ever seen.

Last edited by ewaller (2012-06-12 21:15:01)

Unia · 2012-06-12 21:17:32

^ Thanks! That should at least get me started. In the weekend I'll try to find some in Dutch in the library

Trilby · 2012-06-12 21:42:48

Oops sorry, I had one typo in my suggestion ... then you went and added another one, so when I saw the final result I thought "WTF!"

That line should be

MpdRecvStatus my_status = mpd_recv_status(conn);

and can be read as "decare variable my_status as type MpdRecvStatus and set it equal to the return value of mpd_recv_status(conn)"

If the segfault is gone that was almost certainly do to the multiple calls to mpd_recv_status(conn). That function must change the value of "conn" and or frees it's memory so when it is called a second time there is nothing left for it to read from.

You cannot then call that function twice without resetting conn (which you wouldn't want to do for these purposes). But you can get the return value of mpd_recv_status and store it to a variable (my_status) then you can use it as much as you'd like.

Given the typos, the new error is exactly what should be expected: my_status is never set to anything so the if conditionals never pass (eg, my_status is empty, so my_status does not equal MPD_PLAY or any of those other constants). Only the "else" block will execute and produce the results you now see. There's a good chance fixing those typos in that one line may have it all working smoothly.

Last edited by Trilby (2012-06-12 21:50:35)

Unia · 2012-06-12 21:58:16

Ah, that brings us somewhat further. However, it still complains when compiling:

==> Starting build()...
mpd-notification.c: In functie ‘main’:
mpd-notification.c:61:15: fout: expected ‘;’ before ‘my_status’
mpd-notification.c:62:16: let op: vergelijking tussen pointer en integer [enabled by default]
mpd-notification.c:80:21: let op: vergelijking tussen pointer en integer [enabled by default]
==> ERROR: A failure occurred in build().
    Aborting...

When I add the ; it compiles fine, but I get the same result: TEXT_STOP on all notifications.

So I guess there's still something wrong with either

MpdRecvStatus my_status = mpd_recv_status(conn);

or

char * MpdRecvStatus = NULL;
char * my_status = NULL;

EDIT:

Trilby wrote:

Only the "else" block will execute and produce the results you now see. There's a good chance fixing those typos in that one line may have it all working smoothly.

What typos are you talking about? By the way thanks for contuining even after I said I'd give up and come back to it later. I'm learning lots already and even though I'm not succeeding yet, it's great fun!

Last edited by Unia (2012-06-12 22:01:27)

/dev/zero · 2012-06-12 22:01:33

Unia wrote:

EDIT: Anyone got any suggestion for a good (e-)book to learn the basics of C?

The best way to learn is practice. You can get some good stuff out of some books, but sometimes the author doesn't adhere to best practices, and I've found the best books only tell you what to go and practice anyway.

I suggest choose a non-trivial motivator. For me, I've been trying to beef up my C skills, and my motivator has been "write a simple roguelike with ncurses". Once you have a motivator, start trying to make it work. Stfw for anything you don't know how to do yet, expect to make mistakes that require a re-write, and try to discipline yourself to best practices as soon as possible (eg avoid memory leaks, and test for whether pointers are NULL before using them).

Trilby · 2012-06-12 22:02:21

YES, there is. MpdRecvStatus is not a type. I overlooked that in your version.

That line needs to be:
<variable type> <variable name> = mpd_recv_status(conn);

The variable name can be anything you want, "my_status" is good.

The variable type needs to be a defined type matching that function's return value. I assumed you had found MpdRecvStatus to be the correct type, but that is just another variable name.

The type should match the return type of the function mpd_recv_status. ~~It must be a numerical type judging from context. Perhaps you could declare it as "int" but~~ it would be best to figure out what that function returns.

Edit: it should be

struct mpd_status *mystatus = mpd_recv_status(conn);

if this code snippet is correct.

Last edited by Trilby (2012-06-12 22:06:30)

Unia · 2012-06-12 22:08:38

Trilby wrote:

The type should match the return type of the function mpd_recv_status. ~~It must be a numerical type judging from context. Perhaps you could declare it as "int" but~~ it would be best to figure out what that function returns.

Hm, you mean this?

How can I find the correct type for the variable?

EDIT: Man, you're fast! Hard to keep up with all your edits I have to go to bed now, but tomorrow after my summer job I will check back!

Thanks so much everyone, especially Trilby!

Last edited by Unia (2012-06-12 22:11:44)

Arch Linux

#1 2012-06-11 20:21:51

[SOLVED] Can you tell me why this segfaults?

#2 2012-06-11 20:26:54

Re: [SOLVED] Can you tell me why this segfaults?

#3 2012-06-11 20:38:23

Re: [SOLVED] Can you tell me why this segfaults?

#4 2012-06-11 20:59:15

Re: [SOLVED] Can you tell me why this segfaults?

#5 2012-06-11 21:01:05

Re: [SOLVED] Can you tell me why this segfaults?

#6 2012-06-11 21:11:13

Re: [SOLVED] Can you tell me why this segfaults?

#7 2012-06-11 22:11:24

Re: [SOLVED] Can you tell me why this segfaults?

#8 2012-06-12 14:21:28

Re: [SOLVED] Can you tell me why this segfaults?

#9 2012-06-12 14:49:24

Re: [SOLVED] Can you tell me why this segfaults?

#10 2012-06-12 16:52:41

Re: [SOLVED] Can you tell me why this segfaults?

#11 2012-06-12 17:26:27

Re: [SOLVED] Can you tell me why this segfaults?

#12 2012-06-12 17:37:04

Re: [SOLVED] Can you tell me why this segfaults?

#13 2012-06-12 19:14:14

Re: [SOLVED] Can you tell me why this segfaults?

#14 2012-06-12 19:29:25

Re: [SOLVED] Can you tell me why this segfaults?

#15 2012-06-12 19:34:45

Re: [SOLVED] Can you tell me why this segfaults?

#16 2012-06-12 19:45:55

Re: [SOLVED] Can you tell me why this segfaults?

#17 2012-06-12 19:54:38

Re: [SOLVED] Can you tell me why this segfaults?

#18 2012-06-12 20:31:45

Re: [SOLVED] Can you tell me why this segfaults?

#19 2012-06-12 21:14:29

Re: [SOLVED] Can you tell me why this segfaults?

#20 2012-06-12 21:17:32

Re: [SOLVED] Can you tell me why this segfaults?

#21 2012-06-12 21:42:48

Re: [SOLVED] Can you tell me why this segfaults?

#22 2012-06-12 21:58:16

Re: [SOLVED] Can you tell me why this segfaults?

#23 2012-06-12 22:01:33

Re: [SOLVED] Can you tell me why this segfaults?

#24 2012-06-12 22:02:21

Re: [SOLVED] Can you tell me why this segfaults?

#25 2012-06-12 22:08:38

Re: [SOLVED] Can you tell me why this segfaults?

Board footer