write to shared memory segmentation fault

Question

all I want to do is just write "hey" to my shared memory, but it gets thrown at that line. very simple code as follows:

#include <stdlib.h>
#include <stdio.h>
#include <string.h>
#include <sys/fcntl.h>
#include <unistd.h>
#include <sys/types.h>
#include <sys/ipc.h>
#include <sys/sem.h>
#include <sys/shm.h>
#define SHM_SIZE 1024
#define FLAGS IPC_CREAT | 0644
int main(){
    key_t key;
    int shmid;
    if ((key = ftok("ex31.c", 'k')) == -1){ 
        exit(1);}        
    if ((shmid = shmget(key, SHM_SIZE, FLAGS)) == -1) {
            exit(1);}
    char* shmaddr;
    if( shmaddr=shmat(shmid,0,0) == (char*)-1){   //WRONG ARGUMENTS ??
            exit(0); }
    printf("opened shared memory\n");  //gets here
    strcpy(shmaddr, "hey");     //GETS THROWN HERE
    printf("after writing to memory\n"); //never get here

the error the debugger gives me is:

Program received signal SIGSEGV, Segmentation fault. 0x0000000000401966 in main (argc=1, argv=0x7fffffffe068) at ../ex31.c:449 449 strcpy(shmaddr, "hey"); //GETS THROWN HERE

Did you get any compilation warnings? Have you read them? They should explain exactly what's wrong. — n. m. could be an AI, May 11 '16 at 10:26
`shmaddr=shmat(shmid,0,0) == (char*)-1` --> `(shmaddr=shmat(shmid,0,0)) == (char*)-1` — BLUEPIXY, May 11 '16 at 10:28
Please pause your program before `strcpy`, run `ipcs -m | grep ` in another shell and post the output of the latter — GMichael, May 11 '16 at 10:29
@BLUEPIXY Show a programmer his error, and he will be thankful to you for a day; but show him how to avoid errors, and he will be your competition for your entire life. — n. m. could be an AI, May 11 '16 at 10:33
@n.m. *show him how to avoid errors* **Don't put assignments in conditional clauses.** `if ((shmid = shmget(key, SHM_SIZE, FLAGS)) == -1)` and `if( shmaddr=shmat(shmid,0,0) == (char*)-1)` are *both* bad code - doing that is bug-prone. Two lines are much clearer, and two lines aren't bug-prone: `shmid = shmget(...); if ( shmid == -1 )...` won't ever have this problem. — Andrew Henle, May 11 '16 at 10:36
@AndrewHenle `if ((foo = bar) == foobar)` is idiomatic C. Beginners should learn it for no other reason than to be able to fluently read other peoples code since it is used everywhere. — Art, May 11 '16 at 11:38
@Art It is not idiomatic, it is dangerous rubbish, because it can easily be turned into `(foo == bar) == foobar` or `foo = foo++` or some other buggy mess. People used to write programs like that in the 70s and 80s. People invented obscure things like "Yoda conditions". And then came the compiler warnings against assignment inside conditions. Programs turned better, bugs disappeared, everyone was happy except Yoda. — Lundin, May 11 '16 at 11:49
@Art *`if ((foo = bar) == foobar)` is idiomatic C* The only reason beginners should learn it is so they learn not to use it. There is absolutely no reason to use a bug-prone construct that tries to stuff too much into one line when exactly equivalent constructs exist that are not bug-prone. When humans write code, **they write bugs**. Why on God's good Earth would anyone want to write code in a way that makes it *easier* to introduce bugs? Human minds are limited in ways computers are not. What is gained from trying to stuff 4 or 5 operations in one line other than overloading the coder? — Andrew Henle, May 11 '16 at 11:58
@Lundin It's idiomatic in my world. Kernels, libc, system programs for Unix, etc. I won't argue if it's a good idea or not. But it is used everywhere, still is in modern code. Just checked in some version of Linux I had: `grep 'if ((.* = .*) ==' -r . | wc` gets 830 matches. OpenSSH - 548. gcc - 62, binutils - 173, perl - 74, etc. It's useful to know if you'll be reading other peoples code which almost everyone will. — Art, May 11 '16 at 12:00
@Art The Unix/Linux world is a special snowflake, since there are such vast amounts of ancient junk code still present in those systems. And the awfully-written Linux kernel certainly didn't improve the situation. It might be that *nix is a lost cause, but that doesn't mean that everyone everywhere else should start writing crap code, just because such code is so common in *nix. — Lundin, May 11 '16 at 12:28
@Lundin *it is dangerous rubbish* As is C in general. Here, I can have an opinion too. — n. m. could be an AI, May 11 '16 at 12:31
@n.m. I don't disagree with that - I can only guess how many people there are that get killed by the C language every day. Couple of thousands every year? This would be why safe subsets like MISRA-C and CERT-C have become so successful. — Lundin, May 11 '16 at 12:40

Lundin · Answer 1 · 2016-05-11T11:57:41.850

The problem is operator precedence. In the line

shmaddr=shmat(shmid,0,0) == (char*)-1)

Then the unary - operator and the cast operator have the highest precedence, followed by ==, followed by = which has the lowest precedence. So your code turns out to be equal to

shmaddr=(shmat(shmid,0,0) == (char*)-1))

which is nonsense.

Assignment inside conditions is very bad programming practice. Every half-decent compiler will give you a warning if you try it. Apart from operator precedence issues, it is easy to mix up = and ==. Also, the = operator introduces a side effect to the expression, so mixing it with other operators could cause undefined behavior. And perhaps most importantly, putting = inside conditions usually reduce code readability quite a lot.

Your code should be written as:

shmaddr = shmat(shmid,0,0);
if(chmaddr  == (char*)-1){

It is important to understand that you gain nothing by merging these 2 rows into 1. The generated machine code will be identical, apart from the lack of bugs in the above version.

write to shared memory segmentation fault

1 Answers1