Skip to content

Replace sprintf with snprintf for safety#1329

Open
cxzhong wants to merge 4 commits into
Singular:spielwiesefrom
cxzhong:fix-sprintf-to-snprintf
Open

Replace sprintf with snprintf for safety#1329
cxzhong wants to merge 4 commits into
Singular:spielwiesefrom
cxzhong:fix-sprintf-to-snprintf

Conversation

@cxzhong

@cxzhong cxzhong commented Mar 3, 2026

Copy link
Copy Markdown
Contributor

Convert all sprintf calls to snprintf with proper buffer size limits:

  • Singular/dyn_modules/python/ring_wrap.cc
  • Singular/dyn_modules/machinelearning/mlpredict.c
  • Singular/svd_si.h
  • Singular/svd/libs/amp.h
  • Singular/countedref.h
  • omalloc/omRet2Info.c

Convert all sprintf calls to snprintf with proper buffer size limits:
- Singular/dyn_modules/python/ring_wrap.cc
- Singular/dyn_modules/machinelearning/mlpredict.c
- Singular/svd_si.h
- Singular/svd/libs/amp.h
- Singular/countedref.h
- omalloc/omRet2Info.c
@cxzhong

cxzhong commented Mar 3, 2026

Copy link
Copy Markdown
Contributor Author

CC: @ederc @jankoboehm

@jankoboehm

Copy link
Copy Markdown
Member

Thank you very much for the sprintf snprintf cleanup! Looks mostly good to me.

One point in omalloc/omRet2Info.c you might want to consider: &command[l] means to start writing at position l inside the character array command. Let n be the value returned by snprintf(...). snprintf returns the number of characters it wanted to write (even if there was not enough space). Then l += snprintf(...) can increase l past the end of command, and then the next &command[l] points outside the array. Rather write as n = snprintf(...) and only do l += n when 0 <= n < sizeof(command) - l, otherwise stop appending?

Minor thing in mlpredict.c, sizeof(B) gives the right bound only when B is an actual array; if B is a pointer it becomes pointer-size. Consider passing (buffer, length) explicitly.

@cxzhong

cxzhong commented Mar 9, 2026

Copy link
Copy Markdown
Contributor Author

Thank you very much for the sprintf snprintf cleanup! Looks mostly good to me.

One point in omalloc/omRet2Info.c you might want to consider: &command[l] means to start writing at position l inside the character array command. Let n be the value returned by snprintf(...). snprintf returns the number of characters it wanted to write (even if there was not enough space). Then l += snprintf(...) can increase l past the end of command, and then the next &command[l] points outside the array. Rather write as n = snprintf(...) and only do l += n when 0 <= n < sizeof(command) - l, otherwise stop appending?

Minor thing in mlpredict.c, sizeof(B) gives the right bound only when B is an actual array; if B is a pointer it becomes pointer-size. Consider passing (buffer, length) explicitly.

Can you review this again?

@cxzhong

cxzhong commented Apr 3, 2026

Copy link
Copy Markdown
Contributor Author

@jankoboehm This is ready for review now

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants