Skip to content

Fix src shift in ipp-file.c#1422

Closed
tsudik wants to merge 1 commit intoOpenPrinting:masterfrom
tsudik:tsudik-patch-1
Closed

Fix src shift in ipp-file.c#1422
tsudik wants to merge 1 commit intoOpenPrinting:masterfrom
tsudik:tsudik-patch-1

Conversation

@tsudik
Copy link
Copy Markdown

@tsudik tsudik commented Nov 14, 2025

Each case ($$, $ENV[, ${, and plain $) is supposed to shift src after parsing. But the block responsible for the $ case lacks this shift--it is located out of that block. So for the ${ case src is shifted twice: by src += 2 and by src += tempptr - temp + 1, which results in while (*src) reading beyond the NUL-terminator.

run command:

LD_LIBRARY_PATH=./cups ./tools/ipptool -t ipp://localhost:631/printers/TestPrinter test_file

test_file:

{
 ATTR charset attcharset 
" 
${

MSAN:

==181511==WARNING: MemorySanitizer: use-of-uninitialized-value
    #0 0x7f28ca786892 in ippFileExpandVars /home/tsudik/cups/test/cups/cups/cups/ipp-file.c:150:3
    #1 0x7f28ca78969b in parse_value /home/tsudik/cups/test/cups/cups/cups/ipp-file.c:1632:3
    #2 0x7f28ca787b9f in ippFileRead /home/tsudik/cups/test/cups/cups/cups/ipp-file.c:548:14
    #3 0x56169af31892 in do_tests /home/tsudik/cups/test/cups/cups/tools/ipptool.c:2522:3
    #4 0x56169af2f644 in main /home/tsudik/cups/test/cups/cups/tools/ipptool.c:765:17
    #5 0x7f28ca2295cf in __libc_start_call_main (/lib64/libc.so.6+0x295cf) (BuildId: 38347dcfe861b665e08e8b1263215f626f4e003c)
    #6 0x7f28ca22967f in __libc_start_main@GLIBC_2.2.5 (/lib64/libc.so.6+0x2967f) (BuildId: 38347dcfe861b665e08e8b1263215f626f4e003c)
    #7 0x56169ae93bf4 in _start (/home/tsudik/cups/test/cups/cups/tools/ipptool+0x35bf4) (BuildId: 84b1db2e275d63ce60e3bfc8c2b9b5c762ca0b14)

  Uninitialized value was created by an allocation of 'temp' in the stack frame
    #0 0x7f28ca7895a2 in parse_value /home/tsudik/cups/test/cups/cups/cups/ipp-file.c:1617:3

SUMMARY: MemorySanitizer: use-of-uninitialized-value /home/tsudik/cups/test/cups/cups/cups/ipp-file.c:150:3 in ippFileExpandVars
Exiting

Each case (`$$`, `$ENV[`, `${`, and plain `$`) is supposed to shift `src` after parsing. But the block responsible for the `$` case lacks this shift--it is located out of that block. So for the `${` case `src` is shifted twice: by `src += 2` and by `src += tempptr - temp + 1`, which results in `while (*src)` reading beyond the NUL-terminator.
Copy link
Copy Markdown
Member

@michaelrsweet michaelrsweet left a comment

Choose a reason for hiding this comment

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

OK, valid bug but I don't think your fix is correct since it doesn't advance "src" past the terminating '}' for an otherwise valid "${FOO}bar" - with your change "src" will point to the "F" instead of the "b".

I think we need to treat "${" without a terminating "}" to be an error.

michaelrsweet added a commit that referenced this pull request Nov 14, 2025
… better job

validating "$ENV[name]" and "${name}" expansions (Issue #1422)

Also update the documentation comments.
@michaelrsweet
Copy link
Copy Markdown
Member

OK, I made two sets of changes:

  1. When reading a token, return false if we have a quoted string that is unterminated.
  2. When expanding "$ENV[name]", "${name}", or "$name", manually copy the name to "temp" and advance "src" to the end of the variable. If we are missing the closing ']' or '}', silently try to expand the name we got.

This addresses the general issue you reported and covers a few other edge cases as well. Let me know what you think:

[master 051f63d] Return false if we have an unterminated quoted string token, and do a better job validating "$ENV[name]" and "${name}" expansions (Issue #1422)

@michaelrsweet michaelrsweet self-assigned this Nov 14, 2025
@michaelrsweet michaelrsweet added bug Something isn't working priority-high security Security issue labels Nov 14, 2025
@michaelrsweet michaelrsweet added this to the v2.5 milestone Nov 14, 2025
@tsudik
Copy link
Copy Markdown
Author

tsudik commented Nov 15, 2025

Your solution is good. I agree with you.

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

Labels

bug Something isn't working priority-high security Security issue

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants