Skip to content

Commit

Permalink
fix memory leaks related to PTPCanon_changes_entry.u.info (#1009)
Browse files Browse the repository at this point in the history 
* fix memory leaks related to PTPCanon_changes_entry.u.info

Unpacking canon EOS event data uses heap memory stored in `char *info` for
different types of information (mostly PTP_CANON_EOS_CHANGES_TYPE_UNKNOWN
but also PTP_CANON_EOS_CHANGES_TYPE_FOCUS*). This memory needs to be freed
on every call-sight of ptp_get_one_eos_event, which it wasn't and it needs
to do so for every type using it, but if at all, it only freed it for
PTP_CANON_EOS_CHANGES_TYPE_UNKNOWN data.

This patch replaces `char *info` with `char info[84]` and introduces a
macro called PTP_CANON_SET_INFO() for setting that data. 84 is used,
because the union is at least that large anyway because of the `object`
member and 84 is sufficient for all current use cases. The macro makes
sure there is no buffer overflow in the future.

This change does away with the error prone task of freeing the `info`
memory after every ptp_get_one_eos_event() call.

* fix CodeQL complaint about return value check of sscanf

---------

Co-authored-by: axxel <awagger@web.de>
  • Loading branch information
@axxel
axxel and axxel authored Sep 11, 2024
1 parent dac3b57 commit b8743d8
Show file tree
Hide file tree
Showing 3 changed files with 36 additions and 51 deletions.
16 changes: 6 additions & 10 deletions camlibs/ptp2/library.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3224,11 +3224,8 @@ camera_exit (Camera *camera, GPContext *context)

if ((exit_result = ptp_check_eos_events (params)) != PTP_RC_OK)
goto exitfailed;
while (ptp_get_one_eos_event (params, &entry)) {
while (ptp_get_one_eos_event (params, &entry))
GP_LOG_D ("missed EOS ptp type %d", entry.type);
if (entry.type == PTP_CANON_EOS_CHANGES_TYPE_UNKNOWN)
free (entry.u.info);
}
camera->pl->checkevents = 0;
}
if (params->inliveview && ptp_operation_issupported(params, PTP_OC_CANON_EOS_TerminateViewfinder))
Expand Down Expand Up @@ -4478,7 +4475,6 @@ camera_canon_eos_capture (Camera *camera, CameraCaptureType type, CameraFilePath
switch (entry.type) {
case PTP_CANON_EOS_CHANGES_TYPE_UNKNOWN:
GP_LOG_D ("entry unknown: %s", entry.u.info);
free (entry.u.info);
continue; /* in loop ... do not poll while draining the queue */
case PTP_CANON_EOS_CHANGES_TYPE_OBJECTTRANSFER:
GP_LOG_D ("Found new object! OID 0x%x, name %s", (unsigned int)entry.u.object.oid, entry.u.object.oi.Filename);
Expand Down Expand Up @@ -6124,7 +6120,7 @@ camera_trigger_canon_eos_capture (Camera *camera, GPContext *context)
ptp_check_eos_events (params);
while (ptp_get_one_eos_event (params, &entry)) {
GP_LOG_D ("entry type %04x", entry.type);
if (entry.type == PTP_CANON_EOS_CHANGES_TYPE_UNKNOWN && entry.u.info && sscanf (entry.u.info, "Button %d", &button)) {
if (entry.type == PTP_CANON_EOS_CHANGES_TYPE_UNKNOWN && sscanf (entry.u.info, "Button %d", &button) == 1) {
GP_LOG_D ("Button %d", button);
switch (button) {
/* Indicates a successful Half-Press(?) on M2, where it
Expand Down Expand Up @@ -6810,19 +6806,19 @@ camera_wait_for_event (Camera *camera, int timeout,
return GP_OK;
case PTP_CANON_EOS_CHANGES_TYPE_FOCUSINFO:
*eventtype = GP_EVENT_UNKNOWN;
C_MEM (*eventdata = malloc(strlen("Focus Info 12345678901234567890123456789")+1));
C_MEM (*eventdata = malloc(strlen("Focus Info ") + strlen(entry.u.info)+1));
sprintf (*eventdata, "Focus Info %s", entry.u.info);
return GP_OK;
case PTP_CANON_EOS_CHANGES_TYPE_FOCUSMASK:
*eventtype = GP_EVENT_UNKNOWN;
C_MEM (*eventdata = malloc(strlen("Focus Mask 12345678901234567890123456789")+1));
C_MEM (*eventdata = malloc(strlen("Focus Mask ") + strlen(entry.u.info)+1));
sprintf (*eventdata, "Focus Mask %s", entry.u.info);
return GP_OK;
case PTP_CANON_EOS_CHANGES_TYPE_UNKNOWN:
/* only return if interesting stuff happened */
if (entry.u.info) {
if (entry.u.info[0] != 0) {
*eventtype = GP_EVENT_UNKNOWN;
*eventdata = entry.u.info; /* take over the allocated string allocation ownership */
C_MEM(*eventdata = strdup(entry.u.info));
return GP_OK;
}
/* continue otherwise */
Expand Down
69 changes: 29 additions & 40 deletions camlibs/ptp2/ptp-pack.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2028,6 +2028,13 @@ _lookup_or_allocate_canon_prop(PTPParams *params, uint16_t proptype)
return &params->canon_props[j].dpd;
}

#define PTP_CANON_SET_INFO( ENTRY, MSG, ...) \
do { \
int c = snprintf(ENTRY.u.info, sizeof(ENTRY.u.info), MSG, ##__VA_ARGS__); \
if (c > (int)sizeof(ENTRY.u.info)) \
ptp_debug(params, "buffer overflow in PTP_CANON_SET_INFO, complete msg is: " \
MSG, ##__VA_ARGS__); \
} while (0)

static inline int
ptp_unpack_CANON_changes (PTPParams *params, unsigned char* data, unsigned int datasize, PTPCanon_changes_entry **pce)
Expand Down Expand Up @@ -2095,7 +2102,7 @@ ptp_unpack_CANON_changes (PTPParams *params, unsigned char* data, unsigned int d
}

ce[i].type = PTP_CANON_EOS_CHANGES_TYPE_UNKNOWN;
ce[i].u.info = NULL;
ce[i].u.info[0] = 0;
switch (type) {
case PTP_EC_CANON_EOS_ObjectContentChanged:
if (size < PTP_ece_OA_ObjectID+1) {
Expand Down Expand Up @@ -2614,7 +2621,7 @@ static unsigned int olcsizes[0x15][13] = {
}
if (olcver == 0) {
ce[i].type = PTP_CANON_EOS_CHANGES_TYPE_UNKNOWN;
ce[i].u.info = strdup("OLC version is unknown");
PTP_CANON_SET_INFO(ce[i], "OLC version is unknown");
ptp_debug (params, "event %d: OLC version is 0, skipping (might get set later)", i);
break;
}
Expand All @@ -2631,14 +2638,14 @@ static unsigned int olcsizes[0x15][13] = {
}
if (size < 14) {
ce[i].type = PTP_CANON_EOS_CHANGES_TYPE_UNKNOWN;
ce[i].u.info = strdup("OLC size too small");
PTP_CANON_SET_INFO(ce[i], "OLC size too small");
ptp_debug (params, "event %d: OLC unexpected size %d", i, size);
break;
}
len = dtoh32a(curdata+8);
if ((len != size-8) && (len != size-4)) {
ce[i].type = PTP_CANON_EOS_CHANGES_TYPE_UNKNOWN;
ce[i].u.info = strdup("OLC size unexpected");
PTP_CANON_SET_INFO(ce[i], "OLC size unexpected");
ptp_debug (params, "event %d: OLC unexpected size %d for blob len %d (not -4 nor -8)", i, size, len);
break;
}
Expand All @@ -2664,8 +2671,7 @@ static unsigned int olcsizes[0x15][13] = {
switch (curmask) {
case CANON_EOS_OLC_BUTTON: {
ce[i].type = PTP_CANON_EOS_CHANGES_TYPE_UNKNOWN;
ce[i].u.info = malloc(strlen("Button 1234567")+1);
sprintf(ce[i].u.info, "Button %d", dtoh16a(curdata+curoff));
PTP_CANON_SET_INFO(ce[i], "Button %d", dtoh16a(curdata+curoff));
i++;
break;
}
Expand Down Expand Up @@ -2727,8 +2733,7 @@ static unsigned int olcsizes[0x15][13] = {
case 0x0010: {
/* mask 0x0010: 4 bytes, 04 00 00 00 observed */
ce[i].type = PTP_CANON_EOS_CHANGES_TYPE_UNKNOWN;
ce[i].u.info = malloc(strlen("OLCInfo event 0x0010 content 01234567")+1);
sprintf(ce[i].u.info,"OLCInfo event 0x0010 content %02x%02x%02x%02x",
PTP_CANON_SET_INFO(ce[i], "OLCInfo event 0x0010 content %02x%02x%02x%02x",
curdata[curoff],
curdata[curoff+1],
curdata[curoff+2],
Expand All @@ -2743,8 +2748,7 @@ static unsigned int olcsizes[0x15][13] = {
* has the form of 00 00 01 00 XX XX, where the last two bytes
* stand for the number of seconds remaining until the shot */
ce[i].type = PTP_CANON_EOS_CHANGES_TYPE_UNKNOWN;
ce[i].u.info = malloc(strlen("OLCInfo event 0x0020 content 0123456789ab")+1);
sprintf(ce[i].u.info,"OLCInfo event 0x0020 content %02x%02x%02x%02x%02x%02x",
PTP_CANON_SET_INFO(ce[i], "OLCInfo event 0x0020 content %02x%02x%02x%02x%02x%02x",
curdata[curoff],
curdata[curoff+1],
curdata[curoff+2],
Expand All @@ -2760,8 +2764,7 @@ static unsigned int olcsizes[0x15][13] = {
/* mask 0x0040: 7 bytes, 01 01 00 00 00 00 00 observed */
/* exposure indicator */
ce[i].type = PTP_CANON_EOS_CHANGES_TYPE_UNKNOWN;
ce[i].u.info = malloc(strlen("OLCInfo exposure indicator 012345678901234567890123456789abcd")+1);
sprintf(ce[i].u.info,"OLCInfo exposure indicator %d,%d,%d.%d (%02x%02x%02x%02x)",
PTP_CANON_SET_INFO(ce[i], "OLCInfo exposure indicator %d,%d,%d.%d (%02x%02x%02x%02x)",
curdata[curoff],
curdata[curoff+1],
value/10,abs(value)%10,
Expand All @@ -2776,8 +2779,7 @@ static unsigned int olcsizes[0x15][13] = {
case 0x0080: {
/* mask 0x0080: 4 bytes, 00 00 00 00 observed */
ce[i].type = PTP_CANON_EOS_CHANGES_TYPE_UNKNOWN;
ce[i].u.info = malloc(strlen("OLCInfo event 0x0080 content 01234567")+1);
sprintf(ce[i].u.info,"OLCInfo event 0x0080 content %02x%02x%02x%02x",
PTP_CANON_SET_INFO(ce[i], "OLCInfo event 0x0080 content %02x%02x%02x%02x",
curdata[curoff],
curdata[curoff+1],
curdata[curoff+2],
Expand All @@ -2789,8 +2791,7 @@ static unsigned int olcsizes[0x15][13] = {
case 0x0100: {
/* mask 0x0100: 6 bytes, 00 00 00 00 00 00 (before focus) and 00 00 00 00 01 00 (on focus) observed */
ce[i].type = PTP_CANON_EOS_CHANGES_TYPE_FOCUSINFO;
ce[i].u.info = malloc(strlen("0123456789ab")+1);
sprintf(ce[i].u.info,"%02x%02x%02x%02x%02x%02x",
PTP_CANON_SET_INFO(ce[i], "%02x%02x%02x%02x%02x%02x",
curdata[curoff],
curdata[curoff+1],
curdata[curoff+2],
Expand All @@ -2804,8 +2805,7 @@ static unsigned int olcsizes[0x15][13] = {
case 0x0200: {
/* mask 0x0200: 7 bytes, 00 00 00 00 00 00 00 observed */
ce[i].type = PTP_CANON_EOS_CHANGES_TYPE_FOCUSMASK;
ce[i].u.info = malloc(strlen("0123456789abcd0123456789abcdef")+1);
sprintf(ce[i].u.info,"%02x%02x%02x%02x%02x%02x%02x",
PTP_CANON_SET_INFO(ce[i], "%02x%02x%02x%02x%02x%02x%02x",
curdata[curoff],
curdata[curoff+1],
curdata[curoff+2],
Expand All @@ -2820,8 +2820,7 @@ static unsigned int olcsizes[0x15][13] = {
case 0x0400: {
/* mask 0x0400: 7 bytes, 00 00 00 00 00 00 00 observed */
ce[i].type = PTP_CANON_EOS_CHANGES_TYPE_UNKNOWN;
ce[i].u.info = malloc(strlen("OLCInfo event 0x0400 content 0123456789abcd")+1);
sprintf(ce[i].u.info,"OLCInfo event 0x0400 content %02x%02x%02x%02x%02x%02x%02x",
PTP_CANON_SET_INFO(ce[i], "OLCInfo event 0x0400 content %02x%02x%02x%02x%02x%02x%02x",
curdata[curoff],
curdata[curoff+1],
curdata[curoff+2],
Expand All @@ -2837,8 +2836,7 @@ static unsigned int olcsizes[0x15][13] = {
/* mask 0x0800: 8 bytes, 00 00 00 00 00 00 00 00 and 19 01 00 00 00 00 00 00 and others observed */
/* might be mask of focus points selected */
ce[i].type = PTP_CANON_EOS_CHANGES_TYPE_UNKNOWN;
ce[i].u.info = malloc(strlen("OLCInfo event 0x0800 content 0123456789abcdef")+1);
sprintf(ce[i].u.info,"OLCInfo event 0x0800 content %02x%02x%02x%02x%02x%02x%02x%02x",
PTP_CANON_SET_INFO(ce[i], "OLCInfo event 0x0800 content %02x%02x%02x%02x%02x%02x%02x%02x",
curdata[curoff],
curdata[curoff+1],
curdata[curoff+2],
Expand All @@ -2855,8 +2853,7 @@ static unsigned int olcsizes[0x15][13] = {
/* mask 0x1000: 1 byte, 00 observed */
/* mask 0x1000: 8 byte too on 5ds, type 11 (has shuttercount inside) */
ce[i].type = PTP_CANON_EOS_CHANGES_TYPE_UNKNOWN;
ce[i].u.info = malloc(strlen("OLCInfo event 0x1000 content 01")+1);
sprintf(ce[i].u.info,"OLCInfo event 0x1000 content %02x",
PTP_CANON_SET_INFO(ce[i], "OLCInfo event 0x1000 content %02x",
curdata[curoff]
);
i++;
Expand All @@ -2870,8 +2867,7 @@ static unsigned int olcsizes[0x15][13] = {
}
/* handle more masks */
ce[i].type = PTP_CANON_EOS_CHANGES_TYPE_UNKNOWN;
ce[i].u.info = malloc(strlen("OLCInfo event mask 0123456789")+1);
sprintf(ce[i].u.info, "OLCInfo event mask=%x", mask);
PTP_CANON_SET_INFO(ce[i], "OLCInfo event mask=%x", mask);
break;
}
case PTP_EC_CANON_EOS_CameraStatusChanged:
Expand All @@ -2887,33 +2883,27 @@ static unsigned int olcsizes[0x15][13] = {
break;
case PTP_EC_CANON_EOS_BulbExposureTime:
ce[i].type = PTP_CANON_EOS_CHANGES_TYPE_UNKNOWN;
ce[i].u.info = malloc(strlen("BulbExposureTime 123456789012345678"));
sprintf (ce[i].u.info, "BulbExposureTime %u", dtoh32a(curdata+8));
PTP_CANON_SET_INFO(ce[i], "BulbExposureTime %u", dtoh32a(curdata+8));
break;
case PTP_EC_CANON_EOS_CTGInfoCheckComplete: /* some form of storage catalog ? */
ce[i].type = PTP_CANON_EOS_CHANGES_TYPE_UNKNOWN;
ce[i].u.info = malloc(strlen("CTGInfoCheckComplete 0x012345678")+1);
sprintf (ce[i].u.info, "CTGInfoCheckComplete 0x%08x", dtoh32a(curdata+8));
PTP_CANON_SET_INFO(ce[i], "CTGInfoCheckComplete 0x%08x", dtoh32a(curdata+8));
break;
case PTP_EC_CANON_EOS_StorageStatusChanged:
ce[i].type = PTP_CANON_EOS_CHANGES_TYPE_UNKNOWN;
ce[i].u.info = malloc(strlen("StorageStatusChanged 0x012345678")+1);
sprintf (ce[i].u.info, "StorageStatusChanged 0x%08x", dtoh32a(curdata+8));
PTP_CANON_SET_INFO(ce[i], "StorageStatusChanged 0x%08x", dtoh32a(curdata+8));
break;
case PTP_EC_CANON_EOS_StorageInfoChanged:
ce[i].type = PTP_CANON_EOS_CHANGES_TYPE_UNKNOWN;
ce[i].u.info = malloc(strlen("StorageInfoChanged 0x012345678")+1);
sprintf (ce[i].u.info, "StorageInfoChanged 0x%08x", dtoh32a(curdata+8));
PTP_CANON_SET_INFO(ce[i], "StorageInfoChanged 0x%08x", dtoh32a(curdata+8));
break;
case PTP_EC_CANON_EOS_StoreAdded:
ce[i].type = PTP_CANON_EOS_CHANGES_TYPE_UNKNOWN;
ce[i].u.info = malloc(strlen("StoreAdded 0x012345678")+1);
sprintf (ce[i].u.info, "StoreAdded 0x%08x", dtoh32a(curdata+8));
PTP_CANON_SET_INFO(ce[i], "StoreAdded 0x%08x", dtoh32a(curdata+8));
break;
case PTP_EC_CANON_EOS_StoreRemoved:
ce[i].type = PTP_CANON_EOS_CHANGES_TYPE_UNKNOWN;
ce[i].u.info = malloc(strlen("StoreRemoved 0x012345678")+1);
sprintf (ce[i].u.info, "StoreRemoved 0x%08x", dtoh32a(curdata+8));
PTP_CANON_SET_INFO(ce[i], "StoreRemoved 0x%08x", dtoh32a(curdata+8));
break;
case PTP_EC_CANON_EOS_ObjectRemoved:
ce[i].type = PTP_CANON_EOS_CHANGES_TYPE_OBJECTREMOVED;
Expand All @@ -2923,8 +2913,7 @@ static unsigned int olcsizes[0x15][13] = {
switch (type) {
#define XX(x) case PTP_EC_CANON_EOS_##x: \
ptp_debug (params, "event %u: unhandled EOS event "#x" (size %u)", i, size); \
ce[i].u.info = malloc(strlen("unhandled EOS event "#x" (size 12345678901)")+1); \
sprintf (ce[i].u.info, "unhandled EOS event "#x" (size %u)", size); \
PTP_CANON_SET_INFO(ce[i], "unhandled EOS event "#x" (size %u)", size); \
break;
XX(RequestGetEvent)
XX(RequestGetObjectInfoEx)
Expand Down
2 changes: 1 addition & 1 deletion camlibs/ptp2/ptp.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1833,7 +1833,7 @@ struct _PTPCanon_changes_entry {
enum _PTPCanon_changes_types type;
union {
struct _PTPCanon_New_Object object; /* TYPE_OBJECTINFO */
char *info;
char info[84]; /* 84 is minimum sizeof(object) and sufficient for current use cases */
uint16_t propid;
int status;
} u;
Expand Down

0 comments on commit b8743d8

Please sign in to comment.