Skip to content

Commit 50f3dd8

Browse files
committed
libretro-common/queues: add task_free_error + document set/free protocol
task_set_title() and task_set_error() take ownership of a heap pointer and rely on the task system to free it at completion, but neither setter frees the *previous* value -- so calling either one twice on the same task without an intervening free leaks the prior string. task_free_title() has long existed for the title side of this protocol; the error side had no symmetric helper at all, forcing callers that legitimately needed to update the error mid- task to either guard with task_get_error() or just live with the leak. This commit: * Adds task_free_error(), symmetric to the pre-existing task_free_title() (both lock property_lock under HAVE_THREADS, free if non-NULL, NULL out the field). * Documents the free-then-set protocol clearly in queues/task_queue.h on both setters, both struct fields, and cross-references the helpers via @see -- the previous "@warning This does not free the existing X" line was correct but easy to miss, and three sites in tasks/task_cloudsync.c missed it. * Fixes those three task_cloudsync.c violators (lines 107, 1281, 1331). task_cloudsync sets the initial title at line 1439 via direct assignment, then later replaces it via task_set_title in three callbacks without calling task_free_title first. Each replacement leaks the prior strdup. * Adds a regression test under libretro-common/samples/queues/ task_queue_title_error_test/ exercising both helpers and the free-then-set protocol (single-set, replace-via-free, free-on- null no-op, double-free no-op, 50-iteration stress chain). Wires it into the existing libretro-common-samples CI workflow, so it builds under -fsanitize=address,undefined and LSan flags any future regression to the protocol. The test compiles task_queue.c without HAVE_THREADS so the slock / scond / sthread paths drop out cleanly -- the title/error protocol is identical with and without threading; the locks just serialise it. The single remaining external dependency (cpu_features_get_time_usec) is provided as a trivial stub since no test path drives the queue through gather(). Verified pre-patch: the test fails to link without task_free_error (undefined reference), confirming it genuinely exercises the new symbol rather than passing trivially. Verified post-patch: ASan + UBSan + LSan clean, all ten cases pass.
1 parent a0d08c1 commit 50f3dd8

6 files changed

Lines changed: 506 additions & 5 deletions

File tree

.github/workflows/Linux-libretro-common-samples.yml

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -77,6 +77,7 @@ jobs:
7777
rpng_chunk_overflow_test
7878
rpng_roundtrip_test
7979
word_wrap_overflow_test
80+
task_queue_title_error_test
8081
)
8182
8283
# Per-binary run command (overrides ./<binary> if present).

libretro-common/include/queues/task_queue.h

Lines changed: 53 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -209,7 +209,13 @@ struct retro_task
209209
* Human-readable details about an error that occurred while running the task.
210210
* Should be created and assigned within \c handler if there was an error.
211211
* Will be cleaned up by the task queue with \c free() upon this task's completion.
212+
*
213+
* If \c handler may set this more than once on the same task,
214+
* call \c task_free_error before the subsequent \c task_set_error
215+
* to avoid leaking the previous string.
216+
*
212217
* @see task_set_error
218+
* @see task_free_error
213219
*/
214220
char *error;
215221

@@ -230,8 +236,14 @@ struct retro_task
230236
* May be \c NULL,
231237
* but if not then it will be disposed of by the task system with \c free()
232238
* upon this task's completion.
233-
* Can be modified or replaced at any time.
239+
*
240+
* Can be modified or replaced at any time, but \c task_set_title
241+
* does \em not free the previous value: callers must call
242+
* \c task_free_title first to avoid leaking the prior title.
243+
*
234244
* @see strdup
245+
* @see task_set_title
246+
* @see task_free_title
235247
*/
236248
char *title;
237249

@@ -413,12 +425,27 @@ void task_set_flags(retro_task_t *task, uint8_t flags, bool set);
413425
* Sets \c task::error to the given value.
414426
* Thread-safe if the task queue is threaded.
415427
*
428+
* Ownership of \c error transfers to the task; the task system
429+
* will \c free() it when the task completes (or via
430+
* \c task_free_error). \c error must therefore point to memory
431+
* obtained via \c malloc / \c strdup / similar -- string
432+
* literals or stack buffers will cause an invalid free later.
433+
*
434+
* @warning This does \em not free the previous error message.
435+
* When replacing an already-set error, callers \em must call
436+
* \c task_free_error first to avoid a leak:
437+
* @code
438+
* task_free_error(task);
439+
* task_set_error(task, strdup("New error"));
440+
* @endcode
441+
* Alternatively, guard with \c task_get_error so the second
442+
* \c task_set_error is skipped when an error is already set.
443+
*
416444
* @param task The task to modify.
417445
* Behavior is undefined if \c NULL.
418446
* @param error The error message to set.
419447
* @see retro_task::error
420-
* @warning This does not free the existing error message.
421-
* The caller must do that itself.
448+
* @see task_free_error
422449
*/
423450
void task_set_error(retro_task_t *task, char *error);
424451

