Add new APIs to address vulnerabilities in the V API#881
Conversation
The existing APIs Vgetname, Vgetclass, and Vinquire posed multiple vulnerabilities due to the lack of size checks. Because changing their signature will break compatibilities, three new APIs are added in place of their deprecation instead. New APIs are: int Vgetname40(int32 vkey, char *vgname, size_t *buf_size); int Vgetclass40(int32 vkey, char *vgclass, size_t *buf_size); int Vinquire40(int32 vkey, int32 *nentries, char *vgname, size_t *buf_size); Fixes GH HDFGroup#872
bmribler
requested review from
brtnfld,
byrnHDF,
derobins,
fortnern,
glennsong09,
jhendersonHDF,
lrknox,
mattjala,
schwehr and
vchoi-hdfgroup
as code owners
bmribler
added
Component - C Library
| HDFLIBAPI int32 Vgetnext(int32 vkey, int32 id); | ||
|
|
||
| HDFLIBAPI int32 Vgetname(int32 vkey, char *vgname); | ||
| HDFLIBAPI int Vgetname40(int32 vkey, char *vgname, size_t *buf_size); /* in 4.0 */ |
There was a problem hiding this comment.
Why are there 40 added to these function names? Maybe something in the name that implies it knows the string length?
There was a problem hiding this comment.
My intention for 40 is they are introduced in 4.0. :D
There was a problem hiding this comment.
Actually, I'm reconsidering this position...
There was a problem hiding this comment.
We'll have a clean break, no deprecated functions, cleaner API for 4.0
| HDFLIBAPI int Vinquire(int32 vkey, int32 *nentries, char *vgname); | ||
| HDFLIBAPI int32 Vgetnamelen(int32 vkey, uint16 *name_len); /* deprecated in 4.0 */ | ||
|
|
||
| HDFLIBAPI int32 Vgetclassnamelen(int32 vkey, uint16 *classname_len); /* deprecated in 4.0 */ |
There was a problem hiding this comment.
Why is this also down below?
There was a problem hiding this comment.
I removed them. Thanks!!
| HDFLIBAPI int Vinquire(int32 vkey, int32 *nentries, char *vgname); | ||
| HDFLIBAPI int32 Vgetnamelen(int32 vkey, uint16 *name_len); /* deprecated in 4.0 */ | ||
|
|
||
| HDFLIBAPI int32 Vgetclassnamelen(int32 vkey, uint16 *classname_len); /* deprecated in 4.0 */ |
There was a problem hiding this comment.
Consider requiring the user to #define something to allow use of deprecated calls and default it to set in the CMake system?
There was a problem hiding this comment.
We don't have plan to do much more in HDF4. 4.0 is very likely to be the last release. Applications can continue using 3.x if the user chooses to use deprecated APIs. Please see the note about deprecating.
| HGOTO_ERROR(DFE_ARGS, FAIL); | ||
|
|
||
| /* Get the vgroup struct for access */ | ||
| if ((vg = VIGet_vgdesc(vkey)) == NULL) |
There was a problem hiding this comment.
Switching style how it checks against NULL. Above was if (NULL == . Use the same style across all.
There was a problem hiding this comment.
I prefer (... == NULL) but I'll use the existing style for now.
There was a problem hiding this comment.
Sounds good. I suggest a follow up PR to switch to == NULL as I agree with your take on what is better these days.
| if (buf_size != NULL) { | ||
| if (vgname == NULL || *buf_size == 0) { | ||
| *buf_size = name_len; /* return the name length */ | ||
| } |
There was a problem hiding this comment.
Maybe return here so you don't have to indent for the else?
|
@schwehr Thank you for reviewing this PR! I'll close it but create another PR soon, and apply your comments there, where appropriate. |
2 participants
The existing Vgroup APIs Vgetname, Vgetclass, and Vinquire posed multiple vulnerabilities due to the lack of size checks. Because changing their signature will break compatibilities, three new APIs are added in place of their deprecation instead. New APIs are:
int Vgetname40(int32 vkey, char *vgname, size_t *buf_size);
int Vgetclass40(int32 vkey, char *vgclass, size_t *buf_size);
int Vinquire40(int32 vkey, int32 *nentries, char *vgname, size_t *buf_size);
Fixes GH #872
The removed APIs are no longer needed with the new argument buf_size in the new APIs.
int32 Vgetnamelen(int32 vkey, uint16 name_len); / deprecated in 4.0 */
int32 Vgetclassnamelen(int32 vkey, uint16 classname_len); / deprecated in 4.0 */