Omar Polo <op@omarpolo.com> wrote:
> On 2023/12/20 23:24:31 +1100, Mark Jamsek <mark@jamsek.com> wrote:
> > Omar Polo <op@omarpolo.com> wrote:
> > > 
> > > by the way, create_diff() has a similar problem with f not being closed
> > > if the second got_opentemp() or fclose() fail.  A similar treatment
> > > could be applied to it too.
> > > 
> > 
> > Thanks for letting me know about this one!
> > 
> > We could fix it a few ways, and I'm partial to either of the below so
> > whichever you prefer is ok with me :)
> > 
> > As alluded to in your previous email, we can just cleanup f on error in
> > create_diff() if we haven't already assigned f to s->f as in the topmost
> > diff.
> > 
> > Alternatively, because s->f is always cleaned up in close_diff_view() on
> > error or otherwise, we can just reorder things to close s->f before
> > opening f and then assign f to s->f before calling got_opentemp() the
> > second time, thereby leaving cleanup to close_diff_view() as in the
> > second below diff (which also changes a few goto done to return err as
> > there's no cleanup needed in such cases).
> 
> I prefer the second diff, the first one depends too much on the order of
> the operations at the top of the function.
> 
> > [...]
> > --- tog/tog.c
> > +++ tog/tog.c
> > @@ -5318,21 +5318,17 @@ create_diff(struct tog_diff_view_state *s)
> >  		return got_error_from_errno("malloc");
> >  	s->nlines = 0;
> >  
> > +	if (s->f && fclose(s->f) == EOF)
> > +		return got_error_from_errno("fclose");
> 
> I'm not sure, but should we also set s->f = NULL before returning here?
> 
> Realistically speaking this can fail only if we have a bug elsewhere and
> end up closing the fd backing this FILE.  It's great that we catch the
> error, but then we leave s->f != NULL and attempt to fclose() it again,
> which I'm not sure it's allowed.


Yes, we should. Thanks! Actually, I think this is another UB fix?
Before this diff, whenever this fclose(s->f) failed, we goto done and
call fflush(s->f) and then call fclose(s->f) again in close_diff_view().
I should have spotted this when plugging the leak.

> > +
> >  	f = got_opentemp();
> > -	if (f == NULL) {
> > -		err = got_error_from_errno("got_opentemp");
> > -		goto done;
> > -	}
> > +	if (f == NULL)
> > +		return got_error_from_errno("got_opentemp");
> > +	s->f = f;
> > +
> 
> just a nit, you can replace f with s->f and remove the f variable
> entirely.
> 
> ok op@ either way.
> 
> Thanks!

Thanks, op! That actually fixes stsp's observation too. New diff below.


Stefan Sperling <stsp@stsp.name> wrote:
> On Wed, Dec 20, 2023 at 11:24:31PM +1100, Mark Jamsek wrote:
> > diff /home/mark/src/got
> > commit - 85467924748b0e27f0105bde280878a149df9fc8
> > path + /home/mark/src/got
> > blob - be366266e526e26c8734719d20d29ed05d463b06
> > file + tog/tog.c
> > --- tog/tog.c
> > +++ tog/tog.c
> > @@ -5318,21 +5318,17 @@ create_diff(struct tog_diff_view_state *s)
> >  		return got_error_from_errno("malloc");
> >  	s->nlines = 0;
> >  
> > +	if (s->f && fclose(s->f) == EOF)
> > +		return got_error_from_errno("fclose");
> > +
> 
> s->f should be set to NULL here because ...
> 
> >  	f = got_opentemp();
> > -	if (f == NULL) {
> > -		err = got_error_from_errno("got_opentemp");
> > -		goto done;
> > -	}
> > +	if (f == NULL)
> > +		return got_error_from_errno("got_opentemp");
> 
> ... otherwise if we return above, s->f has been closed while
> s->f is still non-NULL, and some other code path might try
> to close it again.
> 

I think op's suggestion to ditch f entirely and just use s->f fixes
this?  I'm not even sure why I was using the temporary tbh.


diff 85467924748b0e27f0105bde280878a149df9fc8 15c9072be9bc5952579bbb8a3cad7e12c9f3a009
commit - 85467924748b0e27f0105bde280878a149df9fc8
commit + 15c9072be9bc5952579bbb8a3cad7e12c9f3a009
blob - be366266e526e26c8734719d20d29ed05d463b06
blob + 1d2d2883f6f459ee2e81334d5953c0559d5de275
--- tog/tog.c
+++ tog/tog.c
@@ -5304,7 +5304,7 @@ static const struct got_error *
 create_diff(struct tog_diff_view_state *s)
 {
 	const struct got_error *err = NULL;
-	FILE *f = NULL, *tmp_diff_file = NULL;
+	FILE *tmp_diff_file = NULL;
 	int obj_type;
 	struct got_diff_line *lines = NULL;
 	struct got_pathlist_head changed_paths;
@@ -5318,22 +5318,19 @@ create_diff(struct tog_diff_view_state *s)
 		return got_error_from_errno("malloc");
 	s->nlines = 0;
 
-	f = got_opentemp();
-	if (f == NULL) {
-		err = got_error_from_errno("got_opentemp");
-		goto done;
-	}
-	tmp_diff_file = got_opentemp();
-	if (tmp_diff_file == NULL) {
-		err = got_error_from_errno("got_opentemp");
-		goto done;
-	}
 	if (s->f && fclose(s->f) == EOF) {
-		err = got_error_from_errno("fclose");
-		goto done;
+		s->f = NULL;
+		return got_error_from_errno("fclose");
 	}
-	s->f = f;
 
+	s->f = got_opentemp();
+	if (s->f == NULL)
+		return got_error_from_errno("got_opentemp");
+
+	tmp_diff_file = got_opentemp();
+	if (tmp_diff_file == NULL)
+		return got_error_from_errno("got_opentemp");
+
 	if (s->id1)
 		err = got_object_get_type(&obj_type, s->repo, s->id1);
 	else


-- 
Mark Jamsek <https://bsdbox.org>
GPG: F2FF 13DE 6A06 C471 CA80  E6E2 2930 DC66 86EE CF68