CS50: Segmentation Fault in Duration part of pset3, Music (C)

Question

I included the proper libraries and all, this part of the code gives segmentation fault for some reason:

int numerator = atoi(&fraction[0]);  
int denominator = atoi(&fraction[2]);  
return ((numerator / denominator) * 8);

Whereas if I choose not to convert to int using atoi(), and instead do a bunch of if statements like this:

if (fraction[0] == '1' && fraction[2] == '8')
{
    return 1;
}

else if (fraction[0] == '1' && fraction[2] == '4')
{
    return 2;
}
.
.
.
.

It works.
I'm pretty confused, and I would like to use the first method because it is much more efficient.

Edit : This is part of a problem given by Harvard in its course, CS50. The full program uses several files that Harvard wrote and that depend on each other. My role was just to write the snippet I'm asking about here. Of course I read and understood the code in the files they provided. I'm afraid I can't copy the entire program here in a way that'd make it verifiable without it being very long. I was hoping someone familiar with the course's problems could answer, or that the error would be clear in the snippet alone. Thank you regardless, I apologize for being unclear.

Since we don't know what the rest of your code looks like, the snippet you've shown is fairly meaningless. Try putting together a [mcve]. — Retired Ninja, Aug 02 '18 at 05:44
`(1 / 8) * 8 == (0) * 8 == 0` (not `1`) ; `(1 / 4) * 8 == (0) * 8 == 0` (not `2`) — Sander De Dycker, Aug 02 '18 at 06:35
You should favor `strtol` over `atoi` as the latter can't be checked completely for errors. — Inrin, Aug 02 '18 at 06:36
when you return `0` in your alternative code, does it also generate a segmentation fault ? — Sander De Dycker, Aug 02 '18 at 06:41
@Sandar De Dycker Thank you so much ! I didn't realize I was jumbling up data types pretty bad. I used type casting and it works ! I put this after the first two lines: float fr = (float) numerator / (float) denominator; return (int) fr * 8; — Basel Yaser, Aug 02 '18 at 06:41
@BaselYaser : added it as an answer - your solution should do the trick indeed. — Sander De Dycker, Aug 02 '18 at 06:46

Some programmer dude · Answer 1 · 2018-08-02T06:01:30.190

0

You attempt to pass a single character (it seems) to the atoi function, when it wants a null-terminated byte string.

All strings in C must be terminated by the null character '\0'. If that terminator doesn't exist, the string functions will not know where the string ends, and go out of bounds which will lead to undefined behavior.

If you have a single digit character, and want to convert it to an integer value corresponding to the digit, there's a simple trick: Subtract the character '0'. As in fraction[0] - '0'. That will give you the integer value of the digit in fraction[0].

And before you do a division with a user-input value, make sure the denominator is not zero.

edited Aug 02 '18 at 06:01

answered Aug 02 '18 at 05:48

Some programmer dude

400,186
35
402
621

Thank you, I changed the code to this but it still gives a segmentation fault error: int numerator = fraction[0] - '0'; int denominator = fraction[2] - '0'; return ((numerator / denominator) * 8) The error must be coming from this part by the way, because if I remove it and use if statements like I wrote above, the program works. – Basel Yaser Aug 02 '18 at 05:54
@BaselYaser And if you print the values before doing the division, what are they? The denominator is not zero? Are you sure the crash happens where you *think* it happens? If you run a debug build in a debugger and catch the crash, where in your code does it happen? – Some programmer dude Aug 02 '18 at 05:57
I put a printf function before doing the division and it does print out integers and the denominator is never 0. I then removed the division and just put a random value like return 2, and the program did not give a segmentation fault error. So it is coming from the return statement with the division. I'm not sure what about the division is causing a segmentation fault. – Basel Yaser Aug 02 '18 at 06:06
@BaselYaser Then it's really time you edit your question with a [Minimal, Complete, and Verifiable Example](http://stackoverflow.com/help/mcve). Otherwise we can only do what I did: Guess, and often guess badly. – Some programmer dude Aug 02 '18 at 06:08

score 0 · Answer 2 · answered Aug 02 '18 at 06:45

0

You're performing integer division, which doesn't give you the results you seem to expect.

Specifically :

(1 / 8) * 8 == (0) * 8 == 0

and not 1. Similarly :

(1 / 4) * 8 == (0) * 8 == 0

and not 2.

I suspect that returning 0 from your function causes a segmentation fault in the calling code. You can verify that by returning 0 in your alternative code, to see if it also generates a segmentation fault.

In order to get the results you expect, one way would be to :

return (int) ((((double) numerator) / denominator) * 8);

answered Aug 02 '18 at 06:45

Sander De Dycker

16,053
1
35
40

Well, given the return type (if it's correct), maybe for OP's use case it's enough to change the order of execution: `return numerator * 8 / denominator;`. – Bob__ Aug 02 '18 at 09:19
@Bob__ : sure - as long as that doesn't cause an overflow (ie. for sufficiently small values of `numerator`) – Sander De Dycker Aug 02 '18 at 09:24

CS50: Segmentation Fault in Duration part of pset3, Music (C)

I included the proper libraries and all, this part of the code gives segmentation fault for some reason:

Whereas if I choose not to convert to int using atoi(), and instead do a bunch of if statements like this:

2 Answers2