"GOT", but the "O" is a cute, smiling pufferfish. Index | Thread | Search

From:
"Todd C. Miller" <millert@openbsd.org>
Subject:
Re: Replace fparseln(3) with getline(3)
To:
Christian Weisgerber <naddy@mips.inka.de>
Cc:
gameoftrees@openbsd.org
Date:
Sat, 02 Jan 2021 13:28:32 -0700

Download raw body.

Thread
On Sat, 02 Jan 2021 20:07:47 +0100, Christian Weisgerber wrote:

> replace fparseln(3) with getline(3), for better portability
>
> This also reduces the malloc/free calls and fixes a memory leak in
> get_modified_file_content_status().

Comments about linesize handling inline.

 - todd

> diff refs/heads/main refs/heads/getline
> blob - 2cd2c5fefcbd23e58b1bef2416f1cf6133544a8e
> blob + cd435b5446b903251e192c7dcaaea046c7f36d55
> --- lib/reference.c
> +++ lib/reference.c
> @@ -359,9 +359,9 @@ open_packed_ref(struct got_reference **ref, FILE *f, c
>  {
>  	const struct got_error *err = NULL;
>  	char *abs_refname;
> -	char *line;
> -	size_t len;
> -	const char delim[3] = {'\0', '\0', '\0'};
> +	char *line = NULL;
> +	size_t linesize = 0;
> +	ssize_t linelen;
>  	int i, ref_is_absolute = (strncmp(refname, "refs/", 5) == 0);
>  
>  	*ref = NULL;
> @@ -369,13 +369,15 @@ open_packed_ref(struct got_reference **ref, FILE *f, c
>  	if (ref_is_absolute)
>  		abs_refname = (char *)refname;
>  	do {
> -		line = fparseln(f, &len, NULL, delim, 0);
> -		if (line == NULL) {
> +		linelen = getline(&line, &linesize, f);
> +		if (linelen == -1) {
>  			if (feof(f))
>  				break;
>  			err = got_ferror(f, GOT_ERR_BAD_REF_DATA);
>  			break;
>  		}
> +		if (linelen > 0 && line[linelen-1] == '\n')
> +			line[linelen-1] = '\0';
>  		for (i = 0; i < nsubdirs; i++) {
>  			if (!ref_is_absolute &&
>  			    asprintf(&abs_refname, "refs/%s/%s", subdirs[i],
> @@ -387,10 +389,10 @@ open_packed_ref(struct got_reference **ref, FILE *f, c
>  			if (err || *ref != NULL)
>  				break;
>  		}
> -		free(line);
>  		if (err)
>  			break;
>  	} while (*ref == NULL);
> +	free(line);
>  
>  	return err;
>  }
> @@ -871,6 +873,7 @@ got_ref_list(struct got_reflist_head *refs, struct got
>  	char *packed_refs_path, *path_refs = NULL;
>  	char *abs_namespace = NULL;
>  	char *buf = NULL, *ondisk_ref_namespace = NULL;
> +	char *line = NULL;
>  	FILE *f = NULL;
>  	struct got_reference *ref;
>  	struct got_reflist_entry *new;
> @@ -963,19 +966,19 @@ got_ref_list(struct got_reflist_head *refs, struct got
>  	f = fopen(packed_refs_path, "r");
>  	free(packed_refs_path);
>  	if (f) {
> -		char *line;
> -		size_t len;
> -		const char delim[3] = {'\0', '\0', '\0'};
> +		size_t linesize;

linesize needs to be initialized to 0

> +		ssize_t linelen;
>  		for (;;) {
> -			line = fparseln(f, &len, NULL, delim, 0);
> -			if (line == NULL) {
> +			linelen = getline(&line, &linesize, f);
> +			if (linelen == -1) {
>  				if (feof(f))
>  					break;
>  				err = got_ferror(f, GOT_ERR_BAD_REF_DATA);
>  				goto done;
>  			}
> +			if (linelen > 0 && line[linelen-1] == '\n')
> +				line[linelen-1] = '\0';
>  			err = parse_packed_ref_line(&ref, NULL, line);
> -			free(line);
>  			if (err)
>  				goto done;
>  			if (ref) {
> @@ -1001,6 +1004,7 @@ got_ref_list(struct got_reflist_head *refs, struct got
>  done:
>  	free(abs_namespace);
>  	free(buf);
> +	free(line);
>  	free(path_refs);
>  	if (f && fclose(f) != 0 && err == NULL)
>  		err = got_error_from_errno("fclose");
> @@ -1174,7 +1178,7 @@ delete_packed_ref(struct got_reference *delref, struct
>  	const struct got_error *err = NULL, *unlock_err = NULL;
>  	struct got_lockfile *lf = NULL;
>  	FILE *f = NULL, *tmpf = NULL;
> -	char *packed_refs_path, *tmppath = NULL;
> +	char *line = NULL, *packed_refs_path, *tmppath = NULL;
>  	struct got_reflist_head refs;
>  	int found_delref = 0;
>  
> @@ -1204,21 +1208,21 @@ delete_packed_ref(struct got_reference *delref, struc
> t
>  		goto done;
>  	}
>  	for (;;) {
> -		char *line;
> -		size_t len;
> -		const char delim[3] = {'\0', '\0', '\0'};
> +		size_t linesize;

linesize needs to be initialized to 0 and should be declared outside
the loop.

> +		ssize_t linelen;
>  		struct got_reference *ref;
>  		struct got_reflist_entry *new;
>  
> -		line = fparseln(f, &len, NULL, delim, 0);
> -		if (line == NULL) {
> +		linelen = getline(&line, &linesize, f);
> +		if (linelen == -1) {
>  			if (feof(f))
>  				break;
>  			err = got_ferror(f, GOT_ERR_BAD_REF_DATA);
>  			goto done;
>  		}
> +		if (linelen > 0 && line[linelen-1] == '\n')
> +			line[linelen-1] = '\0';
>  		err = parse_packed_ref_line(&ref, NULL, line);
> -		free(line);
>  		if (err)
>  			goto done;
>  		if (ref == NULL)
> @@ -1310,6 +1314,7 @@ done:
>  	}
>  	free(tmppath);
>  	free(packed_refs_path);
> +	free(line);
>  	got_ref_list_free(&refs);
>  	return err ? err : unlock_err;
>  }
> blob - 38c6dc6f1c93d31c2b51ea0514de0fbd09d5dfc3
> blob + 5dfe5fbfdef7f0419e78c3302df7d292acfa2f2e
> --- lib/worktree.c
> +++ lib/worktree.c
> @@ -1599,13 +1599,13 @@ get_modified_file_content_status(unsigned char *statu
> s
>  		GOT_DIFF_CONFLICT_MARKER_END
>  	};
>  	int i = 0;
> -	char *line;
> -	size_t len;
> -	const char delim[3] = {'\0', '\0', '\0'};
> +	char *line = NULL;
> +	size_t linesize = 0;
> +	ssize_t linelen;
>  
>  	while (*status == GOT_STATUS_MODIFY) {
> -		line = fparseln(f, &len, NULL, delim, 0);
> -		if (line == NULL) {
> +		linelen = getline(&line, &linesize, f);
> +		if (linelen == -1) {
>  			if (feof(f))
>  				break;
>  			err = got_ferror(f, GOT_ERR_IO);
> @@ -1620,6 +1620,7 @@ get_modified_file_content_status(unsigned char *status
>  				i++;
>  		}
>  	}
> +	free(line);
>  
>  	return err;
>  }
> -- 
> Christian "naddy" Weisgerber                          naddy@mips.inka.de
>