Issue
The following code is supposed to read the file "rules.txt" and write it to a device line by line. The flow should be:
- Read line from rules.txt
- Echo it to device
The following code always end in segfault because of readline and I have no idea why:
#include <stdio.h>
#include <stdlib.h>
#include <unistd.h>
#include <string.h>
#include <errno.h>
#include <signal.h>
#include <fcntl.h>
#include <ctype.h>
#include <termios.h>
#include <sys/types.h>
#include <sys/mman.h>
#define BUFFER_LENGTH 256
int main()
{
char *line;
size_t len = BUFFER_LENGTH;
int fd = open("./rules.txt", O_RDONLY);
if(fd == -1)
{ perror("open failed"); return 0; }
FILE* fout = fopen("/sys/class/Rule_Table_Class/Rule_Table_Class_Rule_Table_Device/sysfs_att", "w+");
if(fout == NULL)
{ close(fd); perror("fopen failed, log.txt is busy!"); return 0; }
while (1)
{
line = (char*) malloc(len*sizeof(char));
if(line==NULL){
perror("malloc failed!"); return 0;
}
int bytesRead = getline(&line, &len, fd);
if (bytesRead == -1)
{
perror("Failed to read the message from the device.");
return errno;
}
sprintf(line,"%s","lala");
printf("line = %s", line);
}
fclose(fout);
close(fd);
return 0;
}
EDIT:
I corrected the code and still get a segfault. Here is the corrected code:
#include <stdio.h>
#include <stdlib.h>
#include <unistd.h>
#include <string.h>
#include <errno.h>
#include <signal.h>
#include <fcntl.h>
#include <ctype.h>
#include <termios.h>
#include <sys/types.h>
#include <sys/mman.h>
#define BUFFER_LENGTH 256
int main()
{
FILE * fp;
char * line = NULL;
size_t len = 0;
ssize_t read;
fp = fopen("./rules.txt", "r");
if(fp == NULL)
{ perror("open failed"); return 0; }
FILE* fout = fopen("/sys/class/Rule_Table_Class/Rule_Table_Class_Rule_Table_Device/sysfs_att", "w+");
if(fout == NULL)
{ perror("fopen failed!"); return 0; }
while (1)
{
ssize_t bytesRead = getline(&line, &len, fp);
if (bytesRead == -1)
{
return 0;
}
printf("line = %s", line);
fprintf(line,"%s",fout);
}
fclose(fout);
fclose(fp);
return 0;
}
Solution
First of all, the proper core getline() loop is
/* FILE *in = fopen(..., "r"); */
char *line = NULL;
size_t size = 0;
while (1) {
ssize_t len = getline(&line, &size, in);
if (len < 0)
break;
/* You have 'len' chars at 'line', with line[len] == '\0'. */
}
so, it is not getline() that causes the segfault, it is your fprintf(line, "%s", fout); that should be either fprintf(fout, "%s", line); or just fputs(line, fout);, or fwrite(line, 1, bytesRead, fout); since the line can contain embedded NUL bytes that fprintf() and fputs() consider an end of string mark.
If we modify the code so that the source and target file names are taken as command-line arguments (with - denoting standard input or standard output), this is what I'd personally like to see:
/* SPDX-License-Identifier: CC0-1.0 */
/* This tells the GNU C library to expose POSIX features, including getline(). */
#define _POSIX_C_SOURCE 200809L
#include <stdlib.h>
#include <string.h>
#include <stdio.h>
#include <errno.h>
int main(int argc, char *argv[])
{
if (argc != 3 || !strcmp(argv[1], "-h") || !strcmp(argv[1], "--help")) {
const char *self = (argc > 0 && argv && argv[0] && argv[0][0]) ? argv[0] : "(this)";
fprintf(stderr, "\n");
fprintf(stderr, "Usage: %s [ -h | --help ]\n", self);
fprintf(stderr, " %s SOURCE TARGET\n", self);
fprintf(stderr, "\n");
fprintf(stderr, "This copies all content from SOURCE to TARGET, line by line.\n");
fprintf(stderr, "Use '-' for standard input source or standard output target.\n");
fprintf(stderr, "\n");
return EXIT_SUCCESS;
}
const char *srcpath = argv[1];
const char *dstpath = argv[2];
/* If the path is "-", set it to NULL. */
if (srcpath && !strcmp(srcpath, "-"))
srcpath = NULL;
if (dstpath && !strcmp(dstpath, "-"))
dstpath = NULL;
FILE *src, *dst;
/* Open source for reading. If srcpath is NULL, use stdin. */
if (srcpath) {
src = fopen(srcpath, "r");
if (!src) {
fprintf(stderr, "%s: %s.\n", srcpath, strerror(errno));
return EXIT_FAILURE;
}
} else {
src = stdin;
}
/* Open target for writing. If dstpath is NULL, use stdout. */
if (dstpath) {
dst = fopen(dstpath, "w");
if (!dst) {
fprintf(stderr, "%s: %s.\n", dstpath, strerror(errno));
fclose(src);
return EXIT_FAILURE;
}
} else {
dst = stdout;
}
char *line = NULL;
size_t size = 0;
unsigned long linenum = 0;
while (1) {
ssize_t len = getline(&line, &size, src);
if (len < 0)
break;
linenum++;
if (fwrite(line, 1, len, dst) != (size_t)len) {
fprintf(stderr, "%s: Write error on line %lu.\n", dstpath ? dstpath : "(standard output)", linenum);
fclose(dst);
fclose(src);
return EXIT_FAILURE;
}
}
/* Technically, we don't need to release dynamically allocated (non-shared) memory,
because we're just about to exit, and the OS will automagically do that for us.
We can do this at any point we want during the loop, too. */
free(line);
line = NULL;
size = 0;
/* We do not know why getline() returned -1. Check if an error occurred, and whether we're at end of input. */
if (ferror(src) || !feof(src)) {
fprintf(stderr, "%s: Read error on line %lu.\n", srcpath ? srcpath : "(standard input)", linenum);
fclose(dst);
fclose(src);
return EXIT_FAILURE;
}
/* Check if any write errors have occurred. */
if (ferror(dst)) {
fprintf(stderr, "%s: Write error on line %lu.\n", dstpath ? dstpath : "(standard output)", linenum);
fclose(dst);
fclose(src);
}
/* Read errors should not occur at close time, but it costs very little for us to test anyway. */
if (fclose(src)) {
fprintf(stderr, "%s: Read error on line %lu.\n", srcpath ? srcpath : "(standard input)", linenum);
fclose(dst);
return EXIT_FAILURE;
}
/* Write errors can occur at close time, if the output has been buffered, or the target is on
a remote filesystem. Again, it costs us very little to check. */
if (fclose(dst)) {
fprintf(stderr, "%s: Write error on line %lu.\n", dstpath ? dstpath : "(standard output)", linenum);
return EXIT_FAILURE;
}
/* No errors; target is an identical copy of the source file. */
fprintf(stderr, "%lu lines copied successfully.\n", linenum);
return EXIT_SUCCESS;
}
The SPDX-License-Identifier is a common way to indicate the license of the code. I use CC0-1.0, which basically means "use as you wish, just don't blame the author for any issues: no guarantees, no warranties."
The #define _POSIX_C_SOURCE 200809L tells the GNU C library that we want it to expose POSIX.1-2008 features, which include getline(). If you look at man 3 getline, you'll see in the Synopsis section that glibc 2.10 and later require _POSIX_C_SOURCE to be defined to at least 200809L.
Most of the code is to print the usage, and getting the source and target file names from the command line, and handling - as a special name, "standard stream", so that it can be used even in pipes by specifying - as the input and/or output file name.
The getline() loop uses fwrite() to write the line to the target file. This way, if the input contains embedded NUL bytes (\0), the target will still be identical to the source file.
After the loop, we discard the line buffer, although since the program is just about to exit, we could omit that (since the OS will release all dynamically allocated (non-shared) memory when we exit anyway).
I like code that checks with ferror(src) || !feof(src) if an error occurred in src or src did not reach end of input; and that checks the return value of fclose(), in case a delayed (write) error is reported. Granted, fclose() should never fail for a read-only file, and fclose() should only fail for files we've written to in specific circumstances, but it costs very little to check, and this way the user running the program will be told if the program detected loss of data.
I believe it is morally reprehensible to ignore checking for such errors (especially "because they occur so rarely"), since such tests are the only way the human user can know whether the operation was successful, or if some odd problem occurred. It is up to the human to investigate any problems, and up to our programs to report detectable issues.
Answered By - Blabbo the Verbose