Skip to content

Conversation

@silverweed
Copy link
Contributor

@silverweed silverweed commented Apr 17, 2025

An attempt to fix and improve some old code in TGNumberEntry by:

  • fixing some broken logic
  • simplifying some functions
  • improving safety by adding bounds checks where appropriate

This is not an exhaustive pass on the file, as there are many other places that could use a pass or a rewrite. However it should address some of the more glaring safety concerns - particularly, it removes all uses of strcpy and the coverity annotations.

I haven't tested this code yet so I'm opening this as a draft PR. Feel free to comment on it already.

Checklist:

  • tested changes locally
  • updated the docs (if necessary)

This PR fixes #17334

@ferdymercury
Copy link
Collaborator

Thanks a lot for this initiative. If you want a reproducer to test against: https://igit.ific.uv.es/ferhue/pulse-surfer

@github-actions
Copy link

github-actions bot commented Apr 17, 2025

Test Results

    19 files      19 suites   3d 22h 21m 45s ⏱️
 2 731 tests  2 731 ✅ 0 💤 0 ❌
50 411 runs  50 411 ✅ 0 💤 0 ❌

Results for commit fc16914.

♻️ This comment has been updated with latest results.

@silverweed silverweed marked this pull request as ready for review April 24, 2025 07:26
@silverweed silverweed requested a review from bellenot as a code owner April 24, 2025 07:26
@silverweed
Copy link
Contributor Author

silverweed commented Apr 24, 2025

I tested on @ferdymercury 's reproducer and it seems to work fine.

Copy link
Collaborator

@ferdymercury ferdymercury left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great, thanks! Maybe just add a small doxygen documentation on what CopyAndEliminate is doing.

Copy link
Member

@dpiparo dpiparo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks for this. LGTM.

- fix some broken logic
- simplify some functions
- improve safety by adding bounds checks where appropriate
@silverweed silverweed merged commit 58472db into root-project:master May 5, 2025
36 of 43 checks passed
@silverweed silverweed deleted the tgnumentry branch May 5, 2025 12:56
@ferdymercury
Copy link
Collaborator

ferdymercury commented Jun 3, 2025

@silverweed I am seeing a regression in TGNumberEntry, that might be connected with this PR.

In 6.36.00
root --web=off $ROOTSYS/tutorials/geom/building.C

then click on Clipping, Plane, numbers look fine.

In master:
root --web=off $ROOTSYS/tutorials/visualisation/geom/building.C

both in first tab and in Clipping, Plane, numbers look weird.
(I am rebuilding to see if sth went wrong with rebasing or so).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

TGNumberEntry string length checks are inaccurate/dangerous.

3 participants