#4 Possible overflow bugs in DIFF_EXT::diff

closed-fixed
Sergey Zorin
None
5
2006-06-06
2006-06-02
elsapo
No

http://diff-ext.cvs.sourceforge.net/diff-ext/diff-ext/diff-ext/src/diff_ext.cpp?revision=1.26&view=markup

DIFF_EXT::diff

It appears to me that the command variable is first
loaded from the registry with unlimited size --
shouldn't it be limited to MAX_PATH? (Against the
unlikely event someone put a really large string into
that registry value, by accident or on purpose, without
your program knowing.)

Also, command is allocated for MAX_PATH*3 + 6, but my
attempt to calculate its max size lead me to believe it
should be at least MAX_PATH*3 +7.

Registry value: I assume this is intended to be up to
MAX_PATH (altho I wonder if it mightn't be more, if it
is up to MAX_PATH for a program exe path, and then some
arguments)

Then

_tcscat(command, TEXT(" \""));

+2 tchars

_tcsncpy(tmp, _file_name1, MAX_PATH);

+MAX_PATH tchars

_tcscat(command, tmp);

_tcscat(command, TEXT("\" \""));

+3 tchars

_tcsncpy(tmp, _file_name2, MAX_PATH);

+MAX_PATH

_tcscat(command, tmp);

_tcscat(command, TEXT("\""));

+1 tchar

That is +2 +3 +1 = +6 tchars
and you need at least one tchar for the terminating zero
(which _tcscat will put at end)
which would make +7 tchars including trailing zero.

Discussion

  • Sergey Zorin
    Sergey Zorin
    2006-06-03

    Logged In: YES
    user_id=930042

    The command is first loaded from registry with size limited
    to length (wich is initialized to MAX_PATH). So it looks
    like no problems here, except no room is left for possible
    arguments.
    I do agree with your point about buffer allocation for the
    command variable. This one will be fixed.

     
  • Sergey Zorin
    Sergey Zorin
    2006-06-03

    • assigned_to: nobody --> serg_z
    • status: open --> open-accepted
     
  • Sergey Zorin
    Sergey Zorin
    2006-06-06

    • status: open-accepted --> closed-fixed