cppcheck errors: "Buffer is accessed out of bounds"

[onlyjob]

cppcheck found the following errors:

[oci8.c:2335]: (error) Buffer is accessed out of bounds: s_tz_hour
[oci8.c:2338]: (error) Buffer is accessed out of bounds: s_tz_min

https://github.com/pythian/DBD-Oracle/blob/master/oci8.c#L2335
https://github.com/pythian/DBD-Oracle/blob/master/oci8.c#L2338

[tonycoz]

The warnings here are correct - the sprintf() calls overflow the supplied buffers.

But there's another problem, the call to OCIDateTimeToText() already appears to be populating the timezone.

If I make a change that only expands the sizes of the buffers and patch the code in 58object.t to use a timestamp with time zone for the sub type (patch
test-timestamp_tz.diff.txt ) the value returned has two timezones:

not ok 41 - new: Row 1 column 2 object attributes
#   Failed test 'new: Row 1 column 2 object attributes'
#   at t/58object.t line 251.
#     Structures begin differing at:
#          $got->{DATETIME} = '2004-12-03T11:27:18 +69:00 69:00'
#     $expected->{DATETIME} = '2004-11-30T14:27:18'

So it looks like the entire conditional is unneeded.

Another problem you can see looking at the test output above is the very strange time zone offset. The modified code in the test inserts a +05:00 timestamp with time zone:

    sql_do_ok(
        $dbh, qq{ INSERT INTO $table VALUES (2, $sub_type(NULL, 'obj2',
                TO_TIMESTAMP_TZ('2004-11-30 14:27:18 +05:00', 'YYYY-MM-DD HH24:MI:SS TZH:TZM'),
                12345.6789)) }
    );

but the result is strange. This behaviour is also visible in sqlplus (see http://paste.scsys.co.uk/584116 )

Hopefully someone with better Oracle knowledge than I (which isn't hard) will have some idea of the cause.

Testing was done with the versions of Oracle XE and openclient installed by the maint/travis_ci_scripts on Debian Linux.

[djzort]

I have created a v1.90 milestone and attached this issue to it

[djzort]

@ancientwizard would you be kind enough to see if you can resolve this?

[ancientwizard]

@ancientwizard would you be kind enough to see if you can resolve this?

I've had a look.
I'm not seeing the warning. The two char arrays are a bit weak but not completely broken. What I see is:

• both are defined as an array of char having six bytes.
• both are initialized from const of "000".
• sprintf should be replaced with snprintf
• it wouldn't hurt to make the two arrays 8 chars (most of us will likely memory align to 8 bytes anyway. best guess)

I was unable to find a test that triggers its usage in the module. I normally ask the server to send me what I want just the way I want it so I don't have to putz with it. Meaning I ALTER session and QUERY for outputs in the desired format; including TZ conversion etc. Doing conversions oneself is a recipe for mistakes, speaking from experience.

It amazes me how in my own ORG how many apps are clueless about TZ's and make all sorts of assumptions and mistakes. For my own APP I use UTC 100%; I don't care what the agent (client) or DB server says (my server could get moved HOT from different TZ's; UTC goes in and UTC comes out. I leave nothing to chance or defaults. Mostly because I have no control how my DB server is configured and can't change it's configuration. My apps then kick it in the session nuts with a few "THOW SHALL DO" this and that ALTER sessions and life is good. - stepping off soapbox :)

[ancientwizard]

I'm thinking this is more likely formatted by the session during the select. Perhaps you have and environment VAR that is being picked up that I have failed to remember.