art/brlcadplugin: fix thread-safety in brlcad_hit() ray result passing - #229
Open
rupeshca007 wants to merge 7 commits into
Open
art/brlcadplugin: fix thread-safety in brlcad_hit() ray result passing#229rupeshca007 wants to merge 7 commits into
rupeshca007 wants to merge 7 commits into
Conversation
- brlcadplugin.cpp: Fix incorrect header comment (ARTPLUGIN -> BRLCADPLUGIN)
- brlcadplugin.cpp: Fix char loop variable overflow in get_objects() for
object counts > 127 (char is signed), changed to int
- art.cpp: Fix float literal (200.0) assigned to size_t variable
light_intensity, changed to integer literal (200)
- art.cpp: Fix grammar 'a orthographic' -> 'an orthographic' in comment
- art.cpp: Fix missing apostrophe in error messages ('Dont' -> 'Don''t')
across 5 bu_bomb() calls
Replace fixed-size char name[NAMESIZE+1] (17 bytes) buffers and unsafe sprintf calls with 32-byte buffers and snprintf in the pshell and pbar_group loops. When a PID exceeds 6 digits (e.g. near INT_MAX), the old sprintf would silently overflow the stack buffer. snprintf with sizeof(name) bounds the write and prevents the overflow. Fixes BRL-CAD#211
The global thread_local brlcad_ray_info struct was used to pass hit results from the brlcad_hit() callback back to intersect(). This is unsafe when Appleseed renders with multiple worker threads: if rt_shootray dispatches the hit callback on a thread other than the caller, the thread_local read in intersect() returns a different (stale or uninitialized) copy. Fix: allocate a BRLCAD_to_ASR struct on the stack in intersect() and pass its address via app.a_uptr. brlcad_hit() now writes directly into the caller's local struct via ap->a_uptr. This eliminates all shared global state in the intersection hot path. Also removes dead code: the old app.a_uptr = name assignment and commented-out UV/normal-flip calculations that were never enabled. Addresses part of GSoC issue opencax/GSoC#108.
…ation()
Two bug fixes for the Appleseed renderer plugin:
1. get_id() bounds overflow
thread_local IDs were assigned with an ever-incrementing counter but
the result was used as a direct index into resources[MAX_PSW]. If
Appleseed spawned more threads than MAX_PSW, the index went out of
bounds causing undefined behavior / crash.
Fix: clamp with 'id % MAX_PSW' to guarantee [0, MAX_PSW-1].
2. configure_raytrace_application() correctness
- Removed FIXME: reworked for standalone plugin use
- Fixed init order: RT_APPLICATION_INIT + ap.a_rt_i now set AFTER
rtip is built via rt_dirbuild (previously ap.a_rt_i = this->rtip
was set when rtip was still NULL)
- Fixed memory leak: objv array from bu_calloc is now freed with
bu_free after rt_gettrees completes
- Removed unused local 'npsw' variable (hardcoded 1 passed directly)
- Removed all trailing commented-out dead code
- Fixed crash on empty objects vector in error message path
Addresses part of GSoC issue opencax/GSoC#108.
Two resource leak fixes: 1. brlcadplugin.cpp: BrlcadObject::release() - free resources[] The resources[] array was allocated with bu_calloc() in both configure_raytrace_application() and the 3-arg constructor but the bu_free() call in release() was commented out. This caused a leak of (sizeof(resource) * MAX_PSW) bytes on every BrlcadObject destruction. Uncommented and guarded with a NULL check. 2. art.cpp: register_region() - close GED handle after bounding box ged_open() was called once per region to compute bounding boxes via rt_obj_bounds(), but ged_close() was never called. For a scene with N regions, this leaked N file descriptors and all associated GED memory. Added ged_close(gedp) immediately after the bounds query. Addresses part of GSoC issue opencax/GSoC#108.
Three bug fixes in art.cpp build_project(): 1. Remove hardcoded 'build/Debug' shader search path (line 667) This path only exists during in-source debug builds. It would silently fail on release builds and installed binaries, causing shaders not to be found. Correct paths are resolved via APPLESEED_ROOT which is already set below. 2. Fix db handle leak after db_walk_tree() (line 706) db_open() was called to get a db_i for the walk state but db_close() was never called. Every render leaked the open database file descriptor and all associated memory. Added db_close(dbip) after both db_walk_tree() calls complete. 3. Remove duplicate light_intensity *= AmbientIntensity (line 728) AmbientIntensity is already applied once in init_defaults() (see line 253). Applying it again in build_project() caused the light to be (AmbientIntensity^2) times brighter than intended, making renders significantly over-exposed. Addresses part of GSoC issue opencax/GSoC#108.
Multiple bu_vls memory leaks found by grepping for bu_vls_free (which was never called in art.cpp despite many bu_vls_sprintf uses): - build_project(): 'dimensions' vls built via bu_vls_sprintf for film size and frame resolution was never freed. Added bu_vls_free at end. - build_project(): 'fov' vls for perspective camera was never freed. Added bu_vls_free immediately after the camera is created. - art_cm_end(): 'str' vls passed to build_project was never freed. Added bu_vls_free at the end of the function. - main(): 'str' vls built from objv was never freed on the normal render path. Added bu_vls_free before return. Also fixed global Appleseed log target lifetime in main(): log_target was added to the global logger but never removed or deleted before program exit. The logger could dereference the deleted pointer on shutdown. Added remove_target() + delete before return. Addresses part of GSoC issue opencax/GSoC#108.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fixes a thread-safety bug in the Appleseed renderer's ray intersection bridge (
src/art/brlcadplugin.cpp).Related to GSoC 2026 issue: opencax/GSoC#108 (Stabilize Appleseed Rendering)
Problem
brlcad_ray_infowas a globalthread_localstruct used to pass hit results from thebrlcad_hit()callback back tointersect():This is unsafe: if Appleseed's multi-threaded renderer dispatches
brlcad_hit()on a worker thread different from the thread callingintersect(), thethread_localread returns a different (stale/uninitialized) copy, causing silent data corruption or crashes.Fix
Allocate a
BRLCAD_to_ASRstruct on the stack inintersect()and pass its address throughapp.a_uptr.brlcad_hit()now writes directly into the caller's local struct:This eliminates all shared global state in the intersection hot path. Each
intersect()call is now fully self-contained and thread-safe.Also removed
app.a_uptr = nameassignment (was overwritten immediately after)