@@ -437,13 +464,25 @@ void task_set_progress(retro_task_t *task, int8_t progress);
437464
* Sets \c task::title to the given value.
438465
* Thread-safe if the task queue is threaded.
439466
*
467+
* Ownership of \c title transfers to the task; the task system
468+
* will \c free() it when the task completes (or via
469+
* \c task_free_title). \c title must therefore point to memory
470+
* obtained via \c malloc / \c strdup / similar -- string
471+
* literals or stack buffers will cause an invalid free later.
472+
*
473+
* @warning This does \em not free the previous title. When
474+
* replacing an already-set title, callers \em must call
475+
* \c task_free_title first to avoid a leak:
476+
* @code
477+
* task_free_title(task);
478+
* task_set_title(task, strdup("New title"));
479+
* @endcode
480+
*
440481
* @param task The task to modify.
441482
* Behavior is undefined if \c NULL.
442483
* @param title The title to set.
443484
* @see retro_task::title
444485
* @see task_free_title
445-
* @warning This does not free the existing title.
446-
* The caller must do that itself.
447486
*/
448487
void task_set_title(retro_task_t *task, char *title);
449488

@@ -467,6 +506,15 @@ void task_set_data(retro_task_t *task, void *data);
467506
*/
468507
void task_free_title(retro_task_t *task);
469508

509+
/**
510+
* Frees the \c task's error message, if any.
511+
* Thread-safe if the task queue is threaded.
512+
*
513+
* @param task The task to modify.
514+
* @see task_set_error
515+
*/
516+
void task_free_error(retro_task_t *task);
517+
470518
/**
471519
* Returns \c task::error.
472520
* Thread-safe if the task queue is threaded.

libretro-common/queues/task_queue.c

Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1023,6 +1023,19 @@ void task_free_title(retro_task_t *task)
10231023
#endif
10241024
}
10251025

1026+
void task_free_error(retro_task_t *task)
1027+
{
1028+
#ifdef HAVE_THREADS
1029+
slock_lock(property_lock);
1030+
#endif
1031+
if (task->error)
1032+
free(task->error);
1033+
task->error = NULL;
1034+
#ifdef HAVE_THREADS
1035+
slock_unlock(property_lock);
1036+
#endif
1037+
}
1038+
10261039
void* task_get_data(retro_task_t *task)
10271040
{
10281041
void *data = NULL;
Lines changed: 37 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,37 @@
1+
TARGET := task_queue_title_error_test
2+
3+
LIBRETRO_COMM_DIR := ../../..
4+
5+
# task_queue.c (without HAVE_THREADS / HAVE_GCD) is self-contained
6+
# apart from cpu_features_get_time_usec(); the test file provides a
7+
# trivial stub for that symbol so we don't have to drag in
8+
# features_cpu.c (which would pull in a much larger dependency
9+
# graph). Build task_queue.c without HAVE_THREADS so the slock /
10+
# scond / sthread paths compile out cleanly -- the title/error
11+
# protocol is identical with and without threading; the locks just
12+
# serialise it.
13+
SOURCES := \
14+
task_queue_title_error_test.c \
15+
$(LIBRETRO_COMM_DIR)/queues/task_queue.c
16+
17+
OBJS := $(SOURCES:.c=.o)
18+
19+
CFLAGS += -Wall -pedantic -std=gnu99 -g -O0 -I$(LIBRETRO_COMM_DIR)/include
20+
21+
ifneq ($(SANITIZER),)
22+
CFLAGS := -fsanitize=$(SANITIZER) -fno-omit-frame-pointer $(CFLAGS)
23+
LDFLAGS := -fsanitize=$(SANITIZER) $(LDFLAGS)
24+
endif
25+
26+
all: $(TARGET)
27+
28+
%.o: %.c
29+
$(CC) -c -o $@ $< $(CFLAGS)
30+
31+
$(TARGET): $(OBJS)
32+
$(CC) -o $@ $^ $(LDFLAGS)
33+
34+
clean:
35+
rm -f $(TARGET) $(OBJS)
36+
37+
.PHONY: clean

0 commit comments

Comments
 (0